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

[Icenowy Zheng]

在 2022-09-12星期一的 10:13 +0000，Conor.Dooley@xxxxxxxxxxxxx写道：
> 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@xxxxxxxxxxxxx写道：
> > > 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.

In fact, none.

But the issue is that the clock name string is never enforced in DT
binding, so at least we may support unnamed clocks as fallback if we
are trying to allow a DT that sticks to previous 5.19 dt-bindings to
work.

> 
> 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@xxxxxxxxxx/
> 
>  From that patch, should this be changed too?
> 
> -       [PRCI_CLK_PCIEAUX] {
> +       [FU740_PRCI_CLK_PCIE_AUX] {
>                 .name = "pcieaux",

This is an internal name of the driver, I think.

>                 .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@xxxxxxxxxxxxx>
> > > 
> > > > 
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > >   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
> > > > 
> > > 
> > 
> > 
>