diff mbox series

[3/3] ASoC: fsl_sai: add i.MX8M support

Message ID 20190717105635.18514-4-l.stach@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series i.MX8M support for FSL SAI | expand

Commit Message

Lucas Stach July 17, 2019, 10:56 a.m. UTC
The SAI block on the i.MX8M moved the register layout, as 2 version
registers were added at the start of the register map. We deal with
this by moving the start of the regmap, so both register layouts
look the same to accesses going through the regmap.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../devicetree/bindings/sound/fsl-sai.txt     |  2 +-
 sound/soc/fsl/fsl_sai.c                       | 19 +++++++++++++++++++
 sound/soc/fsl/fsl_sai.h                       |  1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

Comments

Angus Ainslie July 17, 2019, 1:45 p.m. UTC | #1
On 2019-07-17 04:56, Lucas Stach wrote:
> The SAI block on the i.MX8M moved the register layout, as 2 version
> registers were added at the start of the register map. We deal with
> this by moving the start of the regmap, so both register layouts
> look the same to accesses going through the regmap.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

checkpatch has a complaint about mixing code and device tree bindings.

Other than that

Tested-by: Angus Ainslie <angus@akkea.ca>
Reviewed-by: Angus Ainslie <angus@akkea.ca>

> ---
>  .../devicetree/bindings/sound/fsl-sai.txt     |  2 +-
>  sound/soc/fsl/fsl_sai.c                       | 19 +++++++++++++++++++
>  sound/soc/fsl/fsl_sai.h                       |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt
> b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> index 2e726b983845..037871d2f505 100644
> --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> @@ -8,7 +8,7 @@ codec/DSP interfaces.
>  Required properties:
> 
>    - compatible		: Compatible list, contains "fsl,vf610-sai",
> -			  "fsl,imx6sx-sai" or "fsl,imx6ul-sai"
> +			  "fsl,imx6sx-sai", "fsl,imx6ul-sai" or "fsl,imx8mq-sai"
> 
>    - reg			: Offset and length of the register set for the device.
> 
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index b3cd055a61c7..a10201078e40 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -808,6 +808,16 @@ static int fsl_sai_probe(struct platform_device 
> *pdev)
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
> 
> +	/*
> +	 * New versions of the SAI have 2 32bit version registers added at 
> the
> +	 * start of the register map. To avoid dealing with this shift all 
> over
> +	 * the driver, we move the start of the regmap to make both register
> +	 * layouts look the same to the regmap. This means we can't read the
> +	 * version registers through the regmap, but this is small price to 
> pay.
> +	 */
> +	if (sai->soc_data->has_version_registers)
> +		base += 8;
> +
>  	sai->regmap = devm_regmap_init_mmio_clk(&pdev->dev,
>  			"bus", base, &fsl_sai_regmap_config);
> 
> @@ -923,18 +933,27 @@ static int fsl_sai_remove(struct platform_device 
> *pdev)
> 
>  static const struct fsl_sai_soc_data fsl_sai_vf610_data = {
>  	.use_imx_pcm = false,
> +	.has_version_registers = false,
>  	.fifo_depth = 32,
>  };
> 
>  static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = {
>  	.use_imx_pcm = true,
> +	.has_version_registers = false,
>  	.fifo_depth = 32,
>  };
> 
> +static const struct fsl_sai_soc_data fsl_sai_imx8m_data = {
> +	.use_imx_pcm = true,
> +	.has_version_registers = true,
> +	.fifo_depth = 128,
> +};
> +
>  static const struct of_device_id fsl_sai_ids[] = {
>  	{ .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data },
>  	{ .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data },
>  	{ .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data },
> +	{ .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8m_data },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, fsl_sai_ids);
> diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
> index 7c1ef671da28..b6a9053ed400 100644
> --- a/sound/soc/fsl/fsl_sai.h
> +++ b/sound/soc/fsl/fsl_sai.h
> @@ -128,6 +128,7 @@
> 
>  struct fsl_sai_soc_data {
>  	bool use_imx_pcm;
> +	bool has_version_registers;
>  	unsigned int fifo_depth;
>  };
Daniel Baluta July 17, 2019, 2:16 p.m. UTC | #2
On Wed, Jul 17, 2019 at 1:59 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> The SAI block on the i.MX8M moved the register layout, as 2 version
> registers were added at the start of the register map. We deal with
> this by moving the start of the regmap, so both register layouts
> look the same to accesses going through the regmap.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

This is a little bit tricky. We need the verid register in order
to differentiate IPs which can support 1:1 ratio for bclk:mclk

I am working now for upstreaming all SAI patches that we have
in our internal tree.

> ---
>  .../devicetree/bindings/sound/fsl-sai.txt     |  2 +-
>  sound/soc/fsl/fsl_sai.c                       | 19 +++++++++++++++++++
>  sound/soc/fsl/fsl_sai.h                       |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> index 2e726b983845..037871d2f505 100644
> --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> @@ -8,7 +8,7 @@ codec/DSP interfaces.
>  Required properties:
>
>    - compatible         : Compatible list, contains "fsl,vf610-sai",
> -                         "fsl,imx6sx-sai" or "fsl,imx6ul-sai"
> +                         "fsl,imx6sx-sai", "fsl,imx6ul-sai" or "fsl,imx8mq-sai"
>
>    - reg                        : Offset and length of the register set for the device.
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index b3cd055a61c7..a10201078e40 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -808,6 +808,16 @@ static int fsl_sai_probe(struct platform_device *pdev)
>         if (IS_ERR(base))
>                 return PTR_ERR(base);
>
> +       /*
> +        * New versions of the SAI have 2 32bit version registers added at the
> +        * start of the register map. To avoid dealing with this shift all over
> +        * the driver, we move the start of the regmap to make both register
> +        * layouts look the same to the regmap. This means we can't read the
> +        * version registers through the regmap, but this is small price to pay.
> +        */
> +       if (sai->soc_data->has_version_registers)
> +               base += 8;
> +
>         sai->regmap = devm_regmap_init_mmio_clk(&pdev->dev,
>                         "bus", base, &fsl_sai_regmap_config);
>
> @@ -923,18 +933,27 @@ static int fsl_sai_remove(struct platform_device *pdev)
>
>  static const struct fsl_sai_soc_data fsl_sai_vf610_data = {
>         .use_imx_pcm = false,
> +       .has_version_registers = false,
>         .fifo_depth = 32,
>  };
>
>  static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = {
>         .use_imx_pcm = true,
> +       .has_version_registers = false,
>         .fifo_depth = 32,
>  };
>
> +static const struct fsl_sai_soc_data fsl_sai_imx8m_data = {
> +       .use_imx_pcm = true,
> +       .has_version_registers = true,
> +       .fifo_depth = 128,
> +};
> +
>  static const struct of_device_id fsl_sai_ids[] = {
>         { .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data },
>         { .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data },
>         { .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data },
> +       { .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8m_data },
>         { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, fsl_sai_ids);
> diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
> index 7c1ef671da28..b6a9053ed400 100644
> --- a/sound/soc/fsl/fsl_sai.h
> +++ b/sound/soc/fsl/fsl_sai.h
> @@ -128,6 +128,7 @@
>
>  struct fsl_sai_soc_data {
>         bool use_imx_pcm;
> +       bool has_version_registers;
>         unsigned int fifo_depth;
>  };
>
> --
> 2.20.1
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Lucas Stach July 17, 2019, 2:33 p.m. UTC | #3
Hi Daniel,

Am Mittwoch, den 17.07.2019, 17:16 +0300 schrieb Daniel Baluta:
> > On Wed, Jul 17, 2019 at 1:59 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > The SAI block on the i.MX8M moved the register layout, as 2 version
> > registers were added at the start of the register map. We deal with
> > this by moving the start of the regmap, so both register layouts
> > look the same to accesses going through the regmap.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> 
> This is a little bit tricky. We need the verid register in order
> to differentiate IPs which can support 1:1 ratio for bclk:mclk

And this patch doesn't prevent this usage. If needed we can just read
the verid via a plain readl on the base mapping in the probe function
and cache it in struct fsl_sai. This seems way less intrusive than
carrying a register offset through all of the regmap accessors and
validation functions. I simply haven't implemented it in this patch, as
I had no need for it right now.

Regards,
Lucas

> I am working now for upstreaming all SAI patches that we have
> in our internal tree.
> 
> > ---
> >  .../devicetree/bindings/sound/fsl-sai.txt     |  2 +-
> >  sound/soc/fsl/fsl_sai.c                       | 19 +++++++++++++++++++
> >  sound/soc/fsl/fsl_sai.h                       |  1 +
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> > index 2e726b983845..037871d2f505 100644
> > --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
> > +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> > @@ -8,7 +8,7 @@ codec/DSP interfaces.
> >  Required properties:
> > 
> >    - compatible         : Compatible list, contains "fsl,vf610-sai",
> > -                         "fsl,imx6sx-sai" or "fsl,imx6ul-sai"
> > +                         "fsl,imx6sx-sai", "fsl,imx6ul-sai" or "fsl,imx8mq-sai"
> > 
> >    - reg                        : Offset and length of the register set for the device.
> > 
> > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > index b3cd055a61c7..a10201078e40 100644
> > --- a/sound/soc/fsl/fsl_sai.c
> > +++ b/sound/soc/fsl/fsl_sai.c
> > @@ -808,6 +808,16 @@ static int fsl_sai_probe(struct platform_device *pdev)
> >         if (IS_ERR(base))
> >                 return PTR_ERR(base);
> > 
> > +       /*
> > +        * New versions of the SAI have 2 32bit version registers added at the
> > +        * start of the register map. To avoid dealing with this shift all over
> > +        * the driver, we move the start of the regmap to make both register
> > +        * layouts look the same to the regmap. This means we can't read the
> > +        * version registers through the regmap, but this is small price to pay.
> > +        */
> > +       if (sai->soc_data->has_version_registers)
> > +               base += 8;
> > +
> >         sai->regmap = devm_regmap_init_mmio_clk(&pdev->dev,
> >                         "bus", base, &fsl_sai_regmap_config);
> > 
> > @@ -923,18 +933,27 @@ static int fsl_sai_remove(struct platform_device *pdev)
> > 
> >  static const struct fsl_sai_soc_data fsl_sai_vf610_data = {
> >         .use_imx_pcm = false,
> > +       .has_version_registers = false,
> >         .fifo_depth = 32,
> >  };
> > 
> >  static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = {
> >         .use_imx_pcm = true,
> > +       .has_version_registers = false,
> >         .fifo_depth = 32,
> >  };
> > 
> > +static const struct fsl_sai_soc_data fsl_sai_imx8m_data = {
> > +       .use_imx_pcm = true,
> > +       .has_version_registers = true,
> > +       .fifo_depth = 128,
> > +};
> > +
> >  static const struct of_device_id fsl_sai_ids[] = {
> >         { .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data },
> >         { .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data },
> >         { .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data },
> > +       { .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8m_data },
> >         { /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, fsl_sai_ids);
> > diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
> > index 7c1ef671da28..b6a9053ed400 100644
> > --- a/sound/soc/fsl/fsl_sai.h
> > +++ b/sound/soc/fsl/fsl_sai.h
> > @@ -128,6 +128,7 @@
> > 
> >  struct fsl_sai_soc_data {
> >         bool use_imx_pcm;
> > +       bool has_version_registers;
> >         unsigned int fifo_depth;
> >  };
> > 
> > --
> > 2.20.1
> > 
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Daniel Baluta July 17, 2019, 3:06 p.m. UTC | #4
On Wed, Jul 17, 2019 at 5:33 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Daniel,
>
> Am Mittwoch, den 17.07.2019, 17:16 +0300 schrieb Daniel Baluta:
> > > On Wed, Jul 17, 2019 at 1:59 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > The SAI block on the i.MX8M moved the register layout, as 2 version
> > > registers were added at the start of the register map. We deal with
> > > this by moving the start of the regmap, so both register layouts
> > > look the same to accesses going through the regmap.
> > >
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >
> > This is a little bit tricky. We need the verid register in order
> > to differentiate IPs which can support 1:1 ratio for bclk:mclk
>
> And this patch doesn't prevent this usage. If needed we can just read
> the verid via a plain readl on the base mapping in the probe function
> and cache it in struct fsl_sai. This seems way less intrusive than
> carrying a register offset through all of the regmap accessors and
> validation functions. I simply haven't implemented it in this patch, as
> I had no need for it right now.

I must admit this is a very clever idea! Anyhow, I'm having some concerns
because unfortunately not all registers were shifted by 8 bytes.

See: imx6sx [1] (page 3575)  and imx8X [2] (page 5512) RMs.

We have something like this:

i.mx6 SX:

00: TCSR
04: TCR1
08: TCR2
0C: TCR3
....
60: TMR
80: RCSR

i.mx 8X

00: VERID
04: PARAM
08: TCSR
0C: TCR1
...
60: TMR
88: RCSR

[1] https://cache.nxp.com/secured/assets/documents/en/reference-manual/IMX6SXRM.pdf?__gda__=1563382650_d60ad6189b2431a35a0757ffc87cfb3f&fileExt=.pdf
[2] https://www.nxp.com/docs/en/reference-manual/IMX8DQXPRM.pdf
Angus Ainslie July 17, 2019, 3:15 p.m. UTC | #5
On 2019-07-17 09:06, Daniel Baluta wrote:
> On Wed, Jul 17, 2019 at 5:33 PM Lucas Stach <l.stach@pengutronix.de> 
> wrote:
>> 
>> Hi Daniel,
>> 
>> Am Mittwoch, den 17.07.2019, 17:16 +0300 schrieb Daniel Baluta:
>> > > On Wed, Jul 17, 2019 at 1:59 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>> > >
>> > > The SAI block on the i.MX8M moved the register layout, as 2 version
>> > > registers were added at the start of the register map. We deal with
>> > > this by moving the start of the regmap, so both register layouts
>> > > look the same to accesses going through the regmap.
>> > >
>> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>> >
>> > This is a little bit tricky. We need the verid register in order
>> > to differentiate IPs which can support 1:1 ratio for bclk:mclk
>> 
>> And this patch doesn't prevent this usage. If needed we can just read
>> the verid via a plain readl on the base mapping in the probe function
>> and cache it in struct fsl_sai. This seems way less intrusive than
>> carrying a register offset through all of the regmap accessors and
>> validation functions. I simply haven't implemented it in this patch, 
>> as
>> I had no need for it right now.
> 
> I must admit this is a very clever idea! Anyhow, I'm having some 
> concerns
> because unfortunately not all registers were shifted by 8 bytes.
> 
> See: imx6sx [1] (page 3575)  and imx8X [2] (page 5512) RMs.
> 
> We have something like this:
> 
> i.mx6 SX:
> 
> 00: TCSR
> 04: TCR1
> 08: TCR2
> 0C: TCR3
> ....
> 60: TMR
> 80: RCSR
> 
> i.mx 8X
> 
> 00: VERID
> 04: PARAM
> 08: TCSR
> 0C: TCR1
> ...
> 60: TMR
> 88: RCSR
> 

We could split it into an upper and a lower regmap. Only the lower would 
need the version register offset.

> [1]
> https://cache.nxp.com/secured/assets/documents/en/reference-manual/IMX6SXRM.pdf?__gda__=1563382650_d60ad6189b2431a35a0757ffc87cfb3f&fileExt=.pdf
> [2] https://www.nxp.com/docs/en/reference-manual/IMX8DQXPRM.pdf
Daniel Baluta July 18, 2019, 8:35 a.m. UTC | #6
On Wed, Jul 17, 2019 at 6:15 PM Angus Ainslie <angus@akkea.ca> wrote:
>
> On 2019-07-17 09:06, Daniel Baluta wrote:
> > On Wed, Jul 17, 2019 at 5:33 PM Lucas Stach <l.stach@pengutronix.de>
> > wrote:
> >>
> >> Hi Daniel,
> >>
> >> Am Mittwoch, den 17.07.2019, 17:16 +0300 schrieb Daniel Baluta:
> >> > > On Wed, Jul 17, 2019 at 1:59 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> >> > >
> >> > > The SAI block on the i.MX8M moved the register layout, as 2 version
> >> > > registers were added at the start of the register map. We deal with
> >> > > this by moving the start of the regmap, so both register layouts
> >> > > look the same to accesses going through the regmap.
> >> > >
> >> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >> >
> >> > This is a little bit tricky. We need the verid register in order
> >> > to differentiate IPs which can support 1:1 ratio for bclk:mclk
> >>
> >> And this patch doesn't prevent this usage. If needed we can just read
> >> the verid via a plain readl on the base mapping in the probe function
> >> and cache it in struct fsl_sai. This seems way less intrusive than
> >> carrying a register offset through all of the regmap accessors and
> >> validation functions. I simply haven't implemented it in this patch,
> >> as
> >> I had no need for it right now.
> >
> > I must admit this is a very clever idea! Anyhow, I'm having some
> > concerns
> > because unfortunately not all registers were shifted by 8 bytes.
> >
> > See: imx6sx [1] (page 3575)  and imx8X [2] (page 5512) RMs.
> >
> > We have something like this:
> >
> > i.mx6 SX:
> >
> > 00: TCSR
> > 04: TCR1
> > 08: TCR2
> > 0C: TCR3
> > ....
> > 60: TMR
> > 80: RCSR
> >
> > i.mx 8X
> >
> > 00: VERID
> > 04: PARAM
> > 08: TCSR
> > 0C: TCR1
> > ...
> > 60: TMR
> > 88: RCSR
> >
>
> We could split it into an upper and a lower regmap. Only the lower would
> need the version register offset.

That would work but will be unnecessary complicated. Let me send the
imx8M support
as we implemented in our tree by Friday.

It has the disadvantage that we've wrapped all shifted register with a
macro but I don't
see other solution.
Cezary Rojewski July 18, 2019, 7:11 p.m. UTC | #7
On 2019-07-17 12:56, Lucas Stach wrote:
>   static const struct fsl_sai_soc_data fsl_sai_vf610_data = {
>   	.use_imx_pcm = false,
> +	.has_version_registers = false,
>   	.fifo_depth = 32,
>   };
>   
>   static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = {
>   	.use_imx_pcm = true,
> +	.has_version_registers = false,
>   	.fifo_depth = 32,
>   };
>   
> +static const struct fsl_sai_soc_data fsl_sai_imx8m_data = {
> +	.use_imx_pcm = true,
> +	.has_version_registers = true,
> +	.fifo_depth = 128,
> +};
> +
>   static const struct of_device_id fsl_sai_ids[] = {
>   	{ .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data },
>   	{ .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data },
>   	{ .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data },
> +	{ .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8m_data },
>   	{ /* sentinel */ }
>   };

These datas are populating quite rapidly. If you're planning for adding 
more (and/ or appending additional fields), declaring helper macro could 
prove useful.

Czarek
Daniel Baluta July 18, 2019, 7:22 p.m. UTC | #8
On Thu, Jul 18, 2019 at 10:12 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> On 2019-07-17 12:56, Lucas Stach wrote:
> >   static const struct fsl_sai_soc_data fsl_sai_vf610_data = {
> >       .use_imx_pcm = false,
> > +     .has_version_registers = false,
> >       .fifo_depth = 32,
> >   };
> >
> >   static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = {
> >       .use_imx_pcm = true,
> > +     .has_version_registers = false,
> >       .fifo_depth = 32,
> >   };
> >
> > +static const struct fsl_sai_soc_data fsl_sai_imx8m_data = {
> > +     .use_imx_pcm = true,
> > +     .has_version_registers = true,
> > +     .fifo_depth = 128,
> > +};
> > +
> >   static const struct of_device_id fsl_sai_ids[] = {
> >       { .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data },
> >       { .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data },
> >       { .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data },
> > +     { .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8m_data },
> >       { /* sentinel */ }
> >   };
>

Hi Czarek,

> These datas are populating quite rapidly. If you're planning for adding
> more (and/ or appending additional fields), declaring helper macro could
> prove useful.

There would be definitely more fields inside *_data structures. Anyhow,
not sure what do you mean by declaring a helper macro.

Can you provide an example and how would that be helpful?

thanks,
Daniel.
Cezary Rojewski July 18, 2019, 8:19 p.m. UTC | #9
On 2019-07-18 21:22, Daniel Baluta wrote:
> On Thu, Jul 18, 2019 at 10:12 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
>>
>> On 2019-07-17 12:56, Lucas Stach wrote:
>>>    static const struct fsl_sai_soc_data fsl_sai_vf610_data = {
>>>        .use_imx_pcm = false,
>>> +     .has_version_registers = false,
>>>        .fifo_depth = 32,
>>>    };
>>>
>>>    static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = {
>>>        .use_imx_pcm = true,
>>> +     .has_version_registers = false,
>>>        .fifo_depth = 32,
>>>    };
>>>
>>> +static const struct fsl_sai_soc_data fsl_sai_imx8m_data = {
>>> +     .use_imx_pcm = true,
>>> +     .has_version_registers = true,
>>> +     .fifo_depth = 128,
>>> +};
>>> +
>>>    static const struct of_device_id fsl_sai_ids[] = {
>>>        { .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data },
>>>        { .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data },
>>>        { .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data },
>>> +     { .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8m_data },
>>>        { /* sentinel */ }
>>>    };
>>
> 
> Hi Czarek,
> 
>> These datas are populating quite rapidly. If you're planning for adding
>> more (and/ or appending additional fields), declaring helper macro could
>> prove useful.
> 
> There would be definitely more fields inside *_data structures. Anyhow,
> not sure what do you mean by declaring a helper macro.
> 
> Can you provide an example and how would that be helpful?
> 
> thanks,
> Daniel.
> 

Hello Daniel,

My suggestion was rather straight-forward - examples of such can be 
found in soc-dapm.h header file. Instead of stating everything 
explicitly, there are e.g.: SND_SOC_DAPM_SPK or SND_SOC_DAPM_PGA 
declared to help me out.

Nothing fancy, just space savers if your structs populate like rabbits.

Czarek
Daniel Baluta July 22, 2019, 10:39 a.m. UTC | #10
On Thu, Jul 18, 2019 at 1:36 PM Angus Ainslie <angus@akkea.ca> wrote:
>
> On 2019-07-17 04:56, Lucas Stach wrote:
> > The SAI block on the i.MX8M moved the register layout, as 2 version
> > registers were added at the start of the register map. We deal with
> > this by moving the start of the regmap, so both register layouts
> > look the same to accesses going through the regmap.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>
> checkpatch has a complaint about mixing code and device tree bindings.

Hello Angus, Lucas,

What imx-sdma firmware have you used to test this with linux-next? I've
finished my changes but aplay gets stuck sending the first audio frame.

Daniel.
Angus Ainslie July 22, 2019, 1:41 p.m. UTC | #11
Hi Daniel,

On 2019-07-22 03:39, Daniel Baluta wrote:
> On Thu, Jul 18, 2019 at 1:36 PM Angus Ainslie <angus@akkea.ca> wrote:
>> 
>> On 2019-07-17 04:56, Lucas Stach wrote:
>> > The SAI block on the i.MX8M moved the register layout, as 2 version
>> > registers were added at the start of the register map. We deal with
>> > this by moving the start of the regmap, so both register layouts
>> > look the same to accesses going through the regmap.
>> >
>> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>> 
>> checkpatch has a complaint about mixing code and device tree bindings.
> 
> Hello Angus, Lucas,
> 
> What imx-sdma firmware have you used to test this with linux-next? I've
> finished my changes but aplay gets stuck sending the first audio frame.
> 

I'm just using the ROM firmware, I have no additional firmware in the 
filesystem.

Angus

> Daniel.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
index 2e726b983845..037871d2f505 100644
--- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
+++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
@@ -8,7 +8,7 @@  codec/DSP interfaces.
 Required properties:
 
   - compatible		: Compatible list, contains "fsl,vf610-sai",
-			  "fsl,imx6sx-sai" or "fsl,imx6ul-sai"
+			  "fsl,imx6sx-sai", "fsl,imx6ul-sai" or "fsl,imx8mq-sai"
 
   - reg			: Offset and length of the register set for the device.
 
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index b3cd055a61c7..a10201078e40 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -808,6 +808,16 @@  static int fsl_sai_probe(struct platform_device *pdev)
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
+	/*
+	 * New versions of the SAI have 2 32bit version registers added at the
+	 * start of the register map. To avoid dealing with this shift all over
+	 * the driver, we move the start of the regmap to make both register
+	 * layouts look the same to the regmap. This means we can't read the
+	 * version registers through the regmap, but this is small price to pay.
+	 */
+	if (sai->soc_data->has_version_registers)
+		base += 8;
+
 	sai->regmap = devm_regmap_init_mmio_clk(&pdev->dev,
 			"bus", base, &fsl_sai_regmap_config);
 
@@ -923,18 +933,27 @@  static int fsl_sai_remove(struct platform_device *pdev)
 
 static const struct fsl_sai_soc_data fsl_sai_vf610_data = {
 	.use_imx_pcm = false,
+	.has_version_registers = false,
 	.fifo_depth = 32,
 };
 
 static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = {
 	.use_imx_pcm = true,
+	.has_version_registers = false,
 	.fifo_depth = 32,
 };
 
+static const struct fsl_sai_soc_data fsl_sai_imx8m_data = {
+	.use_imx_pcm = true,
+	.has_version_registers = true,
+	.fifo_depth = 128,
+};
+
 static const struct of_device_id fsl_sai_ids[] = {
 	{ .compatible = "fsl,vf610-sai", .data = &fsl_sai_vf610_data },
 	{ .compatible = "fsl,imx6sx-sai", .data = &fsl_sai_imx6sx_data },
 	{ .compatible = "fsl,imx6ul-sai", .data = &fsl_sai_imx6sx_data },
+	{ .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8m_data },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, fsl_sai_ids);
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 7c1ef671da28..b6a9053ed400 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -128,6 +128,7 @@ 
 
 struct fsl_sai_soc_data {
 	bool use_imx_pcm;
+	bool has_version_registers;
 	unsigned int fifo_depth;
 };