Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller

[Neil Armstrong <narmstrong@baylibre.com>]

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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel