[PATCH] mtd: brcmnand: Fix NAND_CMD_PARAM byte order

Ray Jui rjui at broadcom.com
Mon Oct 26 11:52:23 PDT 2015


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 at 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 at 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 at 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 at gmail.com>
> Cc: Clay McClure <clay at daemons.net>
> Cc: Ray Jui <rjui at broadcom.com>
> Cc: Scott Branden <sbranden at 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) {
>



More information about the linux-mtd mailing list