Patchwork [v3,25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

login
register
mail settings
Submitter Ezequiel Garcia
Date Nov. 5, 2013, 12:55 p.m.
Message ID <1383656135-8627-26-git-send-email-ezequiel.garcia@free-electrons.com>
Download mbox | patch
Permalink /patch/288503/
State New
Headers show

Comments

Ezequiel Garcia - Nov. 5, 2013, 12:55 p.m.
The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
This commit adds support for it in Armada 370 and Armada XP SoC
common devicetree.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)
Jason - Nov. 5, 2013, 1:29 p.m.
On Tue, Nov 05, 2013 at 09:55:32AM -0300, Ezequiel Garcia wrote:
> The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
> This commit adds support for it in Armada 370 and Armada XP SoC
> common devicetree.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 01e69fc..b4e6898 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -258,6 +258,15 @@
>  				status = "disabled";
>  			};
>  
> +			nand@d0000 {
> +				compatible = "marvell,armada370-nand";

Could you please provide a separate patch updating the devicetree
binding documentation?  You can also Cc the entire series to the
devicetree ml as long as the documentation patch is easy to find in the
series.  eg 'dt: binding: ...'

thx,

Jason.
Ezequiel Garcia - Nov. 5, 2013, 1:51 p.m.
On Tue, Nov 05, 2013 at 08:29:05AM -0500, Jason Cooper wrote:
> On Tue, Nov 05, 2013 at 09:55:32AM -0300, Ezequiel Garcia wrote:
> > The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
> > This commit adds support for it in Armada 370 and Armada XP SoC
> > common devicetree.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> > index 01e69fc..b4e6898 100644
> > --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> > +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> > @@ -258,6 +258,15 @@
> >  				status = "disabled";
> >  			};
> >  
> > +			nand@d0000 {
> > +				compatible = "marvell,armada370-nand";
> 
> Could you please provide a separate patch updating the devicetree
> binding documentation?  You can also Cc the entire series to the
> devicetree ml as long as the documentation patch is easy to find in the
> series.  eg 'dt: binding: ...'
> 

Hm.. actually the controller already supports the new compatible string
so the binding documentation should be added now.

And I'd rather do that in a separate patch, to avoid cluttering the poor
devicetree people with an unrelated 28-piece patch :-)

BTW: who should take such a patch? I'm still a little lost regarding
who takes the binding or dts patches for a given subsystem.
Jason - Nov. 5, 2013, 3:15 p.m.
On Tue, Nov 05, 2013 at 10:51:46AM -0300, Ezequiel Garcia wrote:
> On Tue, Nov 05, 2013 at 08:29:05AM -0500, Jason Cooper wrote:
> > On Tue, Nov 05, 2013 at 09:55:32AM -0300, Ezequiel Garcia wrote:
> > > The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
> > > This commit adds support for it in Armada 370 and Armada XP SoC
> > > common devicetree.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> > > index 01e69fc..b4e6898 100644
> > > --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> > > +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> > > @@ -258,6 +258,15 @@
> > >  				status = "disabled";
> > >  			};
> > >  
> > > +			nand@d0000 {
> > > +				compatible = "marvell,armada370-nand";
> > 
> > Could you please provide a separate patch updating the devicetree
> > binding documentation?  You can also Cc the entire series to the
> > devicetree ml as long as the documentation patch is easy to find in the
> > series.  eg 'dt: binding: ...'
> > 
> 
> Hm.. actually the controller already supports the new compatible string
> so the binding documentation should be added now.

$ git grep -n 'marvell,armada370-nand' -- Documentation/devicetree/bindings/
$

Perhaps in v3.13-rc1?

> And I'd rather do that in a separate patch, to avoid cluttering the poor
> devicetree people with an unrelated 28-piece patch :-)

No (really), according to Grant and Mark during the closing session, I
asked this specific question.  They _do_ want the entire series so they
can refer to the corresponding code changes if necessary.  As I stated
above, we can make their job easier by making the binding a separate
patch that is clearly marked as such.

> BTW: who should take such a patch? I'm still a little lost regarding
> who takes the binding or dts patches for a given subsystem.

The appropriate sub-system maintainer still takes the patches, we simply
wait a bit for the DT binding maintainers to chime in.  If they don't
after a few weeks, we can take it without their Ack.

If the maintainer is unsure, or needs help reviewing the binding, they
can always ping the DT folks for assistance.

thx,

Jason.
Ezequiel Garcia - Nov. 5, 2013, 3:37 p.m.
On Tue, Nov 05, 2013 at 10:15:31AM -0500, Jason Cooper wrote:
> On Tue, Nov 05, 2013 at 10:51:46AM -0300, Ezequiel Garcia wrote:
> > On Tue, Nov 05, 2013 at 08:29:05AM -0500, Jason Cooper wrote:
> > > On Tue, Nov 05, 2013 at 09:55:32AM -0300, Ezequiel Garcia wrote:
> > > > The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
> > > > This commit adds support for it in Armada 370 and Armada XP SoC
> > > > common devicetree.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > > ---
> > > >  arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> > > > index 01e69fc..b4e6898 100644
> > > > --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> > > > +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> > > > @@ -258,6 +258,15 @@
> > > >  				status = "disabled";
> > > >  			};
> > > >  
> > > > +			nand@d0000 {
> > > > +				compatible = "marvell,armada370-nand";
> > > 
> > > Could you please provide a separate patch updating the devicetree
> > > binding documentation?  You can also Cc the entire series to the
> > > devicetree ml as long as the documentation patch is easy to find in the
> > > series.  eg 'dt: binding: ...'
> > > 
> > 
> > Hm.. actually the controller already supports the new compatible string
> > so the binding documentation should be added now.
> 
> $ git grep -n 'marvell,armada370-nand' -- Documentation/devicetree/bindings/
> $

Well the controller supports it, but I never updated the binding:

$ git grep -n 'marvell,armada370-nand' -- drivers/mtd/nand/pxa3xx-nand.c

So that's why I think a separate patch to be taken by Brian now is more
appropriate.

> > And I'd rather do that in a separate patch, to avoid cluttering the poor
> > devicetree people with an unrelated 28-piece patch :-)
> 
> No (really), according to Grant and Mark during the closing session, I
> asked this specific question.  They _do_ want the entire series so they
> can refer to the corresponding code changes if necessary.  As I stated
> above, we can make their job easier by making the binding a separate
> patch that is clearly marked as such.
> 

Ah, good to know.

> > BTW: who should take such a patch? I'm still a little lost regarding
> > who takes the binding or dts patches for a given subsystem.
> 
> The appropriate sub-system maintainer still takes the patches, we simply
> wait a bit for the DT binding maintainers to chime in.  If they don't
> after a few weeks, we can take it without their Ack.
> 
> If the maintainer is unsure, or needs help reviewing the binding, they
> can always ping the DT folks for assistance.
> 

Ok, great.
Thomas Petazzoni - Nov. 6, 2013, 8:24 a.m.
Dear Jason Cooper,

On Tue, 5 Nov 2013 10:15:31 -0500, Jason Cooper wrote:

> > And I'd rather do that in a separate patch, to avoid cluttering the poor
> > devicetree people with an unrelated 28-piece patch :-)
> 
> No (really), according to Grant and Mark during the closing session, I
> asked this specific question.  They _do_ want the entire series so they
> can refer to the corresponding code changes if necessary.  As I stated
> above, we can make their job easier by making the binding a separate
> patch that is clearly marked as such.

Interesting, because I had understood exactly the opposite from the
discussions at the ARM kernel summit. But maybe the DT folks changed
their mind between the ARM kernel summit and the closing session of the
kernel summit :-)

Thomas
Jason - Nov. 6, 2013, 11:42 a.m.
Thomas,

Adding Grant and Mark to the Cc to make sure they don't have buyer's
remorse ;-)

On Wed, Nov 06, 2013 at 09:24:35AM +0100, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 5 Nov 2013 10:15:31 -0500, Jason Cooper wrote:
> 
> > > And I'd rather do that in a separate patch, to avoid cluttering the poor
> > > devicetree people with an unrelated 28-piece patch :-)
> > 
> > No (really), according to Grant and Mark during the closing session, I
> > asked this specific question.  They _do_ want the entire series so they
> > can refer to the corresponding code changes if necessary.  As I stated
> > above, we can make their job easier by making the binding a separate
> > patch that is clearly marked as such.
> 
> Interesting, because I had understood exactly the opposite from the
> discussions at the ARM kernel summit. But maybe the DT folks changed
> their mind between the ARM kernel summit and the closing session of the
> kernel summit :-)

Yes, that's why I asked the question.  Mark Rutland said he wanted to
see the code changes when he was reviewing a binding.  Which is indeed
the opposite of what we had arrived at during the ARM mini-summit.

So I asked Grant (last question of the Q&A) what he wanted us to tell
the contributors.  He said (bad paraphrasing here) "I have better
filters now for finding the binding changes, so send the whole series"

As him and Mark aren't the only reviewers, I took the extra step of
saying send the DT bindings as a separate patch with it clearly
identified in the subject line...

thx,

Jason.
Thomas Petazzoni - Nov. 6, 2013, 12:56 p.m.
Dear Jason Cooper,

On Wed, 6 Nov 2013 06:42:44 -0500, Jason Cooper wrote:

> > Interesting, because I had understood exactly the opposite from the
> > discussions at the ARM kernel summit. But maybe the DT folks changed
> > their mind between the ARM kernel summit and the closing session of
> > the kernel summit :-)
> 
> Yes, that's why I asked the question.  Mark Rutland said he wanted to
> see the code changes when he was reviewing a binding.  Which is indeed
> the opposite of what we had arrived at during the ARM mini-summit.
> 
> So I asked Grant (last question of the Q&A) what he wanted us to tell
> the contributors.  He said (bad paraphrasing here) "I have better
> filters now for finding the binding changes, so send the whole series"
> 
> As him and Mark aren't the only reviewers, I took the extra step of
> saying send the DT bindings as a separate patch with it clearly
> identified in the subject line...

Ok, thanks for the clarification. Those rules seem to be changing from
one day to the next, not really easy to follow :)

Is there some .txt file in Documentation/ that explains what the DT
maintainers expect?

Thanks,

Thomas
Jason - Nov. 6, 2013, 5:21 p.m.
On Wed, Nov 06, 2013 at 01:56:54PM +0100, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Wed, 6 Nov 2013 06:42:44 -0500, Jason Cooper wrote:
> 
> > > Interesting, because I had understood exactly the opposite from the
> > > discussions at the ARM kernel summit. But maybe the DT folks changed
> > > their mind between the ARM kernel summit and the closing session of
> > > the kernel summit :-)
> > 
> > Yes, that's why I asked the question.  Mark Rutland said he wanted to
> > see the code changes when he was reviewing a binding.  Which is indeed
> > the opposite of what we had arrived at during the ARM mini-summit.
> > 
> > So I asked Grant (last question of the Q&A) what he wanted us to tell
> > the contributors.  He said (bad paraphrasing here) "I have better
> > filters now for finding the binding changes, so send the whole series"
> > 
> > As him and Mark aren't the only reviewers, I took the extra step of
> > saying send the DT bindings as a separate patch with it clearly
> > identified in the subject line...
> 
> Ok, thanks for the clarification. Those rules seem to be changing from
> one day to the next, not really easy to follow :)
> 
> Is there some .txt file in Documentation/ that explains what the DT
> maintainers expect?

hmmm.  Give me a bit. :)

thx,

Jason.

Patch

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 01e69fc..b4e6898 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -258,6 +258,15 @@ 
 				status = "disabled";
 			};
 
+			nand@d0000 {
+				compatible = "marvell,armada370-nand";
+				reg = <0xd0000 0x54>;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				interrupts = <113>;
+				clocks = <&coredivclk 0>;
+				status = "disabled";
+			};
 		};
 	};