ARM: kirkwood: extend the kirkwood i2s driver for DT usage
Subsystems (3)
PATCH v3 1/1 ARM: kirkwood: extend the kirkwood i2s driver for DT usage
The kirkwood i2s driver is used without DT in the Kirkwood machine. This patch adds a DT compatible definition for use in other Marvell machines as the Armada 88AP510 (Dove).
---
v3
- change compatible from kirkwood to mvebu (Andrew Lunn)
v2
- this patch replaces the previous
[PATCH 2/2] ARM: kirkwood: extend the kirkwood i2s driver for DT usage
- 2 possible clocks (Sebastian Hesselbarth)
- shorter io mapping (Andrew Lunn)
- less #ifdef's
--- Documentation/devicetree/bindings/sound/kirkwood-i2s.txt | 24 +++++++++++ sound/soc/kirkwood/kirkwood-i2s.c | 58 ++++++++++++++++++++------ 2 files changed, 70 insertions(+), 12 deletions(-)
Documentation/devicetree/bindings/sound/kirkwood-i2s.txt new file +24 -0
@@ -0,0 +1,24 @@ +* mvebu (Kirkwood, Dove, Armada 370) I2S controller + +Required properties: + +- compatible: "marvell,mvebu-i2s" + +- reg: physical base address of the controller and length of memory mapped + region. + +- interrupts: list of two irq numbers. + The first irq is used for data flow and the second one is used for errors. + +- clocks: one or two phandles. + The first one is mandatory and defines the internal clock. + The second one is optional and defines an external clock. + +Example: + +i2s1: audio-controller@b4000 { + compatible = "marvell,mvebu-i2s"; + reg = <0xb4000 0x2210>; + interrupts = <21>, <22>; + clocks = <&gate_clk 13>; +};
sound/soc/kirkwood/kirkwood-i2s.c +46 -12
124 lines not on this page · raw message
Signature · 2 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
Quoted 3 lines · Jean-Francois Moine
The kirkwood i2s driver is used without DT in the Kirkwood machine. This patch adds a DT compatible definition for use in other Marvell machines as the Armada 88AP510 (Dove).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of devm_clk_get() to of_clk_get() necessitates a clk_put().
Can someone please provide a devm_of_clk_get() function before we accept any more patches doing this kind of thing?
The other issue is that kirkwood-dma.c and kirkwood-i2s.c should be one driver; they're not separate hardware on these platforms, so that needs fixing before coming up with any kind of DT model for this stuff.
Quoted 8 lines · Russell King - ARM Linux
Quoted 3 lines · Jean-Francois Moine
The kirkwood i2s driver is used without DT in the Kirkwood machine. This patch adds a DT compatible definition for use in other Marvell machines as the Armada 88AP510 (Dove).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of
devm_clk_get()toof_clk_get()necessitates aclk_put().
That's just a very poor usage of the API in this patch. devm_clk_get() works fine on DT and non-DT platforms. The driver shouldn't need any changes in that regard.
- Lars
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Jean-Francois Moine
The kirkwood i2s driver is used without DT in the Kirkwood machine. This patch adds a DT compatible definition for use in other Marvell machines as the Armada 88AP510 (Dove).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of
devm_clk_get()toof_clk_get()necessitates aclk_put().Can someone please provide a
devm_of_clk_get()function before we accept any more patches doing this kind of thing?
The question is why isn't regular [devm_]clk_get used here? I don't know what the Marvell stuff is doing here, but if the clk stuff is implemented correctly then
np = pdev->dev.of_node; priv->clk = of_clk_get(np, 0);
and
devm_clk_get(&pdev->dev, "label");
should both work
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Quoted 17 lines · Sascha Hauer
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Jean-Francois Moine
The kirkwood i2s driver is used without DT in the Kirkwood machine. This patch adds a DT compatible definition for use in other Marvell machines as the Armada 88AP510 (Dove).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of
devm_clk_get()toof_clk_get()necessitates aclk_put().Can someone please provide a
devm_of_clk_get()function before we accept any more patches doing this kind of thing?The question is why isn't regular [devm_]clk_get used here? I don't know what the Marvell stuff is doing here, but if the clk stuff is implemented correctly then
Quoted 8 lines
np = pdev->dev.of_node; priv->clk = of_clk_get(np, 0);and
devm_clk_get(&pdev->dev, "label");should both work
You tell me - I keep seeing patches on this list converting drivers from using devm_clk_get() to using of_clk_get(). Maybe of_clk_get() should be marked deprecated to stop people using it?
Quoted 23 lines · Russell King - ARM Linux
Quoted 22 lines · Sascha Hauer
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Jean-Francois Moine
The kirkwood i2s driver is used without DT in the Kirkwood machine. This patch adds a DT compatible definition for use in other Marvell machines as the Armada 88AP510 (Dove).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of
devm_clk_get()toof_clk_get()necessitates aclk_put().Can someone please provide a
devm_of_clk_get()function before we accept any more patches doing this kind of thing?The question is why isn't regular [devm_]clk_get used here? I don't know what the Marvell stuff is doing here, but if the clk stuff is implemented correctly then
np = pdev->dev.of_node; priv->clk = of_clk_get(np, 0);and
Quoted 7 lines
Quoted 3 lines
devm_clk_get(&pdev->dev, "label");should both work
You tell me - I keep seeing patches on this list converting drivers from using
devm_clk_get()to usingof_clk_get(). Maybeof_clk_get()should be marked deprecated to stop people using it?
I don't think that's possible. There are some valid usecases for of_clk_get() when there's no struct device * available, like in many clocksource drivers.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Mark Brown
maintainersound/soc/kirkwood/kirkwood-i2s.c static int kirkwood_i2s_dev_probe(struct platform_device *pdev) · line 491
Quoted 5 lines · Jean-Francois Moine
sound/soc/kirkwood/kirkwood-i2s.c +5
-0+ np = pdev->dev.of_node; +if (np) { +struct of_phandle_args clkspec; + + priv->burst =128 ;/* might be 32 or 128 */
The comment says this needs to be variable (depending on what?) but it's hard coded.
sound/soc/kirkwood/kirkwood-i2s.c static int kirkwood_i2s_dev_probe(struct platform_device *pdev) · line 495
Quoted 4 lines
sound/soc/kirkwood/kirkwood-i2s.c +4
-0+ priv->clk =of_clk_get (np,0 );/* internal clock */ + err =of_parse_phandle_with_args (np, +"clocks" ,"#clock-cells" ,1 , + &clkspec);
As others have pointed out if you need to change the clock get code there's something wrong here, DT should be handled transparently by the clock API.
Quoted 11 lines · Mark Brown
sound/soc/kirkwood/kirkwood-i2s.c static int kirkwood_i2s_dev_probe(struct platform_device *pdev) · line 491
Quoted 5 lines · Jean-Francois Moine
sound/soc/kirkwood/kirkwood-i2s.c +5
-0+ np = pdev->dev.of_node; +if (np) { +struct of_phandle_args clkspec; + + priv->burst =128 ;/* might be 32 or 128 */ The comment says this needs to be variable (depending on what?) but it's hard coded.
Quoted 8 lines
sound/soc/kirkwood/kirkwood-i2s.c static int kirkwood_i2s_dev_probe(struct platform_device *pdev) · line 495
Quoted 4 lines
sound/soc/kirkwood/kirkwood-i2s.c +4
-0+ priv->clk =of_clk_get (np,0 );/* internal clock */ + err =of_parse_phandle_with_args (np, +"clocks" ,"#clock-cells" ,1 , + &clkspec);As others have pointed out if you need to change the clock get code there's something wrong here, DT should be handled transparently by the clock API.
IMHO the reason why of_clk_get() was/is mis-used in that way is mostly compatibility with legacy platform_data based setup.
Kirkwood-i2s never knew about anything else than internal clock, then Dove allows additional external clock input, aso. All changes are incremental and more or less sane. But now is a good opportunity to clean up this.
As Sascha Hauer pointed out, clocks should be distinguished by names (clock-names property) instead of position and then use devm_clk_get(&pdev->dev, "internal") and devm_clk_get(&pdev->dev, "external") respectively.
This will possibly also require to update platform_data and legacy users of kirkwood-i2s or have different setup functions for non-DT and DT.
Also, while ASoC API separates the audio-controller into cpu-side and codec-side parts, the DT should not. IIRC and as Russell repeated again, we mentioned to merge kirkwood-i2s.c and kirkwood-dma.c into a single file, didn't we?
I know we didn't care that much in the past, but one last thing that I catched while reading another thead about compatible strings:
We should really be more careful about those. The correct usage of compatible strings should be "marvell,mvebu-i2s" as common fallback, but also "marvell,dove-i2s" and "marvell,kirkwood-i2s" for the SoC dtsi files. We do not need to have all possible compatible strings in the _driver's_ of_device_id table but the dtsi should contain them.
Finally, I2S DT node will end up as:
i2s1: audio-controller@b4000 {
compatible = "marvell,dove-i2s", "marvell,mvebu-i2s";
reg = <0xb4000 0x2210>;
interrupts = <21>, <22>;
clocks = <&gate_clk 13>, <&si5351a 1>;
clock-names = "internal", "external";
};Jean-Francois, can you re-spin your patches with the comments made by others and the above summary? I really like to see i2s for DT soon, although we will not be able to support multiple codecs per DAI, yet.
Sebastian
Mark Brown
maintainerQuoted 8 lines · Sebastian Hesselbarth
Quoted 3 lines · Mark Brown
As others have pointed out if you need to change the clock get code there's something wrong here, DT should be handled transparently by the clock API.
IMHO the reason why
of_clk_get()was/is mis-used in that way is mostly compatibility with legacyplatform_databased setup.
I'm sorry, but this doesn't make a great deal of sense to me. Can you be more specific?
Quoted 8 lines
As Sascha Hauer pointed out, clocks should be distinguished by names (clock-names property) instead of position and then use
devm_clk_get(&pdev->dev, "internal")anddevm_clk_get(&pdev->dev, "external")respectively.This will possibly also require to update
platform_dataand legacy users of kirkwood-i2s or have different setup functions for non-DT and DT.
Why would this be required? The driver is already asking for multiple clocks...
Quoted 2 lines
Also, while ASoC API separates the audio-controller into cpu-side and codec-side parts, the DT should not. IIRC and as Russell repeated
You mean DAI and DMA here? I already commented on that in my review of the DMA binding.
Quoted 2 lines
again, we mentioned to merge
kirkwood-i2s.candkirkwood-dma.cinto a single file, didn't we?
That's been discussed several times but nobody's actually done it.
Quoted 18 lines · Mark Brown
Quoted 8 lines · Sebastian Hesselbarth
Quoted 3 lines · Mark Brown
As others have pointed out if you need to change the clock get code there's something wrong here, DT should be handled transparently by the clock API.
IMHO the reason why
of_clk_get()was/is mis-used in that way is mostly compatibility with legacyplatform_databased setup.I'm sorry, but this doesn't make a great deal of sense to me. Can you be more specific?
Quoted 4 lines
As Sascha Hauer pointed out, clocks should be distinguished by names (clock-names property) instead of position and then use
devm_clk_get(&pdev->dev, "internal")anddevm_clk_get(&pdev->dev, "external")respectively.
Quoted 6 lines
Quoted 3 lines
This will possibly also require to update
platform_dataand legacy users of kirkwood-i2s or have different setup functions for non-DT and DT.Why would this be required? The driver is already asking for multiple clocks...
The driver is asking for multiple *DT based* clocks. Legacy platform_data has never been updated to reflect that. Mainly because multiple clocks are only supported on Dove, which has no active non-DT board in mainline.
Quoted 5 lines
Quoted 2 lines
Also, while ASoC API separates the audio-controller into cpu-side and codec-side parts, the DT should not. IIRC and as Russell repeated
You mean DAI and DMA here? I already commented on that in my review of the DMA binding.
Yes.
Quoted 4 lines
Quoted 2 lines
again, we mentioned to merge
kirkwood-i2s.candkirkwood-dma.cinto a single file, didn't we?That's been discussed several times but nobody's actually done it.
Correct, that is why I repeated that request to Jean-Francois.
Sebastian
Quoted 7 lines · Sebastian Hesselbarth
Quoted 4 lines · Mark Brown
Quoted 2 lines
again, we mentioned to merge
kirkwood-i2s.candkirkwood-dma.cinto a single file, didn't we?That's been discussed several times but nobody's actually done it.
Correct, that is why I repeated that request to Jean-Francois.
On that, I have some initial patches which start doing that, but I've not had time to really complete the effort - the problem is working with one tree (mainline) while testing on Rabeeh's kernel tree to get a fully working Cubox platform is quite a hinderance.
Not only that, but the lack of assistance from ASoC people on the multiple substream stuff really doesn't help one iota when it comes to making forward progress with SPDIF support. I've explained to them the problem (SPDIF and I2S if used together must be enabled simultaneously or only one is permitted at any one time); they've suggested using the multiple PCM substream support, and then it's a case that "oh it's undocumented" "oh there's no examples yet".
And then there's the problem of understanding the ASoC DAPM stuff "oh it's just a graph walk, you don't need to know the internals" is what I get from ASoC people. Well, yes I do need to know the internals, so I can find out whether it's going to startup a second substream while a first one is active, and end up with SPDIF enabled non-concurrently with I2S.
Mark Brown
maintainerQuoted 3 lines · Russell King - ARM Linux
Not only that, but the lack of assistance from ASoC people on the multiple substream stuff really doesn't help one iota when it comes to making forward progress with SPDIF support. I've explained to them the
It'd probably help if the people looking at this hardware started to contribute some of the work that's being done here; this should help move things forward by looking at concrete code rather than the very general discussion that's happening at the minute.
Quoted 4 lines
problem (SPDIF and I2S if used together must be enabled simultaneously or only one is permitted at any one time); they've suggested using the multiple PCM substream support, and then it's a case that "oh it's undocumented" "oh there's no examples yet".
As previously mentioned the OMAP4 and Qualcomm stuff is there, it's just out of tree - there's also Haswell but I'm not sure if that's been made public yet (Liam?).
Quoted 6 lines
And then there's the problem of understanding the ASoC DAPM stuff "oh it's just a graph walk, you don't need to know the internals" is what I get from ASoC people. Well, yes I do need to know the internals, so I can find out whether it's going to startup a second substream while a first one is active, and end up with SPDIF enabled non-concurrently with I2S.
As ever the answer is that DAPM will enable any connected audio path and not enable other ones so it is possible to enable only one of multiple paths (as you'd expect, really).
Can you be more specific about the problems you're having following the code? It should be relatively simple to trace through what's going on from dapm_power_widgets()...
Mark Brown
maintainerQuoted 9 lines · Sebastian Hesselbarth
Quoted 2 lines · Mark Brown
Why would this be required? The driver is already asking for multiple clocks...
The driver is asking for multiple *DT based* clocks. Legacy
platform_datahas never been updated to reflect that. Mainly because multiple clocks are only supported on Dove, which has no active non-DT board in mainline.
Why would platform data have anything to do with this? To repeat again the way the clocks are mapped should be totally transparent to the driver requesting them, if it isn't then the driver is not using the API properly.
Quoted 11 lines · Mark Brown
Quoted 9 lines · Sebastian Hesselbarth
Quoted 2 lines · Mark Brown
Why would this be required? The driver is already asking for multiple clocks...
The driver is asking for multiple *DT based* clocks. Legacy
platform_datahas never been updated to reflect that. Mainly because multiple clocks are only supported on Dove, which has no active non-DT board in mainline.
Quoted 4 lines
Why would platform data have anything to do with this? To repeat again the way the clocks are mapped should be totally transparent to the driver requesting them, if it isn't then the driver is not using the API properly.
Total rubbish. Of course the driver needs to know what the clocks are, so that it can program its hardware accordingly.
What you have here is an audio block which has two clock inputs. One clock is the system clock, whose rate can be adjusted but has a massive impact on the rest of the system. The other clock input is via an external pin.
Internally, in one of the audio blocks registers is a set of control bits which select which clock is to be used to generate the internal timing for the block, and a divisor on that input.
Quoted 1 line
From that description, anyone can see that it is absolutely required for
the driver to know which clock is which, so it can program the clock input selection bit appropriately.
In this case, it has always been the rule with the clock API that it shall be used as:
clk_get(device, "internal");
to get the internal clock, and:
clk_get(device, "external");
to get the external clock - or whatever names are appropriate to name the clock _inputs_.
Mark Brown
maintainerQuoted 9 lines · Russell King - ARM Linux
Quoted 4 lines · Mark Brown
Why would platform data have anything to do with this? To repeat again the way the clocks are mapped should be totally transparent to the driver requesting them, if it isn't then the driver is not using the API properly.
Total rubbish. Of course the driver needs to know what the clocks are, so that it can program its hardware accordingly.
Of course, but why through platform data? Though reading your mail I think we're talking at cross purposes here...
Quoted 11 lines
In this case, it has always been the rule with the clock API that it shall be used as:
clk_get(device, "internal");to get the internal clock, and:
clk_get(device, "external");to get the external clock - or whatever names are appropriate to name the clock _inputs_.
This is exactly what I'd expect, yes - but it doesn't need the driver to have platform data as far as I can see. As you say the driver should be requesting the two clock inputs the IP has by name which doesn't need it to have any platform data, it just needs code like you have above. The board specific configuration is all handled within the clk_get() calls.
Quoted 6 lines · On Tue, 23 Jul 2013 15:30:57 +0200 Sebastian Hesselbarth…
Quoted 4 lines
Quoted 2 lines
again, we mentioned to merge
kirkwood-i2s.candkirkwood-dma.cinto a single file, didn't we?That's been discussed several times but nobody's actually done it.
Correct, that is why I repeated that request to Jean-Francois.
Thanks for all your remarks.
I am testing a new version, with mainly:
- "internal" and "external" clocks
- kirkwood-i2s and kirkwood-dma in one module
- no S/PDIF (I have not yet tried i2s with the tda998x)
Signature · 2 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/