Re: [GIT PULL v3] ARM: kirkwood: fdt: convert kirkwood init funcs to fdt

Re: [GIT PULL v3] ARM: kirkwood: fdt: convert kirkwood init funcs to fdt

[Andrew Lunn]

> On Sun, Mar 04, 2012 at 10:41:13PM -0500, Jason Cooper wrote:
> >       rtc-mv: more accurate compatible strings
> >       orion-wdt: more accurate compatible strings
> ...
> >  arch/arm/boot/dts/kirkwood-dreamplug.dts |   34 +++++++++++++++
> >  arch/arm/boot/dts/kirkwood.dtsi          |   12 +++++
> 
> This is still bugging me.  If, say, orion_wdt.c is common to all of
> plat-orion/ shouldn't it be in an 'orion.dtsi'?  With compatible =
> "marvell,orion-wdt"?  Then, if hypothetically, kirkwood used the same
> driver but had a small tweak, then kirkwood.dtsi would have
> "marvell,kirkwood-wdt"?  Which would let the driver know that it needed
> to apply the tweak because it was kirkwood.
> 
> In this scenario, since kirkwood and dreamplug currently need no
> modifications to the default wdt driver, they would just inherit
> orion.dtsi's "marvell,orion-wdt", right?
> 
> On the driver side, since it currently has no subarch or board specific
> tweaks, then it is *only* good for the generic case,
> "marvell,orion-wdt", and thus should only match it for now.
> 
> If the above is correct, then I should create orion.dtsi, which is
> included by kirkwood.dtsi.  orion.dtsi should hold "marvell,orion-wdt"
> and "marvell,orion-rtc".  The corresponding drivers should only match
> those generic strings, since there are no specific tweaks.

Hi Jason

I would agree with you in principal, that most of the drivers used by
kirkwood are generic orion drivers. There is one clear exception to
this, and that is the audio driver, which is specific to Kirkwood.

However, what differs between all the different plat-orion SoC is the
address map and interrupts. So the dove wdt is in a different place to
the orion5x wdt, and uses a different interrupt line, etc.

So i expect in the end we have a dove.dtsi, a orion5x.dtsi, a
mv78xx0.dtsi and a kirkwood.dtsi which all look very similar, but have
different values for addresses and interrupts.

Maybe DT has some why to handle this? 

As to naming, i think you are correct. It should be
"marvell,orion-wdt" in all the different .dtsi files, since its the
same SoC IP in all Orion devices. Same with "marvell,orion-rtc" since
all orion based SoC have the same built in RTC IP.

    Andrew

Re: [GIT PULL v3] ARM: kirkwood: fdt: convert kirkwood init funcs to fdt

[Jason]

On Mon, Mar 05, 2012 at 03:16:56PM +0100, Andrew Lunn wrote:
> > On Sun, Mar 04, 2012 at 10:41:13PM -0500, Jason Cooper wrote:
> > >       rtc-mv: more accurate compatible strings
> > >       orion-wdt: more accurate compatible strings
> > ...
> > >  arch/arm/boot/dts/kirkwood-dreamplug.dts |   34 +++++++++++++++
> > >  arch/arm/boot/dts/kirkwood.dtsi          |   12 +++++
> > 
> > This is still bugging me.  If, say, orion_wdt.c is common to all of
> > plat-orion/ shouldn't it be in an 'orion.dtsi'?  With compatible =
> > "marvell,orion-wdt"?  Then, if hypothetically, kirkwood used the same
> > driver but had a small tweak, then kirkwood.dtsi would have
> > "marvell,kirkwood-wdt"?  Which would let the driver know that it needed
> > to apply the tweak because it was kirkwood.
> > 
> > In this scenario, since kirkwood and dreamplug currently need no
> > modifications to the default wdt driver, they would just inherit
> > orion.dtsi's "marvell,orion-wdt", right?
> > 
> > On the driver side, since it currently has no subarch or board specific
> > tweaks, then it is *only* good for the generic case,
> > "marvell,orion-wdt", and thus should only match it for now.
> > 
> > If the above is correct, then I should create orion.dtsi, which is
> > included by kirkwood.dtsi.  orion.dtsi should hold "marvell,orion-wdt"
> > and "marvell,orion-rtc".  The corresponding drivers should only match
> > those generic strings, since there are no specific tweaks.
> 
> Hi Jason
> 
> I would agree with you in principal, that most of the drivers used by
> kirkwood are generic orion drivers. There is one clear exception to
> this, and that is the audio driver, which is specific to Kirkwood.
> 
> However, what differs between all the different plat-orion SoC is the
> address map and interrupts. So the dove wdt is in a different place to
> the orion5x wdt, and uses a different interrupt line, etc.

Yes, I was just checking into that.  Seems to me then, it should be
defined at the lowest level where everything is the same.  So, since
kirkwood uses the same registers and irq lines across kirkwood boards,
placing orion_wdt in kirkwood.dtsi would be appropriate.

orion.dtsi doesn't make much sense unless there is something in common
across all the subarchs, attributes included.

Also, only devices which are common to *all* boards, and active would go
in kirkwood.dtsi.  Which is why we decided to place uart0 in
kirkwood-dreamplug.dts.  The serial port doesn't need to be enabled in
a production system, so including kirkwood.dtsi shouldn't enable it.

> So i expect in the end we have a dove.dtsi, a orion5x.dtsi, a
> mv78xx0.dtsi and a kirkwood.dtsi which all look very similar, but have
> different values for addresses and interrupts.

I'm coming to the same conclusion.

> Maybe DT has some why to handle this? 

I think this is the way it is Supposed To Be.

> As to naming, i think you are correct. It should be
> "marvell,orion-wdt" in all the different .dtsi files, since its the
> same SoC IP in all Orion devices. Same with "marvell,orion-rtc" since
> all orion based SoC have the same built in RTC IP.

Agreed.

thx,

Jason.

[GIT PULL v3] ARM: kirkwood: fdt: convert kirkwood init funcs to fdt

[Jason Cooper]

The following changes since commit 759a45185ac0e4dfaf8bbfcb390ec73aca4b7a34:

  ARM: kirkwood: convert uart0 to devicetree. (2012-02-27 16:21:44 +0000)

are available in the git repository at:
  git://git.infradead.org/users/jcooper/linux-kirkwood.git kirkwood_dt

Arnd,

Sorry for the redo, here's what changed:
	orion-spi: put #ifdef CONFIG_OF around of_device_id
	rtc-mv: put #ifdef CONFIG_OF around of_device_id
	rtc-mv: more accurate compatible strings
	orion-wdt: put #ifdef CONFIG_OF around of_device_id
	orion-wdt: more accurate compatible strings

Jason Cooper (5):
      ARM: kirkwood: covert orion-spi to fdt.
      ARM: kirkwood: move var setting to correct location.
      ARM: kirkwood: fdt: absorb kirkwood_init()
      ARM: kirkwood: convert rtc-mv to fdt.
      ARM: kirkwood: convert orion-wdt to fdt.

 arch/arm/boot/dts/kirkwood-dreamplug.dts |   34 +++++++++++++++
 arch/arm/boot/dts/kirkwood.dtsi          |   12 +++++
 arch/arm/mach-kirkwood/board-dt.c        |   67 +++++++++++------------------
 arch/arm/mach-kirkwood/common.c          |   21 +++++++--
 arch/arm/mach-kirkwood/common.h          |    6 +++
 arch/arm/plat-orion/common.c             |    7 +++-
 drivers/rtc/rtc-mv.c                     |   10 ++++
 drivers/spi/spi-orion.c                  |   39 +++++++++++++++--
 drivers/watchdog/orion_wdt.c             |   49 ++++++++++++++++-----
 9 files changed, 182 insertions(+), 63 deletions(-)

Re: [GIT PULL v3] ARM: kirkwood: fdt: convert kirkwood init funcs to fdt

[Jason]

On Sun, Mar 04, 2012 at 10:41:13PM -0500, Jason Cooper wrote:
> 	rtc-mv: more accurate compatible strings
> 	orion-wdt: more accurate compatible strings
...
>  arch/arm/boot/dts/kirkwood-dreamplug.dts |   34 +++++++++++++++
>  arch/arm/boot/dts/kirkwood.dtsi          |   12 +++++

This is still bugging me.  If, say, orion_wdt.c is common to all of
plat-orion/ shouldn't it be in an 'orion.dtsi'?  With compatible =
"marvell,orion-wdt"?  Then, if hypothetically, kirkwood used the same
driver but had a small tweak, then kirkwood.dtsi would have
"marvell,kirkwood-wdt"?  Which would let the driver know that it needed
to apply the tweak because it was kirkwood.

In this scenario, since kirkwood and dreamplug currently need no
modifications to the default wdt driver, they would just inherit
orion.dtsi's "marvell,orion-wdt", right?

On the driver side, since it currently has no subarch or board specific
tweaks, then it is *only* good for the generic case,
"marvell,orion-wdt", and thus should only match it for now.

If the above is correct, then I should create orion.dtsi, which is
included by kirkwood.dtsi.  orion.dtsi should hold "marvell,orion-wdt"
and "marvell,orion-rtc".  The corresponding drivers should only match
those generic strings, since there are no specific tweaks.

I hate to keep kicking this horse, but I want to make sure I grok it.

thx,

Jason.

Re: [GIT PULL v3] ARM: kirkwood: fdt: convert kirkwood init funcs to fdt

[Arnd Bergmann]

On Monday 05 March 2012, Jason wrote:
> This is still bugging me.  If, say, orion_wdt.c is common to all of
> plat-orion/ shouldn't it be in an 'orion.dtsi'?  With compatible =
> "marvell,orion-wdt"?  Then, if hypothetically, kirkwood used the same
> driver but had a small tweak, then kirkwood.dtsi would have
> "marvell,kirkwood-wdt"?  Which would let the driver know that it needed
> to apply the tweak because it was kirkwood.

Some drivers are common to all orion, but others are only used in some
out of {orion5k,kirkwood,mv78xx0,dove}. You could of course have
an orion.dtsi that is included in kirkwood.dtsi.

BTW, I just realized that the patches that Haojian Zhuang sent for
pxa/mmp are using mrvl instead of marvell, and you should definitely
be using the same identifier both both. By convention we use the
stock ticker symbol as a unique identifier, which would be mrvl here.

> In this scenario, since kirkwood and dreamplug currently need no
> modifications to the default wdt driver, they would just inherit
> orion.dtsi's "marvell,orion-wdt", right?

I would still list both, so we are future-proof in case we need
to modify the driver in the future but don't want to modify
the device tree.

> On the driver side, since it currently has no subarch or board specific
> tweaks, then it is only good for the generic case,
> "marvell,orion-wdt", and thus should only match it for now.

Yes.

> If the above is correct, then I should create orion.dtsi, which is
> included by kirkwood.dtsi.  orion.dtsi should hold "marvell,orion-wdt"
> and "marvell,orion-rtc".  The corresponding drivers should only match
> those generic strings, since there are no specific tweaks.
> 
> I hate to keep kicking this horse, but I want to make sure I grok it.

Sounds right to me.

	Arnd

Re: [GIT PULL v3] ARM: kirkwood: fdt: convert kirkwood init funcs to fdt

[Jason]

On Mon, Mar 05, 2012 at 02:34:34PM +0000, Arnd Bergmann wrote:
> On Monday 05 March 2012, Jason wrote:
> > This is still bugging me.  If, say, orion_wdt.c is common to all of
> > plat-orion/ shouldn't it be in an 'orion.dtsi'?  With compatible =
> > "marvell,orion-wdt"?  Then, if hypothetically, kirkwood used the same
> > driver but had a small tweak, then kirkwood.dtsi would have
> > "marvell,kirkwood-wdt"?  Which would let the driver know that it needed
> > to apply the tweak because it was kirkwood.
> 
> Some drivers are common to all orion, but others are only used in some
> out of {orion5k,kirkwood,mv78xx0,dove}. You could of course have
> an orion.dtsi that is included in kirkwood.dtsi.

As Andrew just brought up, the drivers are common across all of them,
but the registers and irqs are different.  Which would seem to shoot
down the need for an orion.dtsi.

The registers/irqs are common across kirkwood, so the best place
probably is kirkwood.dtsi.

However, if they are placed in kirkwood.dtsi, having compatible =
"mrvl,88f6821-rtc" probably is not a good idea.  Not all kirkwoods have
the same dev_id.

> BTW, I just realized that the patches that Haojian Zhuang sent for
> pxa/mmp are using mrvl instead of marvell, and you should definitely
> be using the same identifier both both. By convention we use the
> stock ticker symbol as a unique identifier, which would be mrvl here.

Hehehe that was in the Manual, right?  ;-)

> > In this scenario, since kirkwood and dreamplug currently need no
> > modifications to the default wdt driver, they would just inherit
> > orion.dtsi's "marvell,orion-wdt", right?
> 
> I would still list both, so we are future-proof in case we need
> to modify the driver in the future but don't want to modify
> the device tree.

So, in kirkwood.dtsi, compatible = "mrvl,kirkwood-rtc", "mrvl,orion-rtc";

and, drivers without tweaks would match "mrvl,orion-rtc".

> > On the driver side, since it currently has no subarch or board specific
> > tweaks, then it is only good for the generic case,
> > "marvell,orion-wdt", and thus should only match it for now.
> 
> Yes.

Okay.

thx,

Jason.