| Message ID | 20230602090322.1876359-2-alvin@pqrs.dk |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ASoC: support dai-links with symmetric clock roles | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Fri, 02 Jun 2023 11:03:18 +0200, Alvin Šipraga wrote: > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > The new flag specifies that both ends of the dai-link have the same > clock consumer/provider role. This should be used to describe hardware > where e.g. the CPU and codec both receive their bit- and frame-clocks > from an external source. > > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> > --- > .../devicetree/bindings/sound/simple-card.yaml | 11 +++++++++++ > 1 file changed, 11 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/sound/simple-card.yaml:33:5: [error] syntax error: could not find expected ':' (syntax) dtschema/dtc warnings/errors: make[1]: *** Deleting file 'Documentation/devicetree/bindings/sound/simple-card.example.dts' Documentation/devicetree/bindings/sound/simple-card.yaml:33:5: could not find expected ':' make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/sound/simple-card.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/sound/simple-card.yaml:33:5: could not find expected ':' /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/simple-card.yaml: ignoring, error parsing file make: *** [Makefile:1512: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230602090322.1876359-2-alvin@pqrs.dk The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Fri, Jun 02, 2023 at 11:03:18AM +0200, Alvin Šipraga wrote: > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > The new flag specifies that both ends of the dai-link have the same > clock consumer/provider role. This should be used to describe hardware > where e.g. the CPU and codec both receive their bit- and frame-clocks > from an external source. Why would we have a property for this and not just describe whatever the actual clocking arrangement is?
Hi Mark, On Fri, Jun 02, 2023 at 12:43:51PM +0100, Mark Brown wrote: > On Fri, Jun 02, 2023 at 11:03:18AM +0200, Alvin Šipraga wrote: > > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > > > The new flag specifies that both ends of the dai-link have the same > > clock consumer/provider role. This should be used to describe hardware > > where e.g. the CPU and codec both receive their bit- and frame-clocks > > from an external source. > > Why would we have a property for this and not just describe whatever the > actual clocking arrangement is? Sure - let me just elaborate on my thinking and maybe you can help me with a better approach: The clocking arrangement is encoded in the dai_fmt field of snd_soc_dai_link, but this is a single value that describes the format on both ends. The current behaviour of ASoC is to flip the clock roles encoded in dai_fmt when applying it to the CPU side of the link. Looking from a DT perspective, if I do not specify e.g. bitclock-master on either side of the link, then the dai_fmt will describe the codec as a bitclock consumer and (after flipping) the CPU as a provider. That's the default implication of the DT bindings and I can't break compatibility there. The other issue is that for the simple-card the DAI format is only parsed in one place and applied to the whole link. Are you proposing that it be modified to explicitly try and parse both ends in order to determine if both sides want to be clock consumers? In that case I'd have to also introduce bitclock-consumer and frameclock-consumer properties to mirror the existing bitclock-master and frameclock-master properties, as an explicit absence of the *-master property on both sides would have to default to the original ASoC behaviour described above. Or did you have something else in mind? Thanks for your review. Kind regards, Alvin
On Fri, Jun 02, 2023 at 12:12:52PM +0000, Alvin Šipraga wrote: > On Fri, Jun 02, 2023 at 12:43:51PM +0100, Mark Brown wrote: > > Why would we have a property for this and not just describe whatever the > > actual clocking arrangement is? > Sure - let me just elaborate on my thinking and maybe you can help me with a > better approach: > The clocking arrangement is encoded in the dai_fmt field of snd_soc_dai_link, > but this is a single value that describes the format on both ends. The current > behaviour of ASoC is to flip the clock roles encoded in dai_fmt when applying it > to the CPU side of the link. > Looking from a DT perspective, if I do not specify e.g. bitclock-master on > either side of the link, then the dai_fmt will describe the codec as a bitclock > consumer and (after flipping) the CPU as a provider. That's the default > implication of the DT bindings and I can't break compatibility there. None of this addresses my question. To repeat why would we not just describe the actual clocking arrangement here - this property does not specify where the clock actually comes from at all, we're still going to need additional information for that and if we've described that clock then we already know it's there without having to specify any more properties. > The other issue is that for the simple-card the DAI format is only parsed in one > place and applied to the whole link. Are you proposing that it be modified to > explicitly try and parse both ends in order to determine if both sides want to > be clock consumers? In that case I'd have to also introduce bitclock-consumer > and frameclock-consumer properties to mirror the existing bitclock-master and > frameclock-master properties, as an explicit absence of the *-master property on > both sides would have to default to the original ASoC behaviour described above. If simple-card can't be made to work that's fine, it's deprecated anyway.
On Fri, Jun 02, 2023 at 01:19:08PM +0100, Mark Brown wrote: > On Fri, Jun 02, 2023 at 12:12:52PM +0000, Alvin Šipraga wrote: > > On Fri, Jun 02, 2023 at 12:43:51PM +0100, Mark Brown wrote: > > > > Why would we have a property for this and not just describe whatever the > > > actual clocking arrangement is? > > > Sure - let me just elaborate on my thinking and maybe you can help me with a > > better approach: > > > The clocking arrangement is encoded in the dai_fmt field of snd_soc_dai_link, > > but this is a single value that describes the format on both ends. The current > > behaviour of ASoC is to flip the clock roles encoded in dai_fmt when applying it > > to the CPU side of the link. > > > Looking from a DT perspective, if I do not specify e.g. bitclock-master on > > either side of the link, then the dai_fmt will describe the codec as a bitclock > > consumer and (after flipping) the CPU as a provider. That's the default > > implication of the DT bindings and I can't break compatibility there. > > None of this addresses my question. To repeat why would we not just > describe the actual clocking arrangement here - this property does not > specify where the clock actually comes from at all, we're still going to > need additional information for that and if we've described that clock > then we already know it's there without having to specify any more > properties. Yes I see what you mean. On my platform the clock source is actually described by the common clock framework, so I would want to use that. If it were a component driver then it would most likely be a codec that is part of the dai-link anyway. So what about having two struct clk pointers in struct snd_soc_dai? struct snd_soc_dai { /* ... */ struct clk *bitclock_provider; struct clk *frameclock_provider; /* ... */ }; If non-NULL I could then have the ASoC core enable/disable the clocks on demand? I would say in hw_params/hw_free, albeit that runs after set_fmt. Having said that, I see ASoC doesn't really use the CCF much... am I way off? I don't think it's feasible to modify every component driver to explicitly handle this and then ignore any CBP_CFP bits set in its call to set_fmt - this is why I want help from the ASoC core. > > > The other issue is that for the simple-card the DAI format is only parsed in one > > place and applied to the whole link. Are you proposing that it be modified to > > explicitly try and parse both ends in order to determine if both sides want to > > be clock consumers? In that case I'd have to also introduce bitclock-consumer > > and frameclock-consumer properties to mirror the existing bitclock-master and > > frameclock-master properties, as an explicit absence of the *-master property on > > both sides would have to default to the original ASoC behaviour described above. > > If simple-card can't be made to work that's fine, it's deprecated > anyway. Ah OK, I didn't know that. Right now I'm using graph-card2, that's not deprecated, right? Kind regards, Alvin
On Fri, Jun 02, 2023 at 01:19:08PM +0100, Mark Brown wrote: > On Fri, Jun 02, 2023 at 12:12:52PM +0000, Alvin Šipraga wrote: > > On Fri, Jun 02, 2023 at 12:43:51PM +0100, Mark Brown wrote: > > > > Why would we have a property for this and not just describe whatever the > > > actual clocking arrangement is? > > > Sure - let me just elaborate on my thinking and maybe you can help me with a > > better approach: > > > The clocking arrangement is encoded in the dai_fmt field of snd_soc_dai_link, > > but this is a single value that describes the format on both ends. The current > > behaviour of ASoC is to flip the clock roles encoded in dai_fmt when applying it > > to the CPU side of the link. > > > Looking from a DT perspective, if I do not specify e.g. bitclock-master on > > either side of the link, then the dai_fmt will describe the codec as a bitclock > > consumer and (after flipping) the CPU as a provider. That's the default > > implication of the DT bindings and I can't break compatibility there. > > None of this addresses my question. To repeat why would we not just > describe the actual clocking arrangement here - this property does not > specify where the clock actually comes from at all, we're still going to > need additional information for that and if we've described that clock > then we already know it's there without having to specify any more > properties. Maybe I overcomplicated your point with my previous reply. Some questions to clarify: 1. You don't like the DT property because it should be inferrable by other means. Correct? 2. As for the flag added to snd_soc_dai_link, do you think that is an OK approach? Just want to understand which direction you would like me to focus the effort. Kind regards, Alvin
On Fri, Jun 02, 2023 at 12:42:49PM +0000, Alvin Šipraga wrote: > Yes I see what you mean. On my platform the clock source is actually described > by the common clock framework, so I would want to use that. If it were a > component driver then it would most likely be a codec that is part of the > dai-link anyway. So what about having two struct clk pointers in struct > snd_soc_dai? > > struct snd_soc_dai { > /* ... */ > struct clk *bitclock_provider; > struct clk *frameclock_provider; > /* ... */ > }; > If non-NULL I could then have the ASoC core enable/disable the clocks on demand? > I would say in hw_params/hw_free, albeit that runs after set_fmt. hw_params() can be called repeatedly so that's not a good fit but broadly yes. > Having said that, I see ASoC doesn't really use the CCF much... am I way off? Ideally we'd be representing more of the clocking via the clock framework but at present yes. > I don't think it's feasible to modify every component driver to explicitly > handle this and then ignore any CBP_CFP bits set in its call to set_fmt - this > is why I want help from the ASoC core. Sure, but that's not going to impact the DT bindings. All these things are driven from the machine driver. > > If simple-card can't be made to work that's fine, it's deprecated > > anyway. > Ah OK, I didn't know that. Right now I'm using graph-card2, that's not > deprecated, right? Yes, audio-graph-card replaces simple-card.
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index b05e05c81cc4..ce738d1a394d 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -27,6 +27,11 @@ definitions: description: dai-link uses bit clock inversion $ref: /schemas/types.yaml#/definitions/flag + symmetric-clock-roles: + description: | + dai-link uses same clock consumer/provider role for both CPU and Codec + $ref: /schemas/types.yaml#/definitions/flag + dai-tdm-slot-num: description: see tdm-slot.txt. $ref: /schemas/types.yaml#/definitions/uint32 @@ -128,6 +133,8 @@ definitions: $ref: "#/definitions/frame-inversion" bitclock-inversion: $ref: "#/definitions/bitclock-inversion" + symmetric-clock-roles: + $ref: "#/definitions/symmetric-clock-roles" frame-master: $ref: /schemas/types.yaml#/definitions/flag bitclock-master: @@ -181,6 +188,8 @@ properties: $ref: "#/definitions/frame-inversion" simple-audio-card,bitclock-inversion: $ref: "#/definitions/bitclock-inversion" + simple-audio-card,symmetric-clock-roles: + $ref: "#/definitions/symmetric-clock-roles" simple-audio-card,format: $ref: "#/definitions/format" simple-audio-card,mclk-fs: @@ -230,6 +239,8 @@ patternProperties: $ref: "#/definitions/frame-inversion" bitclock-inversion: $ref: "#/definitions/bitclock-inversion" + symmetric-clock-roles: + $ref: "#/definitions/symmetric-clock-roles" format: $ref: "#/definitions/format" mclk-fs: