| Message ID | 20220422071534.15653-1-tinghan.shen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] dt-bindings: dsp: mediatek: add mt8186 dsp document | expand |
On Fri, 2022-04-22 at 15:15 +0800, Tinghan Shen wrote: > This patch adds mt8186 dsp document. The dsp is used for Sound Open > Firmware driver node. It includes registers, clocks, memory regions, > and mailbox for dsp. > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com> > --- > > This patch depends on MT8186 clock bindings. > https://lore.kernel.org/all/20220409132251.31725-2-chun-jie.chen@mediatek.com/ > > --- > .../bindings/dsp/mediatek,mt8186-dsp.yaml | 93 > +++++++++++++++++++ > 1 file changed, 93 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > > diff --git a/Documentation/devicetree/bindings/dsp/mediatek,mt8186- > dsp.yaml b/Documentation/devicetree/bindings/dsp/mediatek,mt8186- > dsp.yaml > new file mode 100644 > index 000000000000..00a79e880895 > --- /dev/null > +++ b/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > @@ -0,0 +1,93 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/dsp/mediatek,mt8186-dsp.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mediatek mt8186 DSP core Hello Tinghan, Please use MediaTek. Thanks. BRs, Rex > + > +maintainers: > + - Tinghan Shen <tinghan.shen@mediatek.com> > + > +description: | > + MediaTek mt8186 SoC contains a DSP core used for > + advanced pre- and post- audio processing. > + > +properties: > + compatible: > + const: mediatek,mt8186-dsp > + > + reg: > + items: > + - description: Address and size of the DSP config registers > + - description: Address and size of the DSP SRAM > + - description: Address and size of the DSP secure registers > + - description: Address and size of the DSP bus registers > + > + reg-names: > + items: > + - const: cfg > + - const: sram > + - const: sec > + - const: bus > + > + clocks: > + items: > + - description: mux for audio dsp clock > + - description: mux for audio dsp local bus > + > + clock-names: > + items: > + - const: audiodsp_sel > + - const: adsp_bus_sel > + > + power-domains: > + maxItems: 1 > + > + mboxes: > + items: > + - description: ipc reply between host and audio DSP. > + - description: ipc request between host and audio DSP. > + > + mbox-names: > + items: > + - const: mbox0 > + - const: mbox1 > + > + memory-region: > + items: > + - description: dma buffer between host and DSP. > + - description: DSP system memory. > + > +required: > + - compatible > + - reg > + - reg-names > + - clocks > + - clock-names > + - power-domains > + - mbox-names > + - mboxes > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/clock/mt8186-clk.h> > + dsp@10680000 { > + compatible = "mediatek,mt8186-dsp"; > + reg = <0x10680000 0x2000>, > + <0x10800000 0x100000>, > + <0x1068b000 0x100>, > + <0x1068f000 0x1000>; > + reg-names = "cfg", "sram", "sec", "bus"; > + clocks = <&topckgen CLK_TOP_AUDIODSP>, > + <&topckgen CLK_TOP_ADSP_BUS>; > + clock-names = "audiodsp_sel", > + "adsp_bus_sel"; > + power-domains = <&spm 6>; > + mbox-names = "mbox0", "mbox1"; > + mboxes = <&adsp_mailbox0>, <&adsp_mailbox1>; > + };
On 22/04/2022 09:15, Tinghan Shen wrote: > This patch adds mt8186 dsp document. The dsp is used for Sound Open > Firmware driver node. It includes registers, clocks, memory regions, > and mailbox for dsp. > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com> > --- > > This patch depends on MT8186 clock bindings. > https://lore.kernel.org/all/20220409132251.31725-2-chun-jie.chen@mediatek.com/ > > --- > .../bindings/dsp/mediatek,mt8186-dsp.yaml | 93 +++++++++++++++++++ > 1 file changed, 93 insertions(+) > create mode 100644 Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > > diff --git a/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml b/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > new file mode 100644 > index 000000000000..00a79e880895 > --- /dev/null > +++ b/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > @@ -0,0 +1,93 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/dsp/mediatek,mt8186-dsp.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mediatek mt8186 DSP core > + > +maintainers: > + - Tinghan Shen <tinghan.shen@mediatek.com> > + > +description: | > + MediaTek mt8186 SoC contains a DSP core used for > + advanced pre- and post- audio processing. > + > +properties: > + compatible: > + const: mediatek,mt8186-dsp > + > + reg: > + items: > + - description: Address and size of the DSP config registers > + - description: Address and size of the DSP SRAM > + - description: Address and size of the DSP secure registers > + - description: Address and size of the DSP bus registers > + > + reg-names: > + items: > + - const: cfg > + - const: sram > + - const: sec > + - const: bus > + > + clocks: > + items: > + - description: mux for audio dsp clock > + - description: mux for audio dsp local bus > + > + clock-names: > + items: > + - const: audiodsp_sel > + - const: adsp_bus_sel What does the "sel" stands for? Maybe just skip the "_sel" suffixes? > + > + power-domains: > + maxItems: 1 > + > + mboxes: > + items: > + - description: ipc reply between host and audio DSP. > + - description: ipc request between host and audio DSP. > + > + mbox-names: > + items: > + - const: mbox0 > + - const: mbox1 These should be rather some meaningful names, e.g. "rx" and "tx". > + > + memory-region: > + items: > + - description: dma buffer between host and DSP. > + - description: DSP system memory. > + > +required: > + - compatible > + - reg > + - reg-names > + - clocks > + - clock-names > + - power-domains > + - mbox-names > + - mboxes > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> You do not use these headers. > + #include <dt-bindings/clock/mt8186-clk.h> > + dsp@10680000 { Best regards, Krzysztof
On Fri, 22 Apr 2022 15:15:34 +0800, Tinghan Shen wrote: > This patch adds mt8186 dsp document. The dsp is used for Sound Open > Firmware driver node. It includes registers, clocks, memory regions, > and mailbox for dsp. > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com> > --- > > This patch depends on MT8186 clock bindings. > https://lore.kernel.org/all/20220409132251.31725-2-chun-jie.chen@mediatek.com/ > > --- > .../bindings/dsp/mediatek,mt8186-dsp.yaml | 93 +++++++++++++++++++ > 1 file changed, 93 insertions(+) > create mode 100644 Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > 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: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.example.dts:22:18: fatal error: dt-bindings/clock/mt8186-clk.h: No such file or directory 22 | #include <dt-bindings/clock/mt8186-clk.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:364: Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.example.dtb] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1401: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
Hi Rex, On Fri, 2022-04-22 at 15:59 +0800, Rex-BC Chen wrote: > On Fri, 2022-04-22 at 15:15 +0800, Tinghan Shen wrote: > > This patch adds mt8186 dsp document. The dsp is used for Sound Open > > Firmware driver node. It includes registers, clocks, memory regions, > > and mailbox for dsp. > > > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com> > > --- > > > > This patch depends on MT8186 clock bindings. > > > > https://lore.kernel.org/all/20220409132251.31725-2-chun-jie.chen@mediatek.com/ > > > > --- > > .../bindings/dsp/mediatek,mt8186-dsp.yaml | 93 > > +++++++++++++++++++ > > 1 file changed, 93 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > > > > diff --git a/Documentation/devicetree/bindings/dsp/mediatek,mt8186- > > dsp.yaml b/Documentation/devicetree/bindings/dsp/mediatek,mt8186- > > dsp.yaml > > new file mode 100644 > > index 000000000000..00a79e880895 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > > @@ -0,0 +1,93 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/dsp/mediatek,mt8186-dsp.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Mediatek mt8186 DSP core > > Hello Tinghan, > > Please use MediaTek. Ok. I'll update it at next version. Thanks, TingHan > Thanks. > > BRs, > Rex > > > + > > +maintainers: > > + - Tinghan Shen <tinghan.shen@mediatek.com> > > + > > +description: | > > + MediaTek mt8186 SoC contains a DSP core used for > > + advanced pre- and post- audio processing. > > + > > +properties: > > + compatible: > > + const: mediatek,mt8186-dsp > > + > > + reg: > > + items: > > + - description: Address and size of the DSP config registers > > + - description: Address and size of the DSP SRAM > > + - description: Address and size of the DSP secure registers > > + - description: Address and size of the DSP bus registers > > + > > + reg-names: > > + items: > > + - const: cfg > > + - const: sram > > + - const: sec > > + - const: bus > > + > > + clocks: > > + items: > > + - description: mux for audio dsp clock > > + - description: mux for audio dsp local bus > > + > > + clock-names: > > + items: > > + - const: audiodsp_sel > > + - const: adsp_bus_sel > > + > > + power-domains: > > + maxItems: 1 > > + > > + mboxes: > > + items: > > + - description: ipc reply between host and audio DSP. > > + - description: ipc request between host and audio DSP. > > + > > + mbox-names: > > + items: > > + - const: mbox0 > > + - const: mbox1 > > + > > + memory-region: > > + items: > > + - description: dma buffer between host and DSP. > > + - description: DSP system memory. > > + > > +required: > > + - compatible > > + - reg > > + - reg-names > > + - clocks > > + - clock-names > > + - power-domains > > + - mbox-names > > + - mboxes > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/clock/mt8186-clk.h> > > + dsp@10680000 { > > + compatible = "mediatek,mt8186-dsp"; > > + reg = <0x10680000 0x2000>, > > + <0x10800000 0x100000>, > > + <0x1068b000 0x100>, > > + <0x1068f000 0x1000>; > > + reg-names = "cfg", "sram", "sec", "bus"; > > + clocks = <&topckgen CLK_TOP_AUDIODSP>, > > + <&topckgen CLK_TOP_ADSP_BUS>; > > + clock-names = "audiodsp_sel", > > + "adsp_bus_sel"; > > + power-domains = <&spm 6>; > > + mbox-names = "mbox0", "mbox1"; > > + mboxes = <&adsp_mailbox0>, <&adsp_mailbox1>; > > + }; > >
Hi Krzysztof, Thanks for your review and sorry for late response. On Fri, 2022-04-22 at 17:49 +0200, Krzysztof Kozlowski wrote: > On 22/04/2022 09:15, Tinghan Shen wrote: > > This patch adds mt8186 dsp document. The dsp is used for Sound Open > > Firmware driver node. It includes registers, clocks, memory regions, > > and mailbox for dsp. > > > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com> > > --- > > > > This patch depends on MT8186 clock bindings. > > https://urldefense.com/v3/__https://lore.kernel.org/all/20220409132251.31725-2-chun-jie.chen@mediatek.com/__;!!CTRNKA9wMg0ARbw!xNJkwM9Z-raJLjgIoriOnjolphzSHqpnATbd3XC7wU7iaUns8gqGB2omSOkliuoDf3g$ > > > > > > --- > > .../bindings/dsp/mediatek,mt8186-dsp.yaml | 93 +++++++++++++++++++ > > 1 file changed, 93 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > > > > diff --git a/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > > b/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > > new file mode 100644 > > index 000000000000..00a79e880895 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml > > @@ -0,0 +1,93 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: > > https://urldefense.com/v3/__http://devicetree.org/schemas/dsp/mediatek,mt8186-dsp.yaml*__;Iw!!CTRNKA9wMg0ARbw!xNJkwM9Z-raJLjgIoriOnjolphzSHqpnATbd3XC7wU7iaUns8gqGB2omSOklA_t3nMM$ > > > > +$schema: > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!xNJkwM9Z-raJLjgIoriOnjolphzSHqpnATbd3XC7wU7iaUns8gqGB2omSOkl3OXmT2U$ > > > > + > > +title: Mediatek mt8186 DSP core > > + (...snip...) > > + > > + clock-names: > > + items: > > + - const: audiodsp_sel > > + - const: adsp_bus_sel > > What does the "sel" stands for? Maybe just skip the "_sel" suffixes? No problem. I'll remove '_sel' in next version. > > > + > > + power-domains: > > + maxItems: 1 > > + > > + mboxes: > > + items: > > + - description: ipc reply between host and audio DSP. > > + - description: ipc request between host and audio DSP. > > + > > + mbox-names: > > + items: > > + - const: mbox0 > > + - const: mbox1 > > These should be rather some meaningful names, e.g. "rx" and "tx". The mbox name has to align with the adsp ipc driver. The adsp ipc driver is using 'mbox%d' for mailbox channels. https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=9db69df4bdd37eb1f65b6931ee067fb15b9a4d5c chan_name = kasprintf(GFP_KERNEL, "mbox%d", i); /* ...snip... */ adsp_chan->ch = mbox_request_channel_byname(cl, chan_name); Is it ok to continue using these names? > > > + > > + memory-region: > > + items: > > + - description: dma buffer between host and DSP. > > + - description: DSP system memory. > > + > > +required: > > + - compatible > > + - reg > > + - reg-names > > + - clocks > > + - clock-names > > + - power-domains > > + - mbox-names > > + - mboxes > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > You do not use these headers. Ok. I'll remove them at next version. Thanks, TingHan > > > + #include <dt-bindings/clock/mt8186-clk.h> > > + dsp@10680000 { > > > > Best regards, > Krzysztof
On 02/06/2022 08:44, Tinghan Shen wrote: >>> + mbox-names: >>> + items: >>> + - const: mbox0 >>> + - const: mbox1 >> >> These should be rather some meaningful names, e.g. "rx" and "tx". > > The mbox name has to align with the adsp ipc driver. > The adsp ipc driver is using 'mbox%d' for mailbox channels. > > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=9db69df4bdd37eb1f65b6931ee067fb15b9a4d5c > > chan_name = kasprintf(GFP_KERNEL, "mbox%d", i); > > /* ...snip... */ > > adsp_chan->ch = mbox_request_channel_byname(cl, chan_name); > > Is it ok to continue using these names? It is a bit confusing... how did that driver got merged recently without bindings? Why bindings are separate? The bindings always come together in one patchset with the driver implementing them. Bindings are though a separate patch, yet still followed by the driver which uses them. I do not see any compatibles in that driver, which suggests there is no other binding using it. If that's correct, then you need to change the driver. Best regards, Krzysztof
Hi Krzysztof, On Thu, 2022-06-02 at 09:40 +0200, Krzysztof Kozlowski wrote: > On 02/06/2022 08:44, Tinghan Shen wrote: > > > > + mbox-names: > > > > + items: > > > > + - const: mbox0 > > > > + - const: mbox1 > > > > > > These should be rather some meaningful names, e.g. "rx" and "tx". > > > > The mbox name has to align with the adsp ipc driver. > > The adsp ipc driver is using 'mbox%d' for mailbox channels. > > > > > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=9db69df4bdd37eb1f65b6931ee067fb15b9a4d5c__;!!CTRNKA9wMg0ARbw!1TmempNkQhC5QuLBhyfWo_AC97MoLuWipsGV-LPaW9RKNPheU7Bgc-eboNi1JA1nC5I$ > > > > > > chan_name = kasprintf(GFP_KERNEL, "mbox%d", i); > > > > /* ...snip... */ > > > > adsp_chan->ch = mbox_request_channel_byname(cl, chan_name); > > > > Is it ok to continue using these names? > > It is a bit confusing... how did that driver got merged recently without > bindings? Why bindings are separate? > > The bindings always come together in one patchset with the driver > implementing them. Bindings are though a separate patch, yet still > followed by the driver which uses them. > > I do not see any compatibles in that driver, which suggests there is no > other binding using it. If that's correct, then you need to change the > driver. > The mtk-adsp-ipc driver's sole function is to encapsulate the operations of mailbox framework from adsp ipc users. The mtk-adsp-ipc is not defined in the dts file and we don't need it to be defined. The creation of mtk-adsp-ipc device is requested by adsp ipc users via the use of 'platform_device_register_data'[1]. the driver implemented the mailbox framework is 'mtk-adsp-mailbox'[2]. it has corresponding hardwares and a yaml file[3] to describe it. I'll send a patch to change the mbox name at next version. It's better to have some meaningful names as you said. [1] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=e0100bfd383c7d994d2e957e85ca56a5fe5a3f43 [2] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=af2dfa96c52d042df5deb29fb6e32d3ff4d76a61 [3] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=afa092e1e8824363d3174fef329c034445c111d5 Thanks, TingHan
On 02/06/2022 12:19, Tinghan Shen wrote: > Hi Krzysztof, > > On Thu, 2022-06-02 at 09:40 +0200, Krzysztof Kozlowski wrote: >> On 02/06/2022 08:44, Tinghan Shen wrote: >>>>> + mbox-names: >>>>> + items: >>>>> + - const: mbox0 >>>>> + - const: mbox1 >>>> >>>> These should be rather some meaningful names, e.g. "rx" and "tx". >>> >>> The mbox name has to align with the adsp ipc driver. >>> The adsp ipc driver is using 'mbox%d' for mailbox channels. >>> >>> >>> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=9db69df4bdd37eb1f65b6931ee067fb15b9a4d5c__;!!CTRNKA9wMg0ARbw!1TmempNkQhC5QuLBhyfWo_AC97MoLuWipsGV-LPaW9RKNPheU7Bgc-eboNi1JA1nC5I$ >>> >>> >>> chan_name = kasprintf(GFP_KERNEL, "mbox%d", i); >>> >>> /* ...snip... */ >>> >>> adsp_chan->ch = mbox_request_channel_byname(cl, chan_name); >>> >>> Is it ok to continue using these names? >> >> It is a bit confusing... how did that driver got merged recently without >> bindings? Why bindings are separate? >> >> The bindings always come together in one patchset with the driver >> implementing them. Bindings are though a separate patch, yet still >> followed by the driver which uses them. >> >> I do not see any compatibles in that driver, which suggests there is no >> other binding using it. If that's correct, then you need to change the >> driver. >> > > The mtk-adsp-ipc driver's sole function is to encapsulate the operations > of mailbox framework from adsp ipc users. The mtk-adsp-ipc is not defined > in the dts file and we don't need it to be defined. The creation of mtk-adsp-ipc > device is requested by adsp ipc users via the use of 'platform_device_register_data'[1]. > > the driver implemented the mailbox framework is 'mtk-adsp-mailbox'[2]. it has > corresponding hardwares and a yaml file[3] to describe it. I don't understand how is this related. We talk here about the mbox-names for this bindings file. You replied, that these bindings are already used by something, but now you say that they are not? So why do you need to change anything in any driver? Simple question - do the bindings here "add mt8186 dsp document" are used by any specific Linux driver already? Best regards, Krzysztof
Hi Krzysztof, On Thu, 2022-06-02 at 12:45 +0200, Krzysztof Kozlowski wrote: > On 02/06/2022 12:19, Tinghan Shen wrote: > > Hi Krzysztof, > > > > On Thu, 2022-06-02 at 09:40 +0200, Krzysztof Kozlowski wrote: > > > On 02/06/2022 08:44, Tinghan Shen wrote: > > > > > > + mbox-names: > > > > > > + items: > > > > > > + - const: mbox0 > > > > > > + - const: mbox1 > > > > > > > > > > These should be rather some meaningful names, e.g. "rx" and "tx". > > > > > > > > The mbox name has to align with the adsp ipc driver. > > > > The adsp ipc driver is using 'mbox%d' for mailbox channels. > > > > > > > > > > > > > > > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=9db69df4bdd37eb1f65b6931ee067fb15b9a4d5c__;!!CTRNKA9wMg0ARbw!1TmempNkQhC5QuLBhyfWo_AC97MoLuWipsGV-LPaW9RKNPheU7Bgc-eboNi1JA1nC5I$ > > > > > > > > > > > > chan_name = kasprintf(GFP_KERNEL, "mbox%d", i); > > > > > > > > /* ...snip... */ > > > > > > > > adsp_chan->ch = mbox_request_channel_byname(cl, chan_name); > > > > > > > > Is it ok to continue using these names? > > > > > > It is a bit confusing... how did that driver got merged recently without > > > bindings? Why bindings are separate? > > > > > > The bindings always come together in one patchset with the driver > > > implementing them. Bindings are though a separate patch, yet still > > > followed by the driver which uses them. > > > > > > I do not see any compatibles in that driver, which suggests there is no > > > other binding using it. If that's correct, then you need to change the > > > driver. > > > > > > > The mtk-adsp-ipc driver's sole function is to encapsulate the operations > > of mailbox framework from adsp ipc users. The mtk-adsp-ipc is not defined > > in the dts file and we don't need it to be defined. The creation of mtk-adsp-ipc > > device is requested by adsp ipc users via the use of 'platform_device_register_data'[1]. > > > > the driver implemented the mailbox framework is 'mtk-adsp-mailbox'[2]. it has > > corresponding hardwares and a yaml file[3] to describe it. > > I don't understand how is this related. We talk here about the > mbox-names for this bindings file. You replied, that these bindings are > already used by something, but now you say that they are not? So why do > you need to change anything in any driver? > > Simple question - do the bindings here "add mt8186 dsp document" are > used by any specific Linux driver already? This bindings, 'add mt8186 dsp document', are used by the SOF sound driver of MT8186[1]. I'm sorry for miss leading you in previous reply. I was thought that you're asking why the mtk-adsp-ipc driver got merged without bindings. So, I tried to explain why mtk-adsp-ipc doesn't have bindings. [1] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=1f0214a86de87011ecb96f22545dd6e5c7324cd7 Thanks, TingHan
On 02/06/2022 13:53, Tinghan Shen wrote: > Hi Krzysztof, > > On Thu, 2022-06-02 at 12:45 +0200, Krzysztof Kozlowski wrote: >> On 02/06/2022 12:19, Tinghan Shen wrote: >>> Hi Krzysztof, >>> >>> On Thu, 2022-06-02 at 09:40 +0200, Krzysztof Kozlowski wrote: >>>> On 02/06/2022 08:44, Tinghan Shen wrote: >>>>>>> + mbox-names: >>>>>>> + items: >>>>>>> + - const: mbox0 >>>>>>> + - const: mbox1 >>>>>> >>>>>> These should be rather some meaningful names, e.g. "rx" and "tx". >>>>> >>>>> The mbox name has to align with the adsp ipc driver. >>>>> The adsp ipc driver is using 'mbox%d' for mailbox channels. >>>>> >>>>> >>>>> >>> >>> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=9db69df4bdd37eb1f65b6931ee067fb15b9a4d5c__;!!CTRNKA9wMg0ARbw!1TmempNkQhC5QuLBhyfWo_AC97MoLuWipsGV-LPaW9RKNPheU7Bgc-eboNi1JA1nC5I$ >>>>> >>>>> >>>>> chan_name = kasprintf(GFP_KERNEL, "mbox%d", i); >>>>> >>>>> /* ...snip... */ >>>>> >>>>> adsp_chan->ch = mbox_request_channel_byname(cl, chan_name); >>>>> >>>>> Is it ok to continue using these names? >>>> >>>> It is a bit confusing... how did that driver got merged recently without >>>> bindings? Why bindings are separate? >>>> >>>> The bindings always come together in one patchset with the driver >>>> implementing them. Bindings are though a separate patch, yet still >>>> followed by the driver which uses them. >>>> >>>> I do not see any compatibles in that driver, which suggests there is no >>>> other binding using it. If that's correct, then you need to change the >>>> driver. >>>> >>> >>> The mtk-adsp-ipc driver's sole function is to encapsulate the operations >>> of mailbox framework from adsp ipc users. The mtk-adsp-ipc is not defined >>> in the dts file and we don't need it to be defined. The creation of mtk-adsp-ipc >>> device is requested by adsp ipc users via the use of 'platform_device_register_data'[1]. >>> >>> the driver implemented the mailbox framework is 'mtk-adsp-mailbox'[2]. it has >>> corresponding hardwares and a yaml file[3] to describe it. >> >> I don't understand how is this related. We talk here about the >> mbox-names for this bindings file. You replied, that these bindings are >> already used by something, but now you say that they are not? So why do >> you need to change anything in any driver? >> >> Simple question - do the bindings here "add mt8186 dsp document" are >> used by any specific Linux driver already? > > This bindings, 'add mt8186 dsp document', are used by the SOF sound driver of MT8186[1]. > > I'm sorry for miss leading you in previous reply. I was thought that you're > asking why the mtk-adsp-ipc driver got merged without bindings. So, I tried > to explain why mtk-adsp-ipc doesn't have bindings. Then my question is kind of still valid: How did "mt8186 SOF" driver got merged recently without bindings? Why bindings are separate? You cannot just sneak in usage of bindings in a driver, then submit bindings and say "we already have an user!". No, the bindings come with the driver. Always. Linked patch [1] brings undocumented compatible mediatek,mt8186-dsp, so you should see big fat warning when running checkpatch. So this points that you did not run checkpatch which is another not acceptable submission. :( [1] https://lore.kernel.org/all/20220422055659.8738-2-tinghan.shen@mediatek.com/ Best regards, Krzysztof
Hi Krzysztof, On Thu, 2022-06-02 at 14:26 +0200, Krzysztof Kozlowski wrote: > On 02/06/2022 13:53, Tinghan Shen wrote: > > Hi Krzysztof, > > > > On Thu, 2022-06-02 at 12:45 +0200, Krzysztof Kozlowski wrote: > > > On 02/06/2022 12:19, Tinghan Shen wrote: > > > > Hi Krzysztof, > > > > > > > > On Thu, 2022-06-02 at 09:40 +0200, Krzysztof Kozlowski wrote: > > > > > On 02/06/2022 08:44, Tinghan Shen wrote: > > > > > > > > + mbox-names: > > > > > > > > + items: > > > > > > > > + - const: mbox0 > > > > > > > > + - const: mbox1 > > > > > > > > > > > > > > These should be rather some meaningful names, e.g. "rx" and "tx". > > > > > > > > > > > > The mbox name has to align with the adsp ipc driver. > > > > > > The adsp ipc driver is using 'mbox%d' for mailbox channels. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=9db69df4bdd37eb1f65b6931ee067fb15b9a4d5c__;!!CTRNKA9wMg0ARbw!1TmempNkQhC5QuLBhyfWo_AC97MoLuWipsGV-LPaW9RKNPheU7Bgc-eboNi1JA1nC5I$ > > > > > > > > > > > > > > > > > > chan_name = kasprintf(GFP_KERNEL, "mbox%d", i); > > > > > > > > > > > > /* ...snip... */ > > > > > > > > > > > > adsp_chan->ch = mbox_request_channel_byname(cl, chan_name); > > > > > > > > > > > > Is it ok to continue using these names? > > > > > > > > > > It is a bit confusing... how did that driver got merged recently without > > > > > bindings? Why bindings are separate? > > > > > > > > > > The bindings always come together in one patchset with the driver > > > > > implementing them. Bindings are though a separate patch, yet still > > > > > followed by the driver which uses them. > > > > > > > > > > I do not see any compatibles in that driver, which suggests there is no > > > > > other binding using it. If that's correct, then you need to change the > > > > > driver. > > > > > > > > > > > > > The mtk-adsp-ipc driver's sole function is to encapsulate the operations > > > > of mailbox framework from adsp ipc users. The mtk-adsp-ipc is not defined > > > > in the dts file and we don't need it to be defined. The creation of mtk-adsp-ipc > > > > device is requested by adsp ipc users via the use of 'platform_device_register_data'[1]. > > > > > > > > the driver implemented the mailbox framework is 'mtk-adsp-mailbox'[2]. it has > > > > corresponding hardwares and a yaml file[3] to describe it. > > > > > > I don't understand how is this related. We talk here about the > > > mbox-names for this bindings file. You replied, that these bindings are > > > already used by something, but now you say that they are not? So why do > > > you need to change anything in any driver? > > > > > > Simple question - do the bindings here "add mt8186 dsp document" are > > > used by any specific Linux driver already? > > > > This bindings, 'add mt8186 dsp document', are used by the SOF sound driver of MT8186[1]. > > > > I'm sorry for miss leading you in previous reply. I was thought that you're > > asking why the mtk-adsp-ipc driver got merged without bindings. So, I tried > > to explain why mtk-adsp-ipc doesn't have bindings. > > Then my question is kind of still valid: > How did "mt8186 SOF" driver got merged recently without bindings? Why > bindings are separate? > > You cannot just sneak in usage of bindings in a driver, then submit > bindings and say "we already have an user!". No, the bindings come with > the driver. Always. > > Linked patch [1] brings undocumented compatible mediatek,mt8186-dsp, so > you should see big fat warning when running checkpatch. So this points > that you did not run checkpatch which is another not acceptable > submission. :( > > [1] > https://lore.kernel.org/all/20220422055659.8738-2-tinghan.shen@mediatek.com/ > I apologize for breaking the rules and sending inappropriate patches. I was thought that it was acceptable to send community reviewed patches in a series, then followed the bindings at another patch. I was believed that separating un-reviewed binding patch from reviewed driver patches would aid in patch acceptance. Now, I see I make a big mistake. I'm sorry. Best regards, TingHan
diff --git a/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml b/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml new file mode 100644 index 000000000000..00a79e880895 --- /dev/null +++ b/Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml @@ -0,0 +1,93 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/dsp/mediatek,mt8186-dsp.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Mediatek mt8186 DSP core + +maintainers: + - Tinghan Shen <tinghan.shen@mediatek.com> + +description: | + MediaTek mt8186 SoC contains a DSP core used for + advanced pre- and post- audio processing. + +properties: + compatible: + const: mediatek,mt8186-dsp + + reg: + items: + - description: Address and size of the DSP config registers + - description: Address and size of the DSP SRAM + - description: Address and size of the DSP secure registers + - description: Address and size of the DSP bus registers + + reg-names: + items: + - const: cfg + - const: sram + - const: sec + - const: bus + + clocks: + items: + - description: mux for audio dsp clock + - description: mux for audio dsp local bus + + clock-names: + items: + - const: audiodsp_sel + - const: adsp_bus_sel + + power-domains: + maxItems: 1 + + mboxes: + items: + - description: ipc reply between host and audio DSP. + - description: ipc request between host and audio DSP. + + mbox-names: + items: + - const: mbox0 + - const: mbox1 + + memory-region: + items: + - description: dma buffer between host and DSP. + - description: DSP system memory. + +required: + - compatible + - reg + - reg-names + - clocks + - clock-names + - power-domains + - mbox-names + - mboxes + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/clock/mt8186-clk.h> + dsp@10680000 { + compatible = "mediatek,mt8186-dsp"; + reg = <0x10680000 0x2000>, + <0x10800000 0x100000>, + <0x1068b000 0x100>, + <0x1068f000 0x1000>; + reg-names = "cfg", "sram", "sec", "bus"; + clocks = <&topckgen CLK_TOP_AUDIODSP>, + <&topckgen CLK_TOP_ADSP_BUS>; + clock-names = "audiodsp_sel", + "adsp_bus_sel"; + power-domains = <&spm 6>; + mbox-names = "mbox0", "mbox1"; + mboxes = <&adsp_mailbox0>, <&adsp_mailbox1>; + };
This patch adds mt8186 dsp document. The dsp is used for Sound Open Firmware driver node. It includes registers, clocks, memory regions, and mailbox for dsp. Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com> --- This patch depends on MT8186 clock bindings. https://lore.kernel.org/all/20220409132251.31725-2-chun-jie.chen@mediatek.com/ --- .../bindings/dsp/mediatek,mt8186-dsp.yaml | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 Documentation/devicetree/bindings/dsp/mediatek,mt8186-dsp.yaml