Re: [PATCH edk2-platforms 13/14] Silicon/Socionext: add driver for SPI NOR flash

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Sep 12, 2017 at 01:41:34AM +0000, methavanitpong.pipat@socionext.com wrote:
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: Tuesday, September 12, 2017 4:13 AM
> > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: edk2-devel@lists.01.org; Methavanitpong, Pipat/メタワニットポン ピパット
> > <methavanitpong.pipat@socionext.com>; masahisa.kojima@linaro.org;
> > masami.hiramatsu@linaro.org
> > Subject: Re: [PATCH edk2-platforms 13/14] Silicon/Socionext: add driver
> > for SPI NOR flash
> > 
> > On Fri, Sep 08, 2017 at 07:23:14PM +0100, Ard Biesheuvel wrote:
> > > This imports the driver sources provided by Socionext for the FIP006
> > > SPI NOR flash device found on Synquacer SoCs. It has been slightly
> > > tweaked to bring it up to date with the changes made on the EDK2 side
> > > since it was forked.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> [Methavanitpong, Pipat/メタワニットポン ピパット] 
> Hi Leif, 
> 
> 1. Hex magic
> 
> There are 2 hex magic in this driver
> 
>   1. FIP006 command sequencer decode command
> 
>      FIP006 in command sequencer mode detects accesses to its memory region. 
>      An access to this region is translated into commands according to 
>      FIP006_REG_CS_RD and FIP006_REG_CS_WR for read and write accordingly. 
> 
>      A single access can be translated up to 8 1-byte commands, as the 
>      registers have 8 slots each. Each command is transmitted consecutively
>      starting from their base addresses. 
> 
>      Each slot in the registers are decoded as
> 
>      1. Immediate value - CSDC(imm, ..., ..., DEC=0)
>      2. Addr[7:0] - CSDC(0x00, ..., ..., DEC=1)
>      3. Addr[15:8] - CSDC(0x01, ..., ..., DEC=1)
>      4. Addr[23:16] - CSDC(0x02, ..., ..., DEC=1)
>      5. Addr[31:24] - CSDC(0x03, ..., ..., DEC=1)
>      6. HiZ for 1 byte - CSDC(0x04, ..., ..., DEC=1)
>      7. Upper 4-bit immediate value + 4-bit HiZ - CSDC((imm << 4) | 0x05, ..., ..., DEC=1)
>      8. Command termination - CSDC(0x07, ..., ..., DEC=1)
> 
>   2. Micron N25Q flash command
> 
>      In order to send a command to a flash device, FIP006's FIP006_REG_CS_RD 
>      and FIP006_REG_CS_WR must be set properly. NorFlashSetHostCommand() 
>      sets these registers according to an instance's command definition table. 
> 
>      For example; NorFlashSetHostCommand (Instance, 0x13) corresponds to a 
>      read access with 4-byte addressing read serial nor flash command (0x13). 
>      The result FIP006_REG_CS_RD is the following:
> 
>          FIP006_REG_CS_RD[0] = CSDC(0x13, 0, 1-bit lane, DEC=0)
>          FIP006_REG_CS_RD[1] = CSDC(0x03, 0, 1-bit lane, DEC=1)
>          FIP006_REG_CS_RD[2] = CSDC(0x02, 0, 1-bit lane, DEC=1)
>          FIP006_REG_CS_RD[3] = CSDC(0x01, 0, 1-bit lane, DEC=1)
>          FIP006_REG_CS_RD[4] = CSDC(0x00, 0, 1-bit lane, DEC=1)
>          FIP006_REG_CS_RD[5] = CSDC(0x07, 0, 1-bit lane, DEC=1)
>          FIP006_REG_CS_RD[6] = CSDC(0x07, 0, 1-bit lane, DEC=1)
>          FIP006_REG_CS_RD[7] = CSDC(0x07, 0, 1-bit lane, DEC=1)

Thank you for the explanation.
However, the code needs to be be readable by someone who does not have
the instruction set for each given component fresh in their mind.

That means that direct numeral expressions should be avoided wherever
they are not mathematically obvious (and where you consider them
obvious, consider whether everyone else would agree without already
knowing what the code is intended to do).

They should be replaced by #defines or enums as appropriate.

> 2. Bit field declaration
> 
> About the bitfield struct you mentioned in "Fip006Reg.h", 
> What should be a good way to declare it?

The problem with bitfields is that they are not very well defined in
the C language, and hence do not tend to be very portable.

Tedious as it may seem, #defines with shifts and masks are usually the
way to go.

Regards,

Leif