| Message ID | 20190728192429.1514-1-daniel.baluta@nxp.com |
|---|---|
| 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 Sun, Jul 28, 2019 at 10:24:25PM +0300, Daniel Baluta wrote: > SAI supports up to 8 Rx/Tx data lines which can be enabled > using TCE/RCE bits of TCR3/RCR3 registers. > > Data lines to be enabled are read from DT fsl,dl-mask property. > By default (if no DT entry is provided) only data line 0 is enabled. > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > --- > sound/soc/fsl/fsl_sai.c | 11 ++++++++++- > sound/soc/fsl/fsl_sai.h | 4 +++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c > index 637b1d12a575..5e7cb7fd29f5 100644 > --- a/sound/soc/fsl/fsl_sai.c > +++ b/sound/soc/fsl/fsl_sai.c > @@ -601,7 +601,7 @@ static int fsl_sai_startup(struct snd_pcm_substream *substream, > > regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), > FSL_SAI_CR3_TRCE_MASK, > - FSL_SAI_CR3_TRCE); > + FSL_SAI_CR3_TRCE(sai->soc_data->dl_mask[tx]); > > ret = snd_pcm_hw_constraint_list(substream->runtime, 0, > SNDRV_PCM_HW_PARAM_RATE, &fsl_sai_rate_constraints); > @@ -888,6 +888,15 @@ static int fsl_sai_probe(struct platform_device *pdev) > } > } > > + /* > + * active data lines mask for TX/RX, defaults to 1 (only the first > + * data line is enabled > + */ > + sai->dl_mask[RX] = 1; > + sai->dl_mask[TX] = 1; > + of_property_read_u32_index(np, "fsl,dl-mask", RX, &sai->dl_mask[RX]); > + of_property_read_u32_index(np, "fsl,dl-mask", TX, &sai->dl_mask[TX]); Just curious what if we enable 8 data lines through DT bindings while an audio file only has 1 or 2 channels. Will TRCE bits be okay to stay with 8 data channels configurations? Btw, how does DMA work for the data registers? ESAI has one entry at a fixed address for all data channels while SAI seems to have different data registers.
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 Sun, Jul 28, 2019 at 10:24:28PM +0300, Daniel Baluta wrote: > SAI module on imx7ulp/imx8m features 2 new registers (VERID and PARAM) > at the beginning of register address space. > > On imx7ulp FIFOs can held up to 16 x 32 bit samples. > On imx8mq FIFOs can held up to 128 x 32 bit samples. > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> Acked-by: Nicolin Chen <nicoleotsuka@gmail.com> > --- > sound/soc/fsl/fsl_sai.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c > index 54e5e9abae01..0fb6750fefd5 100644 > --- a/sound/soc/fsl/fsl_sai.c > +++ b/sound/soc/fsl/fsl_sai.c > @@ -1030,10 +1030,24 @@ static const struct fsl_sai_soc_data fsl_sai_imx6sx_data = { > .reg_offset = 0, > }; > > +static const struct fsl_sai_soc_data fsl_sai_imx7ulp_data = { > + .use_imx_pcm = true, > + .fifo_depth = 16, > + .reg_offset = 8, > +}; > + > +static const struct fsl_sai_soc_data fsl_sai_imx8mq_data = { > + .use_imx_pcm = true, > + .fifo_depth = 128, > + .reg_offset = 8, > +}; > + > 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,imx7ulp-sai", .data = &fsl_sai_imx7ulp_data }, > + { .compatible = "fsl,imx8mq-sai", .data = &fsl_sai_imx8mq_data }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, fsl_sai_ids); > -- > 2.17.1 >
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 Mon, Jul 29, 2019 at 11:22 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote: > > On Sun, Jul 28, 2019 at 10:24:25PM +0300, Daniel Baluta wrote: > > SAI supports up to 8 Rx/Tx data lines which can be enabled > > using TCE/RCE bits of TCR3/RCR3 registers. > > > > Data lines to be enabled are read from DT fsl,dl-mask property. > > By default (if no DT entry is provided) only data line 0 is enabled. > > > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > > --- > > sound/soc/fsl/fsl_sai.c | 11 ++++++++++- > > sound/soc/fsl/fsl_sai.h | 4 +++- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c > > index 637b1d12a575..5e7cb7fd29f5 100644 > > --- a/sound/soc/fsl/fsl_sai.c > > +++ b/sound/soc/fsl/fsl_sai.c > > @@ -601,7 +601,7 @@ static int fsl_sai_startup(struct snd_pcm_substream *substream, > > > > regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), > > FSL_SAI_CR3_TRCE_MASK, > > - FSL_SAI_CR3_TRCE); > > + FSL_SAI_CR3_TRCE(sai->soc_data->dl_mask[tx]); > > > > ret = snd_pcm_hw_constraint_list(substream->runtime, 0, > > SNDRV_PCM_HW_PARAM_RATE, &fsl_sai_rate_constraints); > > @@ -888,6 +888,15 @@ static int fsl_sai_probe(struct platform_device *pdev) > > } > > } > > > > + /* > > + * active data lines mask for TX/RX, defaults to 1 (only the first > > + * data line is enabled > > + */ > > + sai->dl_mask[RX] = 1; > > + sai->dl_mask[TX] = 1; > > + of_property_read_u32_index(np, "fsl,dl-mask", RX, &sai->dl_mask[RX]); > > + of_property_read_u32_index(np, "fsl,dl-mask", TX, &sai->dl_mask[TX]); > > Just curious what if we enable 8 data lines through DT bindings > while an audio file only has 1 or 2 channels. Will TRCE bits be > okay to stay with 8 data channels configurations? Btw, how does > DMA work for the data registers? ESAI has one entry at a fixed > address for all data channels while SAI seems to have different > data registers. Hi Nicolin, I have sent v3 and removed this patch from the series because we need to find a better solution. I think we should enable TCE based on the number of available datalines and the number of active channels. Will come with a RFC patch later. Pasting here the reply of SAI Audio IP owner regarding to your question above, just for anyone to have more info of our private discussion: If all 8 datalines are enabled using TCE then the transmit FIFO for all 8 datalines need to be serviced, otherwise a FIFO underrun will be generated. Each dataline has a separate transmit FIFO with a separate register to service the FIFO, so each dataline can be serviced separately. Note that configuring FCOMB=2 would make it look like ESAI with a common address for all FIFOs. When performing DMA transfers to multiple datalines, there are a couple of options: * Use 1 DMA channel to copy first slot for each dataline to each FIFO and then update the destination address back to the first register. * Configure separate DMA channel for each dataline and trigger the first one by the DMA request and the subsequent channels by DMA linking or scatter/gather. * Configure FCOMB=2 and treat it the same as the ESAI. This is almost the same as 1, but don’t need to update the destination address. Thanks, Daniel.
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.
On Tue, Aug 06, 2019 at 06:23:27PM +0300, Daniel Baluta wrote: > On Mon, Jul 29, 2019 at 11:22 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote: > > > > On Sun, Jul 28, 2019 at 10:24:25PM +0300, Daniel Baluta wrote: > > > SAI supports up to 8 Rx/Tx data lines which can be enabled > > > using TCE/RCE bits of TCR3/RCR3 registers. > > > > > > Data lines to be enabled are read from DT fsl,dl-mask property. > > > By default (if no DT entry is provided) only data line 0 is enabled. > > > > > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > > > --- > > > sound/soc/fsl/fsl_sai.c | 11 ++++++++++- > > > sound/soc/fsl/fsl_sai.h | 4 +++- > > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c > > > index 637b1d12a575..5e7cb7fd29f5 100644 > > > --- a/sound/soc/fsl/fsl_sai.c > > > +++ b/sound/soc/fsl/fsl_sai.c > > > @@ -601,7 +601,7 @@ static int fsl_sai_startup(struct snd_pcm_substream *substream, > > > > > > regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), > > > FSL_SAI_CR3_TRCE_MASK, > > > - FSL_SAI_CR3_TRCE); > > > + FSL_SAI_CR3_TRCE(sai->soc_data->dl_mask[tx]); > > > > > > ret = snd_pcm_hw_constraint_list(substream->runtime, 0, > > > SNDRV_PCM_HW_PARAM_RATE, &fsl_sai_rate_constraints); > > > @@ -888,6 +888,15 @@ static int fsl_sai_probe(struct platform_device *pdev) > > > } > > > } > > > > > > + /* > > > + * active data lines mask for TX/RX, defaults to 1 (only the first > > > + * data line is enabled > > > + */ > > > + sai->dl_mask[RX] = 1; > > > + sai->dl_mask[TX] = 1; > > > + of_property_read_u32_index(np, "fsl,dl-mask", RX, &sai->dl_mask[RX]); > > > + of_property_read_u32_index(np, "fsl,dl-mask", TX, &sai->dl_mask[TX]); > > > > Just curious what if we enable 8 data lines through DT bindings > > while an audio file only has 1 or 2 channels. Will TRCE bits be > > okay to stay with 8 data channels configurations? Btw, how does > > DMA work for the data registers? ESAI has one entry at a fixed > > address for all data channels while SAI seems to have different > > data registers. > > Hi Nicolin, > > I have sent v3 and removed this patch from the series because we > need to find a better solution. Ack. I was in that private mail thread. So it's totally fine. > > I think we should enable TCE based on the number of available datalines > and the number of active channels. Will come with a RFC patch later. Yea, that's exactly what I suspected during patch review and what I suggested previously too. Look forward to your patch. > Pasting here the reply of SAI Audio IP owner regarding to your question above, > just for anyone to have more info of our private discussion: > > If all 8 datalines are enabled using TCE then the transmit FIFO for > all 8 datalines need to be serviced, otherwise a FIFO underrun will be > generated. > Each dataline has a separate transmit FIFO with a separate register to > service the FIFO, so each dataline can be serviced separately. Note > that configuring FCOMB=2 would make it look like ESAI with a common > address for all FIFOs. > When performing DMA transfers to multiple datalines, there are a > couple of options: > * Use 1 DMA channel to copy first slot for each dataline to each > FIFO and then update the destination address back to the first > register. > * Configure separate DMA channel for each dataline and trigger the > first one by the DMA request and the subsequent channels by DMA > linking or scatter/gather. > * Configure FCOMB=2 and treat it the same as the ESAI. This is > almost the same as 1, but don’t need to update the destination > address. > > Thanks, > Daniel.