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

[Adam Ford]

On Fri, Nov 19, 2021 at 5:51 PM Tim Harvey <tharvey@xxxxxxxxxxxxx> wrote:
>
> On Thu, Nov 11, 2021 at 10:55 PM Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Sat, Nov 6, 2021 at 9:24 PM Adam Ford <aford173@xxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >
> > > 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
> >
> > This is DPHY reset mask, not sure we can handle this via blk-ctrl.
> > Marek has similar patch to support this [1]. we need to phandle the
> > phy in host node in order to work this.
> >
> > However this current patch change seems directly handling dphy reset
> > which indeed fine me as well.
> >
> > >
> > >  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;
> >
> > May be dphy_rst_mask (above comment may not be required, as it
> > understand directly with commit message).
> >
> > >  };
> > >
> > >  #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);
> > > +
> > >         regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
> > >         regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
> > >
> > > @@ -488,6 +505,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_phy_rst_mask = BIT(16) | BIT(17),
> >
> > DPHY has BIT(17) for Master reset and BIT(16) for Slave reset. I think
> > we just need master reset to enable. I've tested only BIT(17) on
> > mipi-dsi gpc and it is working.
> >
>
> Jagan,
>
> In my testing I had to use BIT(16) | BIT(17) in order to capture via CSI.

Lucas,

Based on this, should I redo the patch but without clearing bits 16
and 17?  It seems like both DSI and CSI need at least bit 17, and CSI
appears to need both.  If we clear them, we risk stomping on one
peripheral or another.  If we need both, in theory, we could drop the
need for a DPHY driver on the DSI side.

adam
>
> Best regards,
>
> Tim