| Message ID | 20210817101119.423853-3-frattaroli.nicolas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rockchip I2S/TDM controller | expand |
On Donnerstag, 19. August 2021 14:08:36 CEST Robin Murphy wrote: > On 2021-08-17 11:11, Nicolas Frattaroli wrote: > > + rockchip,trcm-sync: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Which lrck/bclk clocks each direction will sync to. You should use > > the + constants in <dt-bindings/sound/rockchip,i2s-tdm.h> > > + oneOf: > > + - const: 0 > > + description: > > + RK_TRCM_TXRX. Use both the TX and the RX clock for TX and RX. > > + - const: 1 > > + description: > > + RK_TRCM_TX. Use only the TX clock for TX and RX. > > + - const: 2 > > + description: > > + RK_TRCM_RX. Use only the RX clock for TX and RX. > > I wonder if that might make sense to have boolean properties to describe > the latter two cases (which would effectively be mutually-exclusive), > rather than a magic number? Or possibly even just make the respective > clocks optional, if this is something which would be done per-SoC rather > than per-board? > From what I know from downstream vendor device trees, these are per board, not for the SoC as a whole. There are I2S/TDM controllers on the SoC which I think are hardwired to certain other IP blocks, such as I2S0 being connected to HDMI, but I2S1 can be routed outside of the SoC where these come into play I believe. As for making them boolean properties, I'd rather not. If I were to make it two mutually exclusive booleans, this would result in 4 possible states rather than 3, and require complexity to check it both in the schema and in the probe function. Like this, I can get away with a switch case that has a fallthrough, and a list of consts in the schema. > > + > > + "#sound-dai-cells": > > + const: 0 > > + > > + rockchip,no-dmaengine: > > + description: > > + If present, driver will not register a pcm dmaengine, only the dai. > > + If the dai is part of multi-dais, the property should be present. > > + type: boolean > > That sounds a lot more like a policy decision specific to the Linux > driver implementation, than something which really belongs in DT as a > description of the platform. I agree. Should I be refactoring this into a module parameter or something along those lines? I'm unsure of where this goes. > > > + > > + rockchip,playback-only: > > + description: Specify that the controller only has playback > > capability. > > + type: boolean > > + > > + rockchip,capture-only: > > + description: Specify that the controller only has capture capability. > > + type: boolean > > Could those be inferred from the compatible string, or are there cases > where you have multiple instances of the IP block in different > configurations within the same SoC? (Or if it's merely reflecting > whether the respective interface is actually wired up externally, could > that be inferred from the attached codec?) > > Robin. > They can't be inferred from the SoC because there are indeed multiple instances of this IP block in different configurations on the same SoC. The RK3566 and RK3568 have four in total, of two different categories, each being able to be configured for a different format (though the number of channels and available formats vary for the two categories, one group only supports I2S and PCM with two channels) The particular configuration may even vary per-board; an I2S/TDM controller may be connected to an external codec which does not support capture, whereas on another board it may be connected to one that does. As an example, if I understand it correctly, I2S3 on the RK3566 and RK3568 can do 2 channels RX and TX in I2S mode, but only 2 channels either RX or TX in PCM mode, but I'm unsure of the language in the (still not public) documentation I have. Regards, Nicolas Frattaroli
On Donnerstag, 19. August 2021 16:16:17 CEST Mark Brown wrote: > On Thu, Aug 19, 2021 at 03:52:55PM +0200, Nicolas Frattaroli wrote: > > On Donnerstag, 19. August 2021 14:08:36 CEST Robin Murphy wrote: > > > > + rockchip,no-dmaengine: > > > > + description: > > > > + If present, driver will not register a pcm dmaengine, only the > > > > dai. > > > > + If the dai is part of multi-dais, the property should be > > > > present. > > > > + type: boolean > > > > > > That sounds a lot more like a policy decision specific to the Linux > > > driver implementation, than something which really belongs in DT as a > > > description of the platform. > > > > I agree. Should I be refactoring this into a module parameter or > > something along those lines? I'm unsure of where this goes. > > Why is this even required? What is "multi-dais" and why would > registering the DMA stuff cause a problem? After some research, it appears that multi-dais is a special driver that downstream uses to allow multiple sub-DAIs to be combined into one DAI that has all the channels of the sub-DAIs. This does not seem like something that should be done at that level to me, because it seems like it's pushing a sound driver configuration into the realm of hardware description. In retrospect, I should have stripped this out before submitting it, because I should not be submitting things I don't understand completely. I apologise. > > The particular configuration may even vary per-board; an I2S/TDM > > controller may be connected to an external codec which does not > > support capture, whereas on another board it may be connected to > > one that does. > > If the external device doesn't support both directions then why does the > driver for the I2S controller in the CPU care? The constraint handling > code in the core will ensure that nothing tries to start something that > isn't supported I went over the downstream text binding description again and from that it appears that the playback/capture-only capability is something specific to the controller, not to any device connected to it. The downstream device tree for the rk3568 specifies playback-only for I2S0, a.k.a. the one connected to the HDMI that I can't test because we currently don't have a video clock. Another downstream device tree, specific to what appears to be a robot demo for the px30 SoC, uses this property on i2s1, which tells me that this is not an actual description of the controller hardware but just a description of the application. While not relevant to the device tree schema, the driver reacts to these properties by setting the opposite directions _minimum_ channel number to 0 (from the default of 2.) My conclusion from this is that this reeks of nonsense and I will look into what happens when I simply remove these properties and lower the channels_min to 0 in the driver. If it turns out that on some SoC for some I2S/TDM controller instance there is a limitation where specifying that the controller only handles either capture or playback does make sense, we can always add it later. Thank you for your comments, Nicolas Frattaroli
diff --git a/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml b/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml new file mode 100644 index 000000000000..c3022620b47f --- /dev/null +++ b/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml @@ -0,0 +1,221 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/rockchip,i2s-tdm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Rockchip I2S/TDM Controller + +description: + The Rockchip I2S/TDM Controller is a Time Division Multiplexed + audio interface found in various Rockchip SoCs, allowing up + to 8 channels of audio over a serial interface. + +maintainers: + - Nicolas Frattaroli <frattaroli.nicolas@gmail.com> + +properties: + compatible: + enum: + - rockchip,px30-i2s-tdm + - rockchip,rk1808-i2s-tdm + - rockchip,rk3308-i2s-tdm + - rockchip,rk3568-i2s-tdm + - rockchip,rv1126-i2s-tdm + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + dmas: + minItems: 1 + maxItems: 2 + + dma-names: + oneOf: + - const: rx + - items: + - const: tx + - const: rx + + clocks: + items: + - description: clock for TX + - description: clock for RX + - description: clock for I2S bus + + clock-names: + items: + - const: mclk_tx + - const: mclk_rx + - const: hclk + + rockchip,frame-width: + $ref: /schemas/types.yaml#/definitions/uint32 + default: 64 + minimum: 32 + maximum: 512 + description: + Width of a frame, usually slot width multiplied by number of slots. + Must be even. + + resets: + items: + - description: reset for TX + - description: reset for RX + + reset-names: + items: + - const: tx-m + - const: rx-m + + rockchip,cru: + $ref: /schemas/types.yaml#/definitions/phandle + description: + The phandle of the cru. + Required if both playback and capture are used, i.e. if rockchip,clk-trcm + is 0. + + rockchip,grf: + $ref: /schemas/types.yaml#/definitions/phandle + description: + The phandle of the syscon node for the GRF register. + + rockchip,mclk-calibrate: + description: + Enable mclk source calibration. + type: boolean + + rockchip,trcm-sync: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Which lrck/bclk clocks each direction will sync to. You should use the + constants in <dt-bindings/sound/rockchip,i2s-tdm.h> + oneOf: + - const: 0 + description: + RK_TRCM_TXRX. Use both the TX and the RX clock for TX and RX. + - const: 1 + description: + RK_TRCM_TX. Use only the TX clock for TX and RX. + - const: 2 + description: + RK_TRCM_RX. Use only the RX clock for TX and RX. + + "#sound-dai-cells": + const: 0 + + rockchip,no-dmaengine: + description: + If present, driver will not register a pcm dmaengine, only the dai. + If the dai is part of multi-dais, the property should be present. + type: boolean + + rockchip,playback-only: + description: Specify that the controller only has playback capability. + type: boolean + + rockchip,capture-only: + description: Specify that the controller only has capture capability. + type: boolean + + rockchip,i2s-rx-route: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Defines the mapping of I2S RX sdis to I2S data bus lines. + By default, they are mapped one-to-one. + items: + - description: which sdi to connect to data line 0 + - description: which sdi to connect to data line 1 + - description: which sdi to connect to data line 2 + - description: which sdi to connect to data line 3 + + rockchip,i2s-tx-route: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Defines the mapping of I2S TX sdos to I2S data bus lines. + By default, they are mapped one-to-one. + items: + - description: which sdo to connect to data line 0 + - description: which sdo to connect to data line 1 + - description: which sdo to connect to data line 2 + - description: which sdo to connect to data line 3 + + rockchip,tdm-fsync-half-frame: + description: Whether to use half frame fsync. + type: boolean + + +required: + - compatible + - reg + - interrupts + - dmas + - dma-names + - clocks + - clock-names + - resets + - reset-names + - rockchip,grf + - "#sound-dai-cells" + - rockchip,trcm-sync + +allOf: + - if: + properties: + rockchip,clk-trcm: + contains: + enum: [0] + then: + required: + - rockchip,cru + + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/rk3568-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/pinctrl/rockchip.h> + #include <dt-bindings/sound/rockchip,i2s-tdm.h> + + + foo { + #address-cells = <2>; + #size-cells = <2>; + i2s@fe410000 { + compatible = "rockchip,rk3568-i2s-tdm"; + reg = <0x0 0xfe410000 0x0 0x1000>; + interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cru MCLK_I2S1_8CH_TX>, <&cru MCLK_I2S1_8CH_RX>, + <&cru HCLK_I2S1_8CH>; + clock-names = "mclk_tx", "mclk_rx", "hclk"; + dmas = <&dmac1 2>, <&dmac1 3>; + dma-names = "tx", "rx"; + resets = <&cru SRST_M_I2S1_8CH_TX>, <&cru SRST_M_I2S1_8CH_RX>; + reset-names = "tx-m", "rx-m"; + rockchip,trcm-sync = <RK_TRCM_TX>; + rockchip,cru = <&cru>; + rockchip,grf = <&grf>; + #sound-dai-cells = <0>; + pinctrl-names = "default"; + pinctrl-0 = + <&i2s1m0_sclktx + &i2s1m0_sclkrx + &i2s1m0_lrcktx + &i2s1m0_lrckrx + &i2s1m0_sdi0 + &i2s1m0_sdi1 + &i2s1m0_sdi2 + &i2s1m0_sdi3 + &i2s1m0_sdo0 + &i2s1m0_sdo1 + &i2s1m0_sdo2 + &i2s1m0_sdo3>; + status = "disabled"; + }; + }; diff --git a/include/dt-bindings/sound/rockchip,i2s-tdm.h b/include/dt-bindings/sound/rockchip,i2s-tdm.h new file mode 100644 index 000000000000..32494d64cf33 --- /dev/null +++ b/include/dt-bindings/sound/rockchip,i2s-tdm.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _DT_BINDINGS_ROCKCHIP_I2S_TDM_H +#define _DT_BINDINGS_ROCKCHIP_I2S_TDM_H + +#define RK_TRCM_TXRX 0 +#define RK_TRCM_TX 1 +#define RK_TRCM_RX 2 + +#endif /* _DT_BINDINGS_ROCKCHIP_I2S_TDM_H */
This adds the YAML bindings for the Rockchip I2S/TDM audio driver. Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com> --- .../bindings/sound/rockchip,i2s-tdm.yaml | 221 ++++++++++++++++++ include/dt-bindings/sound/rockchip,i2s-tdm.h | 9 + 2 files changed, 230 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml create mode 100644 include/dt-bindings/sound/rockchip,i2s-tdm.h