| Message ID | 1648448050-15237-1-git-send-email-spujar@nvidia.com |
|---|---|
| Headers | show |
| Series | Flexible codec clock configuration | expand |
On 28/03/2022 08:14, Sameer Pujar wrote: > Convert rt5659.txt DT binding to YAML schema. This binding is applicable > to rt5658 and rt5659 audio CODECs. > > Signed-off-by: Sameer Pujar <spujar@nvidia.com> > Cc: Oder Chiou <oder_chiou@realtek.com> > --- > .../devicetree/bindings/sound/realtek,rt5659.yaml | 112 +++++++++++++++++++++ > Documentation/devicetree/bindings/sound/rt5659.txt | 89 ---------------- > 2 files changed, 112 insertions(+), 89 deletions(-) > create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5659.yaml > delete mode 100644 Documentation/devicetree/bindings/sound/rt5659.txt > > diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml > new file mode 100644 > index 0000000..3bd9b6f > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml > @@ -0,0 +1,112 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/realtek,rt5659.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: RT5658 and RT5659 audio CODECs > + > +description: This device supports I2C only. > + > +maintainers: > + - Oder Chiou <oder_chiou@realtek.com> > + > +allOf: > + - $ref: name-prefix.yaml# > + > +properties: > + compatible: > + enum: > + - realtek,rt5658 > + - realtek,rt5659 > + > + reg: > + description: The I2C address of the device Skip the description, it's obvious. > + maxItems: 1 > + > + interrupts: > + description: The CODEC's interrupt output Ditto. > + maxItems: 1 > + > + clocks: > + items: > + - description: Master clock (MCLK) to the CODEC > + > + clock-names: > + items: > + - const: mclk > + > + realtek,in1-differential: > + description: MIC1 input is differntial and not single-ended. typo (differential) > + type: boolean > + > + realtek,in3-differential: > + description: MIC3 input is differntial and not single-ended. > + type: boolean > + > + realtek,in4-differential: > + description: MIC3 input is differntial and not single-ended. MIC4? > + type: boolean > + > + realtek,dmic1-data-pin: > + description: DMIC1 data pin usage > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: > + - 0 # dmic1 is not used > + - 1 # using IN2N pin as dmic1 data pin > + - 2 # using GPIO5 pin as dmic1 data pin > + - 3 # using GPIO9 pin as dmic1 data pin > + - 4 # using GPIO11 pin as dmic1 data pin > + > + realtek,dmic2-data-pin: > + description: DMIC2 data pin usage > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: > + - 0 # dmic2 is not used > + - 1 # using IN2P pin as dmic2 data pin > + - 2 # using GPIO6 pin as dmic2 data pin > + - 3 # using GPIO10 pin as dmic2 data pin > + - 4 # using GPIO12 pin as dmic2 data pin > + > + realtek,jd-src: > + description: Jack detect source > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: > + - 0 # No JD is used > + - 1 # using JD3 as JD source > + - 2 # JD source for Intel HDA header > + > + realtek,ldo1-en-gpios: > + description: The GPIO that controls the CODEC's LDO1_EN pin. maxItems > + > + realtek,reset-gpios: > + description: The GPIO that controls the CODEC's RESET pin. maxItems What about the ports node? Best regards, Krzysztof
On 28/03/2022 08:14, Sameer Pujar wrote: > The rt5658 or rt5659 CODEC system clock (SYSCLK) can be derived from > various clock sources. For example it can be derived either from master > clock (MCLK) or by internal PLL. The internal PLL again can take input > clock references from bit clocks (BCLKs) and MCLK. To enable a flexible > clocking configuration the DT binding is extended here. > > It makes use of standard clock bindings and sets up the clock relation > via DT. > > Signed-off-by: Sameer Pujar <spujar@nvidia.com> > Cc: Oder Chiou <oder_chiou@realtek.com> > --- > .../devicetree/bindings/sound/realtek,rt5659.yaml | 53 ++++++++++++++++++++-- > 1 file changed, 49 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml > index b0485b8..0c2f3cb 100644 > --- a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml > +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml > @@ -29,12 +29,28 @@ properties: > maxItems: 1 > > clocks: > - items: > - - description: Master clock (MCLK) to the CODEC > + description: | > + CODEC can receive multiple clock inputs like Master > + clock (MCLK), I2S bit clocks (BCLK1, BCLK2, BCLK3, > + BCLK4). The CODEC SYSCLK can be generated from MCLK > + or internal PLL. In turn PLL can reference from MCLK > + and BCLKs. > > clock-names: > - items: > - - const: mclk > + description: | > + The clock names can be combination of following: > + "mclk" : Master clock > + "pll_ref" : Reference to CODEC PLL clock > + "sysclk" : CODEC SYSCLK > + "^bclk[1-4]$" : Bit clocks to CODEC No, that does not look correct. You allow anything as clock input (even 20 clocks, different names, any order). That's not how DT schema should work and that's not how hardware looks like. Usually the clock inputs are always there which also you mentioned in description - "multiple clock inputs". All these clocks should be expected, unless really the wires (physical wires) can be left disconnected. Best regards, Krzysztof
On 28-03-2022 12:36, Krzysztof Kozlowski wrote: > External email: Use caution opening links or attachments > > > On 28/03/2022 08:14, Sameer Pujar wrote: >> The rt5658 or rt5659 CODEC system clock (SYSCLK) can be derived from >> various clock sources. For example it can be derived either from master >> clock (MCLK) or by internal PLL. The internal PLL again can take input >> clock references from bit clocks (BCLKs) and MCLK. To enable a flexible >> clocking configuration the DT binding is extended here. >> >> It makes use of standard clock bindings and sets up the clock relation >> via DT. >> >> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >> Cc: Oder Chiou <oder_chiou@realtek.com> >> --- >> .../devicetree/bindings/sound/realtek,rt5659.yaml | 53 ++++++++++++++++++++-- >> 1 file changed, 49 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >> index b0485b8..0c2f3cb 100644 >> --- a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >> +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >> @@ -29,12 +29,28 @@ properties: >> maxItems: 1 >> >> clocks: >> - items: >> - - description: Master clock (MCLK) to the CODEC >> + description: | >> + CODEC can receive multiple clock inputs like Master >> + clock (MCLK), I2S bit clocks (BCLK1, BCLK2, BCLK3, >> + BCLK4). The CODEC SYSCLK can be generated from MCLK >> + or internal PLL. In turn PLL can reference from MCLK >> + and BCLKs. >> >> clock-names: >> - items: >> - - const: mclk >> + description: | >> + The clock names can be combination of following: >> + "mclk" : Master clock >> + "pll_ref" : Reference to CODEC PLL clock >> + "sysclk" : CODEC SYSCLK >> + "^bclk[1-4]$" : Bit clocks to CODEC > No, that does not look correct. You allow anything as clock input (even > 20 clocks, different names, any order). That's not how DT schema should > work and that's not how hardware looks like. > > Usually the clock inputs are always there which also you mentioned in > description - "multiple clock inputs". All these clocks should be > expected, unless really the wires (physical wires) can be left disconnected. The CODEC can receive multiple clocks but all the input clocks need not be present or connected always. If a specific configuration is needed and platform supports such an input, then all these inputs can be added. I don't know how to define this detail in the schema. If I make all of them expected, then binding check throws errors. If I were to list all the possible combinations, the list is going to be big (not sure if this would be OK?).
On 28/03/2022 09:58, Sameer Pujar wrote: > > On 28-03-2022 12:36, Krzysztof Kozlowski wrote: >> External email: Use caution opening links or attachments >> >> >> On 28/03/2022 08:14, Sameer Pujar wrote: >>> The rt5658 or rt5659 CODEC system clock (SYSCLK) can be derived from >>> various clock sources. For example it can be derived either from master >>> clock (MCLK) or by internal PLL. The internal PLL again can take input >>> clock references from bit clocks (BCLKs) and MCLK. To enable a flexible >>> clocking configuration the DT binding is extended here. >>> >>> It makes use of standard clock bindings and sets up the clock relation >>> via DT. >>> >>> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >>> Cc: Oder Chiou <oder_chiou@realtek.com> >>> --- >>> .../devicetree/bindings/sound/realtek,rt5659.yaml | 53 ++++++++++++++++++++-- >>> 1 file changed, 49 insertions(+), 4 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >>> index b0485b8..0c2f3cb 100644 >>> --- a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >>> +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >>> @@ -29,12 +29,28 @@ properties: >>> maxItems: 1 >>> >>> clocks: >>> - items: >>> - - description: Master clock (MCLK) to the CODEC >>> + description: | >>> + CODEC can receive multiple clock inputs like Master >>> + clock (MCLK), I2S bit clocks (BCLK1, BCLK2, BCLK3, >>> + BCLK4). The CODEC SYSCLK can be generated from MCLK >>> + or internal PLL. In turn PLL can reference from MCLK >>> + and BCLKs. >>> >>> clock-names: >>> - items: >>> - - const: mclk >>> + description: | >>> + The clock names can be combination of following: >>> + "mclk" : Master clock >>> + "pll_ref" : Reference to CODEC PLL clock >>> + "sysclk" : CODEC SYSCLK >>> + "^bclk[1-4]$" : Bit clocks to CODEC >> No, that does not look correct. You allow anything as clock input (even >> 20 clocks, different names, any order). That's not how DT schema should >> work and that's not how hardware looks like. > >> >> Usually the clock inputs are always there which also you mentioned in >> description - "multiple clock inputs". All these clocks should be >> expected, unless really the wires (physical wires) can be left disconnected. > > The CODEC can receive multiple clocks but all the input clocks need not > be present or connected always. If a specific configuration is needed > and platform supports such an input, then all these inputs can be added. > I don't know how to define this detail in the schema. If I make all of > them expected, then binding check throws errors. If I were to list all > the possible combinations, the list is going to be big (not sure if this > would be OK?). Thanks for explanation. Please differentiate between these two: 1. clock inputs connected, but unused (not needed for driver or for particular use case), 2. clock inputs really not connected. For the 1. above, such clock inputs should still be listed in the bindings and DTS. For the 2. above, such clocks should actually not be there. How to achieve this depends on number of your combinations. IOW, how many clocks are physically optional. For some small number of variations this can be: oneOf: - const: mclk - items: - const: mclk - enum: - bclk1 - bclk2 - bclk3 - bclk4 - items: - const: mclk - const: pll_ref - enum: - bclk1 - bclk2 - bclk3 - bclk4 For a total flexibility that any clock input can be disconnected, this should be a list of enums I guess (with minItems). However please find the clocks always connected and include them if possible in a fixed way (like this oneOf above). Best regards, Krzysztof
On 28-03-2022 18:21, Rob Herring wrote: > On Mon, 28 Mar 2022 11:44:05 +0530, Sameer Pujar wrote: >> Convert rt5659.txt DT binding to YAML schema. This binding is applicable >> to rt5658 and rt5659 audio CODECs. >> >> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >> Cc: Oder Chiou <oder_chiou@realtek.com> >> --- >> .../devicetree/bindings/sound/realtek,rt5659.yaml | 112 +++++++++++++++++++++ >> Documentation/devicetree/bindings/sound/rt5659.txt | 89 ---------------- >> 2 files changed, 112 insertions(+), 89 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >> delete mode 100644 Documentation/devicetree/bindings/sound/rt5659.txt >> > Running 'make dtbs_check' with the schema in this patch gives the > following warnings. Consider if they are expected or the schema is > incorrect. These may not be new warnings. > > Note that it is not yet a requirement to have 0 warnings for dtbs_check. > This will change in the future. > > Full log is available here: https://patchwork.ozlabs.org/patch/1610026 > > > audio-codec@1a: 'port' does not match any of the regexes: 'pinctrl-[0-9]+' > arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dt.yaml The port/ports binding is not added in 'patch v2 1/6'. I will fix this in next revision by squashing 'patch v2 1/6' and 'patch v2 2/6'. Thanks.
On 28-03-2022 18:58, Krzysztof Kozlowski wrote: > On 28/03/2022 15:19, Sameer Pujar wrote: >> On 28-03-2022 13:37, Krzysztof Kozlowski wrote: >>> On 28/03/2022 09:58, Sameer Pujar wrote: >>>> On 28-03-2022 12:36, Krzysztof Kozlowski wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> On 28/03/2022 08:14, Sameer Pujar wrote: >>>>>> The rt5658 or rt5659 CODEC system clock (SYSCLK) can be derived from >>>>>> various clock sources. For example it can be derived either from master >>>>>> clock (MCLK) or by internal PLL. The internal PLL again can take input >>>>>> clock references from bit clocks (BCLKs) and MCLK. To enable a flexible >>>>>> clocking configuration the DT binding is extended here. >>>>>> >>>>>> It makes use of standard clock bindings and sets up the clock relation >>>>>> via DT. >>>>>> >>>>>> Signed-off-by: Sameer Pujar<spujar@nvidia.com> >>>>>> Cc: Oder Chiou<oder_chiou@realtek.com> >>>>>> --- >>>>>> .../devicetree/bindings/sound/realtek,rt5659.yaml | 53 ++++++++++++++++++++-- >>>>>> 1 file changed, 49 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >>>>>> index b0485b8..0c2f3cb 100644 >>>>>> --- a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >>>>>> +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml >>>>>> @@ -29,12 +29,28 @@ properties: >>>>>> maxItems: 1 >>>>>> >>>>>> clocks: >>>>>> - items: >>>>>> - - description: Master clock (MCLK) to the CODEC >>>>>> + description: | >>>>>> + CODEC can receive multiple clock inputs like Master >>>>>> + clock (MCLK), I2S bit clocks (BCLK1, BCLK2, BCLK3, >>>>>> + BCLK4). The CODEC SYSCLK can be generated from MCLK >>>>>> + or internal PLL. In turn PLL can reference from MCLK >>>>>> + and BCLKs. >>>>>> >>>>>> clock-names: >>>>>> - items: >>>>>> - - const: mclk >>>>>> + description: | >>>>>> + The clock names can be combination of following: >>>>>> + "mclk" : Master clock >>>>>> + "pll_ref" : Reference to CODEC PLL clock >>>>>> + "sysclk" : CODEC SYSCLK >>>>>> + "^bclk[1-4]$" : Bit clocks to CODEC >>>>> No, that does not look correct. You allow anything as clock input (even >>>>> 20 clocks, different names, any order). That's not how DT schema should >>>>> work and that's not how hardware looks like. >>>>> Usually the clock inputs are always there which also you mentioned in >>>>> description - "multiple clock inputs". All these clocks should be >>>>> expected, unless really the wires (physical wires) can be left disconnected. >>>> The CODEC can receive multiple clocks but all the input clocks need not >>>> be present or connected always. If a specific configuration is needed >>>> and platform supports such an input, then all these inputs can be added. >>>> I don't know how to define this detail in the schema. If I make all of >>>> them expected, then binding check throws errors. If I were to list all >>>> the possible combinations, the list is going to be big (not sure if this >>>> would be OK?). >>> Thanks for explanation. Please differentiate between these two: >>> 1. clock inputs connected, but unused (not needed for driver or for >>> particular use case), >>> 2. clock inputs really not connected. >>> >>> For the 1. above, such clock inputs should still be listed in the >>> bindings and DTS. For the 2. above, such clocks should actually not be >>> there. >> Thank you for the suggestion. >> >>> How to achieve this depends on number of your combinations. IOW, >>> how many clocks are physically optional. >> From CODEC point of view all these clock inputs are possible and a >> platform may choose to connect a subset of it depending on the >> application. The binding is expected to support all such cases. To >> support all possibilities, the total combinations can be very big (100+). >> >>> For some small number of >>> variations this can be: >>> oneOf: >>> - const: mclk >>> - items: >>> - const: mclk >>> - enum: >>> - bclk1 >>> - bclk2 >>> - bclk3 >>> - bclk4 >>> - items: >>> - const: mclk >>> - const: pll_ref >>> - enum: >>> - bclk1 >>> - bclk2 >>> - bclk3 >>> - bclk4 >>> >>> For a total flexibility that any clock input can be disconnected, this >>> should be a list of enums I guess (with minItems). However please find >>> the clocks always connected and include them if possible in a fixed way >>> (like this oneOf above). >> May be I can list the most commonly required combinations like below and >> extend it whenever there is a need for specific combination? > Yes, this would work. Relaxing such constraints is possible. Thanks. I will update this in next revision.
Typically the codec drivers require setting up of Sysclk. Sometimes presence of internal PLL can provide more options of Sysclk configuration. Presently ASoC provides callbacks set_sysclk() and set_pll() in such cases. However it comes with following limitations considering generic machine drivers (simple-card or audio-graph-card): 1. The Sysclk source needs to be passed to set_sysclk() callback. Presently simple-card or audio-graph-card card rely on default source value (which is 0). If any other source needs to be used, it is currently not possible. 2. The same would be true for codec PLL configuration as well, though simple-card or audio-graph-card don't have support yet for the PLL configuration. Earlier attempt[0] to address above was not felt suitable. The suggestion was to use standard clock based bindings instead. This RFC series takes RT5659 as a reference and exposes clock relationships via DT. **This is not in the final shape yet**, but I wanted to get some valuable feedback to understand if the idea is right. If this appears fine, this can be extended to other codecs (wherever necessary). This does not completely remove the need of set_sysclk() callback because the clock requirement (MCLK * fs) would come from the machine driver. But machine driver need not worry about Sysclk source. It would be internally managed by Codec via DT clock relationships. [0] https://patchwork.kernel.org/project/alsa-devel/list/?series=438531&archive=both&state=* Changelog: ========== v1 -> v2: --------- * New patch added to convert rt5659 binding doc to YAML format * New patch added to document audio-graph-port binding for rt5659 * New patch added to document clock binding enhancement for rt5659 Sameer Pujar (6): ASoC: dt-bindings: Convert rt5659 bindings to YAML schema ASoC: dt-bindings: Add audio-graph-port bindings to rt5659 ASoC: dt-bindings: Extend clock bindings of rt5659 ASoC: soc-pcm: tweak DPCM BE hw_param() call order ASoC: rt5659: Expose internal clock relationships ASoC: tegra: Get clock rate in consumer mode .../devicetree/bindings/sound/realtek,rt5659.yaml | 179 +++++++++++++++ Documentation/devicetree/bindings/sound/rt5659.txt | 89 -------- sound/soc/codecs/rt5659.c | 248 ++++++++++++++++++++- sound/soc/codecs/rt5659.h | 9 + sound/soc/soc-pcm.c | 60 ++++- sound/soc/tegra/tegra210_i2s.c | 25 ++- 6 files changed, 503 insertions(+), 107 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5659.yaml delete mode 100644 Documentation/devicetree/bindings/sound/rt5659.txt