On Thu, Feb 25, 2021 at 12:28 AM Jacky Bai <[email protected]> wrote: > > > > > -----Original Message----- > > From: Frieder Schrempf [mailto:[email protected]] > > Sent: Thursday, February 25, 2021 4:23 PM > > To: Abel Vesa <[email protected]>; Dong Aisheng <[email protected]> > > Cc: Aisheng Dong <[email protected]>; Rob Herring <[email protected]>; > > Peng Fan <[email protected]>; Jacky Bai <[email protected]>; Anson Huang > > <[email protected]>; devicetree <[email protected]>; > > Stephen Boyd <[email protected]>; Shawn Guo <[email protected]>; > > Mike Turquette <[email protected]>; Linux Kernel Mailing List > > <[email protected]>; Marek Vasut <[email protected]>; > > dl-linux-imx <[email protected]>; Sascha Hauer <[email protected]>; > > Fabio Estevam <[email protected]>; Philipp Zabel > > <[email protected]>; Adam Ford <[email protected]>; linux-clk > > <[email protected]>; moderated list:ARM/FREESCALE IMX / MXC > > ARM ARCHITECTURE <[email protected]>; Lucas Stach > > <[email protected]> > > 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 <[email protected]> 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. >
Jacky, Any update on this? This is still blocking several drivers and major functionality of the i.MX8 SoC's in mainline and I would hope this would be a top priority for NXP. Best regards, Tim
