[v2,1/7] ASoC: fsl_sai: Add registers definition for multiple datalines
diff mbox series

Message ID 20190728192429.1514-2-daniel.baluta@nxp.com
State New
Headers show
Series
  • Add support for new SAI IP version
Related show

Commit Message

Daniel Baluta July 28, 2019, 7:24 p.m. UTC
SAI IP supports up to 8 data lines. The configuration of
supported number of data lines is decided at SoC integration
time.

This patch adds definitions for all related data TX/RX registers:
	* TDR0..7, Transmit data register
	* TFR0..7, Transmit FIFO register
	* RDR0..7, Receive data register
	* RFR0..7, Receive FIFO register

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/fsl/fsl_sai.c | 76 +++++++++++++++++++++++++++++++++++------
 sound/soc/fsl/fsl_sai.h | 36 ++++++++++++++++---
 2 files changed, 98 insertions(+), 14 deletions(-)

Comments

Nicolin Chen July 29, 2019, 7:42 p.m. UTC | #1
On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:
> SAI IP supports up to 8 data lines. The configuration of
> supported number of data lines is decided at SoC integration
> time.
> 
> This patch adds definitions for all related data TX/RX registers:
> 	* TDR0..7, Transmit data register
> 	* TFR0..7, Transmit FIFO register
> 	* RDR0..7, Receive data register
> 	* RFR0..7, Receive FIFO register
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  sound/soc/fsl/fsl_sai.c | 76 +++++++++++++++++++++++++++++++++++------
>  sound/soc/fsl/fsl_sai.h | 36 ++++++++++++++++---
>  2 files changed, 98 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 6d3c6c8d50ce..17b0aff4ee8b 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c

> @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
>  	case FSL_SAI_TCR3:
>  	case FSL_SAI_TCR4:
>  	case FSL_SAI_TCR5:
> -	case FSL_SAI_TFR:
> +	case FSL_SAI_TFR0:
> +	case FSL_SAI_TFR1:
> +	case FSL_SAI_TFR2:
> +	case FSL_SAI_TFR3:
> +	case FSL_SAI_TFR4:
> +	case FSL_SAI_TFR5:
> +	case FSL_SAI_TFR6:
> +	case FSL_SAI_TFR7:
>  	case FSL_SAI_TMR:
>  	case FSL_SAI_RCSR:
>  	case FSL_SAI_RCR1:

A tricky thing here is that those SAI instances on older SoC don't
support multi data lines physically, while seemly having registers
pre-defined. So your change doesn't sound doing anything wrong to
them at all, I am still wondering if it is necessary to apply them
to newer compatible only though, as for older compatibles of SAI,
these registers would be useless and confusing if being exposed.

What do you think?
Daniel Baluta July 29, 2019, 7:57 p.m. UTC | #2
On Mon, Jul 29, 2019 at 10:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:
> > SAI IP supports up to 8 data lines. The configuration of
> > supported number of data lines is decided at SoC integration
> > time.
> >
> > This patch adds definitions for all related data TX/RX registers:
> >       * TDR0..7, Transmit data register
> >       * TFR0..7, Transmit FIFO register
> >       * RDR0..7, Receive data register
> >       * RFR0..7, Receive FIFO register
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  sound/soc/fsl/fsl_sai.c | 76 +++++++++++++++++++++++++++++++++++------
> >  sound/soc/fsl/fsl_sai.h | 36 ++++++++++++++++---
> >  2 files changed, 98 insertions(+), 14 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > index 6d3c6c8d50ce..17b0aff4ee8b 100644
> > --- a/sound/soc/fsl/fsl_sai.c
> > +++ b/sound/soc/fsl/fsl_sai.c
>
> > @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
> >       case FSL_SAI_TCR3:
> >       case FSL_SAI_TCR4:
> >       case FSL_SAI_TCR5:
> > -     case FSL_SAI_TFR:
> > +     case FSL_SAI_TFR0:
> > +     case FSL_SAI_TFR1:
> > +     case FSL_SAI_TFR2:
> > +     case FSL_SAI_TFR3:
> > +     case FSL_SAI_TFR4:
> > +     case FSL_SAI_TFR5:
> > +     case FSL_SAI_TFR6:
> > +     case FSL_SAI_TFR7:
> >       case FSL_SAI_TMR:
> >       case FSL_SAI_RCSR:
> >       case FSL_SAI_RCR1:
>
> A tricky thing here is that those SAI instances on older SoC don't
> support multi data lines physically, while seemly having registers
> pre-defined. So your change doesn't sound doing anything wrong to
> them at all, I am still wondering if it is necessary to apply them
> to newer compatible only though, as for older compatibles of SAI,
> these registers would be useless and confusing if being exposed.
>
> What do you think?

Yes, I thought about this too. But, I tried to keep the code as short
as possible and technically it is not wrong. When 1 data line is supported
for example application will only care about TDR0, TFR0, etc.
Mark Brown July 29, 2019, 8:20 p.m. UTC | #3
On Mon, Jul 29, 2019 at 10:57:43PM +0300, Daniel Baluta wrote:
> On Mon, Jul 29, 2019 at 10:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> > On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:

> > > @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
> > >       case FSL_SAI_TCR3:
> > >       case FSL_SAI_TCR4:
> > >       case FSL_SAI_TCR5:
> > > -     case FSL_SAI_TFR:
> > > +     case FSL_SAI_TFR0:

> > A tricky thing here is that those SAI instances on older SoC don't
> > support multi data lines physically, while seemly having registers
> > pre-defined. So your change doesn't sound doing anything wrong to
> > them at all, I am still wondering if it is necessary to apply them
> > to newer compatible only though, as for older compatibles of SAI,
> > these registers would be useless and confusing if being exposed.

> > What do you think?

> Yes, I thought about this too. But, I tried to keep the code as short
> as possible and technically it is not wrong. When 1 data line is supported
> for example application will only care about TDR0, TFR0, etc.

So long as it's safe to read the registers (you don't get a bus error or
anything) I'd say it's more trouble than it's worth to have separate
regmap configuations just for this.  The main reasons for restricting
readability are where there's physical problems with doing the reads or
to keep the size of the debugfs files under control for usability and
performance reasons.
Nicolin Chen July 30, 2019, 7:59 a.m. UTC | #4
On Mon, Jul 29, 2019 at 09:20:01PM +0100, Mark Brown wrote:
> On Mon, Jul 29, 2019 at 10:57:43PM +0300, Daniel Baluta wrote:
> > On Mon, Jul 29, 2019 at 10:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> > > On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:
> 
> > > > @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
> > > >       case FSL_SAI_TCR3:
> > > >       case FSL_SAI_TCR4:
> > > >       case FSL_SAI_TCR5:
> > > > -     case FSL_SAI_TFR:
> > > > +     case FSL_SAI_TFR0:
> 
> > > A tricky thing here is that those SAI instances on older SoC don't
> > > support multi data lines physically, while seemly having registers
> > > pre-defined. So your change doesn't sound doing anything wrong to
> > > them at all, I am still wondering if it is necessary to apply them
> > > to newer compatible only though, as for older compatibles of SAI,
> > > these registers would be useless and confusing if being exposed.
> 
> > > What do you think?
> 
> > Yes, I thought about this too. But, I tried to keep the code as short
> > as possible and technically it is not wrong. When 1 data line is supported
> > for example application will only care about TDR0, TFR0, etc.
> 
> So long as it's safe to read the registers (you don't get a bus error or
> anything) I'd say it's more trouble than it's worth to have separate
> regmap configuations just for this.  The main reasons for restricting
> readability are where there's physical problems with doing the reads or
> to keep the size of the debugfs files under control for usability and
> performance reasons.

Thanks for the input, Mark.

Daniel, did you get a chance to test it on older SoCs? At least
nothing breaks like bus errors?
Daniel Baluta Aug. 6, 2019, 11:15 a.m. UTC | #5
On Tue, Jul 30, 2019 at 10:59 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Mon, Jul 29, 2019 at 09:20:01PM +0100, Mark Brown wrote:
> > On Mon, Jul 29, 2019 at 10:57:43PM +0300, Daniel Baluta wrote:
> > > On Mon, Jul 29, 2019 at 10:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> > > > On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:
> >
> > > > > @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
> > > > >       case FSL_SAI_TCR3:
> > > > >       case FSL_SAI_TCR4:
> > > > >       case FSL_SAI_TCR5:
> > > > > -     case FSL_SAI_TFR:
> > > > > +     case FSL_SAI_TFR0:
> >
> > > > A tricky thing here is that those SAI instances on older SoC don't
> > > > support multi data lines physically, while seemly having registers
> > > > pre-defined. So your change doesn't sound doing anything wrong to
> > > > them at all, I am still wondering if it is necessary to apply them
> > > > to newer compatible only though, as for older compatibles of SAI,
> > > > these registers would be useless and confusing if being exposed.
> >
> > > > What do you think?
> >
> > > Yes, I thought about this too. But, I tried to keep the code as short
> > > as possible and technically it is not wrong. When 1 data line is supported
> > > for example application will only care about TDR0, TFR0, etc.
> >
> > So long as it's safe to read the registers (you don't get a bus error or
> > anything) I'd say it's more trouble than it's worth to have separate
> > regmap configuations just for this.  The main reasons for restricting
> > readability are where there's physical problems with doing the reads or
> > to keep the size of the debugfs files under control for usability and
> > performance reasons.
>
> Thanks for the input, Mark.
>
> Daniel, did you get a chance to test it on older SoCs? At least
> nothing breaks like bus errors?

Tested on imx6sx-sdb, everything looks good. No bus errors.
Nicolin Chen Aug. 7, 2019, 1:12 a.m. UTC | #6
On Tue, Aug 06, 2019 at 02:15:03PM +0300, Daniel Baluta wrote:
> On Tue, Jul 30, 2019 at 10:59 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> >
> > On Mon, Jul 29, 2019 at 09:20:01PM +0100, Mark Brown wrote:
> > > On Mon, Jul 29, 2019 at 10:57:43PM +0300, Daniel Baluta wrote:
> > > > On Mon, Jul 29, 2019 at 10:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> > > > > On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:
> > >
> > > > > > @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
> > > > > >       case FSL_SAI_TCR3:
> > > > > >       case FSL_SAI_TCR4:
> > > > > >       case FSL_SAI_TCR5:
> > > > > > -     case FSL_SAI_TFR:
> > > > > > +     case FSL_SAI_TFR0:
> > >
> > > > > A tricky thing here is that those SAI instances on older SoC don't
> > > > > support multi data lines physically, while seemly having registers
> > > > > pre-defined. So your change doesn't sound doing anything wrong to
> > > > > them at all, I am still wondering if it is necessary to apply them
> > > > > to newer compatible only though, as for older compatibles of SAI,
> > > > > these registers would be useless and confusing if being exposed.
> > >
> > > > > What do you think?
> > >
> > > > Yes, I thought about this too. But, I tried to keep the code as short
> > > > as possible and technically it is not wrong. When 1 data line is supported
> > > > for example application will only care about TDR0, TFR0, etc.
> > >
> > > So long as it's safe to read the registers (you don't get a bus error or
> > > anything) I'd say it's more trouble than it's worth to have separate
> > > regmap configuations just for this.  The main reasons for restricting
> > > readability are where there's physical problems with doing the reads or
> > > to keep the size of the debugfs files under control for usability and
> > > performance reasons.
> >
> > Thanks for the input, Mark.
> >
> > Daniel, did you get a chance to test it on older SoCs? At least
> > nothing breaks like bus errors?
> 
> Tested on imx6sx-sdb, everything looks good. No bus errors.

Okay. Let's just stick to it then. Thanks for the reply.

Patch
diff mbox series

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 6d3c6c8d50ce..17b0aff4ee8b 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -685,7 +685,14 @@  static struct reg_default fsl_sai_reg_defaults[] = {
 	{FSL_SAI_TCR3, 0},
 	{FSL_SAI_TCR4, 0},
 	{FSL_SAI_TCR5, 0},
-	{FSL_SAI_TDR,  0},
+	{FSL_SAI_TDR0, 0},
+	{FSL_SAI_TDR1, 0},
+	{FSL_SAI_TDR2, 0},
+	{FSL_SAI_TDR3, 0},
+	{FSL_SAI_TDR4, 0},
+	{FSL_SAI_TDR5, 0},
+	{FSL_SAI_TDR6, 0},
+	{FSL_SAI_TDR7, 0},
 	{FSL_SAI_TMR,  0},
 	{FSL_SAI_RCR1, 0},
 	{FSL_SAI_RCR2, 0},
@@ -704,7 +711,14 @@  static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
 	case FSL_SAI_TCR3:
 	case FSL_SAI_TCR4:
 	case FSL_SAI_TCR5:
-	case FSL_SAI_TFR:
+	case FSL_SAI_TFR0:
+	case FSL_SAI_TFR1:
+	case FSL_SAI_TFR2:
+	case FSL_SAI_TFR3:
+	case FSL_SAI_TFR4:
+	case FSL_SAI_TFR5:
+	case FSL_SAI_TFR6:
+	case FSL_SAI_TFR7:
 	case FSL_SAI_TMR:
 	case FSL_SAI_RCSR:
 	case FSL_SAI_RCR1:
@@ -712,8 +726,22 @@  static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
 	case FSL_SAI_RCR3:
 	case FSL_SAI_RCR4:
 	case FSL_SAI_RCR5:
-	case FSL_SAI_RDR:
-	case FSL_SAI_RFR:
+	case FSL_SAI_RDR0:
+	case FSL_SAI_RDR1:
+	case FSL_SAI_RDR2:
+	case FSL_SAI_RDR3:
+	case FSL_SAI_RDR4:
+	case FSL_SAI_RDR5:
+	case FSL_SAI_RDR6:
+	case FSL_SAI_RDR7:
+	case FSL_SAI_RFR0:
+	case FSL_SAI_RFR1:
+	case FSL_SAI_RFR2:
+	case FSL_SAI_RFR3:
+	case FSL_SAI_RFR4:
+	case FSL_SAI_RFR5:
+	case FSL_SAI_RFR6:
+	case FSL_SAI_RFR7:
 	case FSL_SAI_RMR:
 		return true;
 	default:
@@ -726,9 +754,30 @@  static bool fsl_sai_volatile_reg(struct device *dev, unsigned int reg)
 	switch (reg) {
 	case FSL_SAI_TCSR:
 	case FSL_SAI_RCSR:
-	case FSL_SAI_TFR:
-	case FSL_SAI_RFR:
-	case FSL_SAI_RDR:
+	case FSL_SAI_TFR0:
+	case FSL_SAI_TFR1:
+	case FSL_SAI_TFR2:
+	case FSL_SAI_TFR3:
+	case FSL_SAI_TFR4:
+	case FSL_SAI_TFR5:
+	case FSL_SAI_TFR6:
+	case FSL_SAI_TFR7:
+	case FSL_SAI_RFR0:
+	case FSL_SAI_RFR1:
+	case FSL_SAI_RFR2:
+	case FSL_SAI_RFR3:
+	case FSL_SAI_RFR4:
+	case FSL_SAI_RFR5:
+	case FSL_SAI_RFR6:
+	case FSL_SAI_RFR7:
+	case FSL_SAI_RDR0:
+	case FSL_SAI_RDR1:
+	case FSL_SAI_RDR2:
+	case FSL_SAI_RDR3:
+	case FSL_SAI_RDR4:
+	case FSL_SAI_RDR5:
+	case FSL_SAI_RDR6:
+	case FSL_SAI_RDR7:
 		return true;
 	default:
 		return false;
@@ -744,7 +793,14 @@  static bool fsl_sai_writeable_reg(struct device *dev, unsigned int reg)
 	case FSL_SAI_TCR3:
 	case FSL_SAI_TCR4:
 	case FSL_SAI_TCR5:
-	case FSL_SAI_TDR:
+	case FSL_SAI_TDR0:
+	case FSL_SAI_TDR1:
+	case FSL_SAI_TDR2:
+	case FSL_SAI_TDR3:
+	case FSL_SAI_TDR4:
+	case FSL_SAI_TDR5:
+	case FSL_SAI_TDR6:
+	case FSL_SAI_TDR7:
 	case FSL_SAI_TMR:
 	case FSL_SAI_RCSR:
 	case FSL_SAI_RCR1:
@@ -885,8 +941,8 @@  static int fsl_sai_probe(struct platform_device *pdev)
 				   MCLK_DIR(index));
 	}
 
-	sai->dma_params_rx.addr = res->start + FSL_SAI_RDR;
-	sai->dma_params_tx.addr = res->start + FSL_SAI_TDR;
+	sai->dma_params_rx.addr = res->start + FSL_SAI_RDR0;
+	sai->dma_params_tx.addr = res->start + FSL_SAI_TDR0;
 	sai->dma_params_rx.maxburst = FSL_SAI_MAXBURST_RX;
 	sai->dma_params_tx.maxburst = FSL_SAI_MAXBURST_TX;
 
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 7c1ef671da28..4bb478041d67 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -20,8 +20,22 @@ 
 #define FSL_SAI_TCR3	0x0c /* SAI Transmit Configuration 3 */
 #define FSL_SAI_TCR4	0x10 /* SAI Transmit Configuration 4 */
 #define FSL_SAI_TCR5	0x14 /* SAI Transmit Configuration 5 */
-#define FSL_SAI_TDR	0x20 /* SAI Transmit Data */
-#define FSL_SAI_TFR	0x40 /* SAI Transmit FIFO */
+#define FSL_SAI_TDR0	0x20 /* SAI Transmit Data 0 */
+#define FSL_SAI_TDR1	0x24 /* SAI Transmit Data 1 */
+#define FSL_SAI_TDR2	0x28 /* SAI Transmit Data 2 */
+#define FSL_SAI_TDR3	0x2C /* SAI Transmit Data 3 */
+#define FSL_SAI_TDR4	0x30 /* SAI Transmit Data 4 */
+#define FSL_SAI_TDR5	0x34 /* SAI Transmit Data 5 */
+#define FSL_SAI_TDR6	0x38 /* SAI Transmit Data 6 */
+#define FSL_SAI_TDR7	0x3C /* SAI Transmit Data 7 */
+#define FSL_SAI_TFR0	0x40 /* SAI Transmit FIFO 0 */
+#define FSL_SAI_TFR1	0x44 /* SAI Transmit FIFO 1 */
+#define FSL_SAI_TFR2	0x48 /* SAI Transmit FIFO 2 */
+#define FSL_SAI_TFR3	0x4C /* SAI Transmit FIFO 3 */
+#define FSL_SAI_TFR4	0x50 /* SAI Transmit FIFO 4 */
+#define FSL_SAI_TFR5	0x54 /* SAI Transmit FIFO 5 */
+#define FSL_SAI_TFR6	0x58 /* SAI Transmit FIFO 6 */
+#define FSL_SAI_TFR7	0x5C /* SAI Transmit FIFO 7 */
 #define FSL_SAI_TMR	0x60 /* SAI Transmit Mask */
 #define FSL_SAI_RCSR	0x80 /* SAI Receive Control */
 #define FSL_SAI_RCR1	0x84 /* SAI Receive Configuration 1 */
@@ -29,8 +43,22 @@ 
 #define FSL_SAI_RCR3	0x8c /* SAI Receive Configuration 3 */
 #define FSL_SAI_RCR4	0x90 /* SAI Receive Configuration 4 */
 #define FSL_SAI_RCR5	0x94 /* SAI Receive Configuration 5 */
-#define FSL_SAI_RDR	0xa0 /* SAI Receive Data */
-#define FSL_SAI_RFR	0xc0 /* SAI Receive FIFO */
+#define FSL_SAI_RDR0	0xa0 /* SAI Receive Data 0 */
+#define FSL_SAI_RDR1	0xa4 /* SAI Receive Data 1 */
+#define FSL_SAI_RDR2	0xa8 /* SAI Receive Data 2 */
+#define FSL_SAI_RDR3	0xac /* SAI Receive Data 3 */
+#define FSL_SAI_RDR4	0xb0 /* SAI Receive Data 4 */
+#define FSL_SAI_RDR5	0xb4 /* SAI Receive Data 5 */
+#define FSL_SAI_RDR6	0xb8 /* SAI Receive Data 6 */
+#define FSL_SAI_RDR7	0xbc /* SAI Receive Data 7 */
+#define FSL_SAI_RFR0	0xc0 /* SAI Receive FIFO 0 */
+#define FSL_SAI_RFR1	0xc4 /* SAI Receive FIFO 1 */
+#define FSL_SAI_RFR2	0xc8 /* SAI Receive FIFO 2 */
+#define FSL_SAI_RFR3	0xcc /* SAI Receive FIFO 3 */
+#define FSL_SAI_RFR4	0xd0 /* SAI Receive FIFO 4 */
+#define FSL_SAI_RFR5	0xd4 /* SAI Receive FIFO 5 */
+#define FSL_SAI_RFR6	0xd8 /* SAI Receive FIFO 6 */
+#define FSL_SAI_RFR7	0xdc /* SAI Receive FIFO 7 */
 #define FSL_SAI_RMR	0xe0 /* SAI Receive Mask */
 
 #define FSL_SAI_xCSR(tx)	(tx ? FSL_SAI_TCSR : FSL_SAI_RCSR)