* [PATCH] mtd: nuc900_nand: read correct SMISR register
@ 2016-01-13 21:38 Arnd Bergmann
2016-01-13 21:50 ` Brian Norris
2016-01-15 18:03 ` Brian Norris
0 siblings, 2 replies; 10+ messages in thread
From: Arnd Bergmann @ 2016-01-13 21:38 UTC (permalink / raw)
To: Brian Norris
Cc: Wan ZongShun, David Woodhouse, linux-arm-kernel, linux-mtd, linux-kernel
The nuc900_nand driver has always passed an incorrect register
address in its nuc900_check_rb() function, which cannot possibly
work, and in some configurations gives us a build warning:
drivers/mtd/nand/nuc900_nand.c: In function 'nuc900_check_rb':
drivers/mtd/nand/nuc900_nand.c:27:23: warning: passing argument 1 of '__raw_readl' makes pointer from integer without a cast [-Wint-conversion]
#define REG_SMISR 0xac
drivers/mtd/nand/nuc900_nand.c:118:20: note: in expansion of macro 'REG_SMISR'
val = __raw_readl(REG_SMISR);
This makes sure we actually read from the register rather than
from (void *)0x000000ac in user space.
I suspect nobody noticed this before because the nuc900_nand_devready()
function never gets called, or nobody uses this driver on an upstream
kernel. Possibly even both.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/drivers/mtd/nand/nuc900_nand.c b/drivers/mtd/nand/nuc900_nand.c
index 220ddfcf29f5..dbc5b571c2bb 100644
--- a/drivers/mtd/nand/nuc900_nand.c
+++ b/drivers/mtd/nand/nuc900_nand.c
@@ -113,7 +113,7 @@ static int nuc900_check_rb(struct nuc900_nand *nand)
{
unsigned int val;
spin_lock(&nand->lock);
- val = __raw_readl(REG_SMISR);
+ val = __raw_readl(nand->reg + REG_SMISR);
val &= READYBUSY;
spin_unlock(&nand->lock);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-13 21:38 [PATCH] mtd: nuc900_nand: read correct SMISR register Arnd Bergmann
@ 2016-01-13 21:50 ` Brian Norris
2016-01-13 22:02 ` Arnd Bergmann
2016-01-15 18:03 ` Brian Norris
1 sibling, 1 reply; 10+ messages in thread
From: Brian Norris @ 2016-01-13 21:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Wan ZongShun, David Woodhouse, linux-arm-kernel, linux-mtd, linux-kernel
On Wed, Jan 13, 2016 at 10:38:08PM +0100, Arnd Bergmann wrote:
> The nuc900_nand driver has always passed an incorrect register
> address in its nuc900_check_rb() function, which cannot possibly
> work, and in some configurations gives us a build warning:
>
> drivers/mtd/nand/nuc900_nand.c: In function 'nuc900_check_rb':
> drivers/mtd/nand/nuc900_nand.c:27:23: warning: passing argument 1 of '__raw_readl' makes pointer from integer without a cast [-Wint-conversion]
> #define REG_SMISR 0xac
> drivers/mtd/nand/nuc900_nand.c:118:20: note: in expansion of macro 'REG_SMISR'
> val = __raw_readl(REG_SMISR);
>
> This makes sure we actually read from the register rather than
> from (void *)0x000000ac in user space.
>
> I suspect nobody noticed this before because the nuc900_nand_devready()
> function never gets called, or nobody uses this driver on an upstream
> kernel. Possibly even both.
Almost definitely not the first. That's an absolutely essential
function for this driver (it doesn't have ->waitfunc(), so we use
->dev_ready() all the time). Quite likely the latter.
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/drivers/mtd/nand/nuc900_nand.c b/drivers/mtd/nand/nuc900_nand.c
> index 220ddfcf29f5..dbc5b571c2bb 100644
> --- a/drivers/mtd/nand/nuc900_nand.c
> +++ b/drivers/mtd/nand/nuc900_nand.c
> @@ -113,7 +113,7 @@ static int nuc900_check_rb(struct nuc900_nand *nand)
> {
> unsigned int val;
> spin_lock(&nand->lock);
> - val = __raw_readl(REG_SMISR);
> + val = __raw_readl(nand->reg + REG_SMISR);
> val &= READYBUSY;
> spin_unlock(&nand->lock);
>
Looks OK to me, though I kinda hate dragging on support for
obviously-unused drivers...
Brian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-13 21:50 ` Brian Norris
@ 2016-01-13 22:02 ` Arnd Bergmann
2016-01-14 0:29 ` Brian Norris
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2016-01-13 22:02 UTC (permalink / raw)
To: Brian Norris
Cc: Wan ZongShun, David Woodhouse, linux-arm-kernel, linux-mtd, linux-kernel
On Wednesday 13 January 2016 13:50:13 Brian Norris wrote:
>
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> > diff --git a/drivers/mtd/nand/nuc900_nand.c b/drivers/mtd/nand/nuc900_nand.c
> > index 220ddfcf29f5..dbc5b571c2bb 100644
> > --- a/drivers/mtd/nand/nuc900_nand.c
> > +++ b/drivers/mtd/nand/nuc900_nand.c
> > @@ -113,7 +113,7 @@ static int nuc900_check_rb(struct nuc900_nand *nand)
> > {
> > unsigned int val;
> > spin_lock(&nand->lock);
> > - val = __raw_readl(REG_SMISR);
> > + val = __raw_readl(nand->reg + REG_SMISR);
> > val &= READYBUSY;
> > spin_unlock(&nand->lock);
> >
>
> Looks OK to me, though I kinda hate dragging on support for
> obviously-unused drivers...
Should we mark that driver in Kconfig as obviously broken then?
Let's wait for Wan ZongShun to reply first, it's possible that the
entire w90x900 platform has come to the point where we are better off
removing it than fixing ancient bugs.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-13 22:02 ` Arnd Bergmann
@ 2016-01-14 0:29 ` Brian Norris
2016-01-14 9:34 ` Wan Zongshun
0 siblings, 1 reply; 10+ messages in thread
From: Brian Norris @ 2016-01-14 0:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Wan ZongShun, David Woodhouse, linux-arm-kernel, linux-mtd, linux-kernel
On Wed, Jan 13, 2016 at 11:02:06PM +0100, Arnd Bergmann wrote:
> On Wednesday 13 January 2016 13:50:13 Brian Norris wrote:
> >
> > Looks OK to me, though I kinda hate dragging on support for
> > obviously-unused drivers...
>
> Should we mark that driver in Kconfig as obviously broken then?
Well, it won't be obviously broken if I apply your patch... But if
that's the right step toward removal, then I could be OK with that.
> Let's wait for Wan ZongShun to reply first, it's possible that the
> entire w90x900 platform has come to the point where we are better off
> removing it than fixing ancient bugs.
Sounds good.
Brian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-14 0:29 ` Brian Norris
@ 2016-01-14 9:34 ` Wan Zongshun
2016-01-14 12:08 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Wan Zongshun @ 2016-01-14 9:34 UTC (permalink / raw)
To: Brian Norris, Arnd Bergmann
Cc: Wan ZongShun, David Woodhouse, linux-arm-kernel, linux-mtd, linux-kernel
-------- Original Message --------
> On Wed, Jan 13, 2016 at 11:02:06PM +0100, Arnd Bergmann wrote:
>> On Wednesday 13 January 2016 13:50:13 Brian Norris wrote:
>>>
>>> Looks OK to me, though I kinda hate dragging on support for
>>> obviously-unused drivers...
>>
>> Should we mark that driver in Kconfig as obviously broken then?
>
> Well, it won't be obviously broken if I apply your patch... But if
> that's the right step toward removal, then I could be OK with that.
>
>> Let's wait for Wan ZongShun to reply first, it's possible that the
>> entire w90x900 platform has come to the point where we are better off
>> removing it than fixing ancient bugs.
>
> Sounds good.
Actually, Nuvoton should still leverage this upstream w90x900 codes for
their old and new arm chip BSP, but I am not sure their open source plan
for the new chip now, I will check with Nuvoton for this topic, and give
you feedback here, so please hold on its removal.
>
> Brian
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-14 9:34 ` Wan Zongshun
@ 2016-01-14 12:08 ` Arnd Bergmann
2016-01-15 7:53 ` Wan Zongshun
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2016-01-14 12:08 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Wan Zongshun, Brian Norris, linux-mtd, David Woodhouse,
Wan ZongShun, linux-kernel, Mike Thompson
On Thursday 14 January 2016 17:34:33 Wan Zongshun wrote:
> -------- Original Message --------
> > On Wed, Jan 13, 2016 at 11:02:06PM +0100, Arnd Bergmann wrote:
> >> On Wednesday 13 January 2016 13:50:13 Brian Norris wrote:
> >>>
> >>> Looks OK to me, though I kinda hate dragging on support for
> >>> obviously-unused drivers...
> >>
> >> Should we mark that driver in Kconfig as obviously broken then?
> >
> > Well, it won't be obviously broken if I apply your patch... But if
> > that's the right step toward removal, then I could be OK with that.
> >
> >> Let's wait for Wan ZongShun to reply first, it's possible that the
> >> entire w90x900 platform has come to the point where we are better off
> >> removing it than fixing ancient bugs.
> >
> > Sounds good.
>
> Actually, Nuvoton should still leverage this upstream w90x900 codes for
> their old and new arm chip BSP, but I am not sure their open source plan
> for the new chip now, I will check with Nuvoton for this topic, and give
> you feedback here, so please hold on its removal.
Ok, sure. Thanks for the quick reply!
I've had a look around at the current produce lineup, and it seems that
nuc900 (w90x900) is still marketed, and as you say is similar to the n329
series.
There has been one attempt to do a modern port for n329 in 2014 but
it never got submitted. See http://comments.gmane.org/gmane.linux.kernel.kernelnewbies/49077
and https://github.com/mpthompson/linux/tree/n329
The code looks rather nice, so it's a pity that the effort stalled,
but it should not be hard for anyone to start out with Mike's tree
and forward-port it to 4.5.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-14 12:08 ` Arnd Bergmann
@ 2016-01-15 7:53 ` Wan Zongshun
2016-01-15 11:03 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Wan Zongshun @ 2016-01-15 7:53 UTC (permalink / raw)
To: Arnd Bergmann, linux-arm-kernel
Cc: Brian Norris, linux-mtd, David Woodhouse, Wan ZongShun,
linux-kernel, Mike Thompson
>>
>> Actually, Nuvoton should still leverage this upstream w90x900 codes for
>> their old and new arm chip BSP, but I am not sure their open source plan
>> for the new chip now, I will check with Nuvoton for this topic, and give
>> you feedback here, so please hold on its removal.
>
> Ok, sure. Thanks for the quick reply!
>
> I've had a look around at the current produce lineup, and it seems that
> nuc900 (w90x900) is still marketed, and as you say is similar to the n329
> series.
>
> There has been one attempt to do a modern port for n329 in 2014 but
> it never got submitted. See http://comments.gmane.org/gmane.linux.kernel.kernelnewbies/49077
> and https://github.com/mpthompson/linux/tree/n329
>
> The code looks rather nice, so it's a pity that the effort stalled,
> but it should not be hard for anyone to start out with Mike's tree
> and forward-port it to 4.5.
I will try to get this board you mentioned and try to cowork Mike to
submit it into upstream.
After checking with Nuvoton people, and I will get another board and
help them update the latest kernel BSP and also submit new nuc970 chip
BSP into upstream.
spec:
http://www.nuvoton.com/hq/products/microprocessors/arm9-mpus/nuc900-series/?__locale=en
>
> Arnd
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-15 7:53 ` Wan Zongshun
@ 2016-01-15 11:03 ` Arnd Bergmann
2016-01-15 15:01 ` Wan Zongshun
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2016-01-15 11:03 UTC (permalink / raw)
To: Wan Zongshun
Cc: linux-arm-kernel, Brian Norris, linux-mtd, David Woodhouse,
Wan ZongShun, linux-kernel, Mike Thompson
On Friday 15 January 2016 15:53:41 Wan Zongshun wrote:
>
> >>
> >> Actually, Nuvoton should still leverage this upstream w90x900 codes for
> >> their old and new arm chip BSP, but I am not sure their open source plan
> >> for the new chip now, I will check with Nuvoton for this topic, and give
> >> you feedback here, so please hold on its removal.
> >
> > Ok, sure. Thanks for the quick reply!
> >
> > I've had a look around at the current produce lineup, and it seems that
> > nuc900 (w90x900) is still marketed, and as you say is similar to the n329
> > series.
> >
> > There has been one attempt to do a modern port for n329 in 2014 but
> > it never got submitted. See http://comments.gmane.org/gmane.linux.kernel.kernelnewbies/49077
> > and https://github.com/mpthompson/linux/tree/n329
> >
> > The code looks rather nice, so it's a pity that the effort stalled,
> > but it should not be hard for anyone to start out with Mike's tree
> > and forward-port it to 4.5.
>
> I will try to get this board you mentioned and try to cowork Mike to
> submit it into upstream.
>
> After checking with Nuvoton people, and I will get another board and
> help them update the latest kernel BSP and also submit new nuc970 chip
> BSP into upstream.
>
> spec:
> http://www.nuvoton.com/hq/products/microprocessors/arm9-mpus/nuc900-series/?__locale=en
>
This sounds great, good to hear!
I see that your last feature contribution to mach-w90x900 was a little over
5 years ago, before we started the current arm-soc process, after that
we only had bug fixes and smaller cleanups.
I don't know to what degree you have been following what the other platforms
have done in the meantime, so let me try to get you up to speed:
* All patches from platform maintainers by default get merged through the
arm-soc tree at http://git.kernel.org/cgit/linux/kernel/git/arm/arm-soc.git
Please send patches "To: arm@kernel.org", Cc the relevant mailing lists,
and split them up to you submit bug fixes, cleanups, and new features
separately (they go into different branches)
* All actively maintained platforms should use CONFIG_ARCH_MULTIPLATFORM.
This will take significant work for mach-w90x900, but you don't have to
do it all at once. As a rule of thumb, I'd expect that for any work
you do on new features in the platform, an at least equal amount of work
is spent on getting closer to multiplatform support until you are done.
This has worked well for most other platforms, and with the work that
Mike has done it is all complete.
* We have basically stopped taking new board files and moved on to
using devicetree descriptions of the hardware. It's probably ok to
add one more board file for the nuc970 reference design here if you
also work on the cleanup and you don't expect to add multiple other
machines. Again, Mike's port seems already uses DT.
* The multiplatform work does not require the dts conversion, but it
does require converting the clock.c file into a driver for drivers/clk/,
and it requires converting the interrupt handling to use an irqchip
driver with CONFIG_IRQ_DOMAIN and CONFIG_MULTI_IRQ_HANDLER. These
are usually not hard to do if you have the hardware for testing, but
it's not easy otherwise.
* I've merged a patch that moves all mach/*.h headers out of the globally
visible directory to arch/arm/mach-w90x900/*.h directly unless they are
used by some driver outside of mach-w90x900. To complete the multiplatform
work, all remaining headers have to be moved as well, either into the
driver or into the platform directory.
* I'd suggest you start by looking at n329 code from Mike before you
submit the nuc970 support. As I think it gets all of the above right
already, it may even be possible to support the nuc970 through the
new mach-n329 support, or to share a large portion of the code.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-15 11:03 ` Arnd Bergmann
@ 2016-01-15 15:01 ` Wan Zongshun
0 siblings, 0 replies; 10+ messages in thread
From: Wan Zongshun @ 2016-01-15 15:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, Brian Norris, linux-mtd, David Woodhouse,
Wan ZongShun, linux-kernel, Mike Thompson
> This sounds great, good to hear!
>
> I see that your last feature contribution to mach-w90x900 was a little over
> 5 years ago, before we started the current arm-soc process, after that
> we only had bug fixes and smaller cleanups.
>
> I don't know to what degree you have been following what the other platforms
> have done in the meantime, so let me try to get you up to speed:
>
> * All patches from platform maintainers by default get merged through the
> arm-soc tree at http://git.kernel.org/cgit/linux/kernel/git/arm/arm-soc.git
> Please send patches "To: arm@kernel.org", Cc the relevant mailing lists,
> and split them up to you submit bug fixes, cleanups, and new features
> separately (they go into different branches)
>
> * All actively maintained platforms should use CONFIG_ARCH_MULTIPLATFORM.
> This will take significant work for mach-w90x900, but you don't have to
> do it all at once. As a rule of thumb, I'd expect that for any work
> you do on new features in the platform, an at least equal amount of work
> is spent on getting closer to multiplatform support until you are done.
> This has worked well for most other platforms, and with the work that
> Mike has done it is all complete.
>
> * We have basically stopped taking new board files and moved on to
> using devicetree descriptions of the hardware. It's probably ok to
> add one more board file for the nuc970 reference design here if you
> also work on the cleanup and you don't expect to add multiple other
> machines. Again, Mike's port seems already uses DT.
>
> * The multiplatform work does not require the dts conversion, but it
> does require converting the clock.c file into a driver for drivers/clk/,
> and it requires converting the interrupt handling to use an irqchip
> driver with CONFIG_IRQ_DOMAIN and CONFIG_MULTI_IRQ_HANDLER. These
> are usually not hard to do if you have the hardware for testing, but
> it's not easy otherwise.
>
> * I've merged a patch that moves all mach/*.h headers out of the globally
> visible directory to arch/arm/mach-w90x900/*.h directly unless they are
> used by some driver outside of mach-w90x900. To complete the multiplatform
> work, all remaining headers have to be moved as well, either into the
> driver or into the platform directory.
>
> * I'd suggest you start by looking at n329 code from Mike before you
> submit the nuc970 support. As I think it gets all of the above right
> already, it may even be possible to support the nuc970 through the
> new mach-n329 support, or to share a large portion of the code.
>
Arnd,
I really appreciate your great nice help.
I will fully consider your proposal and try my best to clean nuc900 plat
codes to keep up to date.
Wan Zongshun.
> Arnd
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nuc900_nand: read correct SMISR register
2016-01-13 21:38 [PATCH] mtd: nuc900_nand: read correct SMISR register Arnd Bergmann
2016-01-13 21:50 ` Brian Norris
@ 2016-01-15 18:03 ` Brian Norris
1 sibling, 0 replies; 10+ messages in thread
From: Brian Norris @ 2016-01-15 18:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Wan ZongShun, David Woodhouse, linux-arm-kernel, linux-mtd, linux-kernel
On Wed, Jan 13, 2016 at 10:38:08PM +0100, Arnd Bergmann wrote:
> The nuc900_nand driver has always passed an incorrect register
> address in its nuc900_check_rb() function, which cannot possibly
> work, and in some configurations gives us a build warning:
>
> drivers/mtd/nand/nuc900_nand.c: In function 'nuc900_check_rb':
> drivers/mtd/nand/nuc900_nand.c:27:23: warning: passing argument 1 of '__raw_readl' makes pointer from integer without a cast [-Wint-conversion]
> #define REG_SMISR 0xac
> drivers/mtd/nand/nuc900_nand.c:118:20: note: in expansion of macro 'REG_SMISR'
> val = __raw_readl(REG_SMISR);
>
> This makes sure we actually read from the register rather than
> from (void *)0x000000ac in user space.
>
> I suspect nobody noticed this before because the nuc900_nand_devready()
> function never gets called, or nobody uses this driver on an upstream
> kernel. Possibly even both.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Pushed to l2-mtd.git. Thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-01-15 18:03 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-13 21:38 [PATCH] mtd: nuc900_nand: read correct SMISR register Arnd Bergmann
2016-01-13 21:50 ` Brian Norris
2016-01-13 22:02 ` Arnd Bergmann
2016-01-14 0:29 ` Brian Norris
2016-01-14 9:34 ` Wan Zongshun
2016-01-14 12:08 ` Arnd Bergmann
2016-01-15 7:53 ` Wan Zongshun
2016-01-15 11:03 ` Arnd Bergmann
2016-01-15 15:01 ` Wan Zongshun
2016-01-15 18:03 ` Brian Norris
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).