Re: [PATCH v2 2/2] PCI: xilinx-nwl: Enable the clock through CCF

[Michal Simek <michal.simek@xilinx.com>]

Hi Krzysztof,

On 6/23/21 3:53 PM, Krzysztof Wilczyński wrote:
> [+cc Sasha for visibility]
> 
> Hi Michal,
> 
> Thank you for sending v2 so promptly!  And for all the extra changes and
> fixes.  Much appreciated!
> 
>> Enable PCIE reference clock. There is no remove function that's why
>> this should be enough for simple operation.
>> Normally this clock is enabled by default by firmware but there are
>> usecases where this clock should be enabled by driver itself.
>> It is also good that clock user is recorded in clock framework.
> 
> Small nitpicks: it would be PCIe here in the above and in the error
> message (this is as per [1]), and "use cases" also in the above. 
> 
> This can be corrected when the patch will be merged by either Bjorn or
> Lorenzo, to avoid sending v3 unnecessarily, provided that they would
> have a moment to do it, of course.

Ok. Will wait for them.

> 
>> Fixes: ab597d35ef11 ("PCI: xilinx-nwl: Add support for Xilinx NWL PCIe Host Controller")
> 
> Thank you!
> 
> Does it make sense for this change to be back-ported to stable and
> long-term kernels?
> 
> I am asking to make sure we do the right thing here, as I can imagine
> that older kernels (primarily because some folks could use, for example,
> Ubuntu LTS releases for development) might often be used by people who
> work with the Xilinx FPGAs and such.

I think that make sense to do so. I haven't had a time to take look at
it closely but I think on Xilinx ZynqMP zcu102 board this missing patch
is causing hang when standard debian 5.10 is used.


> 
> [...]
>> +	err = clk_prepare_enable(pcie->clk);
>> +	if (err) {
>> +		dev_err(dev, "can't enable pcie ref clock\n");
>> +		return err;
>> +	}
>> +
> 
> As per the nitpick above, it would be "PCIe", but probably no need to
> send v3 to correct this.

I will keep my eyes on it and will update it if v3 is required.

Thanks,
Michal