RE: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver
From: Jacky Bai
Date: Thu Feb 25 2021 - 03:29:12 EST
>
-----Original Message-----
>
From: Frieder Schrempf [mailto:frieder.schrempf@xxxxxxxxxx]
>
Sent: Thursday, February 25, 2021 4:23 PM
>
To: Abel Vesa <abel.vesa@xxxxxxx>; Dong Aisheng <dongas86@xxxxxxxxx>
>
Cc: Aisheng Dong <aisheng.dong@xxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
>
Peng Fan <peng.fan@xxxxxxx>; Jacky Bai <ping.bai@xxxxxxx>; Anson Huang
>
<anson.huang@xxxxxxx>; devicetree <devicetree@xxxxxxxxxxxxxxx>;
>
Stephen Boyd <sboyd@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>;
>
Mike Turquette <mturquette@xxxxxxxxxxxx>; Linux Kernel Mailing List
>
<linux-kernel@xxxxxxxxxxxxxxx>; Marek Vasut <marek.vasut@xxxxxxxxx>;
>
dl-linux-imx <linux-imx@xxxxxxx>; Sascha Hauer <kernel@xxxxxxxxxxxxxx>;
>
Fabio Estevam <fabio.estevam@xxxxxxx>; Philipp Zabel
>
<p.zabel@xxxxxxxxxxxxxx>; Adam Ford <aford173@xxxxxxxxx>; linux-clk
>
<linux-clk@xxxxxxxxxxxxxxx>; moderated list:ARM/FREESCALE IMX / MXC
>
ARM ARCHITECTURE <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; Lucas Stach
>
<l.stach@xxxxxxxxxxxxxx>
>
Subject: Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver
>
>
Hi Abel,
>
>
On 17.11.20 15:48, Abel Vesa wrote:
>
> On 20-11-11 17:13:25, Dong Aisheng wrote:
>
>> On Tue, Nov 3, 2020 at 7:22 PM Abel Vesa <abel.vesa@xxxxxxx> wrote:
>
>> ...
>
>>> +static int imx_blk_ctl_reset_set(struct reset_controller_dev *rcdev,
>
>>> + unsigned long id, bool assert) {
>
>>> + struct imx_blk_ctl_drvdata *drvdata = container_of(rcdev,
>
>>> + struct imx_blk_ctl_drvdata, rcdev);
>
>>> + unsigned int offset = drvdata->rst_hws[id].offset;
>
>>> + unsigned int shift = drvdata->rst_hws[id].shift;
>
>>> + unsigned int mask = drvdata->rst_hws[id].mask;
>
>>> + void __iomem *reg_addr = drvdata->base + offset;
>
>>> + unsigned long flags;
>
>>> + u32 reg;
>
>>> +
>
>>> + if (!assert && !test_bit(1, &drvdata->rst_hws[id].asserted))
>
>>> + return -ENODEV;
>
>>
>
>> What if consumers call deassert first in probe which seems common in
>
kernel?
>
>> It seems will fail.
>
>> e.g.
>
>> probe() {
>
>> reset_control_get()
>
>> reset_control_deassert()
>
>> }
>
>>
>
>> Regards
>
>> Aisheng
>
>>
>
>
>
> OK, I'm trying to explain here how I know the resets are supposed to
>
> be working and how the BLK_CTL IP is working.
>
>
>
>
>
> First of, the BLK_CTL bits (resets and clocks) all have the HW init
>
> (default) values as 0. Basically, after the blk_ctl PD is powered on,
>
> the resets are deasserted and clocks are gated by default. Since the
>
> blk_ctl is not the parent of any of the consumers in devicetree (the
>
> reg maps are entirely different anyway), there is no way of ordering
>
> the runtime callbacks between the consumer and the blk_ctl. So we
>
> might end up having the runtime resume callback after the one from
>
> EARC (consumer), for example, which will basically overwrite the value
>
written by EARC driver with whatever was saved on suspend.
>
>
>
> Now, about the usage of the reset bits. AFAICT, it would make more
>
> sense to assert the reset, then enable the clock, then deassert. This
>
> way, you're keeping the EARC (consumer) in reset (with the clocks on)
>
> until you eventually release it out of reset by deasserting. This is
>
> how the runtime resume should deal with the reset and the clock. As
>
> for the runtime suspend, the reset can be entirely ignored as long as you're
>
disabling the clock.
>
>
>
> This last part will allow the blk_ctl to make the following assumption:
>
> if all the clocks are disabled and none of the reset bits are asserted, I can
>
power off.
>
>
>
> Now, I know there are drivers outthere that do assert on suspend, but
>
> as long as the clocks are disabled, the assert will have no impact.
>
> But maybe in their case the reset controller cannot power down itself.
>
>
>
> As for the safekeeping of the register, I'll just drop it due to the following
>
arguments:
>
> 1. all the clocks are gated by default 2. all resets are deasserted by
>
> default 3. when blk_ctl goes down, all the consumers go down. (all
>
> have the same PD)
>
>
>
> From 1 and 2 results the IP will not be running and from 3 results
>
> the HW state of every IP becomes HW init state.
>
>
Are there any plans to continue this work? As BLK-CTL it is not only relevant
>
for the i.MX8MP, but also for i.MX8MM and i.MX8MN, it would be nice to get
>
this ready in order to prepare for proper graphics/display support.
>
Before continuing this work, we need to find out a way to resolve the cycling dependency issue between power domain and blk-ctrl.
it is indeed introduced some troubles in NXP latest internal release when the blk-ctrl driver is added.
BR
Jacky Bai
>
Thanks
>
Frieder