[PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Laurent Pinchart]

The hardware registers are accessed from atomic contexts (the
rsnd_soc_dai_trigger function, for instance, is called with the PCM
substream spinlock held). They thus can't be protected by a mutex.

Protect regmap register accesses with a spinlock instead of a mutex by
setting the fast_io flag.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 sound/soc/sh/rcar/gen.c | 1 +
 1 file changed, 1 insertion(+)

An even better solution might be to use regmap-mmio instead of a custom bus.
Morimoto-san, is there anything that would prevent the driver from switching
to regmap-mmio ?

diff --git a/sound/soc/sh/rcar/gen.c b/sound/soc/sh/rcar/gen.c
index 73ce4c9..ec9ac5e 100644
--- a/sound/soc/sh/rcar/gen.c
+++ b/sound/soc/sh/rcar/gen.c
@@ -66,6 +66,7 @@ static int rsnd_regmap_read32(void *context,
 }
 
 static struct regmap_bus rsnd_regmap_bus = {
+	.fast_io			= true,
 	.write				= rsnd_regmap_write32,
 	.read				= rsnd_regmap_read32,
 	.reg_format_endian_default	= REGMAP_ENDIAN_NATIVE,
-- 
Regards,

Laurent Pinchart

[PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Laurent Pinchart]

The hardware registers are accessed from atomic contexts (the
rsnd_soc_dai_trigger function, for instance, is called with the PCM
substream spinlock held). They thus can't be protected by a mutex.

Protect regmap register accesses with a spinlock instead of a mutex by
setting the fast_io flag.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 sound/soc/sh/rcar/gen.c | 1 +
 1 file changed, 1 insertion(+)

An even better solution might be to use regmap-mmio instead of a custom bus.
Morimoto-san, is there anything that would prevent the driver from switching
to regmap-mmio ?

diff --git a/sound/soc/sh/rcar/gen.c b/sound/soc/sh/rcar/gen.c
index 73ce4c9..ec9ac5e 100644
--- a/sound/soc/sh/rcar/gen.c
+++ b/sound/soc/sh/rcar/gen.c
@@ -66,6 +66,7 @@ static int rsnd_regmap_read32(void *context,
 }
 
 static struct regmap_bus rsnd_regmap_bus = {
+	.fast_io			= true,
 	.write				= rsnd_regmap_write32,
 	.read				= rsnd_regmap_read32,
 	.reg_format_endian_default	= REGMAP_ENDIAN_NATIVE,
-- 
Regards,

Laurent Pinchart

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Kuninori Morimoto]

Hi Laurent

> The hardware registers are accessed from atomic contexts (the
> rsnd_soc_dai_trigger function, for instance, is called with the PCM
> substream spinlock held). They thus can't be protected by a mutex.
> 
> Protect regmap register accesses with a spinlock instead of a mutex by
> setting the fast_io flag.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  sound/soc/sh/rcar/gen.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> An even better solution might be to use regmap-mmio instead of a custom bus.
> Morimoto-san, is there anything that would prevent the driver from switching
> to regmap-mmio ?

I guess it is possilbe to use regmap-mmio.
I check it.

> 
> diff --git a/sound/soc/sh/rcar/gen.c b/sound/soc/sh/rcar/gen.c
> index 73ce4c9..ec9ac5e 100644
> --- a/sound/soc/sh/rcar/gen.c
> +++ b/sound/soc/sh/rcar/gen.c
> @@ -66,6 +66,7 @@ static int rsnd_regmap_read32(void *context,
>  }
>  
>  static struct regmap_bus rsnd_regmap_bus = {
> +	.fast_io			= true,
>  	.write				= rsnd_regmap_write32,
>  	.read				= rsnd_regmap_read32,
>  	.reg_format_endian_default	= REGMAP_ENDIAN_NATIVE,
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Kuninori Morimoto]

Hi Laurent

> The hardware registers are accessed from atomic contexts (the
> rsnd_soc_dai_trigger function, for instance, is called with the PCM
> substream spinlock held). They thus can't be protected by a mutex.
> 
> Protect regmap register accesses with a spinlock instead of a mutex by
> setting the fast_io flag.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  sound/soc/sh/rcar/gen.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> An even better solution might be to use regmap-mmio instead of a custom bus.
> Morimoto-san, is there anything that would prevent the driver from switching
> to regmap-mmio ?

I guess it is possilbe to use regmap-mmio.
I check it.

> 
> diff --git a/sound/soc/sh/rcar/gen.c b/sound/soc/sh/rcar/gen.c
> index 73ce4c9..ec9ac5e 100644
> --- a/sound/soc/sh/rcar/gen.c
> +++ b/sound/soc/sh/rcar/gen.c
> @@ -66,6 +66,7 @@ static int rsnd_regmap_read32(void *context,
>  }
>  
>  static struct regmap_bus rsnd_regmap_bus = {
> +	.fast_io			= true,
>  	.write				= rsnd_regmap_write32,
>  	.read				= rsnd_regmap_read32,
>  	.reg_format_endian_default	= REGMAP_ENDIAN_NATIVE,
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Kuninori Morimoto]

Hi Laurent

> > The hardware registers are accessed from atomic contexts (the
> > rsnd_soc_dai_trigger function, for instance, is called with the PCM
> > substream spinlock held). They thus can't be protected by a mutex.
> > 
> > Protect regmap register accesses with a spinlock instead of a mutex by
> > setting the fast_io flag.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  sound/soc/sh/rcar/gen.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > An even better solution might be to use regmap-mmio instead of a custom bus.
> > Morimoto-san, is there anything that would prevent the driver from switching
> > to regmap-mmio ?
> 
> I guess it is possilbe to use regmap-mmio.
> I check it.

Hmm... rsnd driver is using "regmap_field".
regmap-mmio  requests "offset"  on "reg"
regmap-filed requests "address" on "reg"

So, if rsnd driver uses regmap-mmio,
then, it needs tricky initialize like...

      regmap_init_mmio(dev, 0, config)

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Kuninori Morimoto]

Hi Laurent

> > The hardware registers are accessed from atomic contexts (the
> > rsnd_soc_dai_trigger function, for instance, is called with the PCM
> > substream spinlock held). They thus can't be protected by a mutex.
> > 
> > Protect regmap register accesses with a spinlock instead of a mutex by
> > setting the fast_io flag.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  sound/soc/sh/rcar/gen.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > An even better solution might be to use regmap-mmio instead of a custom bus.
> > Morimoto-san, is there anything that would prevent the driver from switching
> > to regmap-mmio ?
> 
> I guess it is possilbe to use regmap-mmio.
> I check it.

Hmm... rsnd driver is using "regmap_field".
regmap-mmio  requests "offset"  on "reg"
regmap-filed requests "address" on "reg"

So, if rsnd driver uses regmap-mmio,
then, it needs tricky initialize like...

      regmap_init_mmio(dev, 0, config)

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Lars-Peter Clausen]

On 07/24/2014 04:16 AM, Kuninori Morimoto wrote:
>
> Hi Laurent
>
>>> The hardware registers are accessed from atomic contexts (the
>>> rsnd_soc_dai_trigger function, for instance, is called with the PCM
>>> substream spinlock held). They thus can't be protected by a mutex.
>>>
>>> Protect regmap register accesses with a spinlock instead of a mutex by
>>> setting the fast_io flag.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>   sound/soc/sh/rcar/gen.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> An even better solution might be to use regmap-mmio instead of a custom bus.
>>> Morimoto-san, is there anything that would prevent the driver from switching
>>> to regmap-mmio ?
>>
>> I guess it is possilbe to use regmap-mmio.
>> I check it.
>
> Hmm... rsnd driver is using "regmap_field".
> regmap-mmio  requests "offset"  on "reg"
> regmap-filed requests "address" on "reg"
>
> So, if rsnd driver uses regmap-mmio,
> then, it needs tricky initialize like...
>
>        regmap_init_mmio(dev, 0, config)
>

Create one regmap instance per base address and in the regmap_fields use an 
relative offset to the base address rather than the absolute address. That's 
how the API is intended to be used, the current implementation is quite a hack.

- Lars

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Lars-Peter Clausen]

On 07/24/2014 04:16 AM, Kuninori Morimoto wrote:
>
> Hi Laurent
>
>>> The hardware registers are accessed from atomic contexts (the
>>> rsnd_soc_dai_trigger function, for instance, is called with the PCM
>>> substream spinlock held). They thus can't be protected by a mutex.
>>>
>>> Protect regmap register accesses with a spinlock instead of a mutex by
>>> setting the fast_io flag.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>   sound/soc/sh/rcar/gen.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> An even better solution might be to use regmap-mmio instead of a custom bus.
>>> Morimoto-san, is there anything that would prevent the driver from switching
>>> to regmap-mmio ?
>>
>> I guess it is possilbe to use regmap-mmio.
>> I check it.
>
> Hmm... rsnd driver is using "regmap_field".
> regmap-mmio  requests "offset"  on "reg"
> regmap-filed requests "address" on "reg"
>
> So, if rsnd driver uses regmap-mmio,
> then, it needs tricky initialize like...
>
>        regmap_init_mmio(dev, 0, config)
>

Create one regmap instance per base address and in the regmap_fields use an 
relative offset to the base address rather than the absolute address. That's 
how the API is intended to be used, the current implementation is quite a hack.

- Lars

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Kuninori Morimoto]

Hi Lars
# added Mark

> > Hmm... rsnd driver is using "regmap_field".
> > regmap-mmio  requests "offset"  on "reg"
> > regmap-filed requests "address" on "reg"
> >
> > So, if rsnd driver uses regmap-mmio,
> > then, it needs tricky initialize like...
> >
> >        regmap_init_mmio(dev, 0, config)
> >
> 
> Create one regmap instance per base address and in the regmap_fields use an 
> relative offset to the base address rather than the absolute address. That's 
> how the API is intended to be used, the current implementation is quite a hack.

Hmm...
But it (= base address mapping was not fixed between Gen1 and Gen2)
was the reason why I was asked to use regmap_fields

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Kuninori Morimoto]

Hi Lars
# added Mark

> > Hmm... rsnd driver is using "regmap_field".
> > regmap-mmio  requests "offset"  on "reg"
> > regmap-filed requests "address" on "reg"
> >
> > So, if rsnd driver uses regmap-mmio,
> > then, it needs tricky initialize like...
> >
> >        regmap_init_mmio(dev, 0, config)
> >
> 
> Create one regmap instance per base address and in the regmap_fields use an 
> relative offset to the base address rather than the absolute address. That's 
> how the API is intended to be used, the current implementation is quite a hack.

Hmm...
But it (= base address mapping was not fixed between Gen1 and Gen2)
was the reason why I was asked to use regmap_fields

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Lars-Peter Clausen]

On 07/28/2014 09:40 AM, Kuninori Morimoto wrote:
>
> Hi Lars
> # added Mark
>
>>> Hmm... rsnd driver is using "regmap_field".
>>> regmap-mmio  requests "offset"  on "reg"
>>> regmap-filed requests "address" on "reg"
>>>
>>> So, if rsnd driver uses regmap-mmio,
>>> then, it needs tricky initialize like...
>>>
>>>         regmap_init_mmio(dev, 0, config)
>>>
>>
>> Create one regmap instance per base address and in the regmap_fields use an
>> relative offset to the base address rather than the absolute address. That's
>> how the API is intended to be used, the current implementation is quite a hack.
>
> Hmm...
> But it (= base address mapping was not fixed between Gen1 and Gen2)
> was the reason why I was asked to use regmap_fields
>

I think that is fine. But you are only using a single regmap instance even 
though there are multiple unrelated register maps used and then you specify 
the offset in the regmap_fields as a absolute address. This is supposed to 
be a relative offset to the base address.

So basically use: ".reg = offset" instead of ".reg = (unsigned 
int)gen->base[reg_id] + offset" and when creating the field instead of 
passing a global regmap instance pass the regmap instance for the register 
map in who's range the register falls.


- Lars

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Lars-Peter Clausen]

On 07/28/2014 09:40 AM, Kuninori Morimoto wrote:
>
> Hi Lars
> # added Mark
>
>>> Hmm... rsnd driver is using "regmap_field".
>>> regmap-mmio  requests "offset"  on "reg"
>>> regmap-filed requests "address" on "reg"
>>>
>>> So, if rsnd driver uses regmap-mmio,
>>> then, it needs tricky initialize like...
>>>
>>>         regmap_init_mmio(dev, 0, config)
>>>
>>
>> Create one regmap instance per base address and in the regmap_fields use an
>> relative offset to the base address rather than the absolute address. That's
>> how the API is intended to be used, the current implementation is quite a hack.
>
> Hmm...
> But it (= base address mapping was not fixed between Gen1 and Gen2)
> was the reason why I was asked to use regmap_fields
>

I think that is fine. But you are only using a single regmap instance even 
though there are multiple unrelated register maps used and then you specify 
the offset in the regmap_fields as a absolute address. This is supposed to 
be a relative offset to the base address.

So basically use: ".reg = offset" instead of ".reg = (unsigned 
int)gen->base[reg_id] + offset" and when creating the field instead of 
passing a global regmap instance pass the regmap instance for the register 
map in who's range the register falls.


- Lars

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Kuninori Morimoto]

Hi Lars

> > But it (= base address mapping was not fixed between Gen1 and Gen2)
> > was the reason why I was asked to use regmap_fields
> >
> 
> I think that is fine. But you are only using a single regmap instance even 
> though there are multiple unrelated register maps used and then you specify 
> the offset in the regmap_fields as a absolute address. This is supposed to 
> be a relative offset to the base address.
> 
> So basically use: ".reg = offset" instead of ".reg = (unsigned 
> int)gen->base[reg_id] + offset" and when creating the field instead of 
> passing a global regmap instance pass the regmap instance for the register 
> map in who's range the register falls.

Hmm... I re-checked it, and I could understand.
And, indeed it was hackish.
OK, I try to modify it, then, it seems can use regmap-mmio.

Re: [alsa-devel] [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Kuninori Morimoto]

Hi Lars

> > But it (= base address mapping was not fixed between Gen1 and Gen2)
> > was the reason why I was asked to use regmap_fields
> >
> 
> I think that is fine. But you are only using a single regmap instance even 
> though there are multiple unrelated register maps used and then you specify 
> the offset in the regmap_fields as a absolute address. This is supposed to 
> be a relative offset to the base address.
> 
> So basically use: ".reg = offset" instead of ".reg = (unsigned 
> int)gen->base[reg_id] + offset" and when creating the field instead of 
> passing a global regmap instance pass the regmap instance for the register 
> map in who's range the register falls.

Hmm... I re-checked it, and I could understand.
And, indeed it was hackish.
OK, I try to modify it, then, it seems can use regmap-mmio.

Re: [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Ben Dooks]

On 23/07/14 22:19, Laurent Pinchart wrote:
> The hardware registers are accessed from atomic contexts (the
> rsnd_soc_dai_trigger function, for instance, is called with the PCM
> substream spinlock held). They thus can't be protected by a mutex.
> 
> Protect regmap register accesses with a spinlock instead of a mutex by
> setting the fast_io flag.

I reported this ages ago.

I ended up removing regmap entirely, it just locks the machine
solid and provides no useful functionality for the driver.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ASoC: rsnd: Protect register accesses with a spinlock instead of a mutex

[Ben Dooks]

On 23/07/14 22:19, Laurent Pinchart wrote:
> The hardware registers are accessed from atomic contexts (the
> rsnd_soc_dai_trigger function, for instance, is called with the PCM
> substream spinlock held). They thus can't be protected by a mutex.
> 
> Protect regmap register accesses with a spinlock instead of a mutex by
> setting the fast_io flag.

I reported this ages ago.

I ended up removing regmap entirely, it just locks the machine
solid and provides no useful functionality for the driver.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius