Re: [PATCH] PCI: fu740: do not use clock name when requesting clock

[<Conor.Dooley@microchip.com>]

On 12/09/2022 02:38, Icenowy Zheng wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 在 2022-09-08星期四的 18:14 +0000，Conor.Dooley@microchip.com写道：
>> On 07/09/2022 06:40, Icenowy Zheng wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>> know the content is safe
>>>
>>> The DT binding of FU740 PCIe does not enforce a clock-names property,
>>> and there exist some device tree that has a clock name that does not
>>> stick to the one used by Linux DT (e.g. the one shipped with current
>>> U-Boot mainline).
>>
>> I recently added the missing enforcement:
>> https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/dt&id=b408fad61d34c765c3e01895286332af2d50402a
> 
> Unfortunately binding w/o clock-names enforcement has already entered a
> stable release (5.19), and the real clock name "pcie_aux" is never
> enforced before (there's a DT in U-Boot that uses "pcieaux" instead),
> should this be considered as breakage to stable DT binding?

Does anything in U-Boot actually use that clock name? The clock name is
currently being relied on by both Linux and BSD (although BSD does have
a fallback to the U-Boot provided name. There's only one clock so it
seems fine to me to stop using the name, but the DT in U-Boot should be
fixed so that PCI works IMO.

fwiw:
> 
> Anyway, I had sent out a patch that synchorizes all FU740-related DT
> files to U-Boot, see [1].
> 
> [1]
> https://lore.kernel.org/all/20220825081119.1694007-2-uwu@icenowy.me/

 From that patch, should this be changed too?

-	[PRCI_CLK_PCIEAUX] {
+	[FU740_PRCI_CLK_PCIE_AUX] {
  		.name = "pcieaux",
  		.parent_name = "",
  		.ops = &sifive_fu740_prci_pcieaux_clk_ops,

> 
>>
>> Since there's only one clock though, I'd imagine it makes little to no
>> real difference if the check here is relaxed.
>>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>
>>>
>>> Drop the name in the clock request, instead just pass NULL (because
>>> this device should have only a single clock).
>>>
>>> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
>>> ---
>>>   drivers/pci/controller/dwc/pcie-fu740.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c
>>> b/drivers/pci/controller/dwc/pcie-fu740.c
>>> index 0c90583c078b..edb218a37a4f 100644
>>> --- a/drivers/pci/controller/dwc/pcie-fu740.c
>>> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
>>> @@ -315,7 +315,7 @@ static int fu740_pcie_probe(struct
>>> platform_device *pdev)
>>>                  return dev_err_probe(dev, PTR_ERR(afp->pwren),
>>> "unable to get pwren-gpios\n");
>>>
>>>          /* Fetch clocks */
>>> -       afp->pcie_aux = devm_clk_get(dev, "pcie_aux");
>>> +       afp->pcie_aux = devm_clk_get(dev, NULL);
>>>          if (IS_ERR(afp->pcie_aux))
>>>                  return dev_err_probe(dev, PTR_ERR(afp->pcie_aux),
>>>                                               "pcie_aux clock source
>>> missing or invalid\n");
>>> --
>>> 2.37.1
>>>
>>
> 
>