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

[Krzysztof Kozlowski]

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.

> 
>>>>> 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.

Best regards,
Krzysztof