| Message ID | 20220907054020.745672-1-uwu@icenowy.me |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: fu740: do not use clock name when requesting clock | expand |
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 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 >
在 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? 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/ > > 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 > > >
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 >>> >> > >
在 2022-09-12星期一的 10:13 +0000，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. 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@icenowy.me/ > > 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@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 > > > > > > > > > > > >
On Wed, Sep 07, 2022 at 01:40:20PM +0800, Icenowy Zheng wrote: > 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). > > Drop the name in the clock request, instead just pass NULL (because > this device should have only a single clock). If you rework this for any reason, please capitalize the subject line to match the convention: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/pci/controller/dwc/pcie-fu740.c > 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 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Fri, Sep 23, 2022 at 04:14:39PM -0500, Bjorn Helgaas wrote: > On Wed, Sep 07, 2022 at 01:40:20PM +0800, Icenowy Zheng wrote: > > 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). > > > > Drop the name in the clock request, instead just pass NULL (because > > this device should have only a single clock). > > If you rework this for any reason, please capitalize the subject line > to match the convention: It prob needs a v2 because the first sentence of the commit message is wrong, I recently added the binding enforcement. Thanks, Conor. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/pci/controller/dwc/pcie-fu740.c > > > 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 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
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");
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). 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(-)