Re: [PATCH] clk: bcm: rpi: Don't register as OF provider if !dev->np
On Fri, Mar 26, 2021 at 7:09 PM Stephen Boyd wrote: > Quoting Nicolas Saenz Julienne (2021-03-25 11:57:48) > > There are two ways clk-raspberrypi might be registered: through > > device-tree or through an explicit platform device registration. The > > latter happens after firmware/raspberrypi's probe, and it's limited to > > RPi3s, which solely use the ARM clock to scale CPU's frequency. That > > clock is matched with cpu0's device thanks to the ARM clock being > > registered as a clkdev. > > > > In that scenario, don't register the device as an OF clock provider, as > > it makes no sense and will cause trouble. > > What sort of trouble? A crash https://lore.kernel.org/linux-acpi/d24bebc5-0f78-021f-293f-e58defa32...@samsung.com/ > > Fixes: d4b4f1b6b97e ("clk: bcm: rpi: Add DT provider for the clocks") > > Reported-by: Marek Szyprowski > > Signed-off-by: Nicolas Saenz Julienne > > --- > > drivers/clk/bcm/clk-raspberrypi.c | 10 ++ > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/clk/bcm/clk-raspberrypi.c > > b/drivers/clk/bcm/clk-raspberrypi.c > > index f89b9cfc4309..27e85687326f 100644 > > --- a/drivers/clk/bcm/clk-raspberrypi.c > > +++ b/drivers/clk/bcm/clk-raspberrypi.c > > @@ -337,10 +337,12 @@ static int raspberrypi_clk_probe(struct > > platform_device *pdev) > > if (ret) > > return ret; > > > > - ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > > - clk_data); > > - if (ret) > > - return ret; > > + if (dev->of_node) { > > Can you add a comment to the code indicating the problem this is fixing? > I fear that we'll look back on this later and simply remove this if > condition because it's "redundant". Better to have some code comment so > we don't have to look up git history to figure out that we only call > this when the of node is populated. I'm not sure I understand what goes > wrong though. Won't the absence of dev->of_node mean the provider > doesn't do anything? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH] clk: bcm: rpi: Don't register as OF provider if !dev->np
Quoting Nicolas Saenz Julienne (2021-03-25 11:57:48) > There are two ways clk-raspberrypi might be registered: through > device-tree or through an explicit platform device registration. The > latter happens after firmware/raspberrypi's probe, and it's limited to > RPi3s, which solely use the ARM clock to scale CPU's frequency. That > clock is matched with cpu0's device thanks to the ARM clock being > registered as a clkdev. > > In that scenario, don't register the device as an OF clock provider, as > it makes no sense and will cause trouble. What sort of trouble? > > Fixes: d4b4f1b6b97e ("clk: bcm: rpi: Add DT provider for the clocks") > Reported-by: Marek Szyprowski > Signed-off-by: Nicolas Saenz Julienne > --- > drivers/clk/bcm/clk-raspberrypi.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/bcm/clk-raspberrypi.c > b/drivers/clk/bcm/clk-raspberrypi.c > index f89b9cfc4309..27e85687326f 100644 > --- a/drivers/clk/bcm/clk-raspberrypi.c > +++ b/drivers/clk/bcm/clk-raspberrypi.c > @@ -337,10 +337,12 @@ static int raspberrypi_clk_probe(struct platform_device > *pdev) > if (ret) > return ret; > > - ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > - clk_data); > - if (ret) > - return ret; > + if (dev->of_node) { Can you add a comment to the code indicating the problem this is fixing? I fear that we'll look back on this later and simply remove this if condition because it's "redundant". Better to have some code comment so we don't have to look up git history to figure out that we only call this when the of node is populated. I'm not sure I understand what goes wrong though. Won't the absence of dev->of_node mean the provider doesn't do anything? > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > + clk_data); > + if (ret) > + return ret; > + } > > rpi->cpufreq = platform_device_register_data(dev, > "raspberrypi-cpufreq", > -1, NULL, 0);
Re: [PATCH] clk: bcm: rpi: Don't register as OF provider if !dev->np
On 25.03.2021 19:57, Nicolas Saenz Julienne wrote: > There are two ways clk-raspberrypi might be registered: through > device-tree or through an explicit platform device registration. The > latter happens after firmware/raspberrypi's probe, and it's limited to > RPi3s, which solely use the ARM clock to scale CPU's frequency. That > clock is matched with cpu0's device thanks to the ARM clock being > registered as a clkdev. > > In that scenario, don't register the device as an OF clock provider, as > it makes no sense and will cause trouble. > > Fixes: d4b4f1b6b97e ("clk: bcm: rpi: Add DT provider for the clocks") > Reported-by: Marek Szyprowski > Signed-off-by: Nicolas Saenz Julienne Tested-by: Marek Szyprowski > --- > drivers/clk/bcm/clk-raspberrypi.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/bcm/clk-raspberrypi.c > b/drivers/clk/bcm/clk-raspberrypi.c > index f89b9cfc4309..27e85687326f 100644 > --- a/drivers/clk/bcm/clk-raspberrypi.c > +++ b/drivers/clk/bcm/clk-raspberrypi.c > @@ -337,10 +337,12 @@ static int raspberrypi_clk_probe(struct platform_device > *pdev) > if (ret) > return ret; > > - ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > - clk_data); > - if (ret) > - return ret; > + if (dev->of_node) { > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > + clk_data); > + if (ret) > + return ret; > + } > > rpi->cpufreq = platform_device_register_data(dev, "raspberrypi-cpufreq", >-1, NULL, 0); Best regards -- Marek Szyprowski, PhD Samsung R Institute Poland
[PATCH] clk: bcm: rpi: Don't register as OF provider if !dev->np
There are two ways clk-raspberrypi might be registered: through device-tree or through an explicit platform device registration. The latter happens after firmware/raspberrypi's probe, and it's limited to RPi3s, which solely use the ARM clock to scale CPU's frequency. That clock is matched with cpu0's device thanks to the ARM clock being registered as a clkdev. In that scenario, don't register the device as an OF clock provider, as it makes no sense and will cause trouble. Fixes: d4b4f1b6b97e ("clk: bcm: rpi: Add DT provider for the clocks") Reported-by: Marek Szyprowski Signed-off-by: Nicolas Saenz Julienne --- drivers/clk/bcm/clk-raspberrypi.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index f89b9cfc4309..27e85687326f 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -337,10 +337,12 @@ static int raspberrypi_clk_probe(struct platform_device *pdev) if (ret) return ret; - ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, - clk_data); - if (ret) - return ret; + if (dev->of_node) { + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, + clk_data); + if (ret) + return ret; + } rpi->cpufreq = platform_device_register_data(dev, "raspberrypi-cpufreq", -1, NULL, 0); -- 2.30.2