[PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Clay McClure]

On systems using Broadcom Cortex-A9 SoCs (BCM585XX, BCM586XX, BCM56340),
nand_flash_detect_onfi() fails at boot with:

    Could not find valid ONFI parameter page; aborting

brcmnand_read_byte()'s NAND_CMD_PARAM handler assumes the in-memory
cache of the NAND controller's FLASH_CACHE registers is big-endian.
But the iproc_nand driver forces little-endian APB bus transfers,
so the in-memory cache ends up exactly backwards.

The solution is to swap flash_cache byte order before extracting
bytes from it. NAND_CMD_PARAM is not an oft-used command, so we
don't need to worry about the overhead of byte swaps here.

Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
Signed-off-by: Clay McClure <clay@daemons.net>
---
 drivers/mtd/nand/brcmnand/brcmnand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index 7c1c306..932bc49 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -1228,7 +1228,7 @@ static uint8_t brcmnand_read_byte(struct mtd_info *mtd)
 		if (host->last_byte > 0 && offs == 0)
 			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, addr, -1);
 
-		ret = ctrl->flash_cache[offs >> 2] >>
+		ret = __cpu_to_be32(ctrl->flash_cache[offs >> 2]) >>
 					(24 - ((offs & 0x03) << 3));
 		break;
 	case NAND_CMD_GET_FEATURES:
-- 
2.1.4

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Brian Norris]

+ others

On Fri, Oct 23, 2015 at 12:47:06PM -0700, Clay McClure wrote:
> On systems using Broadcom Cortex-A9 SoCs (BCM585XX, BCM586XX, BCM56340),
> nand_flash_detect_onfi() fails at boot with:
> 
>     Could not find valid ONFI parameter page; aborting
> 
> brcmnand_read_byte()'s NAND_CMD_PARAM handler assumes the in-memory
> cache of the NAND controller's FLASH_CACHE registers is big-endian.
> But the iproc_nand driver forces little-endian APB bus transfers,
> so the in-memory cache ends up exactly backwards.
> 
> The solution is to swap flash_cache byte order before extracting
> bytes from it. NAND_CMD_PARAM is not an oft-used command, so we
> don't need to worry about the overhead of byte swaps here.
> 
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> Signed-off-by: Clay McClure <clay@daemons.net>
> ---
>  drivers/mtd/nand/brcmnand/brcmnand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index 7c1c306..932bc49 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -1228,7 +1228,7 @@ static uint8_t brcmnand_read_byte(struct mtd_info *mtd)
>  		if (host->last_byte > 0 && offs == 0)
>  			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, addr, -1);
>  
> -		ret = ctrl->flash_cache[offs >> 2] >>
> +		ret = __cpu_to_be32(ctrl->flash_cache[offs >> 2]) >>

This is most definitely wrong, as it breaks all the other little endian
systems that are using this. Not to say that the original code is
pretty... (It really should be cleaned up a bit.)

Anyway, since looks like you're using iproc_nand, I'd solicit Scott and
Ray's opinions on what's really wrong here. Have you guys tested ONFI
paramater pages for Cygnus?

Brian

>  					(24 - ((offs & 0x03) << 3));
>  		break;
>  	case NAND_CMD_GET_FEATURES:
> -- 
> 2.1.4
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Ray Jui]

On 10/23/2015 4:27 PM, Brian Norris wrote:
> + others
>
> On Fri, Oct 23, 2015 at 12:47:06PM -0700, Clay McClure wrote:
>> On systems using Broadcom Cortex-A9 SoCs (BCM585XX, BCM586XX, BCM56340),
>> nand_flash_detect_onfi() fails at boot with:
>>
>>      Could not find valid ONFI parameter page; aborting
>>
>> brcmnand_read_byte()'s NAND_CMD_PARAM handler assumes the in-memory
>> cache of the NAND controller's FLASH_CACHE registers is big-endian.
>> But the iproc_nand driver forces little-endian APB bus transfers,
>> so the in-memory cache ends up exactly backwards.
>>
>> The solution is to swap flash_cache byte order before extracting
>> bytes from it. NAND_CMD_PARAM is not an oft-used command, so we
>> don't need to worry about the overhead of byte swaps here.
>>
>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
>> Signed-off-by: Clay McClure <clay@daemons.net>
>> ---
>>   drivers/mtd/nand/brcmnand/brcmnand.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
>> index 7c1c306..932bc49 100644
>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
>> @@ -1228,7 +1228,7 @@ static uint8_t brcmnand_read_byte(struct mtd_info *mtd)
>>   		if (host->last_byte > 0 && offs == 0)
>>   			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, addr, -1);
>>
>> -		ret = ctrl->flash_cache[offs >> 2] >>
>> +		ret = __cpu_to_be32(ctrl->flash_cache[offs >> 2]) >>
>
> This is most definitely wrong, as it breaks all the other little endian
> systems that are using this. Not to say that the original code is
> pretty... (It really should be cleaned up a bit.)
>
> Anyway, since looks like you're using iproc_nand, I'd solicit Scott and
> Ray's opinions on what's really wrong here. Have you guys tested ONFI
> paramater pages for Cygnus?
>
> Brian
>

No, we have not tested ONFI parameter pages on Cygnus. And I believe 
this is observed on NSP instead of Cygnus? It looks like Cygnus and 
other iProc chips should have the same issue though.

According to the commit message, it looks like the original logic is 
making the assumption that data in the flash cache buffer is in BE 
format? I thought the raw NAND data in is expected to be in LE format, 
and data stored in the flash cache buffer here would be in the native 
CPU endian format since __raw_readl was used?

If so, then should the logic here be the following?

ret = cpu_to_le32(ctrl->flash_cache[offs >> 2]) >> ((offs & 0x03) << 3);

>>   					(24 - ((offs & 0x03) << 3));
>>   		break;
>>   	case NAND_CMD_GET_FEATURES:
>> --
>> 2.1.4
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Brian Norris]

On Fri, Oct 23, 2015 at 05:48:11PM -0700, Ray Jui wrote:
> On 10/23/2015 4:27 PM, Brian Norris wrote:
> >On Fri, Oct 23, 2015 at 12:47:06PM -0700, Clay McClure wrote:
> >>On systems using Broadcom Cortex-A9 SoCs (BCM585XX, BCM586XX, BCM56340),
> >>nand_flash_detect_onfi() fails at boot with:
> >>
> >>     Could not find valid ONFI parameter page; aborting
> >>
> >>brcmnand_read_byte()'s NAND_CMD_PARAM handler assumes the in-memory
> >>cache of the NAND controller's FLASH_CACHE registers is big-endian.
> >>But the iproc_nand driver forces little-endian APB bus transfers,
> >>so the in-memory cache ends up exactly backwards.
> >>
> >>The solution is to swap flash_cache byte order before extracting
> >>bytes from it. NAND_CMD_PARAM is not an oft-used command, so we
> >>don't need to worry about the overhead of byte swaps here.
> >>
> >>Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> >>Signed-off-by: Clay McClure <clay@daemons.net>
> >>---
> >>  drivers/mtd/nand/brcmnand/brcmnand.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> >>index 7c1c306..932bc49 100644
> >>--- a/drivers/mtd/nand/brcmnand/brcmnand.c
> >>+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> >>@@ -1228,7 +1228,7 @@ static uint8_t brcmnand_read_byte(struct mtd_info *mtd)
> >>  		if (host->last_byte > 0 && offs == 0)
> >>  			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, addr, -1);
> >>
> >>-		ret = ctrl->flash_cache[offs >> 2] >>
> >>+		ret = __cpu_to_be32(ctrl->flash_cache[offs >> 2]) >>
> >
> >This is most definitely wrong, as it breaks all the other little endian
> >systems that are using this. Not to say that the original code is
> >pretty... (It really should be cleaned up a bit.)
> >
> >Anyway, since looks like you're using iproc_nand, I'd solicit Scott and
> >Ray's opinions on what's really wrong here. Have you guys tested ONFI
> >paramater pages for Cygnus?
> >
> >Brian
> >
> 
> No, we have not tested ONFI parameter pages on Cygnus. And I believe
> this is observed on NSP instead of Cygnus? It looks like Cygnus and
> other iProc chips should have the same issue though.

OK, that's why I was asking. I expected that if Clay is messing with the
APB control, then you'd have the same problem.

> According to the commit message, it looks like the original logic is
> making the assumption that data in the flash cache buffer is in BE
> format?

Right, the current logic is treating it like big endian. All I can say
is that it works for a large class of little endian MIPS and ARM brcmstb
systems that I (used to) maintain, so I guess the cached data *is* in a
big endian format on those systems.

Really, I'm not too surprised that the behavior would be context
dependent, depending on the type of command sent to the controller. I
can easily imagine the hardware designers have the PARAMATER_READ path
looking different than the data path.

> I thought the raw NAND data in is expected to be in LE
> format, and data stored in the flash cache buffer here would be in
> the native CPU endian format since __raw_readl was used?
> 
> If so, then should the logic here be the following?
> 
> ret = cpu_to_le32(ctrl->flash_cache[offs >> 2]) >> ((offs & 0x03) << 3);

I guess so, but that's getting even uglier, not prettier. I have a
different patch locally that I can post to help clean it up.

But to be clear: none of the systems in question are running with big
endian CPUs, correct? If not, then it's apparent we have to do something
different for iProc-based chips than for STB chips; we can't just fiddle
with the cpu_to_*() macros. Maybe this works?

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index 961a9ee4369c..073dbe4ce9bc 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -1168,8 +1168,6 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd, unsigned command,
 			native_cmd == CMD_PARAMETER_CHANGE_COL) {
 		int i;
 
-		brcmnand_soc_data_bus_prepare(ctrl->soc);
-
 		/*
 		 * Must cache the FLASH_CACHE now, since changes in
 		 * SECTOR_SIZE_1K may invalidate it
@@ -1177,8 +1175,6 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd, unsigned command,
 		for (i = 0; i < FC_WORDS; i++)
 			ctrl->flash_cache[i] = brcmnand_read_fc(ctrl, i);
 
-		brcmnand_soc_data_bus_unprepare(ctrl->soc);
-
 		/* Cleanup from HW quirk: restore SECTOR_SIZE_1K */
 		if (host->hwcfg.sector_size_1k)
 			brcmnand_set_sector_size_1k(host,


Brian

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Brian Norris]

On Fri, Oct 23, 2015 at 06:12:19PM -0700, Brian Norris wrote:
> On Fri, Oct 23, 2015 at 05:48:11PM -0700, Ray Jui wrote:
> > If so, then should the logic here be the following?
> > 
> > ret = cpu_to_le32(ctrl->flash_cache[offs >> 2]) >> ((offs & 0x03) << 3);
> 
> I guess so, but that's getting even uglier, not prettier.

Sorry, I misread. No, that will break brcmstb too. The logic as-is is
tested and works for little endian BCM7xxx MIPS and ARM, regardless of
theory. You'll need to figure out the quirks of why/how it's really
*supposed* to work (and then to write good code around that) to get more
platforms to work with it.

Brian

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Brian Norris]

On Sat, Oct 24, 2015 at 11:35:11AM -0700, Clay McClure wrote:
> On Friday, October 23, 2015, Brian Norris <computersforpeace@gmail.com> wrote:
> 
> > But to be clear: none of the systems in question are running with big
> > endian CPUs, correct? If not, then it's apparent we have to do something
> > different for iProc-based chips than for STB chips; we can't just fiddle
> > with the cpu_to_*() macros. Maybe this works?
> >
> > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c
> > b/drivers/mtd/nand/brcmnand/brcmnand.c
> > index 9ü61a9ee4369c..073dbe4ce9bc 100644
> > --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> > @@ -1168,8 +1168,6 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd,
> > unsigned command,
> >                         native_cmd == CMD_PARAMETER_CHANGE_COL) {
> >                 int i;
> >
> > -               brcmnand_soc_data_bus_prepare(ctrl->soc);
> > -
> >                 /*
> >                  * Must cache the FLASH_CACHE now, since changes in
> >                  * SECTOR_SIZE_1K may invalidate it
> > @@ -1177,8 +1175,6 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd,
> > unsigned command,
> >                 for (i = 0; i < FC_WORDS; i++)
> >                         ctrl->flash_cache[i] = brcmnand_read_fc(ctrl, i);
> >
> > -               brcmnand_soc_data_bus_unprepare(ctrl->soc);
> > -
> >                 /* Cleanup from HW quirk: restore SECTOR_SIZE_1K */
> >                 if (host->hwcfg.sector_size_1k)
> >                         brcmnand_set_sector_size_1k(host,
> >
> 
> Yes, that does solve the problem I'm seeing. I actually tried
> this approach before submitting my previous patch, but opted not to submit
> this because it seemed that some thought and effort had gone into making
> little-endian APB transfers (it's one of the only things iproc_nand
> actually does), and I assumed it was being done for a reason.
> 
> If you're happy with this, would you mind if I submit this patch?

Feel free, though I suppose technically it'd have my authorship. So for
the above:

Signed-off-by: Brian Norris <computersforpeace@gmail.com>

I'd like an ack (or test) from Ray too, if possible. Perhaps a comment
is in order too, to explain why there is no bus swapping for that
instance of brcmnand_read_fc(). (AIUI, the reason is that the base STB
design pushes out this data in big endian, so we need to match this on
iProc.)

> Also, you mentioned that the original code could stand to be cleaned up a
> bit. Any specific clean ups you'd like to see?

I wrote up this patch, but it's untested. If you can test this in
addition with the above, that'd be great. I'd also like a test from the
STB folks, if possible.

Brian

----8<----
>From 8efc0791d4dbb4a326e2223b14aeae04933ee9af Mon Sep 17 00:00:00 2001
From: Brian Norris <computersforpeace@gmail.com>
Date: Fri, 23 Oct 2015 17:39:40 -0700
Subject: [PATCH] mtd: brcmnand: clean up flash cache for parameter pages

The read_byte() handling for accessing the flash cache has some awkward
swapping being done in the read_byte() function. Let's just make this a
byte array, and do the swapping with the word-level macros during the
initial buffer copy.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Clay McClure <clay@daemons.net>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
---
 drivers/mtd/nand/brcmnand/brcmnand.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index d694d876631e..249b3728dc75 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -134,7 +134,7 @@ struct brcmnand_controller {
 	dma_addr_t		dma_pa;
 
 	/* in-memory cache of the FLASH_CACHE, used only for some commands */
-	u32			flash_cache[FC_WORDS];
+	u8			flash_cache[FC_BYTES];
 
 	/* Controller revision details */
 	const u16		*reg_offsets;
@@ -1166,6 +1166,8 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd, unsigned command,
 
 	if (native_cmd == CMD_PARAMETER_READ ||
 			native_cmd == CMD_PARAMETER_CHANGE_COL) {
+		/* Copy flash cache word-wise */
+		u32 *flash_cache = (u32 *)ctrl->flash_cache;
 		int i;
 
 		brcmnand_soc_data_bus_prepare(ctrl->soc);
@@ -1175,7 +1177,8 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd, unsigned command,
 		 * SECTOR_SIZE_1K may invalidate it
 		 */
 		for (i = 0; i < FC_WORDS; i++)
-			ctrl->flash_cache[i] = brcmnand_read_fc(ctrl, i);
+			/* cache is big endian for parameter pages? */
+			flash_cache[i] = be32_to_cpu(brcmnand_read_fc(ctrl, i));
 
 		brcmnand_soc_data_bus_unprepare(ctrl->soc);
 
@@ -1228,8 +1231,7 @@ static uint8_t brcmnand_read_byte(struct mtd_info *mtd)
 		if (host->last_byte > 0 && offs == 0)
 			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, addr, -1);
 
-		ret = ctrl->flash_cache[offs >> 2] >>
-					(24 - ((offs & 0x03) << 3));
+		ret = ctrl->flash_cache[offs];
 		break;
 	case NAND_CMD_GET_FEATURES:
 		if (host->last_byte >= ONFI_SUBFEATURE_PARAM_LEN) {
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Ray Jui]

Hi Brian,

On 10/26/2015 11:14 AM, Brian Norris wrote:
> On Sat, Oct 24, 2015 at 11:35:11AM -0700, Clay McClure wrote:
>> On Friday, October 23, 2015, Brian Norris <computersforpeace@gmail.com> wrote:
>>
>>> But to be clear: none of the systems in question are running with big
>>> endian CPUs, correct? If not, then it's apparent we have to do something
>>> different for iProc-based chips than for STB chips; we can't just fiddle
>>> with the cpu_to_*() macros. Maybe this works?
>>>
>>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c
>>> b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> index 9ü61a9ee4369c..073dbe4ce9bc 100644
>>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
>>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> @@ -1168,8 +1168,6 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd,
>>> unsigned command,
>>>                          native_cmd == CMD_PARAMETER_CHANGE_COL) {
>>>                  int i;
>>>
>>> -               brcmnand_soc_data_bus_prepare(ctrl->soc);
>>> -
>>>                  /*
>>>                   * Must cache the FLASH_CACHE now, since changes in
>>>                   * SECTOR_SIZE_1K may invalidate it
>>> @@ -1177,8 +1175,6 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd,
>>> unsigned command,
>>>                  for (i = 0; i < FC_WORDS; i++)
>>>                          ctrl->flash_cache[i] = brcmnand_read_fc(ctrl, i);
>>>
>>> -               brcmnand_soc_data_bus_unprepare(ctrl->soc);
>>> -
>>>                  /* Cleanup from HW quirk: restore SECTOR_SIZE_1K */
>>>                  if (host->hwcfg.sector_size_1k)
>>>                          brcmnand_set_sector_size_1k(host,
>>>
>>
>> Yes, that does solve the problem I'm seeing. I actually tried
>> this approach before submitting my previous patch, but opted not to submit
>> this because it seemed that some thought and effort had gone into making
>> little-endian APB transfers (it's one of the only things iproc_nand
>> actually does), and I assumed it was being done for a reason.
>>
>> If you're happy with this, would you mind if I submit this patch?
>
> Feel free, though I suppose technically it'd have my authorship. So for
> the above:
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>
> I'd like an ack (or test) from Ray too, if possible. Perhaps a comment
> is in order too, to explain why there is no bus swapping for that
> instance of brcmnand_read_fc(). (AIUI, the reason is that the base STB
> design pushes out this data in big endian, so we need to match this on
> iProc.)

I guess I still need to get this straight in my brain and I have a few 
questions here:

1) I remember we had some other discussions related to this before, 
regarding the NAND data endianness. We confirmed that the raw NAND data 
programmed with a standard flasher is in LE format. And this is why we 
need to configure apb bus to LE before accessing these data/cache registers?

2) Are we saying here that, by standard, the ONFI parameters should be 
in BE format as opposed to the raw NAND data format, and therefore no 
apb bus config to LE should be done? Or, your current logic seems to do 
the following:

1) still configure the APB to LE before access data/cache registers
2) do a be32_to_cpu, which in effect causes a byte swap on ARM CPU 
running in LE (our current case). If data read from APB is already in 
LE, this makes it become BE
3) return directly from the byte location of the buffer memory, instead 
of doing a 32-bit based read and doing some arithmetic, to get the byte data

Thanks,

Ray

>
>> Also, you mentioned that the original code could stand to be cleaned up a
>> bit. Any specific clean ups you'd like to see?
>
> I wrote up this patch, but it's untested. If you can test this in
> addition with the above, that'd be great. I'd also like a test from the
> STB folks, if possible.
>
> Brian
>
> ----8<----
>  From 8efc0791d4dbb4a326e2223b14aeae04933ee9af Mon Sep 17 00:00:00 2001
> From: Brian Norris <computersforpeace@gmail.com>
> Date: Fri, 23 Oct 2015 17:39:40 -0700
> Subject: [PATCH] mtd: brcmnand: clean up flash cache for parameter pages
>
> The read_byte() handling for accessing the flash cache has some awkward
> swapping being done in the read_byte() function. Let's just make this a
> byte array, and do the swapping with the word-level macros during the
> initial buffer copy.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Clay McClure <clay@daemons.net>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> ---
>   drivers/mtd/nand/brcmnand/brcmnand.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index d694d876631e..249b3728dc75 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -134,7 +134,7 @@ struct brcmnand_controller {
>   	dma_addr_t		dma_pa;
>
>   	/* in-memory cache of the FLASH_CACHE, used only for some commands */
> -	u32			flash_cache[FC_WORDS];
> +	u8			flash_cache[FC_BYTES];
>
>   	/* Controller revision details */
>   	const u16		*reg_offsets;
> @@ -1166,6 +1166,8 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd, unsigned command,
>
>   	if (native_cmd == CMD_PARAMETER_READ ||
>   			native_cmd == CMD_PARAMETER_CHANGE_COL) {
> +		/* Copy flash cache word-wise */
> +		u32 *flash_cache = (u32 *)ctrl->flash_cache;
>   		int i;
>
>   		brcmnand_soc_data_bus_prepare(ctrl->soc);
> @@ -1175,7 +1177,8 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd, unsigned command,
>   		 * SECTOR_SIZE_1K may invalidate it
>   		 */
>   		for (i = 0; i < FC_WORDS; i++)
> -			ctrl->flash_cache[i] = brcmnand_read_fc(ctrl, i);
> +			/* cache is big endian for parameter pages? */
> +			flash_cache[i] = be32_to_cpu(brcmnand_read_fc(ctrl, i));
>
>   		brcmnand_soc_data_bus_unprepare(ctrl->soc);
>
> @@ -1228,8 +1231,7 @@ static uint8_t brcmnand_read_byte(struct mtd_info *mtd)
>   		if (host->last_byte > 0 && offs == 0)
>   			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, addr, -1);
>
> -		ret = ctrl->flash_cache[offs >> 2] >>
> -					(24 - ((offs & 0x03) << 3));
> +		ret = ctrl->flash_cache[offs];
>   		break;
>   	case NAND_CMD_GET_FEATURES:
>   		if (host->last_byte >= ONFI_SUBFEATURE_PARAM_LEN) {
>

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Clay McClure]

On Mon, Oct 26, 2015 at 11:52 AM, Ray Jui <rjui@broadcom.com> wrote:

> I guess I still need to get this straight in my brain and I have a few
> questions here:
>
> 1) I remember we had some other discussions related to this before,
> regarding the NAND data endianness. We confirmed that the raw NAND data
> programmed with a standard flasher is in LE format. And this is why we need
> to configure apb bus to LE before accessing these data/cache registers?

According to the ONFI specifications[1], the parameter page is little-endian:

"For parameters that span multiple bytes, the least significant byte
of the parameter corresponds to the first byte."

I'm not sure if that's what drove the decision to use LE APB
transfers, but it does simplify the code to use the same byte ordering
on the bus as in the CPU.

> 2) Are we saying here that, by standard, the ONFI parameters should be in BE
> format as opposed to the raw NAND data format, and therefore no apb bus
> config to LE should be done? Or, your current logic seems to do the
> following:
>
> 1) still configure the APB to LE before access data/cache registers
> 2) do a be32_to_cpu, which in effect causes a byte swap on ARM CPU running
> in LE (our current case). If data read from APB is already in LE, this makes
> it become BE
> 3) return directly from the byte location of the buffer memory, instead of
> doing a 32-bit based read and doing some arithmetic, to get the byte data

The bug in read_byte() is due to the fact that it currently assumes
the host CPU byte order is big-endian (i.e., it always extracts byte 0
from the high bits of the word). If we continue to use LE APB
transfers, then I think we can exclude the be32_to_cpu() call from
Brian's patch. Converting flash_cache to a byte array I believe is all
we need to solve the problem with nand_flash_detect_onfi(). That
function already includes the necessary byte-order conversions for
big-endian machines.

Brian, I'll test a variation of this patch on our iProc-based
platforms and report back.

Thanks,

Clay

[1]: http://www.onfi.org/specifications

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Brian Norris]

Hi,

I'm not sure I can properly answer all of Ray's questions, but I'll try
to come back to them. There is at least one wrong statement here to
correct, and perhaps that can help inform Ray before we return to his
questions.

On Mon, Oct 26, 2015 at 01:08:41PM -0700, Clay McClure wrote:
> The bug in read_byte() is due to the fact that it currently assumes
> the host CPU byte order is big-endian (i.e., it always extracts byte 0
> from the high bits of the word).

No, the bug isn't really in read_byte() specifically, and it definitely
isn't about the CPU being big-endian. As I said before, this is
primarily tested on *little* endian MIPS and ARM STB chips.

It appears that on these SoCs, at least, somehow (I have to wave my
hands here; I really don't know why; I can only presume poor HW design)
the PARAMETER_READ command fills the flash cache with big endian -like
data. So when I looked at the parameter page in a register viewer, I'd
see data like this:

FLASH_CACHE[0] = 0x4f4e4649  (i.e., "ONFI")
FLASH_CAHCE[1] = ....

That is, byte[0] is 0x49 ("I"), byte[1] is 0x46 ("F"), byte[2] is 0x4e
("N"), byte[3] is 0x4f ("O"), and so on. That means this data needs to
be byte-swapped before we can push it out of the brcmnand driver. There
are no sideband toggles to reconfigure buses or anything, so the driver
must do the swap on its own. Currently, that is done via ugly word
shifting, but it can be more clearly done with this patch [1]. Note that
this patch [1] just a simple refactoring and does not actually fix
anything. It is just to clarify the logic.

(Incidentally, I believe the above is all correct even for STB
big-endian MIPS too, but I haven't tested.)

> If we continue to use LE APB
> transfers, then I think we can exclude the be32_to_cpu() call from
> Brian's patch.

I don't believe we can safely drop the be32_to_cpu(). That is just a
refactoring of the existing logic. Please stop suggesting ways to break
STB SoCs.

> Converting flash_cache to a byte array I believe is all
> we need to solve the problem with nand_flash_detect_onfi(). That
> function already includes the necessary byte-order conversions for
> big-endian machines.

How we pull data out of our NAND controller core is completely
orthogonal to the data structure contained within that data. i.e., it
has nothing to do with ONFI standards and nand_flash_detect_onfi()'s
byte swapping. It just means we need to get (the right) byte[0] into
byte[0], byte[1] into byte[1], etc.

Thus, the byte-order conversions in nand_flash_detect_onfi() shouldn't
really have much to do with this conversation. They just make sure that
after *we* pull the data out correctly, nand_base interprets it
correctly.

> Brian, I'll test a variation of this patch on our iProc-based
> platforms and report back.

OK, getting testing feedback from an iProc-based system using ONFI would
be somewhat illuminating.

Brian

> Thanks,
> 
> Clay

[1] http://patchwork.ozlabs.org/patch/536148/

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Ray Jui]

Hi Brian,

On 10/27/2015 1:40 PM, Brian Norris wrote:
> Hi,
>
> I'm not sure I can properly answer all of Ray's questions, but I'll try
> to come back to them. There is at least one wrong statement here to
> correct, and perhaps that can help inform Ray before we return to his
> questions.
>
> On Mon, Oct 26, 2015 at 01:08:41PM -0700, Clay McClure wrote:
>> The bug in read_byte() is due to the fact that it currently assumes
>> the host CPU byte order is big-endian (i.e., it always extracts byte 0
>> from the high bits of the word).
>
> No, the bug isn't really in read_byte() specifically, and it definitely
> isn't about the CPU being big-endian. As I said before, this is
> primarily tested on *little* endian MIPS and ARM STB chips.
>
> It appears that on these SoCs, at least, somehow (I have to wave my
> hands here; I really don't know why; I can only presume poor HW design)
> the PARAMETER_READ command fills the flash cache with big endian -like
> data. So when I looked at the parameter page in a register viewer, I'd
> see data like this:
>
> FLASH_CACHE[0] = 0x4f4e4649  (i.e., "ONFI")
> FLASH_CAHCE[1] = ....
>
> That is, byte[0] is 0x49 ("I"), byte[1] is 0x46 ("F"), byte[2] is 0x4e
> ("N"), byte[3] is 0x4f ("O"), and so on. That means this data needs to
> be byte-swapped before we can push it out of the brcmnand driver. There
> are no sideband toggles to reconfigure buses or anything, so the driver
> must do the swap on its own. Currently, that is done via ugly word
> shifting, but it can be more clearly done with this patch [1]. Note that
> this patch [1] just a simple refactoring and does not actually fix
> anything. It is just to clarify the logic.
>
> (Incidentally, I believe the above is all correct even for STB
> big-endian MIPS too, but I haven't tested.)
>

Don't you think this may have something to do with the APB bus 
endianness setting that's not used for all non-iProc SoCs? I suspect not 
just the ONFI paramter page, but all other NAND data read into 
FLASH_CACHE are in BE format on those SoCs (or am I completely out of my 
mind and missing something here?)

That is why the original code (before this change) did not work for 
iProc based SoCs. Because the APB bus has been configured to LE before 
accessing the flash cache registers for iProc SoCs, data read from NAND, 
through APB, to DDR, are in LE. But the driver logic is obviously 
assuming that data is in BE. This may explain why it did not work for 
iProc SoCs?

>> If we continue to use LE APB
>> transfers, then I think we can exclude the be32_to_cpu() call from
>> Brian's patch.
>
> I don't believe we can safely drop the be32_to_cpu(). That is just a
> refactoring of the existing logic. Please stop suggesting ways to break
> STB SoCs.
>
>> Converting flash_cache to a byte array I believe is all
>> we need to solve the problem with nand_flash_detect_onfi(). That
>> function already includes the necessary byte-order conversions for
>> big-endian machines.
>
> How we pull data out of our NAND controller core is completely
> orthogonal to the data structure contained within that data. i.e., it
> has nothing to do with ONFI standards and nand_flash_detect_onfi()'s
> byte swapping. It just means we need to get (the right) byte[0] into
> byte[0], byte[1] into byte[1], etc.
>
> Thus, the byte-order conversions in nand_flash_detect_onfi() shouldn't
> really have much to do with this conversation. They just make sure that
> after *we* pull the data out correctly, nand_base interprets it
> correctly.
>
>> Brian, I'll test a variation of this patch on our iProc-based
>> platforms and report back.
>
> OK, getting testing feedback from an iProc-based system using ONFI would
> be somewhat illuminating.
>
> Brian
>
>> Thanks,
>>
>> Clay
>
> [1] http://patchwork.ozlabs.org/patch/536148/
>

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Clay McClure]

On Tue, Oct 27, 2015 at 1:55 PM, Ray Jui <rjui@broadcom.com> wrote:

> Don't you think this may have something to do with the APB bus endianness
> setting that's not used for all non-iProc SoCs? I suspect not just the ONFI
> paramter page, but all other NAND data read into FLASH_CACHE are in BE
> format on those SoCs (or am I completely out of my mind and missing
> something here?)

It's a good question (whether all flash cache data is big-endian), and
I don't know the answer, but I've verified that the below patch does
fix the problem with PARAMETER_READ on the Helix4 (an iProc-based
switch chip). Whether the other uses of
brcmnand_soc_data_bus_prepare() should also be removed, I can't say; I
don't know why they were originally needed.

Clay

----8<----
>From c0ab411165ceedc5afc5a2c49c4d1bf9ba035dc8 Mon Sep 17 00:00:00 2001
From: Clay McClure <clay@daemons.net>
Date: Mon, 2 Nov 2015 13:07:59 -0800
Subject: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM on iProc

On Broadcom's iProc-based SoCs, nand_flash_detect_onfi() fails at
boot with:

    Could not find valid ONFI parameter page; aborting

The endianness of the in-memory parameter page cache is backwards,
causing the ONFI CRC validation to fail. It seems that the NAND
controllers on these chips handle PARAMETER_READ commands differently
from other commands, filling the flash cache with big endian-like
data. Because of that quirk, we don't need to implicitly swap bytes
on the APB bus while reading the flash cache for parameter pages.

Signed-off-by: Clay McClure <clay@daemons.net>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/brcmnand/brcmnand.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c
b/drivers/mtd/nand/brcmnand/brcmnand.c
index 12c6190..36a676d 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -1190,8 +1190,6 @@ static void brcmnand_cmdfunc(struct mtd_info
*mtd, unsigned command,
  native_cmd == CMD_PARAMETER_CHANGE_COL) {
  int i;

- brcmnand_soc_data_bus_prepare(ctrl->soc);
-
  /*
  * Must cache the FLASH_CACHE now, since changes in
  * SECTOR_SIZE_1K may invalidate it
@@ -1199,8 +1197,6 @@ static void brcmnand_cmdfunc(struct mtd_info
*mtd, unsigned command,
  for (i = 0; i < FC_WORDS; i++)
  ctrl->flash_cache[i] = brcmnand_read_fc(ctrl, i);

- brcmnand_soc_data_bus_unprepare(ctrl->soc);
-
  /* Cleanup from HW quirk: restore SECTOR_SIZE_1K */
  if (host->hwcfg.sector_size_1k)
  brcmnand_set_sector_size_1k(host,
-- 
2.1.4

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Brian Norris]

On Tue, Oct 27, 2015 at 01:55:32PM -0700, Ray Jui wrote:
> On 10/27/2015 1:40 PM, Brian Norris wrote:
> >On Mon, Oct 26, 2015 at 01:08:41PM -0700, Clay McClure wrote:
> >>The bug in read_byte() is due to the fact that it currently assumes
> >>the host CPU byte order is big-endian (i.e., it always extracts byte 0
> >>from the high bits of the word).
> >
> >No, the bug isn't really in read_byte() specifically, and it definitely
> >isn't about the CPU being big-endian. As I said before, this is
> >primarily tested on *little* endian MIPS and ARM STB chips.
> >
> >It appears that on these SoCs, at least, somehow (I have to wave my
> >hands here; I really don't know why; I can only presume poor HW design)
> >the PARAMETER_READ command fills the flash cache with big endian -like
> >data. So when I looked at the parameter page in a register viewer, I'd
> >see data like this:
> >
> >FLASH_CACHE[0] = 0x4f4e4649  (i.e., "ONFI")
> >FLASH_CAHCE[1] = ....
> >
> >That is, byte[0] is 0x49 ("I"), byte[1] is 0x46 ("F"), byte[2] is 0x4e
> >("N"), byte[3] is 0x4f ("O"), and so on. That means this data needs to
> >be byte-swapped before we can push it out of the brcmnand driver. There
> >are no sideband toggles to reconfigure buses or anything, so the driver
> >must do the swap on its own. Currently, that is done via ugly word
> >shifting, but it can be more clearly done with this patch [1]. Note that
> >this patch [1] just a simple refactoring and does not actually fix
> >anything. It is just to clarify the logic.
> >
> >(Incidentally, I believe the above is all correct even for STB
> >big-endian MIPS too, but I haven't tested.)
> >
> 
> Don't you think this may have something to do with the APB bus
> endianness setting that's not used for all non-iProc SoCs? I suspect
> not just the ONFI paramter page, but all other NAND data read into
> FLASH_CACHE are in BE format on those SoCs (or am I completely out
> of my mind and missing something here?)

No, I don't think the NAND data in FLASH_CACHE is always in BE format.
If you look at brcmnand_read_by_pio() and consider the LE STB case
(i.e., no APB swapping to configure), then data is copied word-wise
(with no swapping) into a byte array (note the cast in
brcmnand_read_page(), for instance).

So in the read page case, the FLASH_CACHE is currently treated as native
endianness -- i.e., "little endian".

Now, I don't think any of this means that you're out of your mind. I can
definitively claim that I do *not* know what's going on here. I expect
that this is some convoluted concoction of a disorganized and
distributed (across different business units) "design" of a NAND
controller.

I think you need to consult with:
(1) real hardware for all affected cases (i.e., both STB SoCs and iProc,
    for starters) and
(2) the core designers. (At a minimum, I think they should answer for
    why ONFI parameter pages show up in big endian on STB chips. But
    there are definitely more questions than that.)

As I am no longer at Broadcom, I don't have access to my usual hardware
for (1), and I definitely don't have access to (2).

Incidentally, I'm not sure it's best I'm the (sole) maintainer for
drivers/mtd/nand/brcmnand/ any more, given my lack of hardware. (I could
probably acquire consumer BRCM-based routers to hack on, but that's not
quite my bread and butter, nor the source of this particular thorn.) I'm
open to other additions to MAINTAINERS, and specifically someone who can
answer for STB SoCs, where this mess all originated from. Florian?
Kamal?

Brian

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Ray Jui]

On 11/3/2015 6:04 PM, Brian Norris wrote:
> On Tue, Oct 27, 2015 at 01:55:32PM -0700, Ray Jui wrote:
>> On 10/27/2015 1:40 PM, Brian Norris wrote:
>>> On Mon, Oct 26, 2015 at 01:08:41PM -0700, Clay McClure wrote:
>>>> The bug in read_byte() is due to the fact that it currently assumes
>>>> the host CPU byte order is big-endian (i.e., it always extracts byte 0
>>> >from the high bits of the word).
>>>
>>> No, the bug isn't really in read_byte() specifically, and it definitely
>>> isn't about the CPU being big-endian. As I said before, this is
>>> primarily tested on *little* endian MIPS and ARM STB chips.
>>>
>>> It appears that on these SoCs, at least, somehow (I have to wave my
>>> hands here; I really don't know why; I can only presume poor HW design)
>>> the PARAMETER_READ command fills the flash cache with big endian -like
>>> data. So when I looked at the parameter page in a register viewer, I'd
>>> see data like this:
>>>
>>> FLASH_CACHE[0] = 0x4f4e4649  (i.e., "ONFI")
>>> FLASH_CAHCE[1] = ....
>>>
>>> That is, byte[0] is 0x49 ("I"), byte[1] is 0x46 ("F"), byte[2] is 0x4e
>>> ("N"), byte[3] is 0x4f ("O"), and so on. That means this data needs to
>>> be byte-swapped before we can push it out of the brcmnand driver. There
>>> are no sideband toggles to reconfigure buses or anything, so the driver
>>> must do the swap on its own. Currently, that is done via ugly word
>>> shifting, but it can be more clearly done with this patch [1]. Note that
>>> this patch [1] just a simple refactoring and does not actually fix
>>> anything. It is just to clarify the logic.
>>>
>>> (Incidentally, I believe the above is all correct even for STB
>>> big-endian MIPS too, but I haven't tested.)
>>>
>>
>> Don't you think this may have something to do with the APB bus
>> endianness setting that's not used for all non-iProc SoCs? I suspect
>> not just the ONFI paramter page, but all other NAND data read into
>> FLASH_CACHE are in BE format on those SoCs (or am I completely out
>> of my mind and missing something here?)
>
> No, I don't think the NAND data in FLASH_CACHE is always in BE format.
> If you look at brcmnand_read_by_pio() and consider the LE STB case
> (i.e., no APB swapping to configure), then data is copied word-wise
> (with no swapping) into a byte array (note the cast in
> brcmnand_read_page(), for instance).
>
> So in the read page case, the FLASH_CACHE is currently treated as native
> endianness -- i.e., "little endian".
>
> Now, I don't think any of this means that you're out of your mind. I can
> definitively claim that I do *not* know what's going on here. I expect
> that this is some convoluted concoction of a disorganized and
> distributed (across different business units) "design" of a NAND
> controller.
>
> I think you need to consult with:
> (1) real hardware for all affected cases (i.e., both STB SoCs and iProc,
>      for starters) and
> (2) the core designers. (At a minimum, I think they should answer for
>      why ONFI parameter pages show up in big endian on STB chips. But
>      there are definitely more questions than that.)
>
> As I am no longer at Broadcom, I don't have access to my usual hardware
> for (1), and I definitely don't have access to (2).
>

Yes, I agree with you that this should really be sorted out with the 
ASIC designers. We really need to understand the effect of the APB bus 
endianness configuration at different levels.

I'm not currently working on NAND for the next-gen of iProc SoCs. But I 
should be able to identify the ASIC designer and start up an email 
thread on this subject with developers working on NAND for iProc SoCs 
included.

Thanks,

Ray

> Incidentally, I'm not sure it's best I'm the (sole) maintainer for
> drivers/mtd/nand/brcmnand/ any more, given my lack of hardware. (I could
> probably acquire consumer BRCM-based routers to hack on, but that's not
> quite my bread and butter, nor the source of this particular thorn.) I'm
> open to other additions to MAINTAINERS, and specifically someone who can
> answer for STB SoCs, where this mess all originated from. Florian?
> Kamal?
>
> Brian
>

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Clay McClure]

On Tue, Oct 27, 2015 at 1:40 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

>> If we continue to use LE APB
>> transfers, then I think we can exclude the be32_to_cpu() call from
>> Brian's patch.
>
> I don't believe we can safely drop the be32_to_cpu(). That is just a
> refactoring of the existing logic. Please stop suggesting ways to break
> STB SoCs.

Yes, you're right; I misunderstood your patch. Thanks for your patient
explanation! :-)

>> Brian, I'll test a variation of this patch on our iProc-based
>> platforms and report back.
>
> OK, getting testing feedback from an iProc-based system using ONFI would
> be somewhat illuminating.

I've tested your refactoring patch [1] on Helix4 (an iProc-based
switch chip); seems to work fine.

Thanks,

Clay

[1] http://patchwork.ozlabs.org/patch/536148/

Re: [PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

[Brian Norris]

On Mon, Nov 02, 2015 at 03:10:05PM -0800, Clay McClure wrote:
> On Tue, Oct 27, 2015 at 1:40 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> >
> > OK, getting testing feedback from an iProc-based system using ONFI would
> > be somewhat illuminating.
> 
> I've tested your refactoring patch [1] on Helix4 (an iProc-based
> switch chip); seems to work fine.

You mean, you tested with a combo of my [1] and either [2] or [3]? Can I
get a 'Tested-by' line to add to them?

e.g.: 'Tested-by: Your Name <your@email>'

Regards,
Brian

> 
> [1] http://patchwork.ozlabs.org/patch/536148/

[2] http://patchwork.ozlabs.org/patch/539145/ (except your patch is
    whitespace damaged and patchwork didn't like it)

[3] http://patchwork.ozlabs.org/patch/535330/