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

[Jagan Teki <jagan@amarulasolutions.com>]

On Tue, Nov 23, 2021 at 7:29 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Sun, Nov 21, 2021 at 4:25 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Adam,
> >
> > On Sat, Nov 06, 2021 at 10:54:23AM -0500, Adam Ford wrote:
> > > 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.
> > >
> > > Fixes: 926e57c065df ("soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > >
> > > V2:  Make a note that the extra register is only for Mini/Nano DISPLAY_BLK_CTRL
> > >      Rename the new register to mipi_phy_rst_mask
> > >      Encapsulate the edits to this register with an if-statement
> > >
> > >  drivers/soc/imx/imx8m-blk-ctrl.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c
> > > index 519b3651d1d9..581eb4bc7f7d 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 /* Mini/Nano DISPLAY_BLK_CTRL only */
> > >
> > >  struct imx8m_blk_ctrl_domain;
> > >
> > > @@ -36,6 +37,15 @@ struct imx8m_blk_ctrl_domain_data {
> > >       const char *gpc_name;
> > >       u32 rst_mask;
> > >       u32 clk_mask;
> > > +
> > > +     /*
> > > +      * i.MX8M Mini and Nano have a third DISPLAY_BLK_CTRL register
> > > +      * which is used to control the reset for the MIPI Phy.
> > > +      * Since it's only present in certain circumstances,
> > > +      * an if-statement should be used before setting and clearing this
> > > +      * register.
> > > +      */
> > > +     u32 mipi_phy_rst_mask;
> > >  };
> > >
> > >  #define DOMAIN_MAX_CLKS 3
> > > @@ -78,6 +88,8 @@ 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);
> > > +     if (data->mipi_phy_rst_mask)
> > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > >
> > >       /* enable upstream and blk-ctrl clocks to allow reset to propagate */
> > >       ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
> > > @@ -99,6 +111,8 @@ 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);
> > > +     if (data->mipi_phy_rst_mask)
> > > +             regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > >
> > >       /* disable upstream clocks */
> > >       clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > > @@ -120,6 +134,9 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > >       struct imx8m_blk_ctrl *bc = domain->bc;
> > >
> > >       /* put devices into reset and disable clocks */
> > > +     if (data->mipi_phy_rst_mask)
> > > +             regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > +
> >
> > Is it the best option to enable/disable both the master and slave MIPI
> > DPHY, regardless of whether they're used or not ? Or would it be better
> > to implement a reset controller to expose the two resets independently,
> > and acquire them from the corresponding display and camera drivers ?
>
> In some early attempts to implement the blk-ctrl driver, there was an
> attempt to enable a reset controller, but it caused some hanging and
> issues with suspend-resume due to chicken-egg issues where some items
> were coming up in the wrong order.  I think the decision was made to
> make the resets part of the power domain so it's very clear that the
> order of operations.  Lucas might be able to elaborate more on this.

I think supporting via phy driver make sense to me since this resent
is DPHY specific and nothing related to blk-ctrl.

>
> If bits 16 and 17 can act independently and bit 16 only impacts the
> CSI  and doesn't require bit 17, it seems reasonable to me to have the
> power-domain part of  the CSI, since this would only be enabled when
> the CSI is active.  The power domain is idled when the CSI is idled
> which would effectively place the phy in and out of reset only
> depending on the state of the CSI.  I am guessing this reset bit
> should be assigned to DISPBLK_PD_MIPI_CSI and not
> DISPBLK_PD_CSI_BRIDGE, but I can run some more tests.
>
> AFAIK, there is no phy driver for the CSI like there is the DSI, so
> adding that would require additional work to the CSI driver to work
> around this quirk.  We don't have an acceptable DSI driver yet, so I'd
> like to push a V3 with just the corresponding bit enabled for MIPI_CSI
> after some testing.  FWICT, NXP set both bits 16 and 17 in their ATF
> gpc code, and it never gets cleared, so I think having the bit set and
> cleared on demand is an improvement.

How about using the previous one that Marek sent. Add it via CSI
pipeline and i think it would directly.

https://www.spinics.net/lists/devicetree/msg381691.html

Jagan.