[v2,7/9] ARM: STi: DT: STiH407: Add FDMA driver dt nodes.

Message ID 1441980871-24475-8-git-send-email-peter.griffin@linaro.org
State New
Headers show

Commit Message

Peter Griffin Sept. 11, 2015, 2:14 p.m.
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(+)

Comments

Lee Jones Sept. 11, 2015, 4:27 p.m. | #1
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";
> +		};
>  	};
>  };
Peter Griffin Sept. 11, 2015, 4:48 p.m. | #2
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
Lee Jones Sept. 11, 2015, 5:55 p.m. | #3
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.
Peter Griffin Sept. 11, 2015, 6:06 p.m. | #4
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
Lee Jones Sept. 11, 2015, 7:33 p.m. | #5
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.
Peter Griffin Sept. 12, 2015, 12:23 p.m. | #6
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/

Patch

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";
+		};
 	};
 };