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

[Ray Jui <rjui@broadcom.com>]

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/
>