| Message ID | 20190728192429.1514-2-daniel.baluta@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for new SAI IP version | expand |
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?
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.
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.
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?
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.
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.
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)
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(-)