Re: [PATCH 1/5] soc: imx: imx8m-blk-ctrl: Fix imx8mm mipi reset

[Lucas Stach]

Am Freitag, dem 05.11.2021 um 09:21 -0500 schrieb Adam Ford:
> On Fri, Nov 5, 2021 at 9:01 AM Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
> > 
> > Hi Adam,
> > 
> > Am Freitag, dem 05.11.2021 um 08:42 -0500 schrieb Adam Ford:
> > > Most of the blk-ctrl reset bits are found in one register, however
> > > there are two bits in offset 8 for pulling the MIPI DPHY out of reset
> > > and these need to be set when IMX8MM_DISPBLK_PD_MIPI_CSI is brought
> > > out of reset or the MIPI_CSI hangs.
> > > 
> > I'm undecided yet if I like this solution, as register 0x8 has
> > different content on different instances of the blk-ctrl, so naming it
> > mipi_rst_mask is confusing for others.
> 
>  Both Mini and Nano use it, so I could add a comment that this
> register only applies to Mini and Nano.  For all others, it would be
> empty and do nothing anyway.  If you have a name you'd prefer, I can
> do that.
> 
Register 0x8 in the vpu-blk-ctrl also has a very different meaning. :)

I don't really have a better suggestion for the name. Maybe just keep
it or mipi_phy_rst_mask or something, but a comment would certainly be
welcome.

> > 
> > My initial thought was that we should habe a MIPI PHY driver
> > controlling all the registers in the disp-blk-ctrl, other than SFT_RSTN
> > and CLK_EN, however I see how the reset handling for the PHY would then
> > be inconsistent with how we handle all the other devices in the disp-
> > blk domain. But then the HW guys seemed to think along the same lines,
> > as they placed the PHY resets into the PHY registers, instead of the
> > SFT_RSTN, which had more than enough spare bits to carry those
> > resets...
> 
> The NXP ATF code sets these bits in the GPC code with the other power
> domain stuff, so it seems like they intended it to be used as a power
> domain thing and not MIPI PHY driver.

Yea, maybe the HW guys just misplaced those resets and it certainly
makes sense to handle the reset from the power-domain. While the MIPI
PHY has no direct connection to a SoC bus and should thus be unable to
corrupt SoC state it seems like a good idea to bring it into a clean
state after powering up the domain.

> 
> > 
> > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > Signed-off-by: Adam Ford <aford173@xxxxxxxxx>
> > > ---
> > >  drivers/soc/imx/imx8m-blk-ctrl.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > index 519b3651d1d9..5506bd075c35 100644
> > > --- a/drivers/soc/imx/imx8m-blk-ctrl.c
> > > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > @@ -17,6 +17,7 @@
> > > 
> > >  #define BLK_SFT_RSTN 0x0
> > >  #define BLK_CLK_EN   0x4
> > > +#define BLK_MIPI_RESET_DIV   0x8
> > > 
> > >  struct imx8m_blk_ctrl_domain;
> > > 
> > > @@ -36,6 +37,7 @@ struct imx8m_blk_ctrl_domain_data {
> > >       const char *gpc_name;
> > >       u32 rst_mask;
> > >       u32 clk_mask;
> > > +     u32 mipi_rst_mask;
> > >  };
> > > 
> > >  #define DOMAIN_MAX_CLKS 3
> > > @@ -78,6 +80,7 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > 
> > >       /* put devices into reset */
> > >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > +     regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_rst_mask);

Please guard all of those in a if (data->mipi_rst_mask). Regmap doesn't
check if the mask is 0, so we can avoid some MMIO accesses with this
condition and at the same time make it clear in the code that this
additional reset doesn't apply to most of the blk-ctrl instances.

> > > 
> > >       /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > >       ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > @@ -99,6 +102,7 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > > 
> > >       /* release reset */
> > >       regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > > +     regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_rst_mask);
> > > 
> > >       /* disable upstream clocks */
> > >       clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > @@ -122,6 +126,7 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > >       /* put devices into reset and disable clocks */
> > >       regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > >       regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> > > +     regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_rst_mask);
> > 
> > For consistency the reset assertion should be put before the clock
> > disable.
> 
> I can do that.  How do you want me to proceed?
> > > 
I agree that we should do this in the blk-ctrl driver.

Regards,
Lucas

> > >       /* power down upstream GPC domain */
> > >       pm_runtime_put(domain->power_dev);
> > > @@ -488,6 +493,7 @@ static const struct imx8m_blk_ctrl_domain_data imx8mm_disp_blk_ctl_domain_data[]
> > >               .gpc_name = "mipi-csi",
> > >               .rst_mask = BIT(3) | BIT(4),
> > >               .clk_mask = BIT(10) | BIT(11),
> > > +             .mipi_rst_mask = BIT(16) | BIT(17),
> > >       },
> > >  };
> > > 
> > 
> >