| Message ID | 1406150366-10169-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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
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
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.
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,
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 ?