| Message ID | 20200114150937.18304-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: Configure omap5 AESS | expand |
Hi Tony, > Am 14.01.2020 um 16:09 schrieb Tony Lindgren <tony@atomide.com>: > > We are missing AESS for omap5. Looks like it's similar to what we have > for omap4, and this gets ti-sysc interconnect target module driver to > detect it properly. > > Note that we currently have no child device driver available for it. What I have is a non-working and no more compiling driver originally written by Peter Ujfalusi and reworked by Andrej Utkin. We did have it almost running on v4.14 or so except problems with firmware versions and headers... There we used classical hwmods and I could revert them now to try your new patches. Unfortunately, I could only compile-test your two patches but nothing with AESS. We had tried to follow kernel API changes in the sound subsystem but today it is not even compiling any more :( So getting a working device driver is an even bigger task than SGX was. > > Cc: H. Nikolaus Schaller <hns@goldelico.com> > Cc: Matthijs van Duin <matthijsvanduin@gmail.com> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > Cc: Tero Kristo <t-kristo@ti.com> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > > Note that this depends on "[PATCH] clk: ti: omap5: Add missing AESS clock". > > arch/arm/boot/dts/omap5-l4-abe.dtsi | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/omap5-l4-abe.dtsi b/arch/arm/boot/dts/omap5-l4-abe.dtsi > --- a/arch/arm/boot/dts/omap5-l4-abe.dtsi > +++ b/arch/arm/boot/dts/omap5-l4-abe.dtsi > @@ -426,8 +426,20 @@ target-module@c0000 { /* 0x401c0000, ap 30 1e.0 */ > }; > > target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ Here its may be good to have an "aess" label. > - compatible = "ti,sysc"; > - status = "disabled"; > + compatible = "ti,sysc-omap4", "ti,sysc"; > + reg = <0xf1000 0x4>, > + <0xf1010 0x4>; > + reg-names = "rev", "sysc"; > + ti,sysc-midle = <SYSC_IDLE_FORCE>, > + <SYSC_IDLE_NO>, > + <SYSC_IDLE_SMART>, > + <SYSC_IDLE_SMART_WKUP>; > + ti,sysc-sidle = <SYSC_IDLE_FORCE>, > + <SYSC_IDLE_NO>, > + <SYSC_IDLE_SMART>; > + /* Domains (V, P, C): iva, abe_pwrdm, abe_clkdm */ > + clocks = <&abe_clkctrl OMAP5_AESS_CLKCTRL 0>; > + clock-names = "fck"; > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x0 0xf1000 0x1000>, > -- > 2.24.1 BR and thanks, Nikolaus
* H. Nikolaus Schaller <hns@goldelico.com> [200114 16:38]: > Hi Tony, > > > Am 14.01.2020 um 16:09 schrieb Tony Lindgren <tony@atomide.com>: > > > > We are missing AESS for omap5. Looks like it's similar to what we have > > for omap4, and this gets ti-sysc interconnect target module driver to > > detect it properly. > > > > Note that we currently have no child device driver available for it. > > What I have is a non-working and no more compiling driver originally written by > Peter Ujfalusi and reworked by Andrej Utkin. We did have it almost running on > v4.14 or so except problems with firmware versions and headers... > > There we used classical hwmods and I could revert them now to try your new patches. > Unfortunately, I could only compile-test your two patches but nothing with AESS. > > We had tried to follow kernel API changes in the sound subsystem but today it is > not even compiling any more :( > > So getting a working device driver is an even bigger task than SGX was. OK. Well hopefully that's at least a little bit easier now though. > > Cc: H. Nikolaus Schaller <hns@goldelico.com> > > Cc: Matthijs van Duin <matthijsvanduin@gmail.com> > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Cc: Tero Kristo <t-kristo@ti.com> > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > --- > > > > Note that this depends on "[PATCH] clk: ti: omap5: Add missing AESS clock". > > > > arch/arm/boot/dts/omap5-l4-abe.dtsi | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/boot/dts/omap5-l4-abe.dtsi b/arch/arm/boot/dts/omap5-l4-abe.dtsi > > --- a/arch/arm/boot/dts/omap5-l4-abe.dtsi > > +++ b/arch/arm/boot/dts/omap5-l4-abe.dtsi > > @@ -426,8 +426,20 @@ target-module@c0000 { /* 0x401c0000, ap 30 1e.0 */ > > }; > > > > target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ > > Here its may be good to have an "aess" label. Care to clarify what you have in mind? The module is generic, aess device will be the child node. How about just a comment for aess? Regards, Tony
Hi Tony, > Am 14.01.2020 um 17:46 schrieb Tony Lindgren <tony@atomide.com>: > > * H. Nikolaus Schaller <hns@goldelico.com> [200114 16:38]: >> Hi Tony, >> >>> Am 14.01.2020 um 16:09 schrieb Tony Lindgren <tony@atomide.com>: >>> >>> We are missing AESS for omap5. Looks like it's similar to what we have >>> for omap4, and this gets ti-sysc interconnect target module driver to >>> detect it properly. >>> >>> Note that we currently have no child device driver available for it. >> >> What I have is a non-working and no more compiling driver originally written by >> Peter Ujfalusi and reworked by Andrej Utkin. We did have it almost running on >> v4.14 or so except problems with firmware versions and headers... >> >> There we used classical hwmods and I could revert them now to try your new patches. >> Unfortunately, I could only compile-test your two patches but nothing with AESS. >> >> We had tried to follow kernel API changes in the sound subsystem but today it is >> not even compiling any more :( >> >> So getting a working device driver is an even bigger task than SGX was. > > OK. Well hopefully that's at least a little bit easier now though. > >>> Cc: H. Nikolaus Schaller <hns@goldelico.com> >>> Cc: Matthijs van Duin <matthijsvanduin@gmail.com> >>> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> >>> Cc: Tero Kristo <t-kristo@ti.com> >>> Signed-off-by: Tony Lindgren <tony@atomide.com> >>> --- >>> >>> Note that this depends on "[PATCH] clk: ti: omap5: Add missing AESS clock". >>> >>> arch/arm/boot/dts/omap5-l4-abe.dtsi | 16 ++++++++++++++-- >>> 1 file changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/boot/dts/omap5-l4-abe.dtsi b/arch/arm/boot/dts/omap5-l4-abe.dtsi >>> --- a/arch/arm/boot/dts/omap5-l4-abe.dtsi >>> +++ b/arch/arm/boot/dts/omap5-l4-abe.dtsi >>> @@ -426,8 +426,20 @@ target-module@c0000 { /* 0x401c0000, ap 30 1e.0 */ >>> }; >>> >>> target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ >> >> Here its may be good to have an "aess" label. > > Care to clarify what you have in mind? The module is generic, aess > device will be the child node. The existing driver is hooked into the sound root-node and looks for a ti,aess = <&aess>; link: / { sound: sound { compatible = "ti,abe-twl6040"; ti,model = "omap5-uevm"; ti,jack-detection; ti,mclk-freq = <19200000>; ti,mcpdm = <&mcpdm>; ti,mcbsp1 = <&mcbsp1>; ti,mcbsp2 = <&mcbsp2>; ti,mcbsp3 = <&mcbsp3>; ti,twl6040 = <&twl6040>; ti,aess = <&aess>; ... }; }; Well, this could be simply wrong... I.e. the aess node should request all the phandles to mcpdm and mcbsps because it is connected to. Or it is right to use the sound node to "connect" all subsystems. Then the "aess" core could also become the child node of the target-module: target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ ... aess: aess { compatible = "ti,aess"; status = "disabled"; }; }; Although it looks better this way, it may make it even one step more difficult to resurrect the old code... And DT maintainers are not happy with otherwise undefined compatible definitions. So maybe: target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ ... aess: aess { /* revisit compatible = "ti,aess"; status = "disabled"; */ }; }; BR, Nikolaus
* H. Nikolaus Schaller <hns@goldelico.com> [200114 17:05]: > > Am 14.01.2020 um 17:46 schrieb Tony Lindgren <tony@atomide.com>: > > Care to clarify what you have in mind? The module is generic, aess > > device will be the child node. > > The existing driver is hooked into the sound root-node and looks for a > ti,aess = <&aess>; link: > > / { > sound: sound { > compatible = "ti,abe-twl6040"; > ti,model = "omap5-uevm"; > > ti,jack-detection; > ti,mclk-freq = <19200000>; > > ti,mcpdm = <&mcpdm>; > ti,mcbsp1 = <&mcbsp1>; > ti,mcbsp2 = <&mcbsp2>; > ti,mcbsp3 = <&mcbsp3>; > > ti,twl6040 = <&twl6040>; > ti,aess = <&aess>; > > ... > }; > }; > > Well, this could be simply wrong... I.e. the aess node should request > all the phandles to mcpdm and mcbsps because it is connected to. The aess label above should be in the child aess node, not in the target-module. > Or it is right to use the sound node to "connect" all subsystems. Sounds like that's all taken care of nowadays with the generic graph binding: Documentation/devicetree/bindings/graph.txt See also snd-soc-audio-graph-card and various users for it: Documentation/devicetree/bindings/sound/audio-graph-card.txt > Then the "aess" core could also become the child node of the target-module: > > target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ > ... > aess: aess { > compatible = "ti,aess"; > status = "disabled"; > }; > }; Yeah this is how it should be :) > Although it looks better this way, it may make it even one step > more difficult to resurrect the old code... Well the old phandles and properties should work the same, just put them into the child aess node. No need to stuff anything else there at the target-module level AFAIK. > And DT maintainers are not happy with otherwise undefined compatible > definitions. > > So maybe: > > target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ > ... > > aess: aess { > /* revisit > compatible = "ti,aess"; > status = "disabled"; > */ > }; > }; But we have no binding and no driver for the aess at this point.. If and when the aess driver work the child node can be just added :) Regards, Tony
> Am 14.01.2020 um 18:16 schrieb Tony Lindgren <tony@atomide.com>: > > * H. Nikolaus Schaller <hns@goldelico.com> [200114 17:05]: >>> Am 14.01.2020 um 17:46 schrieb Tony Lindgren <tony@atomide.com>: >>> Care to clarify what you have in mind? The module is generic, aess >>> device will be the child node. >> >> The existing driver is hooked into the sound root-node and looks for a >> ti,aess = <&aess>; link: >> >> / { >> sound: sound { >> compatible = "ti,abe-twl6040"; >> ti,model = "omap5-uevm"; >> >> ti,jack-detection; >> ti,mclk-freq = <19200000>; >> >> ti,mcpdm = <&mcpdm>; >> ti,mcbsp1 = <&mcbsp1>; >> ti,mcbsp2 = <&mcbsp2>; >> ti,mcbsp3 = <&mcbsp3>; >> >> ti,twl6040 = <&twl6040>; >> ti,aess = <&aess>; >> >> ... >> }; >> }; >> >> Well, this could be simply wrong... I.e. the aess node should request >> all the phandles to mcpdm and mcbsps because it is connected to. > > The aess label above should be in the child aess node, not in the > target-module. > >> Or it is right to use the sound node to "connect" all subsystems. > > Sounds like that's all taken care of nowadays with the generic > graph binding: > > Documentation/devicetree/bindings/graph.txt > > See also snd-soc-audio-graph-card and various users for it: > > Documentation/devicetree/bindings/sound/audio-graph-card.txt Ok. Will become a major rework of the driver... On the other hand the audio graph library could simplify a lot of things. > >> Then the "aess" core could also become the child node of the target-module: >> >> target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ >> ... >> aess: aess { >> compatible = "ti,aess"; >> status = "disabled"; >> }; >> }; > > Yeah this is how it should be :) > >> Although it looks better this way, it may make it even one step >> more difficult to resurrect the old code... > > Well the old phandles and properties should work the same, just put them > into the child aess node. No need to stuff anything else there at the > target-module level AFAIK. What it might need is to make the aess driver a completely separate module loaded identified through the compatible record. > >> And DT maintainers are not happy with otherwise undefined compatible >> definitions. >> >> So maybe: >> >> target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ >> ... >> >> aess: aess { >> /* revisit >> compatible = "ti,aess"; >> status = "disabled"; >> */ >> }; >> }; > > But we have no binding and no driver for the aess at this point.. That is why I propose a comment around it... But I am not sure if empty nodes are even allowed. > If and when the aess driver work the child node can be just added :) Yes, surely. So it is up to you to decide. Best regards, Nikolaus
> Am 14.01.2020 um 19:29 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > >> Am 14.01.2020 um 18:16 schrieb Tony Lindgren <tony@atomide.com>: >> >> * H. Nikolaus Schaller <hns@goldelico.com> [200114 17:05]: >>>> Am 14.01.2020 um 17:46 schrieb Tony Lindgren <tony@atomide.com>: >>>> Care to clarify what you have in mind? The module is generic, aess >>>> device will be the child node. >>> >>> The existing driver is hooked into the sound root-node and looks for a >>> ti,aess = <&aess>; link: >>> >>> / { >>> sound: sound { >>> compatible = "ti,abe-twl6040"; >>> ti,model = "omap5-uevm"; >>> >>> ti,jack-detection; >>> ti,mclk-freq = <19200000>; >>> >>> ti,mcpdm = <&mcpdm>; >>> ti,mcbsp1 = <&mcbsp1>; >>> ti,mcbsp2 = <&mcbsp2>; >>> ti,mcbsp3 = <&mcbsp3>; >>> >>> ti,twl6040 = <&twl6040>; >>> ti,aess = <&aess>; >>> >>> ... >>> }; >>> }; >>> >>> Well, this could be simply wrong... I.e. the aess node should request >>> all the phandles to mcpdm and mcbsps because it is connected to. >> >> The aess label above should be in the child aess node, not in the >> target-module. >> >>> Or it is right to use the sound node to "connect" all subsystems. >> >> Sounds like that's all taken care of nowadays with the generic >> graph binding: >> >> Documentation/devicetree/bindings/graph.txt >> >> See also snd-soc-audio-graph-card and various users for it: >> >> Documentation/devicetree/bindings/sound/audio-graph-card.txt > > Ok. Will become a major rework of the driver... > On the other hand the audio graph library could simplify a lot of things. > >> >>> Then the "aess" core could also become the child node of the target-module: >>> >>> target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ >>> ... >>> aess: aess { >>> compatible = "ti,aess"; >>> status = "disabled"; >>> }; >>> }; >> >> Yeah this is how it should be :) >> >>> Although it looks better this way, it may make it even one step >>> more difficult to resurrect the old code... >> >> Well the old phandles and properties should work the same, just put them >> into the child aess node. No need to stuff anything else there at the >> target-module level AFAIK. > > What it might need is to make the aess driver a completely separate module > loaded identified through the compatible record. I have checked our tree and it is already built into a separate module with sound/soc/ti/aess/omap-aess-core.c: { .compatible = "ti,omap4-aess", }, So > target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ > ... > aess: aess { > compatible = "ti,omap4-aess"; > status = "disabled"; > }; > }; would be what we will need. BR, Nikolaus
* H. Nikolaus Schaller <hns@goldelico.com> [200114 18:40]: > I have checked our tree and it is already built into a separate module with > > sound/soc/ti/aess/omap-aess-core.c: { .compatible = "ti,omap4-aess", }, > > So > > > target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ > > ... > > aess: aess { > > compatible = "ti,omap4-aess"; > > status = "disabled"; > > }; > > }; > > would be what we will need. OK good to hear. Regards, Tony
> Am 14.01.2020 um 22:00 schrieb Tony Lindgren <tony@atomide.com>: > > * H. Nikolaus Schaller <hns@goldelico.com> [200114 18:40]: >> I have checked our tree and it is already built into a separate module with >> >> sound/soc/ti/aess/omap-aess-core.c: { .compatible = "ti,omap4-aess", }, >> >> So >> >>> target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ >>> ... >>> aess: aess { >>> compatible = "ti,omap4-aess"; >>> status = "disabled"; >>> }; >>> }; >> >> would be what we will need. > > OK good to hear. I have cleaned up my working tree and added your patches and the one above (without status = "disabled" and could a) boot well b) see that the (non-working) aess driver module is loaded through child node So you can add my Tested-by: Nikolaus Schaller <hns@goldelico.com> BR and thanks, Nikolaus
Hi Tony,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on omap/for-next balbi-usb/next v5.5-rc6 next-20200115]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/ARM-dts-Configure-omap5-AESS/20200115-114737
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> Error: arch/arm/boot/dts/omap5-l4-abe.dtsi:447.27-28 syntax error
FATAL ERROR: Unable to parse input tree
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
* kbuild test robot <lkp@intel.com> [200116 15:55]: > Hi Tony, > > I love your patch! Yet something to improve: > > [auto build test ERROR on robh/for-next] > [also build test ERROR on omap/for-next balbi-usb/next v5.5-rc6 next-20200115] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/ARM-dts-Configure-omap5-AESS/20200115-114737 > base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next > config: arm-defconfig (attached as .config) > compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=7.5.0 make.cross ARCH=arm > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > >> Error: arch/arm/boot/dts/omap5-l4-abe.dtsi:447.27-28 syntax error > FATAL ERROR: Unable to parse input tree This patch has a dependency to a clock related patch as described in the original patch email so this error can be ignored. Regards, Tony
* H. Nikolaus Schaller <hns@goldelico.com> [200115 12:50]: > > > Am 14.01.2020 um 22:00 schrieb Tony Lindgren <tony@atomide.com>: > > > > * H. Nikolaus Schaller <hns@goldelico.com> [200114 18:40]: > >> I have checked our tree and it is already built into a separate module with > >> > >> sound/soc/ti/aess/omap-aess-core.c: { .compatible = "ti,omap4-aess", }, > >> > >> So > >> > >>> target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ > >>> ... > >>> aess: aess { > >>> compatible = "ti,omap4-aess"; > >>> status = "disabled"; > >>> }; > >>> }; > >> > >> would be what we will need. > > > > OK good to hear. > > I have cleaned up my working tree and added your patches and the one > above (without status = "disabled" and could > a) boot well > b) see that the (non-working) aess driver module is loaded through child node > > So you can add my Tested-by: Nikolaus Schaller <hns@goldelico.com> OK good to hear, thanks for testing! Tony
diff --git a/arch/arm/boot/dts/omap5-l4-abe.dtsi b/arch/arm/boot/dts/omap5-l4-abe.dtsi --- a/arch/arm/boot/dts/omap5-l4-abe.dtsi +++ b/arch/arm/boot/dts/omap5-l4-abe.dtsi @@ -426,8 +426,20 @@ target-module@c0000 { /* 0x401c0000, ap 30 1e.0 */ }; target-module@f1000 { /* 0x401f1000, ap 32 20.0 */ - compatible = "ti,sysc"; - status = "disabled"; + compatible = "ti,sysc-omap4", "ti,sysc"; + reg = <0xf1000 0x4>, + <0xf1010 0x4>; + reg-names = "rev", "sysc"; + ti,sysc-midle = <SYSC_IDLE_FORCE>, + <SYSC_IDLE_NO>, + <SYSC_IDLE_SMART>, + <SYSC_IDLE_SMART_WKUP>; + ti,sysc-sidle = <SYSC_IDLE_FORCE>, + <SYSC_IDLE_NO>, + <SYSC_IDLE_SMART>; + /* Domains (V, P, C): iva, abe_pwrdm, abe_clkdm */ + clocks = <&abe_clkctrl OMAP5_AESS_CLKCTRL 0>; + clock-names = "fck"; #address-cells = <1>; #size-cells = <1>; ranges = <0x0 0xf1000 0x1000>,
We are missing AESS for omap5. Looks like it's similar to what we have for omap4, and this gets ti-sysc interconnect target module driver to detect it properly. Note that we currently have no child device driver available for it. Cc: H. Nikolaus Schaller <hns@goldelico.com> Cc: Matthijs van Duin <matthijsvanduin@gmail.com> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> Cc: Tero Kristo <t-kristo@ti.com> Signed-off-by: Tony Lindgren <tony@atomide.com> --- Note that this depends on "[PATCH] clk: ti: omap5: Add missing AESS clock". arch/arm/boot/dts/omap5-l4-abe.dtsi | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)