| Message ID | 1541416443-4321-3-git-send-email-claudiu.beznea@microchip.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | power: reset: at91-poweroff: cleanups | expand |
Hi Claudiu, On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote: > static int __init at91_poweroff_probe(struct platform_device *pdev) > @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) > u32 ddr_type; > int ret; > > + at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL); > + if (!at91_shdwc) > + return -ENOMEM; > + Is there any real benefit that will offset the time lost for that allocation at boot time? I understand you are then testing at91_shdwc to know whether the driver already probed once. But, the driver will never probe twice as there is only one shutdown controller on the SoC and anyway, If it was to probe twice, it will still work as expected.
Hi Alexandre, On 06.11.2018 23:09, Alexandre Belloni wrote: > Hi Claudiu, > > On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote: >> static int __init at91_poweroff_probe(struct platform_device *pdev) >> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) >> u32 ddr_type; >> int ret; >> >> + at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL); >> + if (!at91_shdwc) >> + return -ENOMEM; >> + > > Is there any real benefit that will offset the time lost for that > allocation at boot time? No, I haven't run benchmarks on this. I only wanted to have them grouped in one structure. Please let me know if you have some tests in mind. > > I understand you are then testing at91_shdwc to know whether the driver > already probed once. But, the driver will never probe twice as there is > only one shutdown controller on the SoC and anyway, If it was to probe > twice, it will still work as expected. I had in mind the scenario where the driver would be compiled as module. I know insmod already does this checking. I'm ok to remove this checking. I will do it in next version. With this I will also remove devm_kzalloc() of at91_shdwc. Thank you, Claudiu Beznea > >
On 07/11/2018 14:54:17+0000, Claudiu.Beznea@microchip.com wrote: > Hi Alexandre, > > On 06.11.2018 23:09, Alexandre Belloni wrote: > > Hi Claudiu, > > > > On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote: > >> static int __init at91_poweroff_probe(struct platform_device *pdev) > >> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) > >> u32 ddr_type; > >> int ret; > >> > >> + at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL); > >> + if (!at91_shdwc) > >> + return -ENOMEM; > >> + > > > > Is there any real benefit that will offset the time lost for that > > allocation at boot time? > > No, I haven't run benchmarks on this. I only wanted to have them grouped in > one structure. Please let me know if you have some tests in mind. > Well, it is probably not much but small things adds up. Havinf it as a global structure is probably good enough. > > > > I understand you are then testing at91_shdwc to know whether the driver > > already probed once. But, the driver will never probe twice as there is > > only one shutdown controller on the SoC and anyway, If it was to probe > > twice, it will still work as expected. > > I had in mind the scenario where the driver would be compiled as module. I > know insmod already does this checking. I'm ok to remove this checking. I > will do it in next version. With this I will also remove devm_kzalloc() of > at91_shdwc. > Thanks,
Hi, On Wed, Nov 07, 2018 at 06:23:40PM +0100, Alexandre Belloni wrote: > On 07/11/2018 14:54:17+0000, Claudiu.Beznea@microchip.com wrote: > > Hi Alexandre, > > > > On 06.11.2018 23:09, Alexandre Belloni wrote: > > > Hi Claudiu, > > > > > > On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote: > > >> static int __init at91_poweroff_probe(struct platform_device *pdev) > > >> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) > > >> u32 ddr_type; > > >> int ret; > > >> > > >> + at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL); > > >> + if (!at91_shdwc) > > >> + return -ENOMEM; > > >> + > > > > > > Is there any real benefit that will offset the time lost for that > > > allocation at boot time? > > > > No, I haven't run benchmarks on this. I only wanted to have them grouped in > > one structure. Please let me know if you have some tests in mind. > > > > Well, it is probably not much but small things adds up. Having it as a > global structure is probably good enough. I suppose I will get a new patch with this change? -- Sebastian > > > > > > > I understand you are then testing at91_shdwc to know whether the driver > > > already probed once. But, the driver will never probe twice as there is > > > only one shutdown controller on the SoC and anyway, If it was to probe > > > twice, it will still work as expected. > > > > I had in mind the scenario where the driver would be compiled as module. I > > know insmod already does this checking. I'm ok to remove this checking. I > > will do it in next version. With this I will also remove devm_kzalloc() of > > at91_shdwc. > > > > Thanks, > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi Sebastian, On 06.12.2018 00:40, Sebastian Reichel wrote: > Hi, > > On Wed, Nov 07, 2018 at 06:23:40PM +0100, Alexandre Belloni wrote: >> On 07/11/2018 14:54:17+0000, Claudiu.Beznea@microchip.com wrote: >>> Hi Alexandre, >>> >>> On 06.11.2018 23:09, Alexandre Belloni wrote: >>>> Hi Claudiu, >>>> >>>> On 05/11/2018 11:14:26+0000, Claudiu.Beznea@microchip.com wrote: >>>>> static int __init at91_poweroff_probe(struct platform_device *pdev) >>>>> @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) >>>>> u32 ddr_type; >>>>> int ret; >>>>> >>>>> + at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL); >>>>> + if (!at91_shdwc) >>>>> + return -ENOMEM; >>>>> + >>>> >>>> Is there any real benefit that will offset the time lost for that >>>> allocation at boot time? >>> >>> No, I haven't run benchmarks on this. I only wanted to have them grouped in >>> one structure. Please let me know if you have some tests in mind. >>> >> >> Well, it is probably not much but small things adds up. Having it as a >> global structure is probably good enough. > > I suppose I will get a new patch with this change? Yes, I will send a new version for this one. Thank you, Claudiu Beznea > > -- Sebastian > >> >>>> >>>> I understand you are then testing at91_shdwc to know whether the driver >>>> already probed once. But, the driver will never probe twice as there is >>>> only one shutdown controller on the SoC and anyway, If it was to probe >>>> twice, it will still work as expected. >>> >>> I had in mind the scenario where the driver would be compiled as module. I >>> know insmod already does this checking. I'm ok to remove this checking. I >>> will do it in next version. With this I will also remove devm_kzalloc() of >>> at91_shdwc. >>> >> >> Thanks, >> >> -- >> Alexandre Belloni, Bootlin >> Embedded Linux and Kernel engineering >> https://bootlin.com
diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c index 82533f4c72fc..48661e04a3de 100644 --- a/drivers/power/reset/at91-poweroff.c +++ b/drivers/power/reset/at91-poweroff.c @@ -51,14 +51,19 @@ static const char *shdwc_wakeup_modes[] = { [AT91_SHDW_WKMODE0_ANYLEVEL] = "any", }; -static void __iomem *at91_shdwc_base; -static struct clk *sclk; -static void __iomem *mpddrc_base; +struct shdwc { + struct clk *sclk; + void __iomem *shdwc_base; + void __iomem *mpddrc_base; +}; + +static struct shdwc *at91_shdwc; static void __init at91_wakeup_status(struct platform_device *pdev) { + struct shdwc *shdwc = platform_get_drvdata(pdev); const char *reason; - u32 reg = readl(at91_shdwc_base + AT91_SHDW_SR); + u32 reg = readl(shdwc->shdwc_base + AT91_SHDW_SR); /* Simple power-on, just bail out */ if (!reg) @@ -92,9 +97,9 @@ static void at91_poweroff(void) " b .\n\t" : - : "r" (mpddrc_base), + : "r" (at91_shdwc->mpddrc_base), "r" cpu_to_le32(AT91_DDRSDRC_LPDDR2_PWOFF), - "r" (at91_shdwc_base), + "r" (at91_shdwc->shdwc_base), "r" cpu_to_le32(AT91_SHDW_KEY | AT91_SHDW_SHDW) : "r6"); } @@ -118,6 +123,7 @@ static int at91_poweroff_get_wakeup_mode(struct device_node *np) static void at91_poweroff_dt_set_wakeup_mode(struct platform_device *pdev) { + struct shdwc *shdwc = platform_get_drvdata(pdev); struct device_node *np = pdev->dev.of_node; int wakeup_mode; u32 mode = 0, tmp; @@ -144,7 +150,7 @@ static void at91_poweroff_dt_set_wakeup_mode(struct platform_device *pdev) if (of_property_read_bool(np, "atmel,wakeup-rtt-timer")) mode |= AT91_SHDW_RTTWKEN; - writel(wakeup_mode | mode, at91_shdwc_base + AT91_SHDW_MR); + writel(wakeup_mode | mode, shdwc->shdwc_base + AT91_SHDW_MR); } static int __init at91_poweroff_probe(struct platform_device *pdev) @@ -154,16 +160,22 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) u32 ddr_type; int ret; + at91_shdwc = devm_kzalloc(&pdev->dev, sizeof(*at91_shdwc), GFP_KERNEL); + if (!at91_shdwc) + return -ENOMEM; + + platform_set_drvdata(pdev, at91_shdwc); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - at91_shdwc_base = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(at91_shdwc_base)) - return PTR_ERR(at91_shdwc_base); + at91_shdwc->shdwc_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(at91_shdwc->shdwc_base)) + return PTR_ERR(at91_shdwc->shdwc_base); - sclk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(sclk)) - return PTR_ERR(sclk); + at91_shdwc->sclk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(at91_shdwc->sclk)) + return PTR_ERR(at91_shdwc->sclk); - ret = clk_prepare_enable(sclk); + ret = clk_prepare_enable(at91_shdwc->sclk); if (ret) { dev_err(&pdev->dev, "Could not enable slow clock\n"); return ret; @@ -176,20 +188,20 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) np = of_find_compatible_node(NULL, NULL, "atmel,sama5d3-ddramc"); if (np) { - mpddrc_base = of_iomap(np, 0); + at91_shdwc->mpddrc_base = of_iomap(np, 0); of_node_put(np); - if (!mpddrc_base) { + if (!at91_shdwc->mpddrc_base) { ret = -ENOMEM; goto clk_disable; } - ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) & + ddr_type = readl(at91_shdwc->mpddrc_base + AT91_DDRSDRC_MDR) & AT91_DDRSDRC_MD; if (ddr_type != AT91_DDRSDRC_MD_LPDDR2 && ddr_type != AT91_DDRSDRC_MD_LPDDR3) { - iounmap(mpddrc_base); - mpddrc_base = NULL; + iounmap(at91_shdwc->mpddrc_base); + at91_shdwc->mpddrc_base = NULL; } } @@ -198,19 +210,21 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) return 0; clk_disable: - clk_disable_unprepare(sclk); + clk_disable_unprepare(at91_shdwc->sclk); return ret; } static int __exit at91_poweroff_remove(struct platform_device *pdev) { + struct shdwc *shdwc = platform_get_drvdata(pdev); + if (pm_power_off == at91_poweroff) pm_power_off = NULL; - if (mpddrc_base) - iounmap(mpddrc_base); + if (shdwc->mpddrc_base) + iounmap(shdwc->mpddrc_base); - clk_disable_unprepare(sclk); + clk_disable_unprepare(shdwc->sclk); return 0; }
Move SHDWC realted data to only one structure to have them grouped. Inspired from commit 9be74f0d39c1 ("power: reset: at91-poweroff: make mpddrc_base part of struct shdwc"). Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/power/reset/at91-poweroff.c | 60 +++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 23 deletions(-)