Messages in this thread | | | Subject | Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver | From | Frieder Schrempf <> | Date | Thu, 25 Feb 2021 09:23:25 +0100 |
| |
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@nxp.com> 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.
Thanks Frieder
| |