Re: [PATCH v5 1/2] dt-bindings: mtd: marvell-nand: Convert to YAML DT scheme

[Miquel Raynal]

Hi Krzysztof,

krzysztof.kozlowski@xxxxxxxxxx wrote on Fri, 28 Oct 2022 07:31:39 -0400:

> On 28/10/2022 03:47, Miquel Raynal wrote:
> > Hi Krzysztof,
> > 
> > krzysztof.kozlowski@xxxxxxxxxx wrote on Thu, 27 Oct 2022 10:51:29 -0400:
> >   
> >> On 27/10/2022 09:50, Miquel Raynal wrote:  
> >>> Hi Krzysztof,
> >>>
> >>> krzysztof.kozlowski@xxxxxxxxxx wrote on Thu, 27 Oct 2022 09:24:24 -0400:
> >>>     
> >>>> On 27/10/2022 09:18, Miquel Raynal wrote:    
> >>>>> Hi Vadym,
> >>>>>       
> >>>>>>>>> +patternProperties:
> >>>>>>>>> +  "^nand@[0-3]$":
> >>>>>>>>> +    type: object
> >>>>>>>>> +    properties:
> >>>>>>>>> +      reg:
> >>>>>>>>> +        minimum: 0
> >>>>>>>>> +        maximum: 3
> >>>>>>>>> +
> >>>>>>>>> +      nand-rb:
> >>>>>>>>> +        minimum: 0
> >>>>>>>>> +        maximum: 1
> >>>>>>>>> +
> >>>>>>>>> +      nand-ecc-strength:
> >>>>>>>>> +        enum: [1, 4, 8]
> >>>>>>>>> +
> >>>>>>>>> +      nand-on-flash-bbt: true
> >>>>>>>>> +
> >>>>>>>>> +      nand-ecc-mode: true
> >>>>>>>>> +
> >>>>>>>>> +      nand-ecc-algo:
> >>>>>>>>> +        description: |
> >>>>>>>>> +          This property is essentially useful when not using hardware ECC.
> >>>>>>>>> +          Howerver, it may be added when using hardware ECC for clarification
> >>>>>>>>> +          but will be ignored by the driver because ECC mode is chosen depending
> >>>>>>>>> +          on the page size and the strength required by the NAND chip.
> >>>>>>>>> +          This value may be overwritten with nand-ecc-strength property.
> >>>>>>>>> +
> >>>>>>>>> +      nand-ecc-step-size:
> >>>>>>>>> +        description: |
> >>>>>>>>> +          Marvell's NAND flash controller does use fixed strength
> >>>>>>>>> +          (1-bit for Hamming, 16-bit for BCH), so the actual step size
> >>>>>>>>> +          will shrink or grow in order to fit the required strength.
> >>>>>>>>> +          Step sizes are not completely random for all and follow certain
> >>>>>>>>> +          patterns described in AN-379, "Marvell SoC NFC ECC".
> >>>>>>>>> +
> >>>>>>>>> +      label:
> >>>>>>>>> +        $ref: /schemas/types.yaml#/definitions/string
> >>>>>>>>> +
> >>>>>>>>> +      partitions:
> >>>>>>>>> +        type: object        
> >>>>>>>>
> >>>>>>>> That's not what I asked for. Like four times I asked you to add here
> >>>>>>>> unevaluatedProperties: false and I never said that ref to partition.yaml
> >>>>>>>> should be removed and you... instead remove that ref.
> >>>>>>>>
> >>>>>>>> You need to define here children and specify their ref.
> >>>>>>>>
> >>>>>>>> You must use unevaluatedProperties: false here. So this is fifth time I
> >>>>>>>> am writing this feedback.
> >>>>>>>>
> >>>>>>>>        
> >>>>>>>
> >>>>>>> It is a bit confusing that it is needed to define "partitions" and "label" rules particulary
> >>>>>>> in this nand controller instead of some common place like nand-chip.yaml, these properties
> >>>>>>> are common also for the other nand controllers.        
> >>>>>>
> >>>>>> No one speaks about label, I never commented about label, I think...
> >>>>>>
> >>>>>> If you think the property is really generic and every NAND controller
> >>>>>> bindings implement it, then feel free to include them there, in a
> >>>>>> separate patch. It sounds sensible, but I did not check other bindings.      
> >>>>>
> >>>>> FYI, label is already defined in mtd/mtd.yaml.      
> >>>>
> >>>> Which is not included here and in nand-controller.yaml    
> >>>
> >>> Maybe nand-chip.yaml should?    
> >>
> >> mtd.yaml looks a bit more than that - also allows nvmem nodes. Maybe
> >> let's just add label to nand-chip?  
> > 
> > I don't get the reason behind this proposal, mtd.yaml really is
> > kind of a definition of generic properties any mtd device might
> > have, so duplicating label (or whatever else inside) does not seem
> > legitimate to me. The jedec,spi-nor.yaml file already references it for
> > instance.  
> 
> spi-nor is not a NAND chip... By including mtd.yaml in nand-chip you
> also allow the NVMEM properties which are not applicable.

MTD is an NVMEM provider, any MTD device (including NANDs) can use
these properties IMHO. It's not reserved to spi-nor (even though it is
more common, I conceed).

> >>>>> Partitions do not need to be defined in your binding, just don't put
> >>>>> any in your example and you'll be fine. These partitions are either
> >>>>> static and may be described in the DT (see
> >>>>> mtd/partition/partition.yaml) or there is some dynamic discovery
> >>>>> involved and a proper parser shall be referenced (parsers have their
> >>>>> own binding).      
> >>>>
> >>>> I don't think this is correct. Basically you allow any node to be under
> >>>> partitions as there is no schema validating them (without compatibles).    
> >>>
> >>> Sorry if that was unclear, what I meant is: partitions should not be
> >>> defined in the bindings for Marvell NAND controller because they should
> >>> be defined somewhere else already.    
> >>
> >> Ah, right. Then it seems reasonable.
> >>  
> >>>
> >>> NAND controller subnodes should define the storage devices (the
> >>> flashes themselves) connected to the controller. "nand-chip.yaml"
> >>> describes generic properties for these. Additional subnodes are allowed
> >>> and expected to be partitions (this is not enforced anywhere I think),
> >>> they should use one of the existing compatibles to define the parser.
> >>> The most common parser is named fixed-partitions and has its own
> >>> compatible. Every parser references partitions.yaml.
> >>>
> >>> There are a few controller bindings however which reference
> >>> partition.yaml anyway, probably to make the examples validation work,
> >>> I'm not sure it should be done like that though:
> >>> https://elixir.bootlin.com/linux/v6.0/source/Documentation/devicetree/bindings/mtd/ti,gpmc-nand.yaml
> >>> https://elixir.bootlin.com/linux/v6.0/source/Documentation/devicetree/bindings/mtd/ti,gpmc-onenand.yaml    
> >>
> >>
> >> Yes, so the nand-chip implementation (like Marvell NAND) could reference
> >> the parser and we would be done. If it doesn't, then we must have
> >> generic partitions in the nand-chip.  
> > 
> > In this case, I am not aware of any parser that would be relevant.
> > 
> > In the generic case, should we really reference a parser in particular?
> > If yes then maybe we should make a yaml file that just gathers all the
> > parsers and include it within mtd.yaml (and have it referenced in
> > nand-chip.yaml). What do you think?
> >   
> 
> Not all MTD devices have partitions so putting this into mtd.yaml does
> not look correct. Adding it into nand-chip seems fine.

Not all MTD devices have partitions but all of them can have
partitions. It's not a required subnode, but it is definitely common to
all mtd devices. I would then consider mtd.yaml a _very_ generic place
where we put anything that is not specific to the underlying storage
technology.

Anything that is specific to NAND goes into nand-chip.yaml or
nand-controller.yaml, anything specific to SPI-NOR goes into
jedec,spi-nor.yaml.

nand-chip.yaml and jedec,spi-nor.yaml should reference mtd.yaml.

mtd.yaml could probably reference some kind of "partition.yaml" to
define subnodes with partition parsers. I don't yet know exactly how to
make everything coherent but I liked the idea of having a file
referencing all the documented parsers, so that they could all apply if
necessary. I did not understand if you were in favor or opposed to this
idea? If opposed, how could we make all the partition parsers
schemas (and only them) to be validated as MTD devices subnodes?

Thanks a lot for all your feedback,
Miquèl