| Message ID | 20230926-msm8916-modem-v1-3-398eec74bac9@gerhold.net |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: qcom: msm8916/39: Enable sound and modem with QDSP6 | expand |
On 26.09.2023 18:51, Stephan Gerhold wrote: > Most MSM8916/MSM8939 devices use very similar setups for the modem, > because most of the device-specific details are abstracted by the modem > firmware. There are several definitions (status switches, DAI links > etc) that will be exactly the same for every board. > > Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to > simplify enabling the modem for such devices. By default the > digital/analog codec in the SoC/PMIC is used, but boards can define > additional codecs using the templates for Secondary and Quaternary > MI2S. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- I'd rather see at least one usage so that you aren't introducing effectively non-compiled code.. > arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi | 163 ++++++++++++++++++++++ > 1 file changed, 163 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi b/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi > new file mode 100644 > index 000000000000..ddd74d428406 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi > @@ -0,0 +1,163 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) > +/* > + * msm8916-modem-qdsp6.dtsi describes the typical modem setup on MSM8916 devices > + * (or similar SoCs) with audio routed via the QDSP6 services provided by the > + * modem firmware. The digital/analog codec in the SoC/PMIC is used by default, > + * but boards can define additional codecs using the templates for Secondary and > + * Quaternary MI2S. > + */ > + > +#include <dt-bindings/sound/qcom,q6afe.h> > +#include <dt-bindings/sound/qcom,q6asm.h> > + > +&apr { > + status = "okay"; > +}; > + > +&bam_dmux { > + status = "okay"; > +}; > + > +&bam_dmux_dma { > + status = "okay"; > +}; > + > +&lpass { > + status = "reserved"; /* Controlled by QDSP6 */ > +}; > + > +&lpass_codec { > + status = "okay"; > +}; Any reason for it to stay disabled? > + > +&mba_mem { > + status = "okay"; > +}; > + > +&mpss { > + status = "okay"; > +}; > + > +&mpss_mem { > + status = "okay"; > +}; > + > +&pm8916_codec { > + status = "okay"; > +}; Ditto [...] > + multimedia1-dai-link { > + link-name = "MultiMedia1"; Newline before last property and subnodes, please [...] > + sound_dai_secondary: mi2s-secondary-dai-link { > + link-name = "Secondary MI2S"; > + status = "disabled"; /* Needs extra codec configuration */ Hmm.. Potential good user of /omit-if-no-ref/? Konrad
On 26.09.2023 21:06, Stephan Gerhold wrote: > On Tue, Sep 26, 2023 at 08:49:24PM +0200, Konrad Dybcio wrote: >> On 26.09.2023 18:51, Stephan Gerhold wrote: >>> Most MSM8916/MSM8939 devices use very similar setups for the modem, >>> because most of the device-specific details are abstracted by the modem >>> firmware. There are several definitions (status switches, DAI links >>> etc) that will be exactly the same for every board. >>> >>> Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to >>> simplify enabling the modem for such devices. By default the >>> digital/analog codec in the SoC/PMIC is used, but boards can define >>> additional codecs using the templates for Secondary and Quaternary >>> MI2S. >>> >>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net> >>> --- >> I'd rather see at least one usage so that you aren't introducing >> effectively non-compiled code.. >> > > There are 10 usages in the rest of the patch series. > Is that enough? :D > > IMHO it doesn't make sense to squash this with one of the device > patches, especially considering several of them are primarily authored > by others. I see.. Well, I guess I don't have better counter-arguments, but please consider this the next time around. [...] >>> +&lpass_codec { >>> + status = "okay"; >>> +}; >> Any reason for it to stay disabled? >> > > You mean in msm8916.dtsi? Yes > For the SoC dtsi we don't make assumptions > what devices use or not. There could be devices that ignore the internal > codec entirely. If there is nothing connected to the codec lpass_codec > should not be enabled (e.g. the msm8916-ufi.dtsi devices). See my reply to patch 5 [...] >>> + sound_dai_secondary: mi2s-secondary-dai-link { >>> + link-name = "Secondary MI2S"; >>> + status = "disabled"; /* Needs extra codec configuration */ >> Hmm.. Potential good user of /omit-if-no-ref/? >> > > AFAICT /omit-if-no-ref/ is for phandle references only. Basically it > would only work if you would somewhere reference the phandle: > > list-of-sound-dais = <&sound_dai_primary &sound_dai_secondary>; > > But this doesn't exist so /omit-if-no-ref/ cannot be used here. Ahh right, this is the one we don't reference.. Too bad, would be a nice fit :/ I only see one usage of it though (patch 7), perhaps it could be kept local to that one? Konrad
On Tue, Sep 26, 2023 at 10:01:21PM +0200, Konrad Dybcio wrote: > On 26.09.2023 21:06, Stephan Gerhold wrote: > > On Tue, Sep 26, 2023 at 08:49:24PM +0200, Konrad Dybcio wrote: > >> On 26.09.2023 18:51, Stephan Gerhold wrote: > >>> Most MSM8916/MSM8939 devices use very similar setups for the modem, > >>> because most of the device-specific details are abstracted by the modem > >>> firmware. There are several definitions (status switches, DAI links > >>> etc) that will be exactly the same for every board. > >>> > >>> Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to > >>> simplify enabling the modem for such devices. By default the > >>> digital/analog codec in the SoC/PMIC is used, but boards can define > >>> additional codecs using the templates for Secondary and Quaternary > >>> MI2S. > >>> > >>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > >>> --- > >> I'd rather see at least one usage so that you aren't introducing > >> effectively non-compiled code.. > >> > > > > There are 10 usages in the rest of the patch series. > > Is that enough? :D > > > > IMHO it doesn't make sense to squash this with one of the device > > patches, especially considering several of them are primarily authored > > by others. > I see.. > > Well, I guess I don't have better counter-arguments, but please > consider this the next time around. > Will do! > [...] > > >>> +&lpass_codec { > >>> + status = "okay"; > >>> +}; > >> Any reason for it to stay disabled? > >> > > > > You mean in msm8916.dtsi? > Yes > > > For the SoC dtsi we don't make assumptions > > what devices use or not. There could be devices that ignore the internal > > codec entirely. If there is nothing connected to the codec lpass_codec > > should not be enabled (e.g. the msm8916-ufi.dtsi devices). > See my reply to patch 5 > > [...] > Let's continue discussing that there I guess. :D > >>> + sound_dai_secondary: mi2s-secondary-dai-link { > >>> + link-name = "Secondary MI2S"; > >>> + status = "disabled"; /* Needs extra codec configuration */ > >> Hmm.. Potential good user of /omit-if-no-ref/? > >> > > > > AFAICT /omit-if-no-ref/ is for phandle references only. Basically it > > would only work if you would somewhere reference the phandle: > > > > list-of-sound-dais = <&sound_dai_primary &sound_dai_secondary>; > > > > But this doesn't exist so /omit-if-no-ref/ cannot be used here. > Ahh right, this is the one we don't reference.. Too bad, > would be a nice fit :/ > > I only see one usage of it though (patch 7), perhaps it could > be kept local to that one? > This patch series just contains the initial set of msm8916-modem-qdsp6.dtsi users (for devices which are already upstream). We probably have like 20 more that still need to be upstreamed. :D sound_dai_secondary is fairly rare, but there is at least one more user that will probably end up upstream soon. I think the overhead of these template notes is absolutely negligible compared to all the (potentially) unused SoC nodes we have. :D Thanks, Stephan
On Mon, Oct 02, 2023 at 11:59:21AM +0200, Konrad Dybcio wrote: > > > On 9/26/23 22:17, Stephan Gerhold wrote: > > On Tue, Sep 26, 2023 at 10:01:21PM +0200, Konrad Dybcio wrote: > > > On 26.09.2023 21:06, Stephan Gerhold wrote: > > > > On Tue, Sep 26, 2023 at 08:49:24PM +0200, Konrad Dybcio wrote: > > > > > On 26.09.2023 18:51, Stephan Gerhold wrote: > > > > > > Most MSM8916/MSM8939 devices use very similar setups for the modem, > > > > > > because most of the device-specific details are abstracted by the modem > > > > > > firmware. There are several definitions (status switches, DAI links > > > > > > etc) that will be exactly the same for every board. > > > > > > > > > > > > Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to > > > > > > simplify enabling the modem for such devices. By default the > > > > > > digital/analog codec in the SoC/PMIC is used, but boards can define > > > > > > additional codecs using the templates for Secondary and Quaternary > > > > > > MI2S. > > > > > > > > > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > > > > > --- > > > > > I'd rather see at least one usage so that you aren't introducing > > > > > effectively non-compiled code.. > > > > > > > > > > > > > There are 10 usages in the rest of the patch series. > > > > Is that enough? :D > > > > > > > > IMHO it doesn't make sense to squash this with one of the device > > > > patches, especially considering several of them are primarily authored > > > > by others. > > > I see.. > > > > > > Well, I guess I don't have better counter-arguments, but please > > > consider this the next time around. > > > > > > > Will do! > > > > > [...] > > > > > > > > > +&lpass_codec { > > > > > > + status = "okay"; > > > > > > +}; > > > > > Any reason for it to stay disabled? > > > > > > > > > > > > > You mean in msm8916.dtsi? > > > Yes > > > > > > > For the SoC dtsi we don't make assumptions > > > > what devices use or not. There could be devices that ignore the internal > > > > codec entirely. If there is nothing connected to the codec lpass_codec > > > > should not be enabled (e.g. the msm8916-ufi.dtsi devices). > > > See my reply to patch 5 > > > > > > [...] > > > > > > > Let's continue discussing that there I guess. :D > > > > > > > > + sound_dai_secondary: mi2s-secondary-dai-link { > > > > > > + link-name = "Secondary MI2S"; > > > > > > + status = "disabled"; /* Needs extra codec configuration */ > > > > > Hmm.. Potential good user of /omit-if-no-ref/? > > > > > > > > > > > > > AFAICT /omit-if-no-ref/ is for phandle references only. Basically it > > > > would only work if you would somewhere reference the phandle: > > > > > > > > list-of-sound-dais = <&sound_dai_primary &sound_dai_secondary>; > > > > > > > > But this doesn't exist so /omit-if-no-ref/ cannot be used here. > > > Ahh right, this is the one we don't reference.. Too bad, > > > would be a nice fit :/ > > > > > > I only see one usage of it though (patch 7), perhaps it could > > > be kept local to that one? > > > > > > > This patch series just contains the initial set of > > msm8916-modem-qdsp6.dtsi users (for devices which are already upstream). > > We probably have like 20 more that still need to be upstreamed. :D > > > > sound_dai_secondary is fairly rare, but there is at least one more user > > that will probably end up upstream soon. > 2 users don't sound particularly great in a devicetree included by 20 other > non-users > > > I think the overhead of these template notes is absolutely negligible > > compared to all the (potentially) unused SoC nodes we have. :D > Yes, however the unused SoC nodes are mostly standardized and could be used > as-they-are on a vast majority of devices > To be fair we're talking about 152 bytes difference here, in a DTB that is like 60,000 bytes total. But I can't think of enough compelling arguments for my "template node" approach, so I will try to rework this in v2. Let's see if I can get rid of the unused nodes without too much mess. :) Thanks, Stephan
diff --git a/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi b/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi new file mode 100644 index 000000000000..ddd74d428406 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi @@ -0,0 +1,163 @@ +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) +/* + * msm8916-modem-qdsp6.dtsi describes the typical modem setup on MSM8916 devices + * (or similar SoCs) with audio routed via the QDSP6 services provided by the + * modem firmware. The digital/analog codec in the SoC/PMIC is used by default, + * but boards can define additional codecs using the templates for Secondary and + * Quaternary MI2S. + */ + +#include <dt-bindings/sound/qcom,q6afe.h> +#include <dt-bindings/sound/qcom,q6asm.h> + +&apr { + status = "okay"; +}; + +&bam_dmux { + status = "okay"; +}; + +&bam_dmux_dma { + status = "okay"; +}; + +&lpass { + status = "reserved"; /* Controlled by QDSP6 */ +}; + +&lpass_codec { + status = "okay"; +}; + +&mba_mem { + status = "okay"; +}; + +&mpss { + status = "okay"; +}; + +&mpss_mem { + status = "okay"; +}; + +&pm8916_codec { + status = "okay"; +}; + +&q6afedai { + dai@16 { + reg = <PRIMARY_MI2S_RX>; + qcom,sd-lines = <0 1>; + }; + dai@20 { + reg = <TERTIARY_MI2S_TX>; + qcom,sd-lines = <0 1>; + }; +}; + +&q6asmdai { + dai@0 { + reg = <0>; + direction = <Q6ASM_DAI_RX>; + }; + dai@1 { + reg = <1>; + direction = <Q6ASM_DAI_TX>; + }; + dai@2 { + reg = <2>; + direction = <Q6ASM_DAI_RX>; + }; + dai@3 { + reg = <3>; + direction = <Q6ASM_DAI_RX>; + is-compress-dai; + }; +}; + +&sound { + compatible = "qcom,msm8916-qdsp6-sndcard"; + model = "msm8916"; + + pinctrl-0 = <&cdc_pdm_default>; + pinctrl-1 = <&cdc_pdm_sleep>; + pinctrl-names = "default", "sleep"; + + multimedia1-dai-link { + link-name = "MultiMedia1"; + cpu { + sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>; + }; + }; + + multimedia2-dai-link { + link-name = "MultiMedia2"; + cpu { + sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA2>; + }; + }; + + multimedia3-dai-link { + link-name = "MultiMedia3"; + cpu { + sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA3>; + }; + }; + + multimedia4-dai-link { + link-name = "MultiMedia4"; + cpu { + sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA4>; + }; + }; + + sound_dai_primary: mi2s-primary-dai-link { + link-name = "Primary MI2S"; + cpu { + sound-dai = <&q6afedai PRIMARY_MI2S_RX>; + }; + platform { + sound-dai = <&q6routing>; + }; + codec { + sound-dai = <&lpass_codec 0>, <&pm8916_codec 0>; + }; + }; + + sound_dai_secondary: mi2s-secondary-dai-link { + link-name = "Secondary MI2S"; + status = "disabled"; /* Needs extra codec configuration */ + cpu { + sound-dai = <&q6afedai SECONDARY_MI2S_RX>; + }; + platform { + sound-dai = <&q6routing>; + }; + }; + + sound_dai_tertiary: mi2s-tertiary-dai-link { + link-name = "Tertiary MI2S"; + cpu { + sound-dai = <&q6afedai TERTIARY_MI2S_TX>; + }; + platform { + sound-dai = <&q6routing>; + }; + codec { + sound-dai = <&lpass_codec 1>, <&pm8916_codec 1>; + }; + }; + + sound_dai_quaternary: mi2s-quaternary-dai-link { + link-name = "Quaternary MI2S"; + status = "disabled"; /* Needs extra codec configuration */ + cpu { + sound-dai = <&q6afedai QUATERNARY_MI2S_RX>; + }; + platform { + sound-dai = <&q6routing>; + }; + }; +};
Most MSM8916/MSM8939 devices use very similar setups for the modem, because most of the device-specific details are abstracted by the modem firmware. There are several definitions (status switches, DAI links etc) that will be exactly the same for every board. Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to simplify enabling the modem for such devices. By default the digital/analog codec in the SoC/PMIC is used, but boards can define additional codecs using the templates for Secondary and Quaternary MI2S. Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi | 163 ++++++++++++++++++++++ 1 file changed, 163 insertions(+)