| Message ID | 20241025104548.1220076-2-fshao@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Update properties in two ASoC DT bindings | expand |
On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote: > Add "mediatek,adsp" property for the ADSP handle if ADSP is enabled on > the platform. We see this from the diff. > Add "mediatek,dai-link" property for the required DAI links to configure > sound card. We see this from the diff. > > Both properties are commonly used in the MediaTek sound card driver. If they are used, why suddenly they are needed? What changed? > > Signed-off-by: Fei Shao <fshao@chromium.org> > --- > > .../bindings/sound/mediatek,mt8188-mt6359.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > index f94ad0715e32..701cedfa38d2 100644 > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > @@ -29,6 +29,16 @@ properties: > $ref: /schemas/types.yaml#/definitions/phandle > description: The phandle of MT8188 ASoC platform. > > + mediatek,adsp: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of MT8188 ADSP platform. And what is the difference between ASoC and ADSP platforms? What are they used for? > + > + mediatek,dai-link: > + $ref: /schemas/types.yaml#/definitions/string-array > + description: > + A list of the desired dai-links in the sound card. Each entry is a > + name defined in the machine driver. The list is provided below. I don't understand why do you need it. Your msg is pretty useless - you describe what you do, instead of why. Best regards, Krzysztof
On Mon, Oct 28, 2024 at 4:54 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote: > > Add "mediatek,adsp" property for the ADSP handle if ADSP is enabled on > > the platform. > > We see this from the diff. > > > Add "mediatek,dai-link" property for the required DAI links to configure > > sound card. > > We see this from the diff. > > > > > Both properties are commonly used in the MediaTek sound card driver. > > If they are used, why suddenly they are needed? What changed? Nothing has changed. These should have been added altogether when the binding was first introduced. This patch is to fill the gaps and fix dtbs_check warnings, like I mentioned in the cover letter. I can add a line in the commit message saying it's to fix the warning in addition to the cover letter, if that's preferred. > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > --- > > > > .../bindings/sound/mediatek,mt8188-mt6359.yaml | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > > index f94ad0715e32..701cedfa38d2 100644 > > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml > > @@ -29,6 +29,16 @@ properties: > > $ref: /schemas/types.yaml#/definitions/phandle > > description: The phandle of MT8188 ASoC platform. > > > > + mediatek,adsp: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of MT8188 ADSP platform. > > And what is the difference between ASoC and ADSP platforms? What are > they used for? I'm not a MediaTek or audio folks, and I'm afraid that I'm not the best person to explain the details accurately in front of experts on the list... I know it's an audio DSP but that explains nothing. MediaTek didn't provide a meaningful explanation in the tree or commits, and I want to avoid adding additional but likely misleading descriptions from someone who doesn't have enough knowledge, potentially causing even more confusing situations in the future. Plus, the same changes were accepted as-is in the past, so I assumed they might be self-explanatory to people who are familiar with the matter. > > > + > > + mediatek,dai-link: > > + $ref: /schemas/types.yaml#/definitions/string-array > > + description: > > + A list of the desired dai-links in the sound card. Each entry is a > > + name defined in the machine driver. > > The list is provided below. I don't understand why do you need it. Your > msg is pretty useless - you describe what you do, instead of why. I think this is used to explicitly list the intermediate but hidden DAIs, but again, there's not much info about them unless MediaTek can explain more details and why they need a vendor property for this. Regards, Fei > > Best regards, > Krzysztof >
Il 28/10/24 12:10, Fei Shao ha scritto: > On Mon, Oct 28, 2024 at 4:54 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote: >>> Add "mediatek,adsp" property for the ADSP handle if ADSP is enabled on >>> the platform. >> >> We see this from the diff. >> >>> Add "mediatek,dai-link" property for the required DAI links to configure >>> sound card. >> >> We see this from the diff. >> >>> >>> Both properties are commonly used in the MediaTek sound card driver. >> >> If they are used, why suddenly they are needed? What changed? > > Nothing has changed. These should have been added altogether when the > binding was first introduced. This patch is to fill the gaps and fix > dtbs_check warnings, like I mentioned in the cover letter. > I can add a line in the commit message saying it's to fix the warning > in addition to the cover letter, if that's preferred. > >> >>> >>> Signed-off-by: Fei Shao <fshao@chromium.org> >>> --- >>> >>> .../bindings/sound/mediatek,mt8188-mt6359.yaml | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml >>> index f94ad0715e32..701cedfa38d2 100644 >>> --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml >>> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml >>> @@ -29,6 +29,16 @@ properties: >>> $ref: /schemas/types.yaml#/definitions/phandle >>> description: The phandle of MT8188 ASoC platform. >>> >>> + mediatek,adsp: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: The phandle of MT8188 ADSP platform. >> >> And what is the difference between ASoC and ADSP platforms? What are >> they used for? > > I'm not a MediaTek or audio folks, and I'm afraid that I'm not the > best person to explain the details accurately in front of experts on > the list... I know it's an audio DSP but that explains nothing. > MediaTek didn't provide a meaningful explanation in the tree or > commits, and I want to avoid adding additional but likely misleading > descriptions from someone who doesn't have enough knowledge, > potentially causing even more confusing situations in the future. > Plus, the same changes were accepted as-is in the past, so I assumed > they might be self-explanatory to people who are familiar with the > matter. > The Audio DSP is a Tensilica HiFi-5 DSP, and it's a block that is - hardware speaking - separated from the rest of the Audio interfaces of the SoC. The whole sound subsystem can work either with or without the DSP, in the sense that the DSP itself can remain unpowered and completely unconfigured if its functionality is not desired - hence, this is a board specific configuration: if the board wants to use the DSP, we use the DSP - otherwise, we just don't. Regarding the two "platforms", in short: "mediatek,platform" -> Audio Front End (AFE) "mediatek,adsp" -> Audio DSP Now, you can either link the AFE DAIs to the I2S As for "mediatek,platform" - that's used to link the Analog Front End (AFE) as the DAI Link platform (so the path is direct to/from DL/UL DAIs to AFE) or the ADSP one as the DAI Link platform (so that the path is to/from DL/UL DAIs to DSP to AFE), but that - of course - still requires an AFE, otherwise you cannot get the audio out of the speakers or in from the mic anyway. >> >>> + >>> + mediatek,dai-link: >>> + $ref: /schemas/types.yaml#/definitions/string-array >>> + description: >>> + A list of the desired dai-links in the sound card. Each entry is a >>> + name defined in the machine driver. >> >> The list is provided below. I don't understand why do you need it. Your >> msg is pretty useless - you describe what you do, instead of why. > > I think this is used to explicitly list the intermediate but hidden > DAIs, but again, there's not much info about them unless MediaTek can > explain more details and why they need a vendor property for this. > Yes, this is used for exactly that... but I believe that we can deprecate this now that we have support for the "standard" `audio-routing` property and for the DAI Link nodes (examples that you can find in current device trees are mm-dai-link, hs-playback-dai-link, or any other subnode of the sound card). Specifically, those subnodes *do* require a "link-name" property, which *does* effectively contain the same DAI Link names as the ones that are inside of the "mediatek,dai-link" property. On MT8195, you can find both the subnodes and the mediatek,dai-link - yes - but that was done to retain compatibility of the device tree with old drivers (so, an unusual case of new device tree on old kernel). Finally, I believe that we can avoid adding that "mediatek,dai-link" property to the MT8188 binding, and rely on: 1. Whatever is provided in struct snd_soc_card for that device; and 2. Whatever is provided in device trees as dai link subnodes, which would restrict N.1 as that's anyway describing card prelinks. Cheers, Angelo > Regards, > Fei > >> >> Best regards, >> Krzysztof >>
On Mon, 2024-10-28 at 15:25 +0100, AngeloGioacchino Del Regno wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Il 28/10/24 12:10, Fei Shao ha scritto: > > On Mon, Oct 28, 2024 at 4:54 AM Krzysztof Kozlowski < > > krzk@kernel.org> wrote: > > > > > > On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote: > > > > Add "mediatek,adsp" property for the ADSP handle if ADSP is > > > > enabled on > > > > the platform. > > > > > > We see this from the diff. > > > > > > > Add "mediatek,dai-link" property for the required DAI links to > > > > configure > > > > sound card. > > > > > > We see this from the diff. > > > > > > > > > > > Both properties are commonly used in the MediaTek sound card > > > > driver. > > > > > > If they are used, why suddenly they are needed? What changed? > > > > Nothing has changed. These should have been added altogether when > > the > > binding was first introduced. This patch is to fill the gaps and > > fix > > dtbs_check warnings, like I mentioned in the cover letter. > > I can add a line in the commit message saying it's to fix the > > warning > > in addition to the cover letter, if that's preferred. > > > > > > > > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > > --- > > > > > > > > .../bindings/sound/mediatek,mt8188-mt6359.yaml | 10 > > > > ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > mt6359.yaml > > > > b/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > mt6359.yaml > > > > index f94ad0715e32..701cedfa38d2 100644 > > > > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > mt6359.yaml > > > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > mt6359.yaml > > > > @@ -29,6 +29,16 @@ properties: > > > > $ref: /schemas/types.yaml#/definitions/phandle > > > > description: The phandle of MT8188 ASoC platform. > > > > > > > > + mediatek,adsp: > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > + description: The phandle of MT8188 ADSP platform. > > > > > > And what is the difference between ASoC and ADSP platforms? What > > > are > > > they used for? > > > > I'm not a MediaTek or audio folks, and I'm afraid that I'm not the > > best person to explain the details accurately in front of experts > > on > > the list... I know it's an audio DSP but that explains nothing. > > MediaTek didn't provide a meaningful explanation in the tree or > > commits, and I want to avoid adding additional but likely > > misleading > > descriptions from someone who doesn't have enough knowledge, > > potentially causing even more confusing situations in the future. > > Plus, the same changes were accepted as-is in the past, so I > > assumed > > they might be self-explanatory to people who are familiar with the > > matter. > > > > The Audio DSP is a Tensilica HiFi-5 DSP, and it's a block that is - > hardware > speaking - separated from the rest of the Audio interfaces of the > SoC. > > The whole sound subsystem can work either with or without the DSP, in > the sense > that the DSP itself can remain unpowered and completely unconfigured > if its > functionality is not desired - hence, this is a board specific > configuration: > if the board wants to use the DSP, we use the DSP - otherwise, we > just don't. > > Regarding the two "platforms", in short: > "mediatek,platform" -> Audio Front End (AFE) > "mediatek,adsp" -> Audio DSP > > Now, you can either link the AFE DAIs to the I2S > > As for "mediatek,platform" - that's used to link the Analog Front End > (AFE) as > the DAI Link platform (so the path is direct to/from DL/UL DAIs to > AFE) or the > ADSP one as the DAI Link platform (so that the path is to/from DL/UL > DAIs to > DSP to AFE), but that - of course - still requires an AFE, otherwise > you cannot > get the audio out of the speakers or in from the mic anyway. > > > > > > > > + > > > > + mediatek,dai-link: > > > > + $ref: /schemas/types.yaml#/definitions/string-array > > > > + description: > > > > + A list of the desired dai-links in the sound card. Each > > > > entry is a > > > > + name defined in the machine driver. > > > > > > The list is provided below. I don't understand why do you need > > > it. Your > > > msg is pretty useless - you describe what you do, instead of why. > > > > I think this is used to explicitly list the intermediate but hidden > > DAIs, but again, there's not much info about them unless MediaTek > > can > > explain more details and why they need a vendor property for this. > > > > Yes, this is used for exactly that... but I believe that we can > deprecate this > now that we have support for the "standard" `audio-routing` property > and for the > DAI Link nodes (examples that you can find in current device trees > are mm-dai-link, > hs-playback-dai-link, or any other subnode of the sound card). > > Specifically, those subnodes *do* require a "link-name" property, > which *does* > effectively contain the same DAI Link names as the ones that are > inside of the > "mediatek,dai-link" property. > > On MT8195, you can find both the subnodes and the mediatek,dai-link - > yes - but > that was done to retain compatibility of the device tree with old > drivers (so, > an unusual case of new device tree on old kernel). > > Finally, I believe that we can avoid adding that "mediatek,dai-link" > property > to the MT8188 binding, and rely on: > 1. Whatever is provided in struct snd_soc_card for that device; and > 2. Whatever is provided in device trees as dai link subnodes, which > would > restrict N.1 as that's anyway describing card prelinks. > The "mediatek,dai-link" property is utilized to hide the unused dai- links in the sound card. By hiding the unused FE links, it can save the necessary memory and prevent conflicts where both the DSP and AP control the same AFE Memifs. This concept was first implemented in mt8195 as Mark aimed to avoid the need for a separate driver for different system configurations. With the introduction of DSP (SOF), if certain AFE Memifs are already in use in the DSP route, they should be excluded from the PCM nodes created for the AFE platform. At that time, we did not have a better way to handle these scenarios, so we made use of a vendor-defined property. It has been a while since I last kept up with the updates in mt8188, so I'm uncertain if the current mechanism for sound card description is sufficient for handling such scenarios. If it is, I agree that we can deprecate such a vendor property from mt8188. Thanks, Trevor > Cheers, > Angelo > > > Regards, > > Fei > > > > > > > > Best regards, > > > Krzysztof > > > > >
On Tue, Oct 29, 2024 at 11:05 AM Trevor Wu (吳文良) <Trevor.Wu@mediatek.com> wrote: > > On Mon, 2024-10-28 at 15:25 +0100, AngeloGioacchino Del Regno wrote: > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > > > > > Il 28/10/24 12:10, Fei Shao ha scritto: > > > On Mon, Oct 28, 2024 at 4:54 AM Krzysztof Kozlowski < > > > krzk@kernel.org> wrote: > > > > > > > > On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote: > > > > > Add "mediatek,adsp" property for the ADSP handle if ADSP is > > > > > enabled on > > > > > the platform. > > > > > > > > We see this from the diff. > > > > > > > > > Add "mediatek,dai-link" property for the required DAI links to > > > > > configure > > > > > sound card. > > > > > > > > We see this from the diff. > > > > > > > > > > > > > > Both properties are commonly used in the MediaTek sound card > > > > > driver. > > > > > > > > If they are used, why suddenly they are needed? What changed? > > > > > > Nothing has changed. These should have been added altogether when > > > the > > > binding was first introduced. This patch is to fill the gaps and > > > fix > > > dtbs_check warnings, like I mentioned in the cover letter. > > > I can add a line in the commit message saying it's to fix the > > > warning > > > in addition to the cover letter, if that's preferred. > > > > > > > > > > > > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > > > --- > > > > > > > > > > .../bindings/sound/mediatek,mt8188-mt6359.yaml | 10 > > > > > ++++++++++ > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > diff --git > > > > > a/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > > mt6359.yaml > > > > > b/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > > mt6359.yaml > > > > > index f94ad0715e32..701cedfa38d2 100644 > > > > > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > > mt6359.yaml > > > > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188- > > > > > mt6359.yaml > > > > > @@ -29,6 +29,16 @@ properties: > > > > > $ref: /schemas/types.yaml#/definitions/phandle > > > > > description: The phandle of MT8188 ASoC platform. > > > > > > > > > > + mediatek,adsp: > > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > > + description: The phandle of MT8188 ADSP platform. > > > > > > > > And what is the difference between ASoC and ADSP platforms? What > > > > are > > > > they used for? > > > > > > I'm not a MediaTek or audio folks, and I'm afraid that I'm not the > > > best person to explain the details accurately in front of experts > > > on > > > the list... I know it's an audio DSP but that explains nothing. > > > MediaTek didn't provide a meaningful explanation in the tree or > > > commits, and I want to avoid adding additional but likely > > > misleading > > > descriptions from someone who doesn't have enough knowledge, > > > potentially causing even more confusing situations in the future. > > > Plus, the same changes were accepted as-is in the past, so I > > > assumed > > > they might be self-explanatory to people who are familiar with the > > > matter. > > > > > > > The Audio DSP is a Tensilica HiFi-5 DSP, and it's a block that is - > > hardware > > speaking - separated from the rest of the Audio interfaces of the > > SoC. > > > > The whole sound subsystem can work either with or without the DSP, in > > the sense > > that the DSP itself can remain unpowered and completely unconfigured > > if its > > functionality is not desired - hence, this is a board specific > > configuration: > > if the board wants to use the DSP, we use the DSP - otherwise, we > > just don't. > > > > Regarding the two "platforms", in short: > > "mediatek,platform" -> Audio Front End (AFE) > > "mediatek,adsp" -> Audio DSP > > > > Now, you can either link the AFE DAIs to the I2S > > > > As for "mediatek,platform" - that's used to link the Analog Front End > > (AFE) as > > the DAI Link platform (so the path is direct to/from DL/UL DAIs to > > AFE) or the > > ADSP one as the DAI Link platform (so that the path is to/from DL/UL > > DAIs to > > DSP to AFE), but that - of course - still requires an AFE, otherwise > > you cannot > > get the audio out of the speakers or in from the mic anyway. > > > > > > > > > > > + > > > > > + mediatek,dai-link: > > > > > + $ref: /schemas/types.yaml#/definitions/string-array > > > > > + description: > > > > > + A list of the desired dai-links in the sound card. Each > > > > > entry is a > > > > > + name defined in the machine driver. > > > > > > > > The list is provided below. I don't understand why do you need > > > > it. Your > > > > msg is pretty useless - you describe what you do, instead of why. > > > > > > I think this is used to explicitly list the intermediate but hidden > > > DAIs, but again, there's not much info about them unless MediaTek > > > can > > > explain more details and why they need a vendor property for this. > > > > > > > Yes, this is used for exactly that... but I believe that we can > > deprecate this > > now that we have support for the "standard" `audio-routing` property > > and for the > > DAI Link nodes (examples that you can find in current device trees > > are mm-dai-link, > > hs-playback-dai-link, or any other subnode of the sound card). > > > > Specifically, those subnodes *do* require a "link-name" property, > > which *does* > > effectively contain the same DAI Link names as the ones that are > > inside of the > > "mediatek,dai-link" property. > > > > On MT8195, you can find both the subnodes and the mediatek,dai-link - > > yes - but > > that was done to retain compatibility of the device tree with old > > drivers (so, > > an unusual case of new device tree on old kernel). > > > > Finally, I believe that we can avoid adding that "mediatek,dai-link" > > property > > to the MT8188 binding, and rely on: > > 1. Whatever is provided in struct snd_soc_card for that device; and > > 2. Whatever is provided in device trees as dai link subnodes, which > > would > > restrict N.1 as that's anyway describing card prelinks. > > > > The "mediatek,dai-link" property is utilized to hide the unused dai- > links in the sound card. By hiding the unused FE links, it can save the > necessary memory and prevent conflicts where both the DSP and AP > control the same AFE Memifs. > > This concept was first implemented in mt8195 as Mark aimed to avoid the > need for a separate driver for different system configurations. > With the introduction of DSP (SOF), if certain AFE Memifs are already > in use in the DSP route, they should be excluded from the PCM nodes > created for the AFE platform. > > At that time, we did not have a better way to handle these scenarios, > so we made use of a vendor-defined property. > > It has been a while since I last kept up with the updates in mt8188, so > I'm uncertain if the current mechanism for sound card description is > sufficient for handling such scenarios. If it is, I agree that we can > deprecate such a vendor property from mt8188. > > Thanks, > Trevor > > > > Cheers, > > Angelo Many thanks Angelo and Trevor for the information. The audio-routing property is in place, so I'll check if removing the vendor property makes any impact, and refresh the series accordingly. Thanks, Fei > > > > > Regards, > > > Fei > > > > > > > > > > > Best regards, > > > > Krzysztof > > > > > > > >
diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml index f94ad0715e32..701cedfa38d2 100644 --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml @@ -29,6 +29,16 @@ properties: $ref: /schemas/types.yaml#/definitions/phandle description: The phandle of MT8188 ASoC platform. + mediatek,adsp: + $ref: /schemas/types.yaml#/definitions/phandle + description: The phandle of MT8188 ADSP platform. + + mediatek,dai-link: + $ref: /schemas/types.yaml#/definitions/string-array + description: + A list of the desired dai-links in the sound card. Each entry is a + name defined in the machine driver. + patternProperties: "^dai-link-[0-9]+$": type: object
Add "mediatek,adsp" property for the ADSP handle if ADSP is enabled on the platform. Add "mediatek,dai-link" property for the required DAI links to configure sound card. Both properties are commonly used in the MediaTek sound card driver. Signed-off-by: Fei Shao <fshao@chromium.org> --- .../bindings/sound/mediatek,mt8188-mt6359.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+)