| Message ID | 1491493236-2574-2-git-send-email-olivier.moysan@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 06, 2017 at 05:40:35PM +0200, olivier moysan wrote: > Add documentation of device tree bindings for STM32 SPI/I2S. > > Signed-off-by: olivier moysan <olivier.moysan@st.com> > --- > .../devicetree/bindings/sound/st,stm32h7-i2s.txt | 71 ++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/st,stm32h7-i2s.txt > > diff --git a/Documentation/devicetree/bindings/sound/st,stm32h7-i2s.txt b/Documentation/devicetree/bindings/sound/st,stm32h7-i2s.txt > new file mode 100644 > index 0000000..b99467a > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/st,stm32h7-i2s.txt > @@ -0,0 +1,71 @@ > +STMicroelectronics STM32 SPI/I2S Controller > + > +The SPI/I2S block supports I2S/PCM protocols when configured on I2S mode. > +Only some SPI instances support I2S. > + > +Required properties: > + - compatible: Must be "st,stm32h7-i2s" > + - #sound-dai-cells: Must be 1. (one parameter) > + This parameter allows to specify CPU DAI index in soundcard CPU dai link. > + index 0: playback DAI > + index 1: capture DAI > + index 2: full duplex DAI Is this still needed for graph-card? > + - reg: Offset and length of the device's register set. > + - interrupts: Must contain the interrupt line id. > + - clocks: Must contain phandle and clock specifier pairs for each entry > + in clock-names. > + - clock-names: Must contain "i2sclk", "pclk", "x8k" and "x11k". > + "i2sclk": clock which feeds the internal clock generator > + "pclk": clock which feeds the peripheral bus interface > + "x8k": I2S parent clock for sampling rates multiple of 8kHz. > + "x11k": I2S parent clock for sampling rates multiple of 11.025kHz. > + - dmas: DMA specifiers for tx and rx dma. > + See Documentation/devicetree/bindings/dma/stm32-dma.txt. > + - dma-names: Identifier for each DMA request line. Must be "tx" and "rx". > + - pinctrl-names: should contain only value "default" > + - pinctrl-0: see Documentation/devicetree/bindings/pinctrl/pinctrl-stm32.txt > + > +Optional properties: > + - resets: Reference to a reset controller asserting the reset controller > + > +Example: > +sound_card { > + compatible = "audio-graph-card"; > + dais = <&i2s2_port 0>; > +}; > + > +i2s2: audio-controller@40003800 { > + compatible = "st,stm32h7-i2s"; > + #sound-dai-cells = <1>; > + reg = <0x40003800 0x400>; > + interrupts = <36>; > + clocks = <&rcc PCLK1>, <&rcc SPI2_CK>, <&rcc PLL1_Q>, <&rcc PLL2_P>; > + clock-names = "pclk", "i2sclk", "x8k", "x11k"; > + dmas = <&dmamux2 2 39 0x400 0x1>, > + <&dmamux2 3 40 0x400 0x1>; > + dma-names = "rx", "tx"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_i2s2>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + i2s2_port: port@0 { > + reg = <0>; > + cpu_endpoint: endpoint { > + remote-endpoint = <&codec_endpoint>; > + audio-graph-card,format = "i2s"; > + audio-graph-card,bitclock-master = <&codec_endpoint>; > + audio-graph-card,frame-master = <&codec_endpoint>; The 'audio-graph-card,' part has been dropped. > + }; > + }; > +}; > + > +audio-codec { > + codec_port: port { > + codec_endpoint: endpoint { > + remote-endpoint = <&cpu_endpoint>; > + }; > + }; > +}; > -- > 1.9.1 >
On Mon, Apr 10, 2017 at 02:48:32PM -0500, Rob Herring wrote: > On Thu, Apr 06, 2017 at 05:40:35PM +0200, olivier moysan wrote: > > +Required properties: > > + - compatible: Must be "st,stm32h7-i2s" > > + - #sound-dai-cells: Must be 1. (one parameter) > > + This parameter allows to specify CPU DAI index in soundcard CPU dai link. > > + index 0: playback DAI > > + index 1: capture DAI > > + index 2: full duplex DAI > Is this still needed for graph-card? The graph card is blocked on your review... I'm also not clear how without something like this we'd be able to identify a specific DAI within a device if we don't have a way of identifying them.
Hello Rob, Mark Thanks for your review. Please find my comments below. On 04/11/2017 04:32 PM, Mark Brown wrote: > On Mon, Apr 10, 2017 at 02:48:32PM -0500, Rob Herring wrote: >> On Thu, Apr 06, 2017 at 05:40:35PM +0200, olivier moysan wrote: > >>> +Required properties: >>> + - compatible: Must be "st,stm32h7-i2s" >>> + - #sound-dai-cells: Must be 1. (one parameter) >>> + This parameter allows to specify CPU DAI index in soundcard CPU dai link. >>> + index 0: playback DAI >>> + index 1: capture DAI >>> + index 2: full duplex DAI > >> Is this still needed for graph-card? > > The graph card is blocked on your review... I'm also not clear how > without something like this we'd be able to identify a specific DAI > within a device if we don't have a way of identifying them. > Yes, it seems that audio graph card does not allow to select a specific DAI. This will probably be necessary. However, regarding STM32 I2S driver, I'm wondering if selecting DAI is the best way to configure interface as tx, rx or fd. Maybe, it is more relevant to configure DAI according to DMA configuration from I2S node. This would moreover avoid to allocate 2 dmas channels when not necessary (tx or rx only). If you agree with this, I will implement this change in a v2. BRs olivier
On Tue, Apr 11, 2017 at 9:32 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Apr 10, 2017 at 02:48:32PM -0500, Rob Herring wrote: >> On Thu, Apr 06, 2017 at 05:40:35PM +0200, olivier moysan wrote: > >> > +Required properties: >> > + - compatible: Must be "st,stm32h7-i2s" >> > + - #sound-dai-cells: Must be 1. (one parameter) >> > + This parameter allows to specify CPU DAI index in soundcard CPU dai link. >> > + index 0: playback DAI >> > + index 1: capture DAI >> > + index 2: full duplex DAI > >> Is this still needed for graph-card? > > The graph card is blocked on your review... Maybe if there were more reviewers it would move faster. I don't know ASoC that well. > I'm also not clear how > without something like this we'd be able to identify a specific DAI > within a device if we don't have a way of identifying them. Isn't that what the graph does? "dais" points to a list of ports which are the specific DAIs whether there are multiple ones in a single device or multiple devices with a single DAI each. Rob
On Tue, Apr 11, 2017 at 11:02:57AM -0500, Rob Herring wrote: > On Tue, Apr 11, 2017 at 9:32 AM, Mark Brown <broonie@kernel.org> wrote: > > The graph card is blocked on your review... > Maybe if there were more reviewers it would move faster. I don't know > ASoC that well. The times I've looked at it recently it's been stuck in DT style issues rather than anything substantially ASoC related, as far as I can tell the binding is essentially empty from an ASoC point of view and inherited from the of_graph binding. There's bits in that are a bit random like specifically listing the CPU DAIs and only them but that just looks like one of these random DT things that's predetermined. I really can't see anything at all in there to review from an ASoC point of view, I've applied the changes that don't seem blocked on the binding. If there's something you're looking for then please say... We never seem to make any progress on the generic changes in drivers/of at the start of the series either... > > I'm also not clear how > > without something like this we'd be able to identify a specific DAI > > within a device if we don't have a way of identifying them. > Isn't that what the graph does? "dais" points to a list of ports which > are the specific DAIs whether there are multiple ones in a single > device or multiple devices with a single DAI each. But the ports can still have indexes AFAICT (the examples show port@0, port@1 and so on) so we still need to define what those indexes mean which is what this is doing?
On Tue, Apr 11, 2017 at 03:44:52PM +0000, Olivier MOYSAN wrote: > However, regarding STM32 I2S driver, I'm wondering if selecting DAI > is the best way to configure interface as tx, rx or fd. Why do you even need to configure this? > Maybe, it is more relevant to configure DAI according to DMA > configuration from I2S node. > This would moreover avoid to allocate 2 dmas channels when not > necessary (tx or rx only). > If you agree with this, I will implement this change in a v2. That sounds wrong, I'd expect this wiring to be done statically as part of the .dtsi for the SoC (or just grabbed as needed at runtime if things are flexbile enough) rather than being a configuration thing done per board... I had thought that this was configuration reflecting different ways of taping out the IP with different feature sets, is that not the case?
Hello Mark, On 04/11/2017 11:10 PM, Mark Brown wrote: > On Tue, Apr 11, 2017 at 03:44:52PM +0000, Olivier MOYSAN wrote: > >> However, regarding STM32 I2S driver, I'm wondering if selecting DAI >> is the best way to configure interface as tx, rx or fd. > > Why do you even need to configure this? > The IP provides two data wires, SD in and SD out. So it can be configured either as capture or playback only, or full-duplex. This corresponds to a mode selection through a register configuration. >> Maybe, it is more relevant to configure DAI according to DMA >> configuration from I2S node. >> This would moreover avoid to allocate 2 dmas channels when not >> necessary (tx or rx only). >> If you agree with this, I will implement this change in a v2. > > That sounds wrong, I'd expect this wiring to be done statically as part > of the .dtsi for the SoC (or just grabbed as needed at runtime if > things are flexbile enough) rather than being a configuration thing done > per board... I had thought that this was configuration reflecting > different ways of taping out the IP with different feature sets, is that > not the case? > This configuration is board dependent. The IP may be used as rx, tx or fd depending on board. So I think it can make sense to have a DMA configuration linked to board, and to set IP mode accordingly. BRs olivier
On Wed, Apr 12, 2017 at 08:30:31AM +0000, Olivier MOYSAN wrote: > On 04/11/2017 11:10 PM, Mark Brown wrote: > > That sounds wrong, I'd expect this wiring to be done statically as part > > of the .dtsi for the SoC (or just grabbed as needed at runtime if > > things are flexbile enough) rather than being a configuration thing done > > per board... I had thought that this was configuration reflecting > > different ways of taping out the IP with different feature sets, is that > > not the case? > This configuration is board dependent. The IP may be used as rx, tx or > fd depending on board. So I think it can make sense to have a DMA > configuration linked to board, and to set IP mode accordingly. It is totally normal to just not use one direction in a given system, we don't normally need to do anything special to handle things. I'm a bit confused as to what's different here and needs configuring?
Hello Mark, On 04/12/2017 01:32 PM, Mark Brown wrote: > On Wed, Apr 12, 2017 at 08:30:31AM +0000, Olivier MOYSAN wrote: >> On 04/11/2017 11:10 PM, Mark Brown wrote: > >>> That sounds wrong, I'd expect this wiring to be done statically as part >>> of the .dtsi for the SoC (or just grabbed as needed at runtime if >>> things are flexbile enough) rather than being a configuration thing done >>> per board... I had thought that this was configuration reflecting >>> different ways of taping out the IP with different feature sets, is that >>> not the case? > >> This configuration is board dependent. The IP may be used as rx, tx or >> fd depending on board. So I think it can make sense to have a DMA >> configuration linked to board, and to set IP mode accordingly. > > It is totally normal to just not use one direction in a given system, we > don't normally need to do anything special to handle things. I'm a bit > confused as to what's different here and needs configuring? > The IP does not provide an audio channel configured either as rx or tx. I agree, that in such case, the cpu driver does not generally need to worry about direction and there is nothing special required to handle it. Here the IP provides 2 channels, 1 tx and 1 rx, which may be active or not. The driver has to know which channel is used, and if both channels have to be managed. The affected registers depend on selected channel. Moreover specific processing has to be performed if both channels are used. BRs olivier
On Wed, Apr 12, 2017 at 12:58:16PM +0000, Olivier MOYSAN wrote: > On 04/12/2017 01:32 PM, Mark Brown wrote: > > It is totally normal to just not use one direction in a given system, we > > don't normally need to do anything special to handle things. I'm a bit > > confused as to what's different here and needs configuring? > The IP does not provide an audio channel configured either as rx or tx. > I agree, that in such case, the cpu driver does not generally need > to worry about direction and there is nothing special required to handle it. No, that's not what I'm saying - such hardware would be extremely unusual. > Here the IP provides 2 channels, 1 tx and 1 rx, which may be active or > not. The driver has to know which channel is used, and if both channels > have to be managed. The affected registers depend on selected channel. This sounds like essentially every audio controller out there. The overwhelming majority of controllers do exactly as you describe and have both directions in the same IP, this really doesn't seem at all unusual. Off the top of my head I can only think of one SoC family which combines multiple IPs to do bidirectional audio (though I didn't check). It really feels like there is something different here and I'm just missing it. > Moreover specific processing has to be performed if both channels are used. Given that this case has to be supported anyway I'd be more inclined to ask the question the other way around TBH.
Hello Mark, On 04/12/2017 05:38 PM, Mark Brown wrote: > On Wed, Apr 12, 2017 at 12:58:16PM +0000, Olivier MOYSAN wrote: >> On 04/12/2017 01:32 PM, Mark Brown wrote: > >>> It is totally normal to just not use one direction in a given system, we >>> don't normally need to do anything special to handle things. I'm a bit >>> confused as to what's different here and needs configuring? > >> The IP does not provide an audio channel configured either as rx or tx. >> I agree, that in such case, the cpu driver does not generally need >> to worry about direction and there is nothing special required to handle it. > > No, that's not what I'm saying - such hardware would be extremely > unusual. > >> Here the IP provides 2 channels, 1 tx and 1 rx, which may be active or >> not. The driver has to know which channel is used, and if both channels >> have to be managed. The affected registers depend on selected channel. > > This sounds like essentially every audio controller out there. The > overwhelming majority of controllers do exactly as you describe and have > both directions in the same IP, this really doesn't seem at all unusual. > Off the top of my head I can only think of one SoC family which combines > multiple IPs to do bidirectional audio (though I didn't check). > > It really feels like there is something different here and I'm just > missing it. > Yes, I think I need to give more details on design choices. The IP provides 2 channels. I can see mainly 3 solutions to link dais to these channels: 1) 2 static dais NOT exclusive - dai tx - dai rx The IP exhibits a mode register, where you select mode TX, RX or FD. There are 2 two options to manage this register. option 1: start first channel with mode RX or TX when second channel is started, mode has to be changed to FD. Transfers have to be stopped before changing configuration registers, so this leads to cuts in audio stream. option 2: start a first channel with mode FD. In this case, we may have unpredictable behavior for the stream which is not already started. probably underrun/overrun. So, this solution rises problem for full-duplex management. 2) 3 static dais exclusive - dai tx - dai rx - dai rx-tx (fd) This is the current implementation. The choice of the dai is done at probe time. It is provided by DT through sound-dai parameter. When dai fd is selected, after starting first stream, we assume that second stream will be started. In this case we wait for second stream to be available before enabling IP and starting transfers. 3) 1 dynamic dai - dai rx or tx or fd (according to dma conf in IP node) Here the driver exposes only a single dai constructed from dma configuration provided by IP DT node. This allows to get ride of sound-dai parameter. This was my suggestion after Rob comments on DT bindings. Hope this will help clarifying design constraints. >> Moreover specific processing has to be performed if both channels are used. > > Given that this case has to be supported anyway I'd be more inclined to > ask the question the other way around TBH. > Best regards olivier
On Thu, Apr 13, 2017 at 08:01:34AM +0000, Olivier MOYSAN wrote: > 1) 2 static dais NOT exclusive > - dai tx > - dai rx > The IP exhibits a mode register, where you select mode TX, RX or FD. > There are 2 two options to manage this register. > option 1: > start first channel with mode RX or TX > when second channel is started, mode has to be changed to FD. > Transfers have to be stopped before changing configuration > registers, so this leads to cuts in audio stream. > option 2: > start a first channel with mode FD. > In this case, we may have unpredictable behavior for the stream > which is not already started. probably underrun/overrun. > So, this solution rises problem for full-duplex management. > 2) 3 static dais exclusive > - dai tx > - dai rx > - dai rx-tx (fd) > This is the current implementation. > The choice of the dai is done at probe time. It is provided by DT > through sound-dai parameter. > When dai fd is selected, after starting first stream, we assume that > second stream will be started. In this case we wait for second stream > to be available before enabling IP and starting transfers. > 3) 1 dynamic dai > - dai rx or tx or fd (according to dma conf in IP node) > Here the driver exposes only a single dai constructed from dma > configuration provided by IP DT node. > This allows to get ride of sound-dai parameter. None of these options reflect how normal I2S controllers present themselves in DT. To repeat, you should present a single bidirectional DAI for the single physical bidirectional I2S controller that your hardware has. If it's not possible to figure out a way to make the controller support simultaneous playback and record with the two started independently then the driver should just return an error if userspace tries to start the second direction up. This will severely limit the utility of the driver as Linux generally treats playback and record independently but that's going to apply just as much with any of the options involving multiple DAIs or configuration in DT. You might be able to do something with feeding it dummy data I guess?
Hello Mark, On 04/26/2017 06:15 PM, Mark Brown wrote: > On Thu, Apr 13, 2017 at 08:01:34AM +0000, Olivier MOYSAN wrote: > >> 1) 2 static dais NOT exclusive >> - dai tx >> - dai rx >> The IP exhibits a mode register, where you select mode TX, RX or FD. >> There are 2 two options to manage this register. >> option 1: >> start first channel with mode RX or TX >> when second channel is started, mode has to be changed to FD. >> Transfers have to be stopped before changing configuration >> registers, so this leads to cuts in audio stream. >> option 2: >> start a first channel with mode FD. >> In this case, we may have unpredictable behavior for the stream >> which is not already started. probably underrun/overrun. >> So, this solution rises problem for full-duplex management. > >> 2) 3 static dais exclusive >> - dai tx >> - dai rx >> - dai rx-tx (fd) >> This is the current implementation. >> The choice of the dai is done at probe time. It is provided by DT >> through sound-dai parameter. >> When dai fd is selected, after starting first stream, we assume that >> second stream will be started. In this case we wait for second stream >> to be available before enabling IP and starting transfers. > >> 3) 1 dynamic dai >> - dai rx or tx or fd (according to dma conf in IP node) >> Here the driver exposes only a single dai constructed from dma >> configuration provided by IP DT node. >> This allows to get ride of sound-dai parameter. > > None of these options reflect how normal I2S controllers present > themselves in DT. To repeat, you should present a single bidirectional > DAI for the single physical bidirectional I2S controller that your > hardware has. > > If it's not possible to figure out a way to make the controller support > simultaneous playback and record with the two started independently then > the driver should just return an error if userspace tries to start the > second direction up. This will severely limit the utility of the driver > as Linux generally treats playback and record independently but that's > going to apply just as much with any of the options involving multiple > DAIs or configuration in DT. You might be able to do something with > feeding it dummy data I guess? > Ok. I will implement a single bidirectional DAI in v2. Best regards Olivier
diff --git a/Documentation/devicetree/bindings/sound/st,stm32h7-i2s.txt b/Documentation/devicetree/bindings/sound/st,stm32h7-i2s.txt new file mode 100644 index 0000000..b99467a --- /dev/null +++ b/Documentation/devicetree/bindings/sound/st,stm32h7-i2s.txt @@ -0,0 +1,71 @@ +STMicroelectronics STM32 SPI/I2S Controller + +The SPI/I2S block supports I2S/PCM protocols when configured on I2S mode. +Only some SPI instances support I2S. + +Required properties: + - compatible: Must be "st,stm32h7-i2s" + - #sound-dai-cells: Must be 1. (one parameter) + This parameter allows to specify CPU DAI index in soundcard CPU dai link. + index 0: playback DAI + index 1: capture DAI + index 2: full duplex DAI + - reg: Offset and length of the device's register set. + - interrupts: Must contain the interrupt line id. + - clocks: Must contain phandle and clock specifier pairs for each entry + in clock-names. + - clock-names: Must contain "i2sclk", "pclk", "x8k" and "x11k". + "i2sclk": clock which feeds the internal clock generator + "pclk": clock which feeds the peripheral bus interface + "x8k": I2S parent clock for sampling rates multiple of 8kHz. + "x11k": I2S parent clock for sampling rates multiple of 11.025kHz. + - dmas: DMA specifiers for tx and rx dma. + See Documentation/devicetree/bindings/dma/stm32-dma.txt. + - dma-names: Identifier for each DMA request line. Must be "tx" and "rx". + - pinctrl-names: should contain only value "default" + - pinctrl-0: see Documentation/devicetree/bindings/pinctrl/pinctrl-stm32.txt + +Optional properties: + - resets: Reference to a reset controller asserting the reset controller + +Example: +sound_card { + compatible = "audio-graph-card"; + dais = <&i2s2_port 0>; +}; + +i2s2: audio-controller@40003800 { + compatible = "st,stm32h7-i2s"; + #sound-dai-cells = <1>; + reg = <0x40003800 0x400>; + interrupts = <36>; + clocks = <&rcc PCLK1>, <&rcc SPI2_CK>, <&rcc PLL1_Q>, <&rcc PLL2_P>; + clock-names = "pclk", "i2sclk", "x8k", "x11k"; + dmas = <&dmamux2 2 39 0x400 0x1>, + <&dmamux2 3 40 0x400 0x1>; + dma-names = "rx", "tx"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2s2>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + i2s2_port: port@0 { + reg = <0>; + cpu_endpoint: endpoint { + remote-endpoint = <&codec_endpoint>; + audio-graph-card,format = "i2s"; + audio-graph-card,bitclock-master = <&codec_endpoint>; + audio-graph-card,frame-master = <&codec_endpoint>; + }; + }; +}; + +audio-codec { + codec_port: port { + codec_endpoint: endpoint { + remote-endpoint = <&cpu_endpoint>; + }; + }; +};
Add documentation of device tree bindings for STM32 SPI/I2S. Signed-off-by: olivier moysan <olivier.moysan@st.com> --- .../devicetree/bindings/sound/st,stm32h7-i2s.txt | 71 ++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/st,stm32h7-i2s.txt