Re: [PATCH v3 5/8] dt-bindings: mtd: ti,gpmc-nand: Convert to yaml

[Roger Quadros <rogerq@kernel.org>]

On 08/09/2021 01:24, Rob Herring wrote:
> On Tue, Sep 7, 2021 at 11:57 AM Roger Quadros <rogerq@kernel.org> wrote:
>>
>> Hi Miquel,
>>
>> On 07/09/2021 19:35, Miquel Raynal wrote:
>>> Hi Grygorii,
>>>
>>>>>
>>>>>> +
>>>>>> +  nand-bus-width:
>>>>>> +    description:
>>>>>> +      Bus width to the NAND chip
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    enum: [8, 16]
>>>>>> +    default: 8
>>>>>
>>>>> This is part of nand-controller.yaml binding and should not be there.
>>>>>
>>>>>> +
>>>>>> +allOf:
>>>>>> +  - $ref: "../memory-controllers/ti,gpmc-child.yaml"
>>>>>
>>>>> Maybe you need to reference the nand controller bindings as well
>>>>>
>>>>
>>>> This will not work out of the box :( as nand-controller.yaml defines both
>>>>   nand controller and nand memory. It potentially might work if it will be possible to split
>>>> nand memory definition (or nand memory properties) out of and-controller.yaml, similarly to
>>>> ti,gpmc-child.yaml from this series.
>>>
>>> What you think would be the issue?
>>
>> The issue is that dt_binding checks will fail if I reference nand-controller.yaml
>> as we currently represent the controller as follows
>>
>> memory-controller { /* GPMC controller */
>>         memory-controller-props;
>>         nand-chip {
>>                 /* @chip select 0 */
>>                 nand-controller-props;
>>                 memory-controller-timing-props;
>>                 chip-props;
>>         }
>>         nand-chip {
>>                 /* @chip select 1 */
>>                 nand-controller-props;
>>                 memory-controller-timing-props;
>>                 chip-props;
>>         }
>>         nor-chip {
>>                 /* @chip select 2 */
>>                 memory-controller-timing-props;
>>                 chip-props;
>>         }
>> }
>>
>> The NAND controller IO registers are at different addresses for different
>> chip select regions. Also, this is one way we can specify GPMC settings/timings
>> for different chip selects.
>>
>>>
>>> I am not opposed to split nand-controller.yaml into
>>> nand-controller.yaml and nand-chip.yaml if it simplifies the
>>> description of controllers but I don't get why it would be needed. In
>>> particular since we expect all drivers to support the
>>>
>>> nand-controller {
>>>       controller-props;
>>>       nand-chip {
>>>               chip-props;
>>>       }
>>> }
>>
>> Changing to this format will cause a lot of churn in DT files, which I'm not sure
>> if it gives enough benefit.
>> TI platforms will never have 2 NAND chips in the same chip select region.
> 
> Probably best to just leave this alone. Unless this is getting used in
> new chips? If so, I'd say it's a separate change.
> 
>>> organization which has been enforced since at least 2018. Having a
>>> controller vs. chip representation is fundamentally right. But here I
>>> see how "legacy" are these bindings with so much unneeded specific "ti,"
>>> properties... On one side it would be good to verify that the driver
>>> supports this representation (which I believe is true) and on the other
>>> side maybe it's time to advertise "better" bindings as well.
>>
>> Yes, I'm OK to mark ti specific properties deprecated and use standard NAND chip
>> bindings.
> 
> I don't think it's really worth it to go half way using common
> properties but not the common structure.

I agree.
We will be having new chips that will use this driver but we will migrate to new
common structure when adding support for those chips.

So I will leave this patch as it is for now.

cheers,
-roger