ASoC: new ac97 bus and codec clock

ASoC: new ac97 bus and codec clock

[Robert Jarzmik]

Hi Mark,

The new ac97 is working well in a platform data build. But in the device tree
case, there is an issue, triggered by our review in [1].

The main issue is the lack of one clock.

The device hierarchy is :
 - platform device sound@40500000 (the AC97 controller)
   \-> ac97 device sound@40500000:0 (the discovered ac97 IP for a wm9713)

Now the issue is here :
 - sound/ac97/bus.c:419, function ac97_get_enable_clk()
 - the clk_get() fails

The sound@40500000:0 device was automatically created by the AC97 bus code, but
I don't see any way to provide this device the "ac97_clk" ... How do you think
this is supposed to work, and how can my devicetree description provide this
clock ?

-- 
Robert

[1] Discussion reference
https://groups.google.com/forum/#!msg/linux.kernel/6_zIXK_maA4/RMYsmLlRMgAJ
Look for "You probably mean the BITCLK clock."

[2] Device tree extract
ac97: sound@40500000 {
      	compatible = "marvell,pxa270-ac97";
	reg = < 0x40500000 0x1000 >;
	interrupts = <14>;
	reset-gpios = <&gpio 95 GPIO_ACTIVE_HIGH>;
	#sound-dai-cells = <1>;
	pinctrl-names = "default";
	pinctrl-0 = < &pinctrl_ac97_default >;
	clocks = <&clks CLK_AC97>, <&clks CLK_AC97CONF>;
	clock-names = "AC97CLK", "AC97CONFCLK";
};

Re: ASoC: new ac97 bus and codec clock

[Mark Brown]

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

On Sun, Jun 17, 2018 at 10:03:54PM +0200, Robert Jarzmik wrote:

> The new ac97 is working well in a platform data build. But in the device tree
> case, there is an issue, triggered by our review in [1].

> The main issue is the lack of one clock.

> The device hierarchy is :
>  - platform device sound@40500000 (the AC97 controller)
>    \-> ac97 device sound@40500000:0 (the discovered ac97 IP for a wm9713)

> Now the issue is here :
>  - sound/ac97/bus.c:419, function ac97_get_enable_clk()
>  - the clk_get() fails

> The sound@40500000:0 device was automatically created by the AC97 bus code, but
> I don't see any way to provide this device the "ac97_clk" ... How do you think
> this is supposed to work, and how can my devicetree description provide this
> clock ?

This sounds like a DT question - how do you hook things up using DT to a
device that will be enumerated from the hardware?  I'm not sure what
best practice is here, adding Rob and Frank.

> [1] Discussion reference
> https://groups.google.com/forum/#!msg/linux.kernel/6_zIXK_maA4/RMYsmLlRMgAJ
> Look for "You probably mean the BITCLK clock."
> 
> [2] Device tree extract
> ac97: sound@40500000 {
>       	compatible = "marvell,pxa270-ac97";
> 	reg = < 0x40500000 0x1000 >;
> 	interrupts = <14>;
> 	reset-gpios = <&gpio 95 GPIO_ACTIVE_HIGH>;
> 	#sound-dai-cells = <1>;
> 	pinctrl-names = "default";
> 	pinctrl-0 = < &pinctrl_ac97_default >;
> 	clocks = <&clks CLK_AC97>, <&clks CLK_AC97CONF>;
> 	clock-names = "AC97CLK", "AC97CONFCLK";
> };

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

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

Re: ASoC: new ac97 bus and codec clock

[Rob Herring]

On Tue, Jun 19, 2018 at 9:02 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sun, Jun 17, 2018 at 10:03:54PM +0200, Robert Jarzmik wrote:
>
>> The new ac97 is working well in a platform data build. But in the device tree
>> case, there is an issue, triggered by our review in [1].
>
>> The main issue is the lack of one clock.
>
>> The device hierarchy is :
>>  - platform device sound@40500000 (the AC97 controller)
>>    \-> ac97 device sound@40500000:0 (the discovered ac97 IP for a wm9713)
>
>> Now the issue is here :
>>  - sound/ac97/bus.c:419, function ac97_get_enable_clk()
>>  - the clk_get() fails
>
>> The sound@40500000:0 device was automatically created by the AC97 bus code, but
>> I don't see any way to provide this device the "ac97_clk" ... How do you think
>> this is supposed to work, and how can my devicetree description provide this
>> clock ?

So the clock is a sideband signal outside the scope of AC97? If so...

> This sounds like a DT question - how do you hook things up using DT to a
> device that will be enumerated from the hardware?  I'm not sure what
> best practice is here, adding Rob and Frank.

Like PCI, USB, SDIO, etc., you need to define an AC97 bus binding
which defines child node structure, compatible formatting (if you can
base compatibles on something like VID/PID), and addressing (reg and
unit-address formats). Then once you define child nodes, you can add
whatever sideband connections you need. The AC97 core should be able
to populate struct device_node if there are any matching child
devices. It gets a bit more complicated if you need to do things like
enable power or de-assert resets to discover the devices. Sounds like
that might be the next person's problem in this case. :)

Rob

Re: ASoC: new ac97 bus and codec clock

[Robert Jarzmik]

Rob Herring <robh+dt@kernel.org> writes:

> Like PCI, USB, SDIO, etc., you need to define an AC97 bus binding
> which defines child node structure, compatible formatting (if you can
> base compatibles on something like VID/PID), and addressing (reg and
> unit-address formats). Then once you define child nodes, you can add
> whatever sideband connections you need. The AC97 core should be able
> to populate struct device_node if there are any matching child
> devices.
Ok, I thing I understand.

So the device-tree will look like :
ac97: sound@40500000 {
      	compatible = "marvell,pxa270-ac97";
	reg = < 0x40500000 0x1000 >;
	interrupts = <14>;
	reset-gpios = <&gpio 95 GPIO_ACTIVE_HIGH>;
	#sound-dai-cells = <1>;
	pinctrl-names = "default";
	pinctrl-0 = < &pinctrl_ac97_default >;
	clocks = <&clks CLK_AC97>, <&clks CLK_AC97CONF>;
	clock-names = "AC97CLK", "AC97CONFCLK";

        wm9713@0 {
        	reg = <0>; /* Codex index (between 0 and 3) */
                compatible = "ac97-codec";
                clocks = <&fixed_wm9713_clock>;
                clock-names = "ac97_clk";
        }
};

And the function ac97_codec_add() will :
 - scan the device-tree ac97 controller childs
 - match one if its reg equals (in our case wm9713@0)
 - set codec->dev.of_node to the matched one

> It gets a bit more complicated if you need to do things like
> enable power or de-assert resets to discover the devices. Sounds like
> that might be the next person's problem in this case. :)
Yeah ... Crossing fingers that won't be me :)

Cheers.

-- 
Robert

Re: ASoC: new ac97 bus and codec clock

[Rob Herring]

On Wed, Jun 20, 2018 at 8:52 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Rob Herring <robh+dt@kernel.org> writes:
>
>> Like PCI, USB, SDIO, etc., you need to define an AC97 bus binding
>> which defines child node structure, compatible formatting (if you can
>> base compatibles on something like VID/PID), and addressing (reg and
>> unit-address formats). Then once you define child nodes, you can add
>> whatever sideband connections you need. The AC97 core should be able
>> to populate struct device_node if there are any matching child
>> devices.
> Ok, I thing I understand.
>
> So the device-tree will look like :
> ac97: sound@40500000 {
>         compatible = "marvell,pxa270-ac97";
>         reg = < 0x40500000 0x1000 >;
>         interrupts = <14>;
>         reset-gpios = <&gpio 95 GPIO_ACTIVE_HIGH>;
>         #sound-dai-cells = <1>;
>         pinctrl-names = "default";
>         pinctrl-0 = < &pinctrl_ac97_default >;
>         clocks = <&clks CLK_AC97>, <&clks CLK_AC97CONF>;
>         clock-names = "AC97CLK", "AC97CONFCLK";
>
>         wm9713@0 {

audio-codec@0

>                 reg = <0>; /* Codex index (between 0 and 3) */
>                 compatible = "ac97-codec";

This should be something with 'wm9713' in it or if there are ID
registers you can base it on that.

>                 clocks = <&fixed_wm9713_clock>;
>                 clock-names = "ac97_clk";

This is the standard 12MHz clock?

>         }
> };
>
> And the function ac97_codec_add() will :
>  - scan the device-tree ac97 controller childs
>  - match one if its reg equals (in our case wm9713@0)
>  - set codec->dev.of_node to the matched one

Not familiar with that function, but that sounds about right.

Rob

Re: ASoC: new ac97 bus and codec clock

[Robert Jarzmik]

Rob Herring <robh+dt@kernel.org> writes:

> On Wed, Jun 20, 2018 at 8:52 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>> Rob Herring <robh+dt@kernel.org> writes:
Hi Rob,

>>         wm9713@0 {
>
> audio-codec@0
Sure.

>>                 reg = <0>; /* Codex index (between 0 and 3) */
>>                 compatible = "ac97-codec";
>
> This should be something with 'wm9713' in it or if there are ID
> registers you can base it on that.
Well, let's discuss that.
If I take as an example sdio, in mmc-card.txt, the compatible string is
"mmc-card", which describes the "kind" of subnode it is, not _exactly_ the
subnode it is.

If the purpose for this compatible string is to filter the child nodes which are
only ac97 codecs, so that the ac97 framework can act upon them (ie. acquire the
clock for example), then wm9713 is too specific ... That won't work for another
device with an Analog Devices AD1835 for example.

>>                 clocks = <&fixed_wm9713_clock>;
>>                 clock-names = "ac97_clk";
>
> This is the standard 12MHz clock?
Yes it is. The name comes from the original ac97 review, and is in
ac97_get_enable_clk().

Cheers.

-- 
Robert

Re: ASoC: new ac97 bus and codec clock

[Rob Herring]

On Wed, Jun 20, 2018 at 11:04 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Rob Herring <robh+dt@kernel.org> writes:
>
>> On Wed, Jun 20, 2018 at 8:52 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>>> Rob Herring <robh+dt@kernel.org> writes:
> Hi Rob,
>
>>>         wm9713@0 {
>>
>> audio-codec@0
> Sure.
>
>>>                 reg = <0>; /* Codex index (between 0 and 3) */
>>>                 compatible = "ac97-codec";
>>
>> This should be something with 'wm9713' in it or if there are ID
>> registers you can base it on that.
> Well, let's discuss that.
> If I take as an example sdio, in mmc-card.txt, the compatible string is
> "mmc-card", which describes the "kind" of subnode it is, not _exactly_ the
> subnode it is.

"mmc-card" is only for memory cards which are pretty much all the same
and rarely have a node, but SDIO devices always have a specific
compatible.

Does AC97 specify a common programming model? That's were a common
compatible is used (though experience has proven things like
"generic-ehci" are don't work).

It's generally a bad design to mix different types of subnodes, so I
was assuming all child nodes would be the same type. For example, you
can only have 1 address space (i.e. reg definition). If you need
different types, you need to add a level of nodes.

>
> If the purpose for this compatible string is to filter the child nodes which are
> only ac97 codecs, so that the ac97 framework can act upon them (ie. acquire the
> clock for example), then wm9713 is too specific ... That won't work for another
> device with an Analog Devices AD1835 for example.

Are there ID registers? If so, then just construct a compatible string
from them and just find the compatible child node. See USB bindings
for an example using VID/PID.

Rob

Re: ASoC: new ac97 bus and codec clock

[Robert Jarzmik]

Rob Herring <robh+dt@kernel.org> writes:

> Does AC97 specify a common programming model? That's were a common
> compatible is used (though experience has proven things like
> "generic-ehci" are don't work).
If regmap is enough "common programming model" then yes. If you think of a
common layout of AC97 registers and a common dynamics to program them across all
ac97 controllers, then no, as far as I'm aware it does not.

>> If the purpose for this compatible string is to filter the child nodes which are
>> only ac97 codecs, so that the ac97 framework can act upon them (ie. acquire the
>> clock for example), then wm9713 is too specific ... That won't work for another
>> device with an Analog Devices AD1835 for example.

> Are there ID registers? If so, then just construct a compatible string
> from them and just find the compatible child node. See USB bindings
> for an example using VID/PID.
There is an AC97 ID register, yes, a key as VID/PID is a key for USB.

So my example, where the id is 0x574d4c13, would become :

ac97: sound@40500000 {
      	compatible = "marvell,pxa270-ac97";
	reg = < 0x40500000 0x1000 >;
	interrupts = <14>;
	reset-gpios = <&gpio 95 GPIO_ACTIVE_HIGH>;
	#sound-dai-cells = <1>;
	pinctrl-names = "default";
	pinctrl-0 = < &pinctrl_ac97_default >;
	clocks = <&clks CLK_AC97>, <&clks CLK_AC97CONF>;
	clock-names = "AC97CLK", "AC97CONFCLK";

        audio-codec@0 {
        	reg = <0>; /* Codex index (between 0 and 3) */
                compatible = "ac97id574d4c13";
                clocks = <&fixed_wm9713_clock>;
                clock-names = "ac97_clk";
        }
};

Once we have converged on this example, I will post the patches for the binding,
and the adaptation of the ac97 bus of course. Gives me a better chance to shoot
in the right direction first :)

Cheers.

-- 
Robert

Re: ASoC: new ac97 bus and codec clock

[Rob Herring]

On Wed, Jun 20, 2018 at 1:36 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Rob Herring <robh+dt@kernel.org> writes:
>
>> Does AC97 specify a common programming model? That's were a common
>> compatible is used (though experience has proven things like
>> "generic-ehci" are don't work).
> If regmap is enough "common programming model" then yes. If you think of a
> common layout of AC97 registers and a common dynamics to program them across all
> ac97 controllers, then no, as far as I'm aware it does not.

No, not regmap.

>>> If the purpose for this compatible string is to filter the child nodes which are
>>> only ac97 codecs, so that the ac97 framework can act upon them (ie. acquire the
>>> clock for example), then wm9713 is too specific ... That won't work for another
>>> device with an Analog Devices AD1835 for example.
>
>> Are there ID registers? If so, then just construct a compatible string
>> from them and just find the compatible child node. See USB bindings
>> for an example using VID/PID.
> There is an AC97 ID register, yes, a key as VID/PID is a key for USB.
>
> So my example, where the id is 0x574d4c13, would become :
>
> ac97: sound@40500000 {
>         compatible = "marvell,pxa270-ac97";
>         reg = < 0x40500000 0x1000 >;
>         interrupts = <14>;
>         reset-gpios = <&gpio 95 GPIO_ACTIVE_HIGH>;
>         #sound-dai-cells = <1>;
>         pinctrl-names = "default";
>         pinctrl-0 = < &pinctrl_ac97_default >;
>         clocks = <&clks CLK_AC97>, <&clks CLK_AC97CONF>;
>         clock-names = "AC97CLK", "AC97CONFCLK";
>
>         audio-codec@0 {
>                 reg = <0>; /* Codex index (between 0 and 3) */
>                 compatible = "ac97id574d4c13";
>                 clocks = <&fixed_wm9713_clock>;
>                 clock-names = "ac97_clk";
>         }
> };
>
> Once we have converged on this example, I will post the patches for the binding,
> and the adaptation of the ac97 bus of course. Gives me a better chance to shoot
> in the right direction first :)

The example looks good.

Rob

Re: ASoC: new ac97 bus and codec clock

[Mark Brown]

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

On Wed, Jun 20, 2018 at 11:16:15AM -0600, Rob Herring wrote:

> Does AC97 specify a common programming model? That's were a common
> compatible is used (though experience has proven things like
> "generic-ehci" are don't work).

Yes, there is a standard register interface which all AC'97 CODECs
implemented and which does work effectively for drivers that only use
that.  Devices can offer extensions which do need specific drivers, and
there are ID registers which can be used to enumerate.  However one of
the things that some of the devices targetted at embedded systems do is
offer non-standard clocking features that need some configuration during
initialization (and may need to work with input clocks too) which
complicates things, especially in the context of DT.

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

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