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

[Adam Ford <aford173@gmail.com>]

On Wed, Nov 24, 2021 at 11:42 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> 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.

I would disagree that isn't not blk-ctrl.  The blk-ctrl controls the
reset lines for the CSI and enables clocks.  The additional register
does the same thing to the MIPI CSI and DSI.  The imx7-mipi-csis
driver configures the dphy already, but this reset bit is not part of
its IP block.  It seems weird to me that a phy driver would reference
a phy driver.

>
> >
> > 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.

That driver specifically addresses the DSI phy and bringing it out of
reset is just one small part of what that driver does.  I don't think
adding CSI functionality to it would be appropriate for that driver as
they are separate IP blocks.

If people don't want the blk-ctl to control this bit, I would advocate
we should do a separate reset controller to be referenced byt the
mipi-csis driver, but that was proposed before and declined.  Since
blt-ctrl already is pulling seemingly unrelated IP blocks by
controlling their clocks and resets.  The fact that NXP included it in
their ATF power-domain controller tells me they considered it related
to power domains and/or resets and not an explicit phy driver.

adam

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