Re: [RFC PATCH 2/2] nand: cavium: Nand flash controller for Cavium ARM64 SOCs

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Mon, 22 May 2017 13:35:37 +0200
Jan Glauber <jan.glauber@caviumnetworks.com> wrote:

> Hi Boris,
> 
> On Fri, May 19, 2017 at 09:51:40AM +0200, Boris Brezillon wrote:
> > Hi Jan,
> > 
> > A few more comments.
> > 
> > On Mon, 27 Mar 2017 18:05:24 +0200
> > Jan Glauber <jglauber@cavium.com> wrote:
> >   
> > > +
> > > +struct ndf_nop_cmd {
> > > +	u16 opcode	: 4;
> > > +	u16 nop		: 12;
> > > +};
> > > +
> > > +struct ndf_wait_cmd {
> > > +	u16 opcode	: 4;
> > > +	u16 r_b		: 1;	/* wait for one cycle or PBUS_WAIT deassert */
> > > +	u16		: 3;
> > > +	u16 wlen	: 3;	/* timing parameter select */  
> > 
> > Can you clearly describe what this timing is? According to the code,
> > it's tWB, but I'd prefer to have it documented here.  
> 
> It is until PBUS_WAIT deasserts, if that condition is already false it
> waits for one cycle.

I guess PBUS_WAIT is directly related to the R/B pin. So I guess this
timing is tRR.

> 
> > BTW, it's not clear at first glance that the value you put here is
> > actually encoding the tm_par slot.  
> 
> Yes. All the timings use this schematic, so maybe a more prominent
> comment in the header file would help.

It's still easier if the comment is appearing next to the struct
definition. So maybe you can add a generic comment at the beginning of
the file and then refer to it.

> 
> > > +	u16		: 5;
> > > +};  
> > 
> > Hm, are you sure you want to trust the compiler for bitfield placement?
> > AFAIK, bitfield ordering is not standardized and is thus
> > implementation specific. I'd recommend that you switch to plain
> > u16/u32/u64 fields and use macros to define bitfields:
> > 
> > #define NFD_CMD_OPCODE(x)	(x)
> > #define NFD_WAIT_CMD_WAIT_RB	BIT(5)
> > #define NFD_WAIT_CMD_TPAR(x)	((x) << 8)
> > ...  
> 
> Well, I had that discussion before :) Contrary to common wisdom I've not
> seen a problem with bitfields yet. Personaly I would prefer the bitfield
> notation as I found it more readable, but I don't mind converting it.

I'd still prefer to get rid of bitfields, to be consistent with how
Linux deals with bitfields in general.

> 
> > > +
> > > +struct ndf_bus_cmd {
> > > +	u16 opcode	: 4;
> > > +	u16 direction	: 4;	/* 1 = acquire, 0 = release */  
> > 
> > Not sure why this is named direction if the only thing you can do is
> > acquire or release the bus.  
> 
> I'm using the names from the official documentation. I know that this
> might not help everyone as it is not public, but for maintaining the
> driver or fixing bugs I find it more convenient. Sometimes the names
> are sub-optimal though, then I try to add comments as above.

Okay for using datasheet names, but please document their meanings just
before the struct or macro definition (it's done here, but it's not the
case for all fields).

> 
> > > +	u16		: 8;
> > > +};
> > > +
> > > +struct ndf_chip_cmd {
> > > +	u16 opcode	: 4;
> > > +	u16 chip	: 3;	/* select chip, 0 = disable */
> > > +	u16 enable	: 1;	/* 1 = enable, 0 = disable */
> > > +	u16 bus_width	: 2;	/* 10 = 16 bit, 01 = 8 bit */
> > > +	u16		: 6;
> > > +};
> > > +
> > > +struct ndf_cle_cmd {
> > > +	u32 opcode	: 4;
> > > +	u32		: 4;
> > > +	u32 cmd_data	: 8;	/* command sent to the PBUS AD pins */
> > > +	u32 clen1	: 3;	/* time between PBUS CLE and WE asserts */
> > > +	u32 clen2	: 3;	/* time WE remains asserted */
> > > +	u32 clen3	: 3;	/* time between WE deassert and CLE */  
> > 
> > Can you re-use the names defined here [1]?
> > AFAICS, clen2 == tWP, clen3 == tCLH, clen1 == tCLS - tWP.  
> 
> Again, naming is to match the documentation. I found it incredible
> hard to get the timing parameters right, as the Cavium hardware
> adds another abstraction (PBUS), with often uses completely different
> timing names. Not sure what is the best to do here, if I only use
> the 'official' names I would ignore the PBUS layer.

I'm fine with cavium specific names as long as their conversion to
generic timings is clearly explained.