| Message ID | 1441980871-24475-8-git-send-email-peter.griffin@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 11 Sep 2015, Peter Griffin wrote: > These nodes are required to get the fdma driver working > on STiH407 based silicon. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > arch/arm/boot/dts/stih407-family.dtsi | 51 +++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi > index 838b812..da07474b 100644 > --- a/arch/arm/boot/dts/stih407-family.dtsi > +++ b/arch/arm/boot/dts/stih407-family.dtsi > @@ -565,5 +565,56 @@ > <&phy_port2 PHY_TYPE_USB3>; > }; > }; > + > + fdma0: fdma0-audio@8e20000 { I'm not familiar with the FDMA driver, so can't comment knowledgeably, but the <dev> part of <dev>@<base_address> should only describe the type of hardware. I believe in this case it should just be dma@08e20000. Also notice the leading zero in the address, which I believe mitigates possible confusion. Then you be more specific with the label, so something like 'fdma-audio' seems appropriate here. > + compatible = "st,stih407-fdma-mpe31"; > + reg = <0x8e20000 0x20000>; I personally find padding up to 32bits helpful in the addresses. > + interrupts = <GIC_SPI 5 IRQ_TYPE_NONE>; > + dma-channels = <16>; > + #dma-cells = <3>; > + st,fdma-id = <0>; We usually shy away from ID properties. What is it required for in this case? > + clocks = <&clk_s_c0_flexgen CLK_FDMA>, > + <&clk_s_c0_flexgen CLK_EXT2F_A9>, > + <&clk_s_c0_flexgen CLK_EXT2F_A9>, > + <&clk_s_c0_flexgen CLK_EXT2F_A9>; > + clock-names = "fdma_slim", > + "fdma_hi", > + "fdma_low", > + "fdma_ic"; > + }; > + > + fdma1: fdma1-app@8e40000 { > + compatible = "st,stih407-fdma-mpe31"; > + reg = <0x8e40000 0x20000>; > + interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; > + dma-channels = <16>; > + #dma-cells = <3>; > + st,fdma-id = <1>; > + clocks = <&clk_s_c0_flexgen CLK_FDMA>, > + <&clk_s_c0_flexgen CLK_TX_ICN_DMU>, > + <&clk_s_c0_flexgen CLK_TX_ICN_DMU>, > + <&clk_s_c0_flexgen CLK_EXT2F_A9>; > + clock-names = "fdma_slim", > + "fdma_hi", > + "fdma_low", > + "fdma_ic"; > + }; > + > + fdma2: fdma2-free_running@8e60000 { > + compatible = "st,stih407-fdma-mpe31"; > + reg = <0x8e60000 0x20000>; > + interrupts = <GIC_SPI 9 IRQ_TYPE_NONE>; > + dma-channels = <16>; > + #dma-cells = <3>; > + st,fdma-id = <2>; > + clocks = <&clk_s_c0_flexgen CLK_FDMA>, > + <&clk_s_c0_flexgen CLK_EXT2F_A9>, > + <&clk_s_c0_flexgen CLK_TX_ICN_DISP_0>, > + <&clk_s_c0_flexgen CLK_EXT2F_A9>; > + clock-names = "fdma_slim", > + "fdma_hi", > + "fdma_low", > + "fdma_ic"; > + }; > }; > };
Hi Lee, On Fri, 11 Sep 2015, Lee Jones wrote: > On Fri, 11 Sep 2015, Peter Griffin wrote: > > > These nodes are required to get the fdma driver working > > on STiH407 based silicon. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > arch/arm/boot/dts/stih407-family.dtsi | 51 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi > > index 838b812..da07474b 100644 > > --- a/arch/arm/boot/dts/stih407-family.dtsi > > +++ b/arch/arm/boot/dts/stih407-family.dtsi > > @@ -565,5 +565,56 @@ > > <&phy_port2 PHY_TYPE_USB3>; > > }; > > }; > > + > > + fdma0: fdma0-audio@8e20000 { > > I'm not familiar with the FDMA driver, so can't comment knowledgeably, > but the <dev> part of <dev>@<base_address> should only describe the > type of hardware. I believe in this case it should just be > dma@08e20000. Also notice the leading zero in the address, which I > believe mitigates possible confusion. Then you be more specific with > the label, so something like 'fdma-audio' seems appropriate here. Ok, can change to that format in v3. > > > + compatible = "st,stih407-fdma-mpe31"; > > + reg = <0x8e20000 0x20000>; > > I personally find padding up to 32bits helpful in the addresses. None of the stih407-family nodes I can see have this padding, including the ones merged by you. > > > + interrupts = <GIC_SPI 5 IRQ_TYPE_NONE>; > > + dma-channels = <16>; > > + #dma-cells = <3>; > > + st,fdma-id = <0>; > > We usually shy away from ID properties. What is it required for in > this case? Yes Rob did already mention that over here, see my reply at the bottom http://www.spinics.net/lists/devicetree/msg92529.html. However I can't think of any other useful properties we could add to derive this information. The fdma controller number is used by the driver to generate a unique firmware filename. regards, Peter. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 11 Sep 2015, Peter Griffin wrote: > On Fri, 11 Sep 2015, Lee Jones wrote: > > On Fri, 11 Sep 2015, Peter Griffin wrote: > > > > > These nodes are required to get the fdma driver working > > > on STiH407 based silicon. > > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > > --- > > > arch/arm/boot/dts/stih407-family.dtsi | 51 +++++++++++++++++++++++++++++++++++ > > > 1 file changed, 51 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi > > > index 838b812..da07474b 100644 > > > --- a/arch/arm/boot/dts/stih407-family.dtsi > > > +++ b/arch/arm/boot/dts/stih407-family.dtsi > > > @@ -565,5 +565,56 @@ > > > <&phy_port2 PHY_TYPE_USB3>; > > > }; > > > }; > > > + > > > + fdma0: fdma0-audio@8e20000 { > > > > I'm not familiar with the FDMA driver, so can't comment knowledgeably, > > but the <dev> part of <dev>@<base_address> should only describe the > > type of hardware. I believe in this case it should just be > > dma@08e20000. Also notice the leading zero in the address, which I > > believe mitigates possible confusion. Then you be more specific with > > the label, so something like 'fdma-audio' seems appropriate here. > > Ok, can change to that format in v3. > > > > > > + compatible = "st,stih407-fdma-mpe31"; > > > + reg = <0x8e20000 0x20000>; > > > > I personally find padding up to 32bits helpful in the addresses. > > None of the stih407-family nodes I can see have this padding, including > the ones merged by you. Nither of these two facts mean it's correct. I'm happy to write a patch to correct them all. Bear in mind that this isn't a hard and fast rule. Both work and are legal. I just think the padding is more consistent. > > > + interrupts = <GIC_SPI 5 IRQ_TYPE_NONE>; > > > + dma-channels = <16>; > > > + #dma-cells = <3>; > > > + st,fdma-id = <0>; > > > > We usually shy away from ID properties. What is it required for in > > this case? > > Yes Rob did already mention that over here, see my reply at the bottom > http://www.spinics.net/lists/devicetree/msg92529.html. > > However I can't think of any other useful properties we could add > to derive this information. The fdma controller number is used > by the driver to generate a unique firmware filename. Who chooses the naming scheme of the firmware binary? Is there any reason they can't be: fdma_STiH407_audio.elf fdma_STiH407_app.elf fdma_STiH407_free_running.elf Then you can have a different compatible for each.
Hi Lee, On Fri, 11 Sep 2015, Lee Jones wrote: > On Fri, 11 Sep 2015, Peter Griffin wrote: > > On Fri, 11 Sep 2015, Lee Jones wrote: > > > On Fri, 11 Sep 2015, Peter Griffin wrote: > > > > > > > These nodes are required to get the fdma driver working > > > > on STiH407 based silicon. > > > > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > > > --- > > > > arch/arm/boot/dts/stih407-family.dtsi | 51 +++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 51 insertions(+) > > > > > > > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi > > > > index 838b812..da07474b 100644 > > > > --- a/arch/arm/boot/dts/stih407-family.dtsi > > > > +++ b/arch/arm/boot/dts/stih407-family.dtsi > > > > @@ -565,5 +565,56 @@ > > > > <&phy_port2 PHY_TYPE_USB3>; > > > > }; > > > > }; > > > > + > > > > + fdma0: fdma0-audio@8e20000 { > > > > > > I'm not familiar with the FDMA driver, so can't comment knowledgeably, > > > but the <dev> part of <dev>@<base_address> should only describe the > > > type of hardware. I believe in this case it should just be > > > dma@08e20000. Also notice the leading zero in the address, which I > > > believe mitigates possible confusion. Then you be more specific with > > > the label, so something like 'fdma-audio' seems appropriate here. > > > > Ok, can change to that format in v3. > > > > > > > > > + compatible = "st,stih407-fdma-mpe31"; > > > > + reg = <0x8e20000 0x20000>; > > > > > > I personally find padding up to 32bits helpful in the addresses. > > > > None of the stih407-family nodes I can see have this padding, including > > the ones merged by you. > > Nither of these two facts mean it's correct. I thought it was a 'personal' thing. If it is mandated by the spec, then that is different. > > I'm happy to write a patch to correct them all. Are you sure your actually correcting anything? Where does it say you should have a leading zero? > > Bear in mind that this isn't a hard and fast rule. Both work and are > legal. I just think the padding is more consistent. Surely adding a patch with how the nodes are currently formatted, is more consistent than adding a patch with padding? regards, Peter. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 11 Sep 2015, Peter Griffin wrote: > On Fri, 11 Sep 2015, Lee Jones wrote: > > On Fri, 11 Sep 2015, Peter Griffin wrote: > > > On Fri, 11 Sep 2015, Lee Jones wrote: > > > > On Fri, 11 Sep 2015, Peter Griffin wrote: > > > > > > > > > These nodes are required to get the fdma driver working > > > > > on STiH407 based silicon. > > > > > > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > > > > --- > > > > > arch/arm/boot/dts/stih407-family.dtsi | 51 +++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 51 insertions(+) > > > > > > > > > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi > > > > > index 838b812..da07474b 100644 > > > > > --- a/arch/arm/boot/dts/stih407-family.dtsi > > > > > +++ b/arch/arm/boot/dts/stih407-family.dtsi > > > > > @@ -565,5 +565,56 @@ > > > > > <&phy_port2 PHY_TYPE_USB3>; > > > > > }; > > > > > }; > > > > > + > > > > > + fdma0: fdma0-audio@8e20000 { > > > > > > > > I'm not familiar with the FDMA driver, so can't comment knowledgeably, > > > > but the <dev> part of <dev>@<base_address> should only describe the > > > > type of hardware. I believe in this case it should just be > > > > dma@08e20000. Also notice the leading zero in the address, which I > > > > believe mitigates possible confusion. Then you be more specific with > > > > the label, so something like 'fdma-audio' seems appropriate here. > > > > > > Ok, can change to that format in v3. > > > > > > > > > > > > + compatible = "st,stih407-fdma-mpe31"; > > > > > + reg = <0x8e20000 0x20000>; > > > > > > > > I personally find padding up to 32bits helpful in the addresses. > > > > > > None of the stih407-family nodes I can see have this padding, including > > > the ones merged by you. > > > > Nither of these two facts mean it's correct. > > I thought it was a 'personal' thing. If it is mandated by the spec, then that > is different. > > > > > I'm happy to write a patch to correct them all. > > Are you sure your actually correcting anything? Where does it say you > should have a leading zero? > > > > > Bear in mind that this isn't a hard and fast rule. Both work and are > > legal. I just think the padding is more consistent. > > Surely adding a patch with how the nodes are currently formatted, is more consistent > than adding a patch with padding? I did mean consistent with the majority of DTS files, rather than just ours. There are examples of non-padded values and it's perfectly legal, but the vast majority of examples do in fact pad their addresses [see below]. The change is more of a nit than a blocker. git grep '@0[0-9|a-f]\{7\}' -- arch/arm/boot/dts/ 892 git grep '@[0-9|a-f]\{7\} ' -- arch/arm/boot/dts/ | grep -vi sti | \ grep -vi partition | grep -vi data 140 I'll probably still fix them up and resubmit some patches to bring us into line with the vast majority.
Hi Lee, On Fri, 11 Sep 2015, Lee Jones wrote: > On Fri, 11 Sep 2015, Peter Griffin wrote: > > On Fri, 11 Sep 2015, Lee Jones wrote: > > > On Fri, 11 Sep 2015, Peter Griffin wrote: > > > > On Fri, 11 Sep 2015, Lee Jones wrote: > > > > > On Fri, 11 Sep 2015, Peter Griffin wrote: > > > > > > > > > > > These nodes are required to get the fdma driver working > > > > > > on STiH407 based silicon. > > > > > > > > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > > > > > --- > > > > > > arch/arm/boot/dts/stih407-family.dtsi | 51 +++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 51 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi > > > > > > index 838b812..da07474b 100644 > > > > > > --- a/arch/arm/boot/dts/stih407-family.dtsi > > > > > > +++ b/arch/arm/boot/dts/stih407-family.dtsi > > > > > > @@ -565,5 +565,56 @@ > > > > > > <&phy_port2 PHY_TYPE_USB3>; > > > > > > }; > > > > > > }; > > > > > > + > > > > > > + fdma0: fdma0-audio@8e20000 { > > > > > > > > > > I'm not familiar with the FDMA driver, so can't comment knowledgeably, > > > > > but the <dev> part of <dev>@<base_address> should only describe the > > > > > type of hardware. I believe in this case it should just be > > > > > dma@08e20000. Also notice the leading zero in the address, which I > > > > > believe mitigates possible confusion. Then you be more specific with > > > > > the label, so something like 'fdma-audio' seems appropriate here. > > > > > > > > Ok, can change to that format in v3. > > > > > > > > > > > > > > > + compatible = "st,stih407-fdma-mpe31"; > > > > > > + reg = <0x8e20000 0x20000>; > > > > > > > > > > I personally find padding up to 32bits helpful in the addresses. > > > > > > > > None of the stih407-family nodes I can see have this padding, including > > > > the ones merged by you. > > > > > > Nither of these two facts mean it's correct. > > > > I thought it was a 'personal' thing. If it is mandated by the spec, then that > > is different. > > > > > > > > I'm happy to write a patch to correct them all. > > > > Are you sure your actually correcting anything? Where does it say you > > should have a leading zero? > > > > > > > > Bear in mind that this isn't a hard and fast rule. Both work and are > > > legal. I just think the padding is more consistent. > > > > Surely adding a patch with how the nodes are currently formatted, is more consistent > > than adding a patch with padding? > > I did mean consistent with the majority of DTS files, rather than just > ours. There are examples of non-padded values and it's perfectly > legal, but the vast majority of examples do in fact pad their > addresses [see below]. The change is more of a nit than a blocker. If its perdectly legal, than I'm personally against changing it. Mainly because it makes it difficult for git and patch to apply patches from the vendor kernel on top of the upstream kernel without manual intervention. I already ran into this with the recent stih407-pinctrl series where every patch had to be manually fixed up because we changed PIO to pio, on every pin definition. I'm all for changing things where they are actually wrong in the vendor kernel (such as not having the unit address on the reg property, or the node name being wrong e.g. fdma to dma-controller etc. But this doesn't appar to be one of those cases. regards, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi index 838b812..da07474b 100644 --- a/arch/arm/boot/dts/stih407-family.dtsi +++ b/arch/arm/boot/dts/stih407-family.dtsi @@ -565,5 +565,56 @@ <&phy_port2 PHY_TYPE_USB3>; }; }; + + fdma0: fdma0-audio@8e20000 { + compatible = "st,stih407-fdma-mpe31"; + reg = <0x8e20000 0x20000>; + interrupts = <GIC_SPI 5 IRQ_TYPE_NONE>; + dma-channels = <16>; + #dma-cells = <3>; + st,fdma-id = <0>; + clocks = <&clk_s_c0_flexgen CLK_FDMA>, + <&clk_s_c0_flexgen CLK_EXT2F_A9>, + <&clk_s_c0_flexgen CLK_EXT2F_A9>, + <&clk_s_c0_flexgen CLK_EXT2F_A9>; + clock-names = "fdma_slim", + "fdma_hi", + "fdma_low", + "fdma_ic"; + }; + + fdma1: fdma1-app@8e40000 { + compatible = "st,stih407-fdma-mpe31"; + reg = <0x8e40000 0x20000>; + interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; + dma-channels = <16>; + #dma-cells = <3>; + st,fdma-id = <1>; + clocks = <&clk_s_c0_flexgen CLK_FDMA>, + <&clk_s_c0_flexgen CLK_TX_ICN_DMU>, + <&clk_s_c0_flexgen CLK_TX_ICN_DMU>, + <&clk_s_c0_flexgen CLK_EXT2F_A9>; + clock-names = "fdma_slim", + "fdma_hi", + "fdma_low", + "fdma_ic"; + }; + + fdma2: fdma2-free_running@8e60000 { + compatible = "st,stih407-fdma-mpe31"; + reg = <0x8e60000 0x20000>; + interrupts = <GIC_SPI 9 IRQ_TYPE_NONE>; + dma-channels = <16>; + #dma-cells = <3>; + st,fdma-id = <2>; + clocks = <&clk_s_c0_flexgen CLK_FDMA>, + <&clk_s_c0_flexgen CLK_EXT2F_A9>, + <&clk_s_c0_flexgen CLK_TX_ICN_DISP_0>, + <&clk_s_c0_flexgen CLK_EXT2F_A9>; + clock-names = "fdma_slim", + "fdma_hi", + "fdma_low", + "fdma_ic"; + }; }; };
These nodes are required to get the fdma driver working on STiH407 based silicon. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- arch/arm/boot/dts/stih407-family.dtsi | 51 +++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)