[alsa-devel] ASoC: new ac97 bus and codec clock
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 ?
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"; };
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
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.
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
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.
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
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.
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
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.
participants (3)
-
Mark Brown -
Rob Herring -
Robert Jarzmik