mbox series

[v2,00/25] media: imx: imx7-mipi-csis: Add i.MX8MM support

Message ID 20210516014441.5508-1-laurent.pinchart@ideasonboard.com
Headers show
Series media: imx: imx7-mipi-csis: Add i.MX8MM support | expand

Message

Laurent Pinchart May 16, 2021, 1:44 a.m. UTC
Hello,

This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
to the imx7-mipi-csis driver.

The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
The driver currently supports v3.3 of the CSIS, found in SoCs from the
i.MX6 and i.MX7 families. This series extends the driver to support
v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
family.

The first 22 patches are miscellaneous cleanups and improvements. Please
see individual patches for details.

Patch 23/25 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
Support for other members of the i.MX8 family will come later, and for
SoCs including an ISI IP core (such as the i.MX8MP) this will require
more work to handle additional glue logic.

Patch 24/25 then extends the imx7-mipi-csis driver accordingly.

Finally, patch 25/25 updates MAINTAINERS per popular request from people
who believe I have too much free time :-)

The series has been tested on an i.MX6ULL (for the CSIS v3.3) and
i.MX8MM (for the CSIS v3.6.3).

The changes in the integration of the CSIS between i.MX7 and i.MX8, as
described in the DT bindings, have been found through reading of
reference manuals and BSP source code, with different sources of
information contradicting each other. A confirmation from NXP would be
nice (in particular regarding the clocks).

Laurent Pinchart (25):
  media: imx: imx7_mipi_csis: Fix logging of only error event counters
  media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
  media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
    mode
  media: imx: imx7_mipi_csis: Move static data to top of
    mipi_csis_dump_regs()
  media: imx: imx7_mipi_csis: Minimize locking in get/set format
  media: imx: imx7_mipi_csis: Don't set subdev data
  media: imx: imx7_mipi_csis: Reorganize code in sections
  media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
  media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
  media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
  media: imx: imx7_mipi_csis: Drop csi_state phy field
  media: imx: imx7_mipi_csis: Rename mipi_sd to sd
  media: imx: imx7_mipi_csis: Rename csi_state flag field to state
  media: imx: imx7_mipi_csis: Turn csi_state irq field into local
    variable
  media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
  media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
  media: imx: imx7_mipi_csis: Drop csi_state pdev field
  media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
  media: imx: imx7_mipi_csis: Reorganize csi_state structure
  media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
  media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
  media: imx: imx7_mipi_csis: Move PHY control to dedicated functions
  dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
  media: imx: imx7_mipi_csis: Add i.MX8MM support
  media: imx: imx7_mipi_csis: Update MAINTAINERS

 .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 109 +-
 MAINTAINERS                                   |   1 +
 drivers/staging/media/imx/imx7-mipi-csis.c    | 994 ++++++++++--------
 3 files changed, 658 insertions(+), 446 deletions(-)

Comments

Laurent Pinchart May 16, 2021, 2:18 a.m. UTC | #1
On Sun, May 16, 2021 at 04:44:38AM +0300, Laurent Pinchart wrote:
> Move the PHY regulator and reset handling to dedicated functions. This
> groups all related code together, and prepares for i.MX8 support that
> doesn't require control of the PHY regulator and reset.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

My apologies, this is a new patch in v2, and Rui hasn't acked it. I'll
thus wait for acks and reviews before sending a pull request.

> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 64 +++++++++++++---------
>  1 file changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 6e235c86e0aa..a8da8d6ddb7d 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -457,25 +457,6 @@ static void mipi_csis_sw_reset(struct csi_state *state)
>  	usleep_range(10, 20);
>  }
>  
> -static int mipi_csis_phy_init(struct csi_state *state)
> -{
> -	state->mipi_phy_regulator = devm_regulator_get(state->dev, "phy");
> -	if (IS_ERR(state->mipi_phy_regulator))
> -		return PTR_ERR(state->mipi_phy_regulator);
> -
> -	return regulator_set_voltage(state->mipi_phy_regulator, 1000000,
> -				     1000000);
> -}
> -
> -static void mipi_csis_phy_reset(struct csi_state *state)
> -{
> -	reset_control_assert(state->mrst);
> -
> -	msleep(20);
> -
> -	reset_control_deassert(state->mrst);
> -}
> -
>  static void mipi_csis_system_enable(struct csi_state *state, int on)
>  {
>  	u32 val, mask;
> @@ -679,6 +660,42 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * PHY regulator and reset
> + */
> +
> +static int mipi_csis_phy_enable(struct csi_state *state)
> +{
> +	return regulator_enable(state->mipi_phy_regulator);
> +}
> +
> +static int mipi_csis_phy_disable(struct csi_state *state)
> +{
> +	return regulator_disable(state->mipi_phy_regulator);
> +}
> +
> +static void mipi_csis_phy_reset(struct csi_state *state)
> +{
> +	reset_control_assert(state->mrst);
> +	msleep(20);
> +	reset_control_deassert(state->mrst);
> +}
> +
> +static int mipi_csis_phy_init(struct csi_state *state)
> +{
> +	/* Get MIPI PHY reset and regulator. */
> +	state->mrst = devm_reset_control_get_exclusive(state->dev, NULL);
> +	if (IS_ERR(state->mrst))
> +		return PTR_ERR(state->mrst);
> +
> +	state->mipi_phy_regulator = devm_regulator_get(state->dev, "phy");
> +	if (IS_ERR(state->mipi_phy_regulator))
> +		return PTR_ERR(state->mipi_phy_regulator);
> +
> +	return regulator_set_voltage(state->mipi_phy_regulator, 1000000,
> +				     1000000);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Debug
>   */
> @@ -1178,7 +1195,7 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
>  	mutex_lock(&state->lock);
>  	if (state->state & ST_POWERED) {
>  		mipi_csis_stop_stream(state);
> -		ret = regulator_disable(state->mipi_phy_regulator);
> +		ret = mipi_csis_phy_disable(state);
>  		if (ret)
>  			goto unlock;
>  		mipi_csis_clk_disable(state);
> @@ -1204,7 +1221,7 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
>  		goto unlock;
>  
>  	if (!(state->state & ST_POWERED)) {
> -		ret = regulator_enable(state->mipi_phy_regulator);
> +		ret = mipi_csis_phy_enable(state);
>  		if (ret)
>  			goto unlock;
>  
> @@ -1288,11 +1305,6 @@ static int mipi_csis_parse_dt(struct csi_state *state)
>  				 &state->clk_frequency))
>  		state->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
>  
> -	/* Get MIPI PHY resets */
> -	state->mrst = devm_reset_control_get_exclusive(state->dev, NULL);
> -	if (IS_ERR(state->mrst))
> -		return PTR_ERR(state->mrst);
> -
>  	return 0;
>  }
>
Laurent Pinchart May 16, 2021, 2:24 a.m. UTC | #2
On Sun, May 16, 2021 at 04:44:16AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
> to the imx7-mipi-csis driver.
> 
> The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
> The driver currently supports v3.3 of the CSIS, found in SoCs from the
> i.MX6 and i.MX7 families. This series extends the driver to support
> v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
> family.
> 
> The first 22 patches are miscellaneous cleanups and improvements. Please
> see individual patches for details.
> 
> Patch 23/25 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
> Support for other members of the i.MX8 family will come later, and for
> SoCs including an ISI IP core (such as the i.MX8MP) this will require
> more work to handle additional glue logic.
> 
> Patch 24/25 then extends the imx7-mipi-csis driver accordingly.
> 
> Finally, patch 25/25 updates MAINTAINERS per popular request from people
> who believe I have too much free time :-)
> 
> The series has been tested on an i.MX6ULL (for the CSIS v3.3) and

I meant an i.MX7D, sorry.

> i.MX8MM (for the CSIS v3.6.3).
> 
> The changes in the integration of the CSIS between i.MX7 and i.MX8, as
> described in the DT bindings, have been found through reading of
> reference manuals and BSP source code, with different sources of
> information contradicting each other. A confirmation from NXP would be
> nice (in particular regarding the clocks).
> 
> Laurent Pinchart (25):
>   media: imx: imx7_mipi_csis: Fix logging of only error event counters
>   media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
>   media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
>     mode
>   media: imx: imx7_mipi_csis: Move static data to top of
>     mipi_csis_dump_regs()
>   media: imx: imx7_mipi_csis: Minimize locking in get/set format
>   media: imx: imx7_mipi_csis: Don't set subdev data
>   media: imx: imx7_mipi_csis: Reorganize code in sections
>   media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
>   media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
>   media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
>   media: imx: imx7_mipi_csis: Drop csi_state phy field
>   media: imx: imx7_mipi_csis: Rename mipi_sd to sd
>   media: imx: imx7_mipi_csis: Rename csi_state flag field to state
>   media: imx: imx7_mipi_csis: Turn csi_state irq field into local
>     variable
>   media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
>   media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
>   media: imx: imx7_mipi_csis: Drop csi_state pdev field
>   media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
>   media: imx: imx7_mipi_csis: Reorganize csi_state structure
>   media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
>   media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
>   media: imx: imx7_mipi_csis: Move PHY control to dedicated functions
>   dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
>   media: imx: imx7_mipi_csis: Add i.MX8MM support
>   media: imx: imx7_mipi_csis: Update MAINTAINERS
> 
>  .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 109 +-
>  MAINTAINERS                                   |   1 +
>  drivers/staging/media/imx/imx7-mipi-csis.c    | 994 ++++++++++--------
>  3 files changed, 658 insertions(+), 446 deletions(-)
Rui Miguel Silva May 16, 2021, 10 p.m. UTC | #3
Hi Laurent,
On Sun May 16, 2021 at 3:18 AM WEST, Laurent Pinchart wrote:

> On Sun, May 16, 2021 at 04:44:38AM +0300, Laurent Pinchart wrote:
> > Move the PHY regulator and reset handling to dedicated functions. This
> > groups all related code together, and prepares for i.MX8 support that
> > doesn't require control of the PHY regulator and reset.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
>
> My apologies, this is a new patch in v2, and Rui hasn't acked it. I'll
> thus wait for acks and reviews before sending a pull request.

Looks good. Thanks for the heads up.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui
>
> > ---
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 64 +++++++++++++---------
> >  1 file changed, 38 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 6e235c86e0aa..a8da8d6ddb7d 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -457,25 +457,6 @@ static void mipi_csis_sw_reset(struct csi_state *state)
> >  	usleep_range(10, 20);
> >  }
> >  
> > -static int mipi_csis_phy_init(struct csi_state *state)
> > -{
> > -	state->mipi_phy_regulator = devm_regulator_get(state->dev, "phy");
> > -	if (IS_ERR(state->mipi_phy_regulator))
> > -		return PTR_ERR(state->mipi_phy_regulator);
> > -
> > -	return regulator_set_voltage(state->mipi_phy_regulator, 1000000,
> > -				     1000000);
> > -}
> > -
> > -static void mipi_csis_phy_reset(struct csi_state *state)
> > -{
> > -	reset_control_assert(state->mrst);
> > -
> > -	msleep(20);
> > -
> > -	reset_control_deassert(state->mrst);
> > -}
> > -
> >  static void mipi_csis_system_enable(struct csi_state *state, int on)
> >  {
> >  	u32 val, mask;
> > @@ -679,6 +660,42 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +/* -----------------------------------------------------------------------------
> > + * PHY regulator and reset
> > + */
> > +
> > +static int mipi_csis_phy_enable(struct csi_state *state)
> > +{
> > +	return regulator_enable(state->mipi_phy_regulator);
> > +}
> > +
> > +static int mipi_csis_phy_disable(struct csi_state *state)
> > +{
> > +	return regulator_disable(state->mipi_phy_regulator);
> > +}
> > +
> > +static void mipi_csis_phy_reset(struct csi_state *state)
> > +{
> > +	reset_control_assert(state->mrst);
> > +	msleep(20);
> > +	reset_control_deassert(state->mrst);
> > +}
> > +
> > +static int mipi_csis_phy_init(struct csi_state *state)
> > +{
> > +	/* Get MIPI PHY reset and regulator. */
> > +	state->mrst = devm_reset_control_get_exclusive(state->dev, NULL);
> > +	if (IS_ERR(state->mrst))
> > +		return PTR_ERR(state->mrst);
> > +
> > +	state->mipi_phy_regulator = devm_regulator_get(state->dev, "phy");
> > +	if (IS_ERR(state->mipi_phy_regulator))
> > +		return PTR_ERR(state->mipi_phy_regulator);
> > +
> > +	return regulator_set_voltage(state->mipi_phy_regulator, 1000000,
> > +				     1000000);
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Debug
> >   */
> > @@ -1178,7 +1195,7 @@ static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> >  	mutex_lock(&state->lock);
> >  	if (state->state & ST_POWERED) {
> >  		mipi_csis_stop_stream(state);
> > -		ret = regulator_disable(state->mipi_phy_regulator);
> > +		ret = mipi_csis_phy_disable(state);
> >  		if (ret)
> >  			goto unlock;
> >  		mipi_csis_clk_disable(state);
> > @@ -1204,7 +1221,7 @@ static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> >  		goto unlock;
> >  
> >  	if (!(state->state & ST_POWERED)) {
> > -		ret = regulator_enable(state->mipi_phy_regulator);
> > +		ret = mipi_csis_phy_enable(state);
> >  		if (ret)
> >  			goto unlock;
> >  
> > @@ -1288,11 +1305,6 @@ static int mipi_csis_parse_dt(struct csi_state *state)
> >  				 &state->clk_frequency))
> >  		state->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
> >  
> > -	/* Get MIPI PHY resets */
> > -	state->mrst = devm_reset_control_get_exclusive(state->dev, NULL);
> > -	if (IS_ERR(state->mrst))
> > -		return PTR_ERR(state->mrst);
> > -
> >  	return 0;
> >  }
> >  
>
> -- 
> Regards,
>
> Laurent Pinchart
Frieder Schrempf May 17, 2021, 9:57 a.m. UTC | #4
On 16.05.21 03:44, Laurent Pinchart wrote:
> Hello,

> 

> This patch series adds support for the CSIS found in the NXP i.MX8MM SoC

> to the imx7-mipi-csis driver.

> 

> The CSIS is an IP core from Samsung, integrated in different NXP SoCs.

> The driver currently supports v3.3 of the CSIS, found in SoCs from the

> i.MX6 and i.MX7 families. This series extends the driver to support

> v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8

> family.

> 

> The first 22 patches are miscellaneous cleanups and improvements. Please

> see individual patches for details.

> 

> Patch 23/25 extends the imx7-mipi-csis DT bindings with i.MX8MM support.

> Support for other members of the i.MX8 family will come later, and for

> SoCs including an ISI IP core (such as the i.MX8MP) this will require

> more work to handle additional glue logic.

> 

> Patch 24/25 then extends the imx7-mipi-csis driver accordingly.

> 

> Finally, patch 25/25 updates MAINTAINERS per popular request from people

> who believe I have too much free time :-)

> 

> The series has been tested on an i.MX6ULL (for the CSIS v3.3) and

> i.MX8MM (for the CSIS v3.6.3).

> 

> The changes in the integration of the CSIS between i.MX7 and i.MX8, as

> described in the DT bindings, have been found through reading of

> reference manuals and BSP source code, with different sources of

> information contradicting each other. A confirmation from NXP would be

> nice (in particular regarding the clocks).


I already tested your pre-v1 off-list patches with my setup with ADV7280-M and I now tried it again with your v2 patchset and the RFC set for the bridge driver. It works fine with some additional changes specific to the data format as already mentioned here: [1].

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>


[1] https://patchwork.kernel.org/project/linux-media/cover/20210215042741.28850-1-laurent.pinchart@ideasonboard.com/#24139291

> 

> Laurent Pinchart (25):

>   media: imx: imx7_mipi_csis: Fix logging of only error event counters

>   media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts

>   media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel

>     mode

>   media: imx: imx7_mipi_csis: Move static data to top of

>     mipi_csis_dump_regs()

>   media: imx: imx7_mipi_csis: Minimize locking in get/set format

>   media: imx: imx7_mipi_csis: Don't set subdev data

>   media: imx: imx7_mipi_csis: Reorganize code in sections

>   media: imx: imx7_mipi_csis: Set the CLKSETTLE register field

>   media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure

>   media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure

>   media: imx: imx7_mipi_csis: Drop csi_state phy field

>   media: imx: imx7_mipi_csis: Rename mipi_sd to sd

>   media: imx: imx7_mipi_csis: Rename csi_state flag field to state

>   media: imx: imx7_mipi_csis: Turn csi_state irq field into local

>     variable

>   media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()

>   media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()

>   media: imx: imx7_mipi_csis: Drop csi_state pdev field

>   media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned

>   media: imx: imx7_mipi_csis: Reorganize csi_state structure

>   media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()

>   media: imx: imx7_mipi_csis: Reject invalid data-lanes settings

>   media: imx: imx7_mipi_csis: Move PHY control to dedicated functions

>   dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support

>   media: imx: imx7_mipi_csis: Add i.MX8MM support

>   media: imx: imx7_mipi_csis: Update MAINTAINERS

> 

>  .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 109 +-

>  MAINTAINERS                                   |   1 +

>  drivers/staging/media/imx/imx7-mipi-csis.c    | 994 ++++++++++--------

>  3 files changed, 658 insertions(+), 446 deletions(-)

>