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

[<methavanitpong.pipat@socionext.com>]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, September 12, 2017 5:39 PM
> To: Methavanitpong, Pipat/メタワニットポン ピパット
> <methavanitpong.pipat@socionext.com>
> Cc: edk2-devel@lists.01.org; masahisa.kojima@linaro.org;
> masami.hiramatsu@linaro.org; ard.biesheuvel@linaro.org
> Subject: Re: [PATCH edk2-platforms 13/14] Silicon/Socionext: add driver
> for SPI NOR flash
> 
> 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.

Thank you for your comment, Leif. 
I will see what I can do with Ard. 

> 
> Regards,
> 
> Leif

--
Pipat Methavanitpong
Software Developer, S-Project 3
Socionext Inc.