Patchwork [0/6] AT91 Reset cleanup

login
register
mail settings
Submitter Jean-Christophe PLAGNIOL-VILLARD
Date Nov. 29, 2011, 5:35 p.m.
Message ID <20111129173514.GW15008@game.jcrosoft.org>
Download mbox
Permalink /patch/128307/
State New
Headers show

Pull-request

git://github.com/at91linux/linux-at91.git for-arnd-3.3-reset

Comments

Jean-Christophe PLAGNIOL-VILLARD - Nov. 29, 2011, 5:35 p.m.
Hi,

	the following patch series will merge the ddr controler header file
	for sam9 and cap9 and cleanup the reset by

	swtich to arm_pm_restart
	fix the sam9g45 and cap9 reset
	make rstc soc independent

The following changes since commit 15327570807209b5c088382af23ed3fc47558d87:

  ARM: at91/gpio: fix display of number of irq setuped (2011-11-28 22:53:09 +0800)

are available in the git repository at:
  git://github.com/at91linux/linux-at91.git for-arnd-3.3-reset

Jean-Christophe PLAGNIOL-VILLARD (6):
      ARM: at91: fix cap9 ddrsdr register
      ARM: at91: merge at91cap9_ddrsdr.h in at91sam9_ddrsdr.h
      ARM: at91: introduce AT91_SAM9_ALT_RESET to select the at91sam9 alternative reset
      ARM: restart: at91: use new restart hook
      ARM: at91: Fix at91sam9g45 and at91cap9 reset
      ARM: at91: make rstc soc independent

 arch/arm/mach-at91/Kconfig                        |   14 +++
 arch/arm/mach-at91/Makefile                       |   14 ++-
 arch/arm/mach-at91/at91cap9.c                     |    9 +--
 arch/arm/mach-at91/at91rm9200.c                   |    4 +-
 arch/arm/mach-at91/at91sam9260.c                  |    3 +-
 arch/arm/mach-at91/at91sam9261.c                  |    3 +-
 arch/arm/mach-at91/at91sam9263.c                  |    3 +-
 arch/arm/mach-at91/at91sam9_alt_reset.S           |   16 +--
 arch/arm/mach-at91/at91sam9g45.c                  |    9 +--
 arch/arm/mach-at91/at91sam9g45_reset.S            |   40 ++++++++
 arch/arm/mach-at91/at91sam9rl.c                   |    3 +-
 arch/arm/mach-at91/generic.h                      |    5 +-
 arch/arm/mach-at91/include/mach/at91_rstc.h       |   18 +++-
 arch/arm/mach-at91/include/mach/at91cap9.h        |    2 +-
 arch/arm/mach-at91/include/mach/at91cap9_ddrsdr.h |  108 ---------------------
 arch/arm/mach-at91/include/mach/at91sam9260.h     |    2 +-
 arch/arm/mach-at91/include/mach/at91sam9261.h     |    2 +-
 arch/arm/mach-at91/include/mach/at91sam9263.h     |    2 +-
 arch/arm/mach-at91/include/mach/at91sam9_ddrsdr.h |   30 ++++--
 arch/arm/mach-at91/include/mach/at91sam9g45.h     |    2 +-
 arch/arm/mach-at91/include/mach/at91sam9rl.h      |    2 +-
 arch/arm/mach-at91/include/mach/system.h          |    5 -
 arch/arm/mach-at91/pm.c                           |    9 +--
 arch/arm/mach-at91/pm.h                           |    8 +-
 arch/arm/mach-at91/pm_slowclock.S                 |    5 +-
 arch/arm/mach-at91/setup.c                        |    9 ++
 26 files changed, 142 insertions(+), 185 deletions(-)
 create mode 100644 arch/arm/mach-at91/at91sam9g45_reset.S
 delete mode 100644 arch/arm/mach-at91/include/mach/at91cap9_ddrsdr.h

Best Regards,
J.
Russell King - ARM Linux - Nov. 29, 2011, 5:42 p.m.
On Tue, Nov 29, 2011 at 06:35:14PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> Hi,
> 
> 	the following patch series will merge the ddr controler header file
> 	for sam9 and cap9 and cleanup the reset by
> 
> 	swtich to arm_pm_restart
> 	fix the sam9g45 and cap9 reset
> 	make rstc soc independent
> 
> The following changes since commit 15327570807209b5c088382af23ed3fc47558d87:
> 
>   ARM: at91/gpio: fix display of number of irq setuped (2011-11-28 22:53:09 +0800)
> 
> are available in the git repository at:
>   git://github.com/at91linux/linux-at91.git for-arnd-3.3-reset
> 
> Jean-Christophe PLAGNIOL-VILLARD (6):
>       ARM: at91: fix cap9 ddrsdr register
>       ARM: at91: merge at91cap9_ddrsdr.h in at91sam9_ddrsdr.h
>       ARM: at91: introduce AT91_SAM9_ALT_RESET to select the at91sam9 alternative reset
>       ARM: restart: at91: use new restart hook

Well, it looks like you've taken my work here and committed it as yourself.
That's plagerism.  Don't do it - and fix it before someone pulls your crap.
Jean-Christophe PLAGNIOL-VILLARD - Nov. 29, 2011, 5:49 p.m.
On 17:42 Tue 29 Nov     , Russell King - ARM Linux wrote:
> On Tue, Nov 29, 2011 at 06:35:14PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Hi,
> > 
> > 	the following patch series will merge the ddr controler header file
> > 	for sam9 and cap9 and cleanup the reset by
> > 
> > 	swtich to arm_pm_restart
> > 	fix the sam9g45 and cap9 reset
> > 	make rstc soc independent
> > 
> > The following changes since commit 15327570807209b5c088382af23ed3fc47558d87:
> > 
> >   ARM: at91/gpio: fix display of number of irq setuped (2011-11-28 22:53:09 +0800)
> > 
> > are available in the git repository at:
> >   git://github.com/at91linux/linux-at91.git for-arnd-3.3-reset
> > 
> > Jean-Christophe PLAGNIOL-VILLARD (6):
> >       ARM: at91: fix cap9 ddrsdr register
> >       ARM: at91: merge at91cap9_ddrsdr.h in at91sam9_ddrsdr.h
> >       ARM: at91: introduce AT91_SAM9_ALT_RESET to select the at91sam9 alternative reset
> >       ARM: restart: at91: use new restart hook
> 
> Well, it looks like you've taken my work here and committed it as yourself.
> That's plagerism.  Don't do it - and fix it before someone pulls your crap.
sorry I didn't take your work as mine

Best Regards,
J.
Marek Vasut - Nov. 29, 2011, 6:46 p.m.
> On Tue, Nov 29, 2011 at 06:35:14PM +0100, Jean-Christophe PLAGNIOL-VILLARD 
wrote:
> > Hi,
> > 
> > 	the following patch series will merge the ddr controler header file
> > 	for sam9 and cap9 and cleanup the reset by
> > 	
> > 	swtich to arm_pm_restart
> > 	fix the sam9g45 and cap9 reset
> > 	make rstc soc independent
> > 
> > The following changes since commit 15327570807209b5c088382af23ed3fc47558d87:
> >   ARM: at91/gpio: fix display of number of irq setuped (2011-11-28
> >   22:53:09 +0800)
> > 
> > are available in the git repository at:
> >   git://github.com/at91linux/linux-at91.git for-arnd-3.3-reset
> > 
> > Jean-Christophe PLAGNIOL-VILLARD (6):
> >       ARM: at91: fix cap9 ddrsdr register
> >       ARM: at91: merge at91cap9_ddrsdr.h in at91sam9_ddrsdr.h
> >       ARM: at91: introduce AT91_SAM9_ALT_RESET to select the at91sam9
> >       alternative reset ARM: restart: at91: use new restart hook
> 
> Well, it looks like you've taken my work here and committed it as yourself.
> That's plagerism.  Don't do it - and fix it before someone pulls your crap.

Hey,

looking at the patch in question, it looks so trivial the clash is just 
possible. Why not just put two SoB lines there and maybe From: and be done with 
it?

M
Russell King - ARM Linux - Nov. 29, 2011, 7:07 p.m.
On Tue, Nov 29, 2011 at 07:46:13PM +0100, Marek Vasut wrote:
> > On Tue, Nov 29, 2011 at 06:35:14PM +0100, Jean-Christophe PLAGNIOL-VILLARD 
> wrote:
> > > Hi,
> > > 
> > > 	the following patch series will merge the ddr controler header file
> > > 	for sam9 and cap9 and cleanup the reset by
> > > 	
> > > 	swtich to arm_pm_restart
> > > 	fix the sam9g45 and cap9 reset
> > > 	make rstc soc independent
> > > 
> > > The following changes since commit 15327570807209b5c088382af23ed3fc47558d87:
> > >   ARM: at91/gpio: fix display of number of irq setuped (2011-11-28
> > >   22:53:09 +0800)
> > > 
> > > are available in the git repository at:
> > >   git://github.com/at91linux/linux-at91.git for-arnd-3.3-reset
> > > 
> > > Jean-Christophe PLAGNIOL-VILLARD (6):
> > >       ARM: at91: fix cap9 ddrsdr register
> > >       ARM: at91: merge at91cap9_ddrsdr.h in at91sam9_ddrsdr.h
> > >       ARM: at91: introduce AT91_SAM9_ALT_RESET to select the at91sam9
> > >       alternative reset ARM: restart: at91: use new restart hook
> > 
> > Well, it looks like you've taken my work here and committed it as yourself.
> > That's plagerism.  Don't do it - and fix it before someone pulls your crap.
> 
> Hey,
> 
> looking at the patch in question, it looks so trivial the clash is just 
> possible. Why not just put two SoB lines there and maybe From: and be done with 
> it?

Look closer.  The patch which was posted has my sign-off on it, it says
it's from me, but according to the commit summary, it's author is Jean-
Christophe.

So, it _is_ my patch which Jean has decided to _recommit_ into his tree
taking it from my tree _without_ _asking_ _me_ whether it was either a
good idea to do that, whether it was a finished patch, etc.

So all in all I'm pretty disgusted with Jean-Christophe over this.

I would like to see my patch updated at some point not to use
arm_pm_restart, but to use the .restart method directly, like I've been
doing for everyone else.  In other words, this patch is NOT STABLE at
the present time.
Marek Vasut - Nov. 29, 2011, 7:23 p.m.
> On Tue, Nov 29, 2011 at 07:46:13PM +0100, Marek Vasut wrote:
> > > On Tue, Nov 29, 2011 at 06:35:14PM +0100, Jean-Christophe
> > > PLAGNIOL-VILLARD
> > 
> > wrote:
> > > > Hi,
> > > > 
> > > > 	the following patch series will merge the ddr controler header 
file
> > > > 	for sam9 and cap9 and cleanup the reset by
> > > > 	
> > > > 	swtich to arm_pm_restart
> > > > 	fix the sam9g45 and cap9 reset
> > > > 	make rstc soc independent
> > > > 
> > > > The following changes since commit 
15327570807209b5c088382af23ed3fc47558d87:
> > > >   ARM: at91/gpio: fix display of number of irq setuped (2011-11-28
> > > >   22:53:09 +0800)
> > > > 
> > > > are available in the git repository at:
> > > >   git://github.com/at91linux/linux-at91.git for-arnd-3.3-reset
> > > > 
> > > > Jean-Christophe PLAGNIOL-VILLARD (6):
> > > >       ARM: at91: fix cap9 ddrsdr register
> > > >       ARM: at91: merge at91cap9_ddrsdr.h in at91sam9_ddrsdr.h
> > > >       ARM: at91: introduce AT91_SAM9_ALT_RESET to select the at91sam9
> > > >       alternative reset ARM: restart: at91: use new restart hook
> > > 
> > > Well, it looks like you've taken my work here and committed it as
> > > yourself. That's plagerism.  Don't do it - and fix it before someone
> > > pulls your crap.
> > 
> > Hey,
> > 
> > looking at the patch in question, it looks so trivial the clash is just
> > possible. Why not just put two SoB lines there and maybe From: and be
> > done with it?
> 
> Look closer.  The patch which was posted has my sign-off on it, it says
> it's from me, but according to the commit summary, it's author is Jean-
> Christophe.
> 
> So, it _is_ my patch which Jean has decided to _recommit_ into his tree
> taking it from my tree _without_ _asking_ _me_ whether it was either a
> good idea to do that, whether it was a finished patch, etc.

Ah ok, I see.
> 
> So all in all I'm pretty disgusted with Jean-Christophe over this.

Oh come on, calm down. Just keep an eye on him so he won't do it again and be 
done with it ;-) And Jean should certainly drop the patch if it's unfinished 
stuff.
> 
> I would like to see my patch updated at some point not to use
> arm_pm_restart, but to use the .restart method directly, like I've been
> doing for everyone else.  In other words, this patch is NOT STABLE at
> the present time.

OK
Russell King - ARM Linux - Nov. 29, 2011, 10:07 p.m.
On Tue, Nov 29, 2011 at 08:23:19PM +0100, Marek Vasut wrote:
> Just keep an eye on him so he won't do it again and be done with it ;-)
> And Jean should certainly drop the patch if it's unfinished stuff.

Oh so I should say nothing and let people take my work hand over fist.
No thanks, that's not the game I play.
Russell King - ARM Linux - Nov. 29, 2011, 10:12 p.m.
On Tue, Nov 29, 2011 at 06:52:41PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> +			.globl	at91sam9g45_restart
> +
> +at91sam9g45_restart:
> +			ldr	r0, .at91_va_base_sdramc0	@ preload constants
> +			ldr	r1, .at91_va_base_rstc_cr
> +
> +			mov	r2, #1
> +			mov	r3, #AT91_DDRSDRC_LPCB_POWER_DOWN
> +			ldr	r4, =AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST
> +
> +			.balign	32				@ align to cache line
> +
> +			str	r2, [r0, #AT91_DDRSDRC_RTR]	@ disable DDR0 access
> +			str	r3, [r0, #AT91_DDRSDRC_LPR]	@ power down DDR0
> +			str	r4, [r1]			@ reset processor
> +
> +			b	.
> +
> +.at91_va_base_sdramc0:
> +	.word AT91_VA_BASE_SYS + AT91_DDRSDRC0

So is the only change between this new file and arch/arm/mach-at91/at91sam9_alt_reset.S
this line above?

arch/arm/mach-at91/at91sam9_alt_reset.S has:
        .word AT91_VA_BASE_SYS + AT91_SDRAMC0

Maybe the at91sam9_alt_reset.S version should take this as an argument so
the errata fix can be re-used on different AT91 versions, rather than
having to duplicate code just because one register address has changed.
Marek Vasut - Nov. 29, 2011, 10:43 p.m.
> On Tue, Nov 29, 2011 at 08:23:19PM +0100, Marek Vasut wrote:
> > Just keep an eye on him so he won't do it again and be done with it ;-)
> > And Jean should certainly drop the patch if it's unfinished stuff.
> 
> Oh so I should say nothing and let people take my work hand over fist.
> No thanks, that's not the game I play.

I never said that and I understand your point clearly, don't be mistaken. What I 
meant was more like -- don't get that angry so fast and don't use so strong 
words, it's only a piece of code afterall.
Russell King - ARM Linux - Nov. 29, 2011, 10:48 p.m.
On Tue, Nov 29, 2011 at 11:43:30PM +0100, Marek Vasut wrote:
> > On Tue, Nov 29, 2011 at 08:23:19PM +0100, Marek Vasut wrote:
> > > Just keep an eye on him so he won't do it again and be done with it ;-)
> > > And Jean should certainly drop the patch if it's unfinished stuff.
> > 
> > Oh so I should say nothing and let people take my work hand over fist.
> > No thanks, that's not the game I play.
> 
> I never said that and I understand your point clearly, don't be mistaken. What I 
> meant was more like -- don't get that angry so fast and don't use so strong 
> words, it's only a piece of code afterall.

Sorry, I wasn't getting angry - am I not allowed to point out my disgust
at someone who's been around for quite some time making such a mistake?
Marek Vasut - Nov. 29, 2011, 11:03 p.m.
> On Tue, Nov 29, 2011 at 11:43:30PM +0100, Marek Vasut wrote:
> > > On Tue, Nov 29, 2011 at 08:23:19PM +0100, Marek Vasut wrote:
> > > > Just keep an eye on him so he won't do it again and be done with it
> > > > ;-) And Jean should certainly drop the patch if it's unfinished
> > > > stuff.
> > > 
> > > Oh so I should say nothing and let people take my work hand over fist.
> > > No thanks, that's not the game I play.
> > 
> > I never said that and I understand your point clearly, don't be mistaken.
> > What I meant was more like -- don't get that angry so fast and don't use
> > so strong words, it's only a piece of code afterall.
> 
> Sorry, I wasn't getting angry - am I not allowed to point out my disgust
> at someone who's been around for quite some time making such a mistake?

Don't even the best of us make mistakes?

M
Jean-Christophe PLAGNIOL-VILLARD - Nov. 30, 2011, 4:46 a.m.
On 22:48 Tue 29 Nov     , Russell King - ARM Linux wrote:
> On Tue, Nov 29, 2011 at 11:43:30PM +0100, Marek Vasut wrote:
> > > On Tue, Nov 29, 2011 at 08:23:19PM +0100, Marek Vasut wrote:
> > > > Just keep an eye on him so he won't do it again and be done with it ;-)
> > > > And Jean should certainly drop the patch if it's unfinished stuff.
> > > 
> > > Oh so I should say nothing and let people take my work hand over fist.
> > > No thanks, that's not the game I play.
> > 
> > I never said that and I understand your point clearly, don't be mistaken. What I 
> > meant was more like -- don't get that angry so fast and don't use so strong 
> > words, it's only a piece of code afterall.
> 
> Sorry, I wasn't getting angry - am I not allowed to point out my disgust
> at someone who's been around for quite some time making such a mistake?

Russell is right I did the mistake not on purpose but I did

I apologize end of the story

Best Regards,
J.
Jean-Christophe PLAGNIOL-VILLARD - Nov. 30, 2011, 4:55 a.m.
On 22:12 Tue 29 Nov     , Russell King - ARM Linux wrote:
> On Tue, Nov 29, 2011 at 06:52:41PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > +			.globl	at91sam9g45_restart
> > +
> > +at91sam9g45_restart:
> > +			ldr	r0, .at91_va_base_sdramc0	@ preload constants
> > +			ldr	r1, .at91_va_base_rstc_cr
> > +
> > +			mov	r2, #1
> > +			mov	r3, #AT91_DDRSDRC_LPCB_POWER_DOWN
> > +			ldr	r4, =AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST
> > +
> > +			.balign	32				@ align to cache line
> > +
> > +			str	r2, [r0, #AT91_DDRSDRC_RTR]	@ disable DDR0 access
> > +			str	r3, [r0, #AT91_DDRSDRC_LPR]	@ power down DDR0
> > +			str	r4, [r1]			@ reset processor
> > +
> > +			b	.
> > +
> > +.at91_va_base_sdramc0:
> > +	.word AT91_VA_BASE_SYS + AT91_DDRSDRC0
> 
> So is the only change between this new file and arch/arm/mach-at91/at91sam9_alt_reset.S
> this line above?
> 
> arch/arm/mach-at91/at91sam9_alt_reset.S has:
>         .word AT91_VA_BASE_SYS + AT91_SDRAMC0
> 
> Maybe the at91sam9_alt_reset.S version should take this as an argument so
> the errata fix can be re-used on different AT91 versions, rather than
> having to duplicate code just because one register address has changed.
its not only the register base address but also the registers offset

I get the same idea but at the end when I did it I end with more code
and the only patrt in common  are

ldr     r1, at91_va_base_rstc_cre
mov     r2, #1
ldr     r4, =AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST
str     r4, [r1]

so I chosse to split it

as if I want to do the second way I need to pass 5 params to the asm fucntion
and create one c function for sam9 and 9g45

Best Regards,
J.