Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

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

On Wed, 26 Oct 2016 13:52:41 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi Ricard,
> 
> On Wed, 26 Oct 2016 13:27:00 +0200
> Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
> 
> > Hi Boris,
> > 
> > Juggling the rework of the Evatronix NAND flash driver with other projects 
> > I'm plodding on and will hopefully have something that can be reviewed in 
> > a few weeks' time.
> > 
> > In the meantime there is an issue which I want to bring up, regarding the 
> > timing configuration. Back in June we had this discussion, regarding my 
> > suggestion to put the raw values for the timing control registers in the 
> > IP in the DT:
> > 
> > On Fri, 10 Jun 2016, Boris Brezillon wrote:
> >   
> > > On Fri, 10 Jun 2016 17:35:24 +0200
> > > Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
> > >    
> > > > > > > > +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> > > > > > > > +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.        
> > > > > > > 
> > > > > > > 
> > > > > > > Can this be extracted from the timing mode exposed by the NAND chip.
> > > > > > > IMO it shouldn't be defined in the DT.     
> > > >
> > > > [ ... ]
> > > >     
> > > > > Just because it's complicated to extract these information at the driver 
> > > > > level level doesn't mean you should put it in the DT. That's exactly the 
> > > > > opposite: the DT is supposed to encode the hardware representation, not 
> > > > > how you want to configure it.      
> > > > 
> > > > [ ... ]
> > > >     
> > > > > How about using a default/safe timing mode for now (ONFI timing mode 0
> > > > > should work for all NANDs).
> > > > > The only thing you'll have to do is retrieve the source clk rate and
> > > > > calculate timing register values accordingly. Or you can even assume
> > > > > you always have a 100MHz source clk and hardcode it in your driver.      
> > > > 
> > > > Yes, that is certainly possible of course, and the driver already has a 
> > > > hard-coded default setup for this case.
> > > > 
> > > > In that case though the driver could have pre-set setups for other ONFi 
> > > > modes, and we could have an _optional_ DT property to select them, the 
> > > > reason for that being in order to handle non-ONFi flashes whose timing 
> > > > cannot be gleaned from the device itself.
> > > > 
> > > > I.e. something like
> > > > 
> > > > evatronix,onfi-timing-mode = <2>;
> > > >     
> > > Actually this has been added to the nand_flash_dev structure, and it's
> > > called onfi_timing_mode_default.
> > > If you need a different timing, define a full ID entry for your NAND.    
> > 
> > In our products, we use 1, 2 and 4 Gbit SLC NAND flashes, some of which 
> > are ONFi complient and some not, depending on the manufacturer. For the 
> > non-ONFi flashes, the ones which we have looked at and which we specify in 
> > our products actually support ONFi mode 2. However, in practice, as part 
> > of the nand_scan_ident() procedure, the chip->onfi_timing_mode_default 
> > value is set to 0, because the EXTENDED_ID_NAND() macro used for these 
> > flashes doesn't set the timing mode to anything else. There are some full 
> > ID listings which apparently set a different timing mode for certain 
> > flashes (such as Toshiba TC58NVG0S3E).
> > 
> > What I would like to have in the device tree, either as a general mtd or 
> > proprietary property, is something like the above, possibly called 
> > something like force-onfi-timing-mode, which forcibly sets the timing mode 
> > of the hardware. When this property is not set, the timing mode used will 
> > default to onfi_timing_mode_default for non-ONFi flashes, or 
> > onfi_get_async_timing_mode() for ONFi flashes.  
> 
> I'm not against the addition of this onfi-timing-mode property, but not
> for the same reason. Your NAND might be able to support a specific
> timing mode, but for some reason, the board design prevents using this
> mode (interferences on the DATA or CONTROL lines for example).
> 
> To me, this property would just serve as a limitation: if the chip
> supports mode 5 but the board says that it supports up to mode 2, then
> the implementation should stick to mode 2. ITOH, if the 5 is defined in
> the DT, but the chip only supports up to mode 2, then mode 2 should be
> used.
> 
> > 
> > The reationale behind this is that it is impractical to list all specific 
> > flash chips which actually support a given timing mode. Say another chip 
> > comes out on the market either from an existing or a new manufacturer, 
> > which correctly identifies itself using the ID bytes, but not using the 
> > full ID. In order to support the speed of this device, we would need to 
> > upgrade the kernel in the product. If we on the other hand have a hardware 
> > specification that says that we only use flashes which support a certain 
> > specified timing mode (because of memory bandwidth requirements), we can 
> > simply put force-onfi-timing-mode = <2> in the DT, and it will work for 
> > all flashes that meet the specifications, whether they match the full ID 
> > or not.  
> 
> Well, I don't think this is a good argument. If you want to support
> advanced features, you'll still have to define some chip/vendor
> specific implementation, and these will likely be assigned depending on
> the full ID. So, in the end, if you want to have good support for a
> NAND chip that is not ONFi or JEDEC compliant, you'll have to update
> your kernel.
> 
> Also note that I'm trying to get rid of the full-id table, and replace
> it by vendor detection/initialization [1], so that you can tweak
> different things on a per-chip basis (and this includes the timing
> mode).

Oops, forgot to put the reference:

[1]http://lkml.iu.edu/hypermail/linux/kernel/1606.2/02892.html

> 
> > 
> > For the case of a .dts file specifying a more general board which can be 
> > populated with any flash chip, the property can be left out, and the 
> > timing mode will then be determined using the ID and ONFi parameter page, 
> > as applicable.  
> 
> I'd like to leave as much as possible in the NAND
> detection/initialization code, and this includes selecting the best
> timing mode (based on JEDEC, ONFi, or vendor specific ID parsing).
> 
> Also remember that DT is supposed to be represent the HW blocks, not
> their configuration.
> 
> > 
> > Alternatively having this property as a proprietary one (i.e. 
> > "evatronix,force-onfi-timing-mode") is an alternative, but I still wanted 
> > to bring up the discussion for the general case.  
> 
> No, please do not define a private property. I think I'd prefer to
> have a generic property, but I'm still not convinced this is necessary.
> 
> Regards,
> 
> Boris
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/