diff mbox

[1/2,RFC] dmaengine: rcar-audmapp: calculate chcr via src/dst addr

Message ID 8761bqtm34.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kuninori Morimoto Jan. 29, 2015, 1:24 a.m. UTC
Current rcar-audmapp is assuming that CHCR value are specified
from platform data or DTS bindings. but, it is possible to
calculate CHCR settings from src/dst address.
DTS bindings node can be reduced by this patch.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/dma/sh/rcar-audmapp.c |  136 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 124 insertions(+), 12 deletions(-)

Comments

Arnd Bergmann Jan. 29, 2015, 9:15 a.m. UTC | #1
On Thursday 29 January 2015 01:24:06 Kuninori Morimoto wrote:
> Current rcar-audmapp is assuming that CHCR value are specified
> from platform data or DTS bindings. but, it is possible to
> calculate CHCR settings from src/dst address.
> DTS bindings node can be reduced by this patch.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

The intention definitely looks good, but I'm worried that the
table you add here might be too SoC specific.

> +struct id_addr_table {
> +	u8  id;
> +	u16 addr;
> +};
> +
> +static u32 audmapp_addr_to_id(struct device *dev, u32 addr)
> +{
> +	struct id_addr_table ssi_id_table[] = {
> +		{0x00, 0x0000}, /* SSI00 */
> +		{0x01, 0x0400}, /* SSI01 */
> +		{0x02, 0x0800}, /* SSI02 */
> +		{0x03, 0x0C00}, /* SSI03 */
> +		{0x04, 0x1000}, /* SSI10 */
> +		{0x05, 0x1400}, /* SSI11 */
> +		{0x06, 0x1800}, /* SSI12 */
> +		{0x07, 0x1C00}, /* SSI13 */
> +		{0x08, 0x2000}, /* SSI20 */

Isn't the address part here the physical address of the FIFO,
which can be different for each implementation that contains
an audmapp device?

	Arnd
Geert Uytterhoeven Jan. 29, 2015, 9:33 a.m. UTC | #2
On Thu, Jan 29, 2015 at 10:15 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 29 January 2015 01:24:06 Kuninori Morimoto wrote:
>> Current rcar-audmapp is assuming that CHCR value are specified
>> from platform data or DTS bindings. but, it is possible to
>> calculate CHCR settings from src/dst address.
>> DTS bindings node can be reduced by this patch.
>>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> The intention definitely looks good, but I'm worried that the
> table you add here might be too SoC specific.
>
>> +struct id_addr_table {
>> +     u8  id;
>> +     u16 addr;
>> +};
>> +
>> +static u32 audmapp_addr_to_id(struct device *dev, u32 addr)
>> +{
>> +     struct id_addr_table ssi_id_table[] = {

static const, so they don't get copied to the stack on every invocation.

>> +             {0x00, 0x0000}, /* SSI00 */
>> +             {0x01, 0x0400}, /* SSI01 */
>> +             {0x02, 0x0800}, /* SSI02 */
>> +             {0x03, 0x0C00}, /* SSI03 */
>> +             {0x04, 0x1000}, /* SSI10 */
>> +             {0x05, 0x1400}, /* SSI11 */
>> +             {0x06, 0x1800}, /* SSI12 */
>> +             {0x07, 0x1C00}, /* SSI13 */
>> +             {0x08, 0x2000}, /* SSI20 */
>
> Isn't the address part here the physical address of the FIFO,

Yes they are.

> which can be different for each implementation that contains
> an audmapp device?

Fortunately they're identical for all (current) members of the R-Car Gen2
family.

Morimoto-san: Are the base addresses configured in DT?
I did a quick search for 0xec300000, 0xec320000, 0xec40000,
0xec420000, and 0xec000000,  and couldn't find them in DT, only in
source code (comments). They must come from somewhere?
Note that I didn't follow the sound binding discussions that closely.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Kuninori Morimoto Jan. 29, 2015, 9:41 a.m. UTC | #3
Hi xxx

Hi Arnd

> > Current rcar-audmapp is assuming that CHCR value are specified
> > from platform data or DTS bindings. but, it is possible to
> > calculate CHCR settings from src/dst address.
> > DTS bindings node can be reduced by this patch.
> > 
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> The intention definitely looks good, but I'm worried that the
> table you add here might be too SoC specific.

Thank you

> > +struct id_addr_table {
> > +	u8  id;
> > +	u16 addr;
> > +};
> > +
> > +static u32 audmapp_addr_to_id(struct device *dev, u32 addr)
> > +{
> > +	struct id_addr_table ssi_id_table[] = {
> > +		{0x00, 0x0000}, /* SSI00 */
> > +		{0x01, 0x0400}, /* SSI01 */
> > +		{0x02, 0x0800}, /* SSI02 */
> > +		{0x03, 0x0C00}, /* SSI03 */
> > +		{0x04, 0x1000}, /* SSI10 */
> > +		{0x05, 0x1400}, /* SSI11 */
> > +		{0x06, 0x1800}, /* SSI12 */
> > +		{0x07, 0x1C00}, /* SSI13 */
> > +		{0x08, 0x2000}, /* SSI20 */
> 
> Isn't the address part here the physical address of the FIFO,
> which can be different for each implementation that contains
> an audmapp device?

These are IN/OUT physical address,
and hard coded / specified for audmapp.
Other address is not supported. 

ex) 
         audmapp
  [SRCx] --------> [SSIx]
     out          in
   address      address

Best regards
---
Kuninori Morimoto
Kuninori Morimoto Jan. 29, 2015, 9:45 a.m. UTC | #4
Hi Geert

> >> +static u32 audmapp_addr_to_id(struct device *dev, u32 addr)
> >> +{
> >> +     struct id_addr_table ssi_id_table[] = {
> 
> static const, so they don't get copied to the stack on every invocation.

Indeed, thanks

> >> +             {0x00, 0x0000}, /* SSI00 */
> >> +             {0x01, 0x0400}, /* SSI01 */
> >> +             {0x02, 0x0800}, /* SSI02 */
> >> +             {0x03, 0x0C00}, /* SSI03 */
> >> +             {0x04, 0x1000}, /* SSI10 */
> >> +             {0x05, 0x1400}, /* SSI11 */
> >> +             {0x06, 0x1800}, /* SSI12 */
> >> +             {0x07, 0x1C00}, /* SSI13 */
> >> +             {0x08, 0x2000}, /* SSI20 */
> >
> > Isn't the address part here the physical address of the FIFO,
> 
> Yes they are.
> 
> > which can be different for each implementation that contains
> > an audmapp device?
> 
> Fortunately they're identical for all (current) members of the R-Car Gen2
> family.
> 
> Morimoto-san: Are the base addresses configured in DT?
> I did a quick search for 0xec300000, 0xec320000, 0xec40000,
> 0xec420000, and 0xec000000,  and couldn't find them in DT, only in
> source code (comments). They must come from somewhere?
> Note that I didn't follow the sound binding discussions that closely.

This base addresses are specified from sound driver.
 - sound driver specifies src/dst address by DMAEngine API
 - audmapp find necessary ID from src/dst address

Best regards
---
Kuninori Morimoto
Geert Uytterhoeven Jan. 29, 2015, 9:50 a.m. UTC | #5
Hi Morimoto-san,

On Thu, Jan 29, 2015 at 10:45 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>> Morimoto-san: Are the base addresses configured in DT?
>> I did a quick search for 0xec300000, 0xec320000, 0xec40000,
>> 0xec420000, and 0xec000000,  and couldn't find them in DT, only in
>> source code (comments). They must come from somewhere?
>> Note that I didn't follow the sound binding discussions that closely.
>
> This base addresses are specified from sound driver.
>  - sound driver specifies src/dst address by DMAEngine API

From the rcar_sound node? That one has 0xec500000 as the base address.

>  - audmapp find necessary ID from src/dst address

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Kuninori Morimoto Jan. 30, 2015, 12:27 a.m. UTC | #6
Hi Geert

Thank you for your feedback

> >> Morimoto-san: Are the base addresses configured in DT?
> >> I did a quick search for 0xec300000, 0xec320000, 0xec40000,
> >> 0xec420000, and 0xec000000,  and couldn't find them in DT, only in
> >> source code (comments). They must come from somewhere?
> >> Note that I didn't follow the sound binding discussions that closely.
> >
> > This base addresses are specified from sound driver.
> >  - sound driver specifies src/dst address by DMAEngine API
> 
> From the rcar_sound node? That one has 0xec500000 as the base address.

Thank you. I double checked, and remembered.
This is a littile bit confusable.

FIFO address 1) for CPU 2) for Audio DMAC 3) for Audio DMAC peri peri are different
Sound driver is understanding all addresses.
But, hmm..  no one call request_region() for these, is this your concern ?

Best regards
---
Kuninori Morimoto
Kuninori Morimoto Jan. 30, 2015, 1:17 a.m. UTC | #7
Hi Geert, again

> > >> Morimoto-san: Are the base addresses configured in DT?
> > >> I did a quick search for 0xec300000, 0xec320000, 0xec40000,
> > >> 0xec420000, and 0xec000000,  and couldn't find them in DT, only in
> > >> source code (comments). They must come from somewhere?
> > >> Note that I didn't follow the sound binding discussions that closely.
> > >
> > > This base addresses are specified from sound driver.
> > >  - sound driver specifies src/dst address by DMAEngine API
> > 
> > From the rcar_sound node? That one has 0xec500000 as the base address.
> 
> Thank you. I double checked, and remembered.
> This is a littile bit confusable.
> 
> FIFO address 1) for CPU 2) for Audio DMAC 3) for Audio DMAC peri peri are different
> Sound driver is understanding all addresses.
> But, hmm..  no one call request_region() for these, is this your concern ?

Sorry for my confusable mail.
Yes, I need to add these address from DT. I fixup it in v2

0xec420000, 0xec320000 are listed on this patch, but these are not supported yet.
diff mbox

Patch

diff --git a/drivers/dma/sh/rcar-audmapp.c b/drivers/dma/sh/rcar-audmapp.c
index d95bbdd..9690291 100644
--- a/drivers/dma/sh/rcar-audmapp.c
+++ b/drivers/dma/sh/rcar-audmapp.c
@@ -48,7 +48,6 @@  struct audmapp_chan {
 	struct shdma_chan shdma_chan;
 	void __iomem *base;
 	dma_addr_t slave_addr;
-	u32 chcr;
 };
 
 struct audmapp_device {
@@ -100,6 +99,124 @@  static void audmapp_halt(struct shdma_chan *schan)
 	}
 }
 
+struct id_addr_table {
+	u8  id;
+	u16 addr;
+};
+
+static u32 audmapp_addr_to_id(struct device *dev, u32 addr)
+{
+	struct id_addr_table ssi_id_table[] = {
+		{0x00, 0x0000}, /* SSI00 */
+		{0x01, 0x0400}, /* SSI01 */
+		{0x02, 0x0800}, /* SSI02 */
+		{0x03, 0x0C00}, /* SSI03 */
+		{0x04, 0x1000}, /* SSI10 */
+		{0x05, 0x1400}, /* SSI11 */
+		{0x06, 0x1800}, /* SSI12 */
+		{0x07, 0x1C00}, /* SSI13 */
+		{0x08, 0x2000}, /* SSI20 */
+		{0x09, 0x2400}, /* SSI21 */
+		{0x0a, 0x2800}, /* SSI22 */
+		{0x0b, 0x2C00}, /* SSI23 */
+		{0x0c, 0x3000}, /* SSI3  */
+		{0x0d, 0x4000}, /* SSI4  */
+		{0x0e, 0x5000}, /* SSI5  */
+		{0x0f, 0x6000}, /* SSI6  */
+		{0x10, 0x7000}, /* SSI7  */
+		{0x11, 0x8000}, /* SSI8  */
+		{0x12, 0x9000}, /* SSI90 */
+		{0x13, 0x9400}, /* SSI91 */
+		{0x14, 0x9800}, /* SSI92 */
+		{0x15, 0x9C00}, /* SSI93 */
+	};
+	struct id_addr_table dtcp_id_tabel[] = {
+		{0x16, 0x0000}, /* DTCPPP0 */
+		{0x17, 0x0400}, /* DTCPPP1 */
+		{0x18, 0x4000}, /* DTCPCP0 */
+		{0x19, 0x4400}, /* DTCPCP1 */
+	};
+	struct id_addr_table mlm_id_table[] = {
+		{0x25, 0x0000}, /* MLM0 */
+		{0x26, 0x0400}, /* MLM1 */
+		{0x27, 0x0800}, /* MLM2 */
+		{0x28, 0x0C00}, /* MLM3 */
+		{0x29, 0x1000}, /* MLM4 */
+		{0x2a, 0x1400}, /* MLM5 */
+		{0x2b, 0x1800}, /* MLM6 */
+		{0x2c, 0x1C00}, /* MLM7 */
+	};
+	struct id_addr_table scu_id_table[] = {
+		{0x2d, 0x0000}, /* SCU_SRCI0 */
+		{0x2e, 0x0400}, /* SCU_SRCI1 */
+		{0x2f, 0x0800}, /* SCU_SRCI2 */
+		{0x30, 0x0C00}, /* SCU_SRCI3 */
+		{0x31, 0x1000}, /* SCU_SRCI4 */
+		{0x32, 0x1400}, /* SCU_SRCI5 */
+		{0x33, 0x1800}, /* SCU_SRCI6 */
+		{0x34, 0x1C00}, /* SCU_SRCI7 */
+		{0x35, 0x2000}, /* SCU_SRCI8 */
+		{0x36, 0x2400}, /* SCU_SRCI9 */
+
+		{0x2d, 0x4000}, /* SCU_SRCO0 */
+		{0x2e, 0x4400}, /* SCU_SRCO1 */
+		{0x2f, 0x4800}, /* SCU_SRCO2 */
+		{0x30, 0x4C00}, /* SCU_SRCO3 */
+		{0x31, 0x5000}, /* SCU_SRCO4 */
+		{0x32, 0x5400}, /* SCU_SRCO5 */
+		{0x33, 0x5800}, /* SCU_SRCO6 */
+		{0x34, 0x5C00}, /* SCU_SRCO7 */
+		{0x35, 0x6000}, /* SCU_SRCO8 */
+		{0x36, 0x6400}, /* SCU_SRCO9 */
+		{0x37, 0x8000}, /* SCU_CMD0 */
+		{0x38, 0x8400}, /* SCU_CMD1 */
+	};
+	struct id_addr_table *table = NULL;
+	int size;
+	int i;
+
+	switch (addr >> 16) {
+	case 0xEC40:	/* SSI */
+		table = ssi_id_table;
+		size = ARRAY_SIZE(ssi_id_table);
+		break;
+	case 0xEC42:	/* DTCP */
+		table = dtcp_id_tabel;
+		size = ARRAY_SIZE(dtcp_id_tabel);
+		break;
+	case 0xEC32:	/* MLM */
+		table = mlm_id_table;
+		size = ARRAY_SIZE(mlm_id_table);
+		break;
+	case 0xEC30:	/* SRC / CMD */
+		table = scu_id_table;
+		size = ARRAY_SIZE(scu_id_table);
+		break;
+	default:
+		table = NULL;
+		size = 0;
+	}
+
+	for (i = 0; i < size; i++) {
+		if (table[i].addr == (addr & 0xFFFF))
+			return table[i].id;
+	}
+
+	dev_err(dev, "unknown addr (%x)\n", addr);
+
+	return 0xFF;
+}
+
+static u32 audmapp_get_chcr(struct device *dev, struct audmapp_desc *desc)
+{
+	u32 src_id, dst_id;
+
+	src_id = audmapp_addr_to_id(dev, desc->src);
+	dst_id = audmapp_addr_to_id(dev, desc->dst);
+
+	return src_id << 24 | dst_id << 16;
+}
+
 static void audmapp_start_xfer(struct shdma_chan *schan,
 			       struct shdma_desc *sdesc)
 {
@@ -107,7 +224,7 @@  static void audmapp_start_xfer(struct shdma_chan *schan,
 	struct audmapp_device *audev = to_dev(auchan);
 	struct audmapp_desc *desc = to_desc(sdesc);
 	struct device *dev = audev->dev;
-	u32 chcr = auchan->chcr | PDMACHCR_DE;
+	u32 chcr = audmapp_get_chcr(dev, desc) | PDMACHCR_DE;
 
 	dev_dbg(dev, "src/dst/chcr = %pad/%pad/%08x\n",
 		&desc->src, &desc->dst, chcr);
@@ -118,19 +235,17 @@  static void audmapp_start_xfer(struct shdma_chan *schan,
 }
 
 static int audmapp_get_config(struct audmapp_chan *auchan, int slave_id,
-			      u32 *chcr, dma_addr_t *dst)
+			      dma_addr_t *dst)
 {
 	struct audmapp_device *audev = to_dev(auchan);
 	struct audmapp_pdata *pdata = audev->pdata;
 	struct audmapp_slave_config *cfg;
 	int i;
 
-	*chcr	= 0;
 	*dst	= 0;
 
 	if (!pdata) { /* DT */
-		*chcr = ((u32)slave_id) << 16;
-		auchan->shdma_chan.slave_id = (slave_id) >> 8;
+		auchan->shdma_chan.slave_id = slave_id;
 		return 0;
 	}
 
@@ -141,7 +256,6 @@  static int audmapp_get_config(struct audmapp_chan *auchan, int slave_id,
 
 	for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
 		if (cfg->slave_id == slave_id) {
-			*chcr	= cfg->chcr;
 			*dst	= cfg->dst;
 			return 0;
 		}
@@ -153,18 +267,16 @@  static int audmapp_set_slave(struct shdma_chan *schan, int slave_id,
 			     dma_addr_t slave_addr, bool try)
 {
 	struct audmapp_chan *auchan = to_chan(schan);
-	u32 chcr;
 	dma_addr_t dst;
 	int ret;
 
-	ret = audmapp_get_config(auchan, slave_id, &chcr, &dst);
+	ret = audmapp_get_config(auchan, slave_id, &dst);
 	if (ret < 0)
 		return ret;
 
 	if (try)
 		return 0;
 
-	auchan->chcr		= chcr;
 	auchan->slave_addr	= slave_addr ? : dst;
 
 	return 0;
@@ -267,7 +379,7 @@  static struct dma_chan *audmapp_of_xlate(struct of_phandle_args *dma_spec,
 {
 	dma_cap_mask_t mask;
 	struct dma_chan *chan;
-	u32 chcr = dma_spec->args[0];
+	u32 id = dma_spec->args[0];
 
 	if (dma_spec->args_count != 1)
 		return NULL;
@@ -277,7 +389,7 @@  static struct dma_chan *audmapp_of_xlate(struct of_phandle_args *dma_spec,
 
 	chan = dma_request_channel(mask, shdma_chan_filter, NULL);
 	if (chan)
-		to_shdma_chan(chan)->hw_req = chcr;
+		to_shdma_chan(chan)->hw_req = id;
 
 	return chan;
 }