| Message ID | 20230926-msm8916-modem-v1-3-398eec74bac9@gerhold.net (mailing list archive) |
|---|---|
| State | Superseded |
| 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 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. > > 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? > You mean in msm8916.dtsi? 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). This include is a bit more "opinionated", to reduce duplication for the most common setup. But it's separate and optional to use. The SoC dtsi is included by everyone. > > + > > +&mba_mem { > > + status = "okay"; > > +}; > > + > > +&mpss { > > + status = "okay"; > > +}; > > + > > +&mpss_mem { > > + status = "okay"; > > +}; > > + > > +&pm8916_codec { > > + status = "okay"; > > +}; > Ditto > Same as above. > > + multimedia1-dai-link { > > + link-name = "MultiMedia1"; > Newline before last property and subnodes, please > Thanks, will change this! > > > + 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. Thanks, Stephan
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 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 Konrad
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(+)