Messages in this thread | | | Subject | Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller | From | Neil Armstrong <> | Date | Fri, 23 Aug 2019 10:22:28 +0200 |
| |
On 22/08/2019 22:32, Kevin Hilman wrote: > Neil Armstrong <narmstrong@baylibre.com> writes: > >> On 22/08/2019 01:16, Kevin Hilman wrote: >>> Neil Armstrong <narmstrong@baylibre.com> writes: >>> >>>> Add support for the General Purpose Amlogic Everything-Else Power controller, >>>> with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe, >>>> USB, NNA, GE2D and Ethernet Power Domains. >>>> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> >>> Nice! Thanks for generalizing this. >>> >>> A few comments/concerns below, but this is mostly ready. > > [...] > >>>> +#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power) \ >>>> + { \ >>>> + .name = __name, \ >>>> + .reset_names_count = ARRAY_SIZE(__resets), \ >>>> + .reset_names = __resets, \ >>>> + .clk_names_count = ARRAY_SIZE(__clks), \ >>>> + .clk_names = __clks, \ >>>> + .top_pd = __top_pd, \ >>>> + .mem_pd_count = ARRAY_SIZE(__mem), \ >>>> + .mem_pd = __mem, \ >>>> + .get_power = __get_power, \ >>>> + } >>>> + >>>> +#define TOP_PD(__name, __top_pd, __mem) \ >>>> + { \ >>>> + .name = __name, \ >>>> + .top_pd = __top_pd, \ >>>> + .mem_pd_count = ARRAY_SIZE(__mem), \ >>>> + .mem_pd = __mem, \ >>>> + } >>> >>> Why can't the TOP_PD domains also have a __get_power(). Shouldn't we >>> just be able to check the sleep_reg/sleep_mask like in the VPU case? >> >> It can, I can add it later, do we need it for this version ? > > Yes please. Seems a pretty simple addition, let's have it from the > beginning. > >>> Also, for readability, I think the arguments to VPU_PD and TOP_PD to >>> have the same order, at least for the common ones. IOW, __name, >>> __top_pd, __mem should be first. >> >> Sure, will fix > > and you can add __get_power to the common list too. > > [...] > >>>> +static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0 ; i < pwrc_domain->num_clks ; ++i) >>>> + clk_disable(pwrc_domain->clks[i]); >>>> + >>>> + for (i = 0 ; i < pwrc_domain->num_clks ; ++i) >>>> + clk_unprepare(pwrc_domain->clks[i]); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain) >>>> +{ >>>> + int i, ret; >>>> + >>>> + for (i = 0 ; i < pwrc_domain->num_clks ; ++i) { >>>> + ret = clk_prepare(pwrc_domain->clks[i]); >>>> + if (ret) >>>> + goto fail_prepare; >>>> + } >>>> + >>>> + for (i = 0 ; i < pwrc_domain->num_clks ; ++i) { >>>> + ret = clk_enable(pwrc_domain->clks[i]); >>>> + if (ret) >>>> + goto fail_enable; >>>> + } >>>> + >>>> + return 0; >>>> +fail_enable: >>>> + while (--i) >>>> + clk_disable(pwrc_domain->clks[i]); >>>> + >>>> + /* Unprepare all clocks */ >>>> + i = pwrc_domain->num_clks; >>>> + >>>> +fail_prepare: >>>> + while (--i) >>>> + clk_unprepare(pwrc_domain->clks[i]); >>>> + >>>> + return ret; >>>> +} >>> >>> Both the clk enable and disable functions above are just open-coding of >>> the clk_bulk equivalents. Please use clk_bulk_*, then you don't need >>> these helpers. (c.f. the RFT patch I did to convert the old driver to >>> clk_bulk[1]) >> >> Yes, but clk_bulk takes _all_ the clocks from the node, you canot specify >> a list of names, maybe it's overengineered but I wanted to specify the >> exact resets and clocks for each power domain, clk_bulk doesn't provide this. > > I've been going on the assumption that there's no reason to list clocks > in the pwrc DT node that you don't want managed by the driver. This > also seems to match the exisiting driver, and this new one. > > What is the case where you would list clocks in the DT node for the > power-domain, but not want to manage them in the driver? > > If there's no good reason to do that, then clk_bulk greatly simplifies > this code.
I guess I could put back the code if we need to split clocks and resets across power domains in the future.
> >>>> +static int meson_ee_pwrc_off(struct generic_pm_domain *domain) >>>> +{ >>>> + struct meson_ee_pwrc_domain *pwrc_domain = >>>> + container_of(domain, struct meson_ee_pwrc_domain, base); >>>> + int i; >>>> + >>>> + if (pwrc_domain->desc.top_pd) >>>> + regmap_update_bits(pwrc_domain->pwrc->regmap_ao, >>>> + pwrc_domain->desc.top_pd->sleep_reg, >>>> + pwrc_domain->desc.top_pd->sleep_mask, >>>> + pwrc_domain->desc.top_pd->sleep_mask); >>>> + udelay(20); >>>> + >>>> + for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i) >>>> + regmap_update_bits(pwrc_domain->pwrc->regmap_hhi, >>>> + pwrc_domain->desc.mem_pd[i].reg, >>>> + pwrc_domain->desc.mem_pd[i].mask, >>>> + pwrc_domain->desc.mem_pd[i].mask); >>>> + >>>> + udelay(20); >>>> + >>>> + if (pwrc_domain->desc.top_pd) >>>> + regmap_update_bits(pwrc_domain->pwrc->regmap_ao, >>>> + pwrc_domain->desc.top_pd->iso_reg, >>>> + pwrc_domain->desc.top_pd->iso_mask, >>>> + pwrc_domain->desc.top_pd->iso_mask); >>>> + >>>> + if (pwrc_domain->num_clks) { >>>> + msleep(20); >>>> + meson_ee_clk_disable(pwrc_domain); >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int meson_ee_pwrc_on(struct generic_pm_domain *domain) >>>> +{ >>>> + struct meson_ee_pwrc_domain *pwrc_domain = >>>> + container_of(domain, struct meson_ee_pwrc_domain, base); >>>> + int i, ret; >>>> + >>>> + if (pwrc_domain->desc.top_pd) >>>> + regmap_update_bits(pwrc_domain->pwrc->regmap_ao, >>>> + pwrc_domain->desc.top_pd->sleep_reg, >>>> + pwrc_domain->desc.top_pd->sleep_mask, 0); >>>> + udelay(20); >>>> + >>>> + for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i) >>>> + regmap_update_bits(pwrc_domain->pwrc->regmap_hhi, >>>> + pwrc_domain->desc.mem_pd[i].reg, >>>> + pwrc_domain->desc.mem_pd[i].mask, 0); >>>> + >>>> + udelay(20); >>>> + >>>> + ret = meson_ee_reset_assert(pwrc_domain); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (pwrc_domain->desc.top_pd) >>>> + regmap_update_bits(pwrc_domain->pwrc->regmap_ao, >>>> + pwrc_domain->desc.top_pd->iso_reg, >>>> + pwrc_domain->desc.top_pd->iso_mask, 0); >>>> + >>>> + ret = meson_ee_reset_deassert(pwrc_domain); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + return meson_ee_clk_enable(pwrc_domain); >>>> +} >>>> + >>>> +static int meson_ee_pwrc_init_domain(struct platform_device *pdev, >>>> + struct meson_ee_pwrc *sm1_pwrc, >>>> + struct meson_ee_pwrc_domain *dom) >>>> +{ >>>> + dom->pwrc = sm1_pwrc; >>>> + dom->num_rstc = dom->desc.reset_names_count; >>>> + dom->num_clks = dom->desc.clk_names_count; >>>> + >>>> + if (dom->num_rstc) { >>>> + int rst; >>>> + >>>> + dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc, >>>> + sizeof(struct reset_control *), GFP_KERNEL); >>>> + if (!dom->rstc) >>>> + return -ENOMEM; >>>> + >>>> + for (rst = 0 ; rst < dom->num_rstc ; ++rst) { >>>> + dom->rstc[rst] = devm_reset_control_get_exclusive( >>>> + &pdev->dev, >>>> + dom->desc.reset_names[rst]); >>>> + if (IS_ERR(dom->rstc[rst])) >>>> + return PTR_ERR(dom->rstc[rst]); >>>> + } >>> >>> Why not simplify and use the helpers that get multiple reset lines (like >>> devm_reset_control_array_get() used in meson-gx-pwrc-vpu.c)? >> >> Same comment as clk_bulk, we cannot be sure we select the right reset lines. > > Again, what is the case for listing resets in the power-domain node that > you don't want to be managed by the driver? > >>> You could also use reset_control_get_count() and compare to the expected >>> number (dom->num_rstc). >> >> This seems oversimplified > > Similar to above, if you're always going to manage all the reset lines > in the DT node, then simple is good. > > If there are reasons to list reset lines in the DT node that will not be > managed by the driver, I'm defintiely curious why. > > If not, using the reset API helpers is much more readable and > maintainble IMO.
Will simply add the resets and clocks numbers and check the number at probe.
> >>> >>>> + } >>>> + >>>> + if (dom->num_clks) { >>>> + int clk; >>>> + >>>> + dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks, >>>> + sizeof(struct clk *), GFP_KERNEL); >>>> + if (!dom->clks) >>>> + return -ENOMEM; >>>> + >>>> + for (clk = 0 ; clk < dom->num_clks ; ++clk) { >>>> + dom->clks[clk] = devm_clk_get(&pdev->dev, >>>> + dom->desc.clk_names[clk]); >>>> + if (IS_ERR(dom->clks[clk])) >>>> + return PTR_ERR(dom->clks[clk]); >>>> + } >>>> + } >>> >>> Please use clk_bulk API, and then just double-check that the number of >>> clocks found matches the expected number. >> >> Same, I'll either take all the clks and resets for the vpu power domain, >> or keep this code to make sure we get the right clocks and resets. > > Similar to above. IMO, we should be sure to put the "right clocks and > resets" into the DT, and then simplify the code. > >>> >>>> + dom->base.name = dom->desc.name; >>>> + dom->base.power_on = meson_ee_pwrc_on; >>>> + dom->base.power_off = meson_ee_pwrc_off; >>>> + >>>> + if (dom->desc.get_power) { >>>> + bool powered_off = dom->desc.get_power(dom); >>> >>> nit: insert blank line here >>> >>> More importantly, we defintely will have problem here in the >>> !powered_off case. TL;DR; the driver's state does not match the actual >>> hardware state. >>> >>> When powered_off = false, you're telling the genpd core that this domain >>> is already turned on. However, you haven't called _pwrc_on() yet for >>> the domain, which means internal state of the driver for this domain >>> (e.g. clock enables, resets, etc.) is not in sync with the HW. On >>> SEI610 this case is happending for the VPU, which seems to be enabled by >>> u-boot, so this driver detects it as already on, which is fine. But... >>> >>> Remember that the ->power_off() function will be called during suspend, >>> and will lead to the clk unprepare/disable calls. However, for domains >>> that are detected as on (!powered_off), clk prepare/enable will never >>> have been called, so that when suspend happens, you'll get "already >>> unprepared" errors from the clock core >>> >>> IOW, I think you need something like this here: >>> >>> if (!powered_off) >>> meson_ee_pwrc_on(&dom->base); >>> >>> so that the internal state of clock fwk etc. matches the detected state >>> of the HW. The problem with that simple fix, at least for the VPU is >>> that it might cause us to lose any existing display or framebuffer >>> console that's on-going. Probably needs some testing. >> >> Yes, I forgot to take the _shutdown() function from gx_pwrc_vpu driver : >> >> 349 static void meson_gx_pwrc_vpu_shutdown(struct platform_device *pdev) >> 350 { >> 351 struct meson_gx_pwrc_vpu *vpu_pd = platform_get_drvdata(pdev); >> 352 bool powered_off; >> 353 >> 354 powered_off = meson_gx_pwrc_vpu_get_power(vpu_pd); >> 355 if (!powered_off) >> 356 vpu_pd->genpd.power_off(&vpu_pd->genpd); >> 357 } > > OK, yeah, I hadn't noticed that in the original driver. I tested > something simliar with suspend/resume on SEI610 and it gets rid of the > "already unprepared" splats from the clock core. > >>> >>> Anyways, to see what I mean, try suspend/resume (you can test this >>> series on my integ branch with "rtcwake -d rtc0 -m mem -s4") and you'll >>> see error splats from the clock core during suspend. >>> >>> >>> >>>> + pm_genpd_init(&dom->base, &pm_domain_always_on_gov, >>>> + powered_off); >>> >>>> + } else >>>> + pm_genpd_init(&dom->base, NULL, true); >>> >>> nit: the else clause should also have {} to match the if >>> (c.f. CodingStyle) >> >> OK >> >>> >>> Why do you force the always-on governor in the case where ->get_power() >>> exists, but not the other? >>> >>> If you force that, then for any devices connected to these domains that >>> use runtime PM, they will never turn off the domain when it's idle. >>> IOW, these domains will only ever be turned off on system-wide >>> suspend/resume. >>> >>> IMO, unless there's a good reason not to, you should pass NULL for the >>> governor. >> >> It's for legacy when VPU is initialized from vendor U-Boot, look at commit : >> 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader" >> >> In the case the VPU power domain has been powered on by the bootloader >> and no driver are attached to this power domain, the genpd will power it >> off after a certain amount of time, but the clocks hasn't been enabled >> by the kernel itself and the power-off will trigger some faults. >> This patch enable the clocks to have a coherent state for an eventual >> poweroff and switches to the pm_domain_always_on_gov governor. > > The key phrase there being "and no driver is attached". Now that we > have a driver, it claims this domain so I don't think it will be > powered off: > > # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > domain status slaves > /device runtime status > ---------------------------------------------------------------------- > ETH on > /devices/platform/soc/ff3f0000.ethernet unsupported > AUDIO off-0 > GE2D off-0 > PCI off-0 > USB on > /devices/platform/soc/ffe09000.usb active > NNA off-0 > VPU on > /devices/platform/soc/ff900000.vpu unsupported > > In my tests with a framebuffer console (over HDMI), I don't see the > display being powered off.
It's in the case where the driver is a module loaded by the post-initramfs system after the genpd timeout, or if the display driver is disabled.
In the later I had some system failures when vendor u-boot enabled the display and genpd disabled the power domain later on.
> >> I could set always-on governor only if the domain was already enabled, >> what do you think ? > > I don't think that's necessary now that we have a driver. We really > want to be able to power-down this domain when the display is not in > use, and if you use always_on, that will never happen. > >> And seems I'm also missing the "This patch enable the clocks". > > I'm not sure what patch you're referring to.
It's also added in 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"
I would like to keep the same behavior as meson-gx-pwrc-vpu, since it works fine and we debugged all the issues we got.
Neil
> > Kevin >
| |