DAPM over two regmaps (and a mailbox)

DAPM over two regmaps (and a mailbox)

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1718 bytes --]

Hi,

We've been working recently on a later SoC (Allwinner A33) with a
codec directly embedded into it, just like
sound/soc/sunxi/sun4i-codec.c, but this time using the usual i2s
controller we had, instead of a custom DAI.

That codec is mapped in memory, however, we have a bunch of DAPM
widgets that are mapped in a separate register space, that should
probably be exposed through a syscon (but isn't yet).

Those are apparently used to control the analog part of the codec,
including powering up the DAC, so it really feels like they should be
part of DAPM.

However, since we will obviously have a regmap for the main register
space of the codec, that leaves us with two regmaps that we need to
use, depending on the register we want to set, and DAPM doesn't really
seem to be able to handle that.

To make things worse, the register in the syscon behaves as a mailbox,
where you actually have to set in that register the address you want
to modify and the new value, in a single write. This also seem to
deviate from the usual DAPM access pattern.

I'm not really sure how to handle that properly. For now, we just did
those writes outside of DAPM, in the startup, shutdown and prepare
shutdowns. We could also have a meta-regmap, that would have custom
write and read functions, and depending on the register would turn to
our syscon, or do a writel.

Or we could try to make DAPM able to use different regmaps depending
on the register, but that seem do be very intrusive.

Do you have any suggestions or preferences on how to implement this
properly?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DAPM over two regmaps (and a mailbox)

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 752 bytes --]

On Mon, Sep 19, 2016 at 12:54:19PM +0200, Maxime Ripard wrote:

> That codec is mapped in memory, however, we have a bunch of DAPM
> widgets that are mapped in a separate register space, that should
> probably be exposed through a syscon (but isn't yet).

Why not just represent those as a separate device?

> To make things worse, the register in the syscon behaves as a mailbox,
> where you actually have to set in that register the address you want
> to modify and the new value, in a single write. This also seem to
> deviate from the usual DAPM access pattern.

This is totally fine, just use events for the things that aren't simple
register updates - there are a large number of examples in the tree,
simple register access is totally optional.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DAPM over two regmaps (and a mailbox)

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1233 bytes --]

On Mon, Sep 19, 2016 at 12:12:47PM +0100, Mark Brown wrote:
> On Mon, Sep 19, 2016 at 12:54:19PM +0200, Maxime Ripard wrote:
> 
> > That codec is mapped in memory, however, we have a bunch of DAPM
> > widgets that are mapped in a separate register space, that should
> > probably be exposed through a syscon (but isn't yet).
> 
> Why not just represent those as a separate device?

I don't know, this seems to be supplies to muxers, mixers, DACs, the
amplifier (and the amplifier volume too, even though that's not a
widget), and it looks really intertwinned, how would you separate
them?

> > To make things worse, the register in the syscon behaves as a mailbox,
> > where you actually have to set in that register the address you want
> > to modify and the new value, in a single write. This also seem to
> > deviate from the usual DAPM access pattern.
> 
> This is totally fine, just use events for the things that aren't simple
> register updates - there are a large number of examples in the tree,
> simple register access is totally optional.

Ah, yes, that seems to be just right. Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DAPM over two regmaps (and a mailbox)

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

On Mon, Sep 19, 2016 at 01:34:15PM +0200, Maxime Ripard wrote:
> On Mon, Sep 19, 2016 at 12:12:47PM +0100, Mark Brown wrote:

> > Why not just represent those as a separate device?

> I don't know, this seems to be supplies to muxers, mixers, DACs, the
> amplifier (and the amplifier volume too, even though that's not a
> widget), and it looks really intertwinned, how would you separate
> them?

Based on which register map they're in?  They're probably separate IPs
on at least some level...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DAPM over two regmaps (and a mailbox)

[Chen-Yu Tsai]

Hi Maxime,

On Mon, Sep 19, 2016 at 7:34 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Mon, Sep 19, 2016 at 12:12:47PM +0100, Mark Brown wrote:
>> On Mon, Sep 19, 2016 at 12:54:19PM +0200, Maxime Ripard wrote:
>>
>> > That codec is mapped in memory, however, we have a bunch of DAPM
>> > widgets that are mapped in a separate register space, that should
>> > probably be exposed through a syscon (but isn't yet).
>>
>> Why not just represent those as a separate device?
>
> I don't know, this seems to be supplies to muxers, mixers, DACs, the
> amplifier (and the amplifier volume too, even though that's not a
> widget), and it looks really intertwinned, how would you separate
> them?

I asked Mark the same question a while ago. I was looking at the A31s
codec, which is kind of a cross between A31 and A23. Mark recommended
using aux devices. AFAIU the codec would just register the DAC and
ADC widgets, the digital volume control, and whatever digital bits
there are. The analog side would be done in a asoc component driver,
which would register all the analog widgets and controls. You then
tie them together at the card level.

AFAIK the digital parts and analog parts are in different power domains,
probably to keep audio bypass working in standby mode.

As for the A31s, I only got as far as implementing the regmap using
custom read/write callbacks, and part of the register definition.
Unfortunately I saved my work with git stash, which I accidentally
wiped out with git reflog expire...

Regards
ChenYu

>> > To make things worse, the register in the syscon behaves as a mailbox,
>> > where you actually have to set in that register the address you want
>> > to modify and the new value, in a single write. This also seem to
>> > deviate from the usual DAPM access pattern.
>>
>> This is totally fine, just use events for the things that aren't simple
>> register updates - there are a large number of examples in the tree,
>> simple register access is totally optional.
>
> Ah, yes, that seems to be just right. Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: DAPM over two regmaps (and a mailbox)

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2100 bytes --]

Hi,

On Mon, Sep 19, 2016 at 10:56:10PM +0800, Chen-Yu Tsai wrote:
> On Mon, Sep 19, 2016 at 7:34 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Sep 19, 2016 at 12:12:47PM +0100, Mark Brown wrote:
> >> On Mon, Sep 19, 2016 at 12:54:19PM +0200, Maxime Ripard wrote:
> >>
> >> > That codec is mapped in memory, however, we have a bunch of DAPM
> >> > widgets that are mapped in a separate register space, that should
> >> > probably be exposed through a syscon (but isn't yet).
> >>
> >> Why not just represent those as a separate device?
> >
> > I don't know, this seems to be supplies to muxers, mixers, DACs, the
> > amplifier (and the amplifier volume too, even though that's not a
> > widget), and it looks really intertwinned, how would you separate
> > them?
> 
> I asked Mark the same question a while ago. I was looking at the A31s
> codec, which is kind of a cross between A31 and A23.

We're working on the A33, that has yet another setup from the A23..

> Mark recommended using aux devices. AFAIU the codec would just
> register the DAC and ADC widgets, the digital volume control, and
> whatever digital bits there are. The analog side would be done in a
> asoc component driver, which would register all the analog widgets
> and controls. You then tie them together at the card level.

Ok. How would that work? Can you setup routes between widgets defined
in different components?

> AFAIK the digital parts and analog parts are in different power
> domains, probably to keep audio bypass working in standby mode.

I don't think we have control over those power domains though, do we?

> As for the A31s, I only got as far as implementing the regmap using
> custom read/write callbacks, and part of the register definition.
> Unfortunately I saved my work with git stash, which I accidentally
> wiped out with git reflog expire...

I guess some lessons are meant to be learned the hard way :)

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DAPM over two regmaps (and a mailbox)

[Chen-Yu Tsai]

On Tue, Sep 20, 2016 at 3:15 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Mon, Sep 19, 2016 at 10:56:10PM +0800, Chen-Yu Tsai wrote:
>> On Mon, Sep 19, 2016 at 7:34 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Mon, Sep 19, 2016 at 12:12:47PM +0100, Mark Brown wrote:
>> >> On Mon, Sep 19, 2016 at 12:54:19PM +0200, Maxime Ripard wrote:
>> >>
>> >> > That codec is mapped in memory, however, we have a bunch of DAPM
>> >> > widgets that are mapped in a separate register space, that should
>> >> > probably be exposed through a syscon (but isn't yet).
>> >>
>> >> Why not just represent those as a separate device?
>> >
>> > I don't know, this seems to be supplies to muxers, mixers, DACs, the
>> > amplifier (and the amplifier volume too, even though that's not a
>> > widget), and it looks really intertwinned, how would you separate
>> > them?
>>
>> I asked Mark the same question a while ago. I was looking at the A31s
>> codec, which is kind of a cross between A31 and A23.
>
> We're working on the A33, that has yet another setup from the A23..

Looks like the "analog control" bits are similar, if not completely
the same. As described on page 221, the bits are accessed through a
mapping mechanism in register 0x01f015c0.

So the solution would likely be the same for A31s/A23/A33: a platform
device/driver for that particular piece of hardware that registers a
custom regmap and an ASoC component. This would be under the PRCM
mfd. The main codec would list this (I think it's by name?) under
it's aux_components.

>> Mark recommended using aux devices. AFAIU the codec would just
>> register the DAC and ADC widgets, the digital volume control, and
>> whatever digital bits there are. The analog side would be done in a
>> asoc component driver, which would register all the analog widgets
>> and controls. You then tie them together at the card level.
>
> Ok. How would that work? Can you setup routes between widgets defined
> in different components?

Indeed you can. I think that is what the ASoC card is for, to stitch
up various components. Not sure if you can directly use widgets from
aux components in the codec component driver though. I guess it depends
on the probe/binding sequence.

>> AFAIK the digital parts and analog parts are in different power
>> domains, probably to keep audio bypass working in standby mode.
>
> I don't think we have control over those power domains though, do we?

Technically we can control them from external regulators, though
probably just for suspend. The main part of the SoC would be under
the SYS domain, while the analog bits, like the R_ block would be
under the RTC domain.

There might also be a register controlling them in the PRCM block.
I suppose that's out of the scope of this discussion.

>
>> As for the A31s, I only got as far as implementing the regmap using
>> custom read/write callbacks, and part of the register definition.
>> Unfortunately I saved my work with git stash, which I accidentally
>> wiped out with git reflog expire...
>
> I guess some lessons are meant to be learned the hard way :)

Yes. I lost some work on the A31/A31s codec driver. Mainly
the A31s parts, and some bits of renaming for the A31. :(

Regards
ChenYu

Re: DAPM over two regmaps (and a mailbox)

[Chen-Yu Tsai]

On Tue, Sep 20, 2016 at 8:32 AM, Chen-Yu Tsai <wens@csie.org> wrote:
> On Tue, Sep 20, 2016 at 3:15 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> Hi,
>>
>> On Mon, Sep 19, 2016 at 10:56:10PM +0800, Chen-Yu Tsai wrote:
>>> On Mon, Sep 19, 2016 at 7:34 PM, Maxime Ripard
>>> <maxime.ripard@free-electrons.com> wrote:
>>> > On Mon, Sep 19, 2016 at 12:12:47PM +0100, Mark Brown wrote:
>>> >> On Mon, Sep 19, 2016 at 12:54:19PM +0200, Maxime Ripard wrote:
>>> >>
>>> >> > That codec is mapped in memory, however, we have a bunch of DAPM
>>> >> > widgets that are mapped in a separate register space, that should
>>> >> > probably be exposed through a syscon (but isn't yet).
>>> >>
>>> >> Why not just represent those as a separate device?
>>> >
>>> > I don't know, this seems to be supplies to muxers, mixers, DACs, the
>>> > amplifier (and the amplifier volume too, even though that's not a
>>> > widget), and it looks really intertwinned, how would you separate
>>> > them?
>>>
>>> I asked Mark the same question a while ago. I was looking at the A31s
>>> codec, which is kind of a cross between A31 and A23.
>>
>> We're working on the A33, that has yet another setup from the A23..
>
> Looks like the "analog control" bits are similar, if not completely
> the same. As described on page 221, the bits are accessed through a
> mapping mechanism in register 0x01f015c0.
>
> So the solution would likely be the same for A31s/A23/A33: a platform
> device/driver for that particular piece of hardware that registers a
> custom regmap and an ASoC component. This would be under the PRCM
> mfd. The main codec would list this (I think it's by name?) under
> it's aux_components.
>
>>> Mark recommended using aux devices. AFAIU the codec would just
>>> register the DAC and ADC widgets, the digital volume control, and
>>> whatever digital bits there are. The analog side would be done in a
>>> asoc component driver, which would register all the analog widgets
>>> and controls. You then tie them together at the card level.
>>
>> Ok. How would that work? Can you setup routes between widgets defined
>> in different components?
>
> Indeed you can. I think that is what the ASoC card is for, to stitch
> up various components. Not sure if you can directly use widgets from
> aux components in the codec component driver though. I guess it depends
> on the probe/binding sequence.
>
>>> AFAIK the digital parts and analog parts are in different power
>>> domains, probably to keep audio bypass working in standby mode.
>>
>> I don't think we have control over those power domains though, do we?
>
> Technically we can control them from external regulators, though
> probably just for suspend. The main part of the SoC would be under
> the SYS domain, while the analog bits, like the R_ block would be
> under the RTC domain.
>
> There might also be a register controlling them in the PRCM block.
> I suppose that's out of the scope of this discussion.
>
>>
>>> As for the A31s, I only got as far as implementing the regmap using
>>> custom read/write callbacks, and part of the register definition.
>>> Unfortunately I saved my work with git stash, which I accidentally
>>> wiped out with git reflog expire...
>>
>> I guess some lessons are meant to be learned the hard way :)
>
> Yes. I lost some work on the A31/A31s codec driver. Mainly
> the A31s parts, and some bits of renaming for the A31. :(

Turns out I hadn't even added the file to git, so it's still there!

    http://wens.tw/sun6i-a31s-codec-analog.c

Looks like I finished the register definitions, but only copied the
widgets/controls from my A31 driver, and haven't fixed them up yet.

Also, my use of SOC_DAPM_DOUBLE requires some work to the DAPM code
to support updates to two registers, rather than two fields in one
register, which I already implemented. This is, and the driver, are
on my TODO list.

Let me know how you would like to proceed, so we don't end up doing
double the work.

Regards
ChenYu

Re: DAPM over two regmaps (and a mailbox)

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2733 bytes --]

Hi,

On Tue, Sep 20, 2016 at 08:32:50AM +0800, Chen-Yu Tsai wrote:
> On Tue, Sep 20, 2016 at 3:15 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > On Mon, Sep 19, 2016 at 10:56:10PM +0800, Chen-Yu Tsai wrote:
> >> On Mon, Sep 19, 2016 at 7:34 PM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > On Mon, Sep 19, 2016 at 12:12:47PM +0100, Mark Brown wrote:
> >> >> On Mon, Sep 19, 2016 at 12:54:19PM +0200, Maxime Ripard wrote:
> >> >>
> >> >> > That codec is mapped in memory, however, we have a bunch of DAPM
> >> >> > widgets that are mapped in a separate register space, that should
> >> >> > probably be exposed through a syscon (but isn't yet).
> >> >>
> >> >> Why not just represent those as a separate device?
> >> >
> >> > I don't know, this seems to be supplies to muxers, mixers, DACs, the
> >> > amplifier (and the amplifier volume too, even though that's not a
> >> > widget), and it looks really intertwinned, how would you separate
> >> > them?
> >>
> >> I asked Mark the same question a while ago. I was looking at the A31s
> >> codec, which is kind of a cross between A31 and A23.
> >
> > We're working on the A33, that has yet another setup from the A23..
> 
> Looks like the "analog control" bits are similar, if not completely
> the same. As described on page 221, the bits are accessed through a
> mapping mechanism in register 0x01f015c0.

Yes, we've seen that too.

> So the solution would likely be the same for A31s/A23/A33: a platform
> device/driver for that particular piece of hardware that registers a
> custom regmap and an ASoC component. This would be under the PRCM
> mfd. The main codec would list this (I think it's by name?) under
> it's aux_components.
>
> >> Mark recommended using aux devices. AFAIU the codec would just
> >> register the DAC and ADC widgets, the digital volume control, and
> >> whatever digital bits there are. The analog side would be done in a
> >> asoc component driver, which would register all the analog widgets
> >> and controls. You then tie them together at the card level.
> >
> > Ok. How would that work? Can you setup routes between widgets defined
> > in different components?
> 
> Indeed you can. I think that is what the ASoC card is for, to stitch
> up various components. Not sure if you can directly use widgets from
> aux components in the codec component driver though. I guess it depends
> on the probe/binding sequence.

I didn't know about the aux_devs, but yeah, it definitely looks like
the proper solution.

We'll work on that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DAPM over two regmaps (and a mailbox)

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 888 bytes --]

On Tue, Sep 20, 2016 at 08:41:02AM +0800, Chen-Yu Tsai wrote:
> Turns out I hadn't even added the file to git, so it's still there!
> 
>     http://wens.tw/sun6i-a31s-codec-analog.c
> 
> Looks like I finished the register definitions, but only copied the
> widgets/controls from my A31 driver, and haven't fixed them up yet.
> 
> Also, my use of SOC_DAPM_DOUBLE requires some work to the DAPM code
> to support updates to two registers, rather than two fields in one
> register, which I already implemented. This is, and the driver, are
> on my TODO list.
> 
> Let me know how you would like to proceed, so we don't end up doing
> double the work.

We'll probably have something ready by next week. Maybe we can just
send the patches and start from there?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DAPM over two regmaps (and a mailbox)

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 486 bytes --]

On Tue, Sep 20, 2016 at 08:32:50AM +0800, Chen-Yu Tsai wrote:

> > Ok. How would that work? Can you setup routes between widgets defined
> > in different components?

> Indeed you can. I think that is what the ASoC card is for, to stitch
> up various components. Not sure if you can directly use widgets from

Yes.

> aux components in the codec component driver though. I guess it depends
> on the probe/binding sequence.

You can't, you'd need to provide a table for the card to use.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DAPM over two regmaps (and a mailbox)

[Chen-Yu Tsai]

On Tue, Sep 20, 2016 at 2:41 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Sep 20, 2016 at 08:41:02AM +0800, Chen-Yu Tsai wrote:
>> Turns out I hadn't even added the file to git, so it's still there!
>>
>>     http://wens.tw/sun6i-a31s-codec-analog.c
>>
>> Looks like I finished the register definitions, but only copied the
>> widgets/controls from my A31 driver, and haven't fixed them up yet.
>>
>> Also, my use of SOC_DAPM_DOUBLE requires some work to the DAPM code
>> to support updates to two registers, rather than two fields in one
>> register, which I already implemented. This is, and the driver, are
>> on my TODO list.
>>
>> Let me know how you would like to proceed, so we don't end up doing
>> double the work.
>
> We'll probably have something ready by next week. Maybe we can just
> send the patches and start from there?

OK! Hopefully I'll have some time next week to get some work done on
mine. I'll take a look at the 3 SoCs and compare the analog controls
over the weekend, and see if we can't reuse them, at least between
A23/A33.

ChenYu