diff mbox series

[v3,2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml

Message ID 20230509052757.539274-3-claudiu.beznea@microchip.com
State New
Headers
Series dt-bindings: clocks: at91: convert to yaml |

Commit Message

Claudiu Beznea May 9, 2023, 5:27 a.m. UTC 
  Convert Atmel PMC documentation to yaml. Along with it clock names
were adapted according to the current available device trees as
different controller versions accept different clocks (some of them
have 3 clocks as input, some has 2 clocks as inputs and some with 2
input clocks uses different clock names).

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 .../devicetree/bindings/clock/at91-clock.txt  |  28 ----
 .../bindings/clock/atmel,at91rm9200-pmc.yaml  | 152 ++++++++++++++++++
 2 files changed, 152 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml

Comments

Krzysztof Kozlowski May 9, 2023, 6:25 a.m. UTC | #1 
On 09/05/2023 07:27, Claudiu Beznea wrote:
> Convert Atmel PMC documentation to yaml. Along with it clock names
> were adapted according to the current available device trees as
> different controller versions accept different clocks (some of them
> have 3 clocks as input, some has 2 clocks as inputs and some with 2
> input clocks uses different clock names).
> 

Thank you for your patch. There is something to discuss/improve.

> +title: Atmel Power Management Controller (PMC)
> +
> +maintainers:
> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
> +
> +description:
> +  The power management controller optimizes power consumption by controlling all
> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
> +  to many of the peripherals and to the processor.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - atmel,at91sam9g15-pmc
> +              - atmel,at91sam9g20-pmc
> +              - atmel,at91sam9g25-pmc
> +              - atmel,at91sam9g35-pmc
> +              - atmel,at91sam9x25-pmc
> +              - atmel,at91sam9x35-pmc
> +          - enum:
> +              - atmel,at91sam9260-pmc
> +              - atmel,at91sam9x5-pmc

I missed it last time - why you have two enums? We never talked about
this. It's usually wrong... are you sure this is real hardware:
atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
?


> +          - const: syscon
> +      - items:
> +          - enum:
> +              - atmel,at91rm9200-pmc
> +              - atmel,at91sam9260-pmc
> +              - atmel,at91sam9g45-pmc
> +              - atmel,at91sam9n12-pmc
> +              - atmel,at91sam9rl-pmc
> +              - atmel,sama5d2-pmc
> +              - atmel,sama5d3-pmc
> +              - atmel,sama5d4-pmc
> +              - microchip,sam9x60-pmc
> +              - microchip,sama7g5-pmc
> +          - const: syscon
> +


Best regards,
Krzysztof
Claudiu Beznea May 10, 2023, 7 a.m. UTC | #2 
On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 09/05/2023 07:27, Claudiu Beznea wrote:
>> Convert Atmel PMC documentation to yaml. Along with it clock names
>> were adapted according to the current available device trees as
>> different controller versions accept different clocks (some of them
>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
>> input clocks uses different clock names).
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +title: Atmel Power Management Controller (PMC)
>> +
>> +maintainers:
>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>> +
>> +description:
>> +  The power management controller optimizes power consumption by controlling all
>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
>> +  to many of the peripherals and to the processor.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - atmel,at91sam9g15-pmc
>> +              - atmel,at91sam9g20-pmc
>> +              - atmel,at91sam9g25-pmc
>> +              - atmel,at91sam9g35-pmc
>> +              - atmel,at91sam9x25-pmc
>> +              - atmel,at91sam9x35-pmc
>> +          - enum:
>> +              - atmel,at91sam9260-pmc
>> +              - atmel,at91sam9x5-pmc
> 
> I missed it last time - why you have two enums? We never talked about
> this. It's usually wrong... are you sure this is real hardware:
> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
> ?

I have 2 enums because there are some hardware covered by:
"vendor-name,hardware-v1-pmc", "syscon" and some covered by:
"vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".

Many AT91 device trees compatibles were written in this way. Thus when new
versions of the same IP has been introduced the drivers were not
necessarily updated but the compatibles in device trees were updated e.g.
with "vendor-name,hardware-v2-pmc" (the full compatible becoming
"vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon") and
let the drivers fall back to already in driver supported compatible
"vendor-name,hardware-v1-pmc", "syscon". In general v2 comes with new
features in addition to v1.

That way they AT91 ensures the ABI properties of DT and thus when the
drivers were finally updated with the new features of the
"vendor-name,hardware-v2-pmc" DT remained in place.

Please let me know if these could be handled better in YAML.

Thank you,
Claudiu

> 
> 
>> +          - const: syscon
>> +      - items:
>> +          - enum:
>> +              - atmel,at91rm9200-pmc
>> +              - atmel,at91sam9260-pmc
>> +              - atmel,at91sam9g45-pmc
>> +              - atmel,at91sam9n12-pmc
>> +              - atmel,at91sam9rl-pmc
>> +              - atmel,sama5d2-pmc
>> +              - atmel,sama5d3-pmc
>> +              - atmel,sama5d4-pmc
>> +              - microchip,sam9x60-pmc
>> +              - microchip,sama7g5-pmc
>> +          - const: syscon
>> +
> 
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski May 10, 2023, 7:06 a.m. UTC | #3 
On 10/05/2023 09:00, Claudiu.Beznea@microchip.com wrote:
> On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 09/05/2023 07:27, Claudiu Beznea wrote:
>>> Convert Atmel PMC documentation to yaml. Along with it clock names
>>> were adapted according to the current available device trees as
>>> different controller versions accept different clocks (some of them
>>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
>>> input clocks uses different clock names).
>>>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +title: Atmel Power Management Controller (PMC)
>>> +
>>> +maintainers:
>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>>> +
>>> +description:
>>> +  The power management controller optimizes power consumption by controlling all
>>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
>>> +  to many of the peripherals and to the processor.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - items:
>>> +          - enum:
>>> +              - atmel,at91sam9g15-pmc
>>> +              - atmel,at91sam9g20-pmc
>>> +              - atmel,at91sam9g25-pmc
>>> +              - atmel,at91sam9g35-pmc
>>> +              - atmel,at91sam9x25-pmc
>>> +              - atmel,at91sam9x35-pmc
>>> +          - enum:
>>> +              - atmel,at91sam9260-pmc
>>> +              - atmel,at91sam9x5-pmc
>>
>> I missed it last time - why you have two enums? We never talked about
>> this. It's usually wrong... are you sure this is real hardware:
>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>> ?
> 
> I have 2 enums because there are some hardware covered by:
> "vendor-name,hardware-v1-pmc", "syscon" and some covered by:
> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".

The enum does not say this. At all.

So again, answer, do not ignore:
is this valid setup:
atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
?

> 
> Many AT91 device trees compatibles were written in this way. Thus when new
> versions of the same IP has been introduced the drivers were not
> necessarily updated but the compatibles in device trees were updated e.g.
> with "vendor-name,hardware-v2-pmc" (the full compatible becoming
> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon") and
> let the drivers fall back to already in driver supported compatible
> "vendor-name,hardware-v1-pmc", "syscon". In general v2 comes with new
> features in addition to v1.
> 
> That way they AT91 ensures the ABI properties of DT and thus when the
> drivers were finally updated with the new features of the
> "vendor-name,hardware-v2-pmc" DT remained in place.
> 
> Please let me know if these could be handled better in YAML.


enum + const + syscon, like every binding that type does in all
bindings. Don't invent some new syntax.

Best regards,
Krzysztof
Claudiu Beznea May 10, 2023, 7:14 a.m. UTC | #4 
On 10.05.2023 10:06, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 10/05/2023 09:00, Claudiu.Beznea@microchip.com wrote:
>> On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 09/05/2023 07:27, Claudiu Beznea wrote:
>>>> Convert Atmel PMC documentation to yaml. Along with it clock names
>>>> were adapted according to the current available device trees as
>>>> different controller versions accept different clocks (some of them
>>>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
>>>> input clocks uses different clock names).
>>>>
>>>
>>> Thank you for your patch. There is something to discuss/improve.
>>>
>>>> +title: Atmel Power Management Controller (PMC)
>>>> +
>>>> +maintainers:
>>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>>>> +
>>>> +description:
>>>> +  The power management controller optimizes power consumption by controlling all
>>>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
>>>> +  to many of the peripherals and to the processor.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - enum:
>>>> +              - atmel,at91sam9g15-pmc
>>>> +              - atmel,at91sam9g20-pmc
>>>> +              - atmel,at91sam9g25-pmc
>>>> +              - atmel,at91sam9g35-pmc
>>>> +              - atmel,at91sam9x25-pmc
>>>> +              - atmel,at91sam9x35-pmc
>>>> +          - enum:
>>>> +              - atmel,at91sam9260-pmc
>>>> +              - atmel,at91sam9x5-pmc
>>>
>>> I missed it last time - why you have two enums? We never talked about
>>> this. It's usually wrong... are you sure this is real hardware:
>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>> ?
>>
>> I have 2 enums because there are some hardware covered by:
>> "vendor-name,hardware-v1-pmc", "syscon" and some covered by:
>> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".
> 
> The enum does not say this. At all.
> 
> So again, answer, do not ignore:
> is this valid setup:
> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
> ?

Not w/o syscon. This is valid:

compatible = "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc", "syscon";

available in arch/arm/boot/dts/at91sam9g20.dtsi +45

> 
>>
>> Many AT91 device trees compatibles were written in this way. Thus when new
>> versions of the same IP has been introduced the drivers were not
>> necessarily updated but the compatibles in device trees were updated e.g.
>> with "vendor-name,hardware-v2-pmc" (the full compatible becoming
>> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon") and
>> let the drivers fall back to already in driver supported compatible
>> "vendor-name,hardware-v1-pmc", "syscon". In general v2 comes with new
>> features in addition to v1.
>>
>> That way they AT91 ensures the ABI properties of DT and thus when the
>> drivers were finally updated with the new features of the
>> "vendor-name,hardware-v2-pmc" DT remained in place.
>>
>> Please let me know if these could be handled better in YAML.
> 
> 
> enum + const + syscon, like every binding that type does in all
> bindings. Don't invent some new syntax.
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski May 10, 2023, 7:58 a.m. UTC | #5 
On 10/05/2023 09:14, Claudiu.Beznea@microchip.com wrote:
> On 10.05.2023 10:06, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 10/05/2023 09:00, Claudiu.Beznea@microchip.com wrote:
>>> On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 09/05/2023 07:27, Claudiu Beznea wrote:
>>>>> Convert Atmel PMC documentation to yaml. Along with it clock names
>>>>> were adapted according to the current available device trees as
>>>>> different controller versions accept different clocks (some of them
>>>>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
>>>>> input clocks uses different clock names).
>>>>>
>>>>
>>>> Thank you for your patch. There is something to discuss/improve.
>>>>
>>>>> +title: Atmel Power Management Controller (PMC)
>>>>> +
>>>>> +maintainers:
>>>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>>>>> +
>>>>> +description:
>>>>> +  The power management controller optimizes power consumption by controlling all
>>>>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
>>>>> +  to many of the peripherals and to the processor.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - atmel,at91sam9g15-pmc
>>>>> +              - atmel,at91sam9g20-pmc
>>>>> +              - atmel,at91sam9g25-pmc
>>>>> +              - atmel,at91sam9g35-pmc
>>>>> +              - atmel,at91sam9x25-pmc
>>>>> +              - atmel,at91sam9x35-pmc
>>>>> +          - enum:
>>>>> +              - atmel,at91sam9260-pmc
>>>>> +              - atmel,at91sam9x5-pmc
>>>>
>>>> I missed it last time - why you have two enums? We never talked about
>>>> this. It's usually wrong... are you sure this is real hardware:
>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>>> ?
>>>
>>> I have 2 enums because there are some hardware covered by:
>>> "vendor-name,hardware-v1-pmc", "syscon" and some covered by:
>>> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".
>>
>> The enum does not say this. At all.
>>
>> So again, answer, do not ignore:
>> is this valid setup:
>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>> ?
> 
> Not w/o syscon. This is valid:

Syscon is not important here, but indeed I missed it.

> 
> compatible = "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc", "syscon";
> 
> available in arch/arm/boot/dts/at91sam9g20.dtsi +45

Nice, so my random choice was actually correct. Ok, so another:

atmel,at91sam9g15-pmc, atmel,at91sam9260-pmc, syscon

Is it valid hardware?

Best regards,
Krzysztof
Claudiu Beznea May 10, 2023, 8:31 a.m. UTC | #6 
On 10.05.2023 10:58, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 10/05/2023 09:14, Claudiu.Beznea@microchip.com wrote:
>> On 10.05.2023 10:06, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 10/05/2023 09:00, Claudiu.Beznea@microchip.com wrote:
>>>> On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 09/05/2023 07:27, Claudiu Beznea wrote:
>>>>>> Convert Atmel PMC documentation to yaml. Along with it clock names
>>>>>> were adapted according to the current available device trees as
>>>>>> different controller versions accept different clocks (some of them
>>>>>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
>>>>>> input clocks uses different clock names).
>>>>>>
>>>>>
>>>>> Thank you for your patch. There is something to discuss/improve.
>>>>>
>>>>>> +title: Atmel Power Management Controller (PMC)
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>>>>>> +
>>>>>> +description:
>>>>>> +  The power management controller optimizes power consumption by controlling all
>>>>>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
>>>>>> +  to many of the peripherals and to the processor.
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    oneOf:
>>>>>> +      - items:
>>>>>> +          - enum:
>>>>>> +              - atmel,at91sam9g15-pmc
>>>>>> +              - atmel,at91sam9g20-pmc
>>>>>> +              - atmel,at91sam9g25-pmc
>>>>>> +              - atmel,at91sam9g35-pmc
>>>>>> +              - atmel,at91sam9x25-pmc
>>>>>> +              - atmel,at91sam9x35-pmc
>>>>>> +          - enum:
>>>>>> +              - atmel,at91sam9260-pmc
>>>>>> +              - atmel,at91sam9x5-pmc
>>>>>
>>>>> I missed it last time - why you have two enums? We never talked about
>>>>> this. It's usually wrong... are you sure this is real hardware:
>>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>>>> ?
>>>>
>>>> I have 2 enums because there are some hardware covered by:
>>>> "vendor-name,hardware-v1-pmc", "syscon" and some covered by:
>>>> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".
>>>
>>> The enum does not say this. At all.
>>>
>>> So again, answer, do not ignore:
>>> is this valid setup:
>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>> ?
>>
>> Not w/o syscon. This is valid:
> 
> Syscon is not important here, but indeed I missed it.
> 
>>
>> compatible = "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc", "syscon";
>>
>> available in arch/arm/boot/dts/at91sam9g20.dtsi +45
> 
> Nice, so my random choice was actually correct. Ok, so another:
> 
> atmel,at91sam9g15-pmc, atmel,at91sam9260-pmc, syscon
> 
> Is it valid hardware?

This one, no. So, I guess, the wrong here is that there could be
combinations that are not for actual hardware and yet considered valid by
changes in this patch?

> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski May 10, 2023, 10:12 a.m. UTC | #7 
On 10/05/2023 10:31, Claudiu.Beznea@microchip.com wrote:
> On 10.05.2023 10:58, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 10/05/2023 09:14, Claudiu.Beznea@microchip.com wrote:
>>> On 10.05.2023 10:06, Krzysztof Kozlowski wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 10/05/2023 09:00, Claudiu.Beznea@microchip.com wrote:
>>>>> On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> On 09/05/2023 07:27, Claudiu Beznea wrote:
>>>>>>> Convert Atmel PMC documentation to yaml. Along with it clock names
>>>>>>> were adapted according to the current available device trees as
>>>>>>> different controller versions accept different clocks (some of them
>>>>>>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
>>>>>>> input clocks uses different clock names).
>>>>>>>
>>>>>>
>>>>>> Thank you for your patch. There is something to discuss/improve.
>>>>>>
>>>>>>> +title: Atmel Power Management Controller (PMC)
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  The power management controller optimizes power consumption by controlling all
>>>>>>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
>>>>>>> +  to many of the peripherals and to the processor.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    oneOf:
>>>>>>> +      - items:
>>>>>>> +          - enum:
>>>>>>> +              - atmel,at91sam9g15-pmc
>>>>>>> +              - atmel,at91sam9g20-pmc
>>>>>>> +              - atmel,at91sam9g25-pmc
>>>>>>> +              - atmel,at91sam9g35-pmc
>>>>>>> +              - atmel,at91sam9x25-pmc
>>>>>>> +              - atmel,at91sam9x35-pmc
>>>>>>> +          - enum:
>>>>>>> +              - atmel,at91sam9260-pmc
>>>>>>> +              - atmel,at91sam9x5-pmc
>>>>>>
>>>>>> I missed it last time - why you have two enums? We never talked about
>>>>>> this. It's usually wrong... are you sure this is real hardware:
>>>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>>>>> ?
>>>>>
>>>>> I have 2 enums because there are some hardware covered by:
>>>>> "vendor-name,hardware-v1-pmc", "syscon" and some covered by:
>>>>> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".
>>>>
>>>> The enum does not say this. At all.
>>>>
>>>> So again, answer, do not ignore:
>>>> is this valid setup:
>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>>> ?
>>>
>>> Not w/o syscon. This is valid:
>>
>> Syscon is not important here, but indeed I missed it.
>>
>>>
>>> compatible = "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc", "syscon";
>>>
>>> available in arch/arm/boot/dts/at91sam9g20.dtsi +45
>>
>> Nice, so my random choice was actually correct. Ok, so another:
>>
>> atmel,at91sam9g15-pmc, atmel,at91sam9260-pmc, syscon
>>
>> Is it valid hardware?
> 
> This one, no. So, I guess, the wrong here is that there could be
> combinations that are not for actual hardware and yet considered valid by
> changes in this patch?

I just don't understand why you have two enums. This is not a pattern
which is allowed anywhere. It might appear but only as exception or mistake.


Best regards,
Krzysztof
Claudiu Beznea May 11, 2023, 6:29 a.m. UTC | #8 
On 10.05.2023 13:12, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 10/05/2023 10:31, Claudiu.Beznea@microchip.com wrote:
>> On 10.05.2023 10:58, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 10/05/2023 09:14, Claudiu.Beznea@microchip.com wrote:
>>>> On 10.05.2023 10:06, Krzysztof Kozlowski wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 10/05/2023 09:00, Claudiu.Beznea@microchip.com wrote:
>>>>>> On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> On 09/05/2023 07:27, Claudiu Beznea wrote:
>>>>>>>> Convert Atmel PMC documentation to yaml. Along with it clock names
>>>>>>>> were adapted according to the current available device trees as
>>>>>>>> different controller versions accept different clocks (some of them
>>>>>>>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
>>>>>>>> input clocks uses different clock names).
>>>>>>>>
>>>>>>>
>>>>>>> Thank you for your patch. There is something to discuss/improve.
>>>>>>>
>>>>>>>> +title: Atmel Power Management Controller (PMC)
>>>>>>>> +
>>>>>>>> +maintainers:
>>>>>>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>>>>>>>> +
>>>>>>>> +description:
>>>>>>>> +  The power management controller optimizes power consumption by controlling all
>>>>>>>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
>>>>>>>> +  to many of the peripherals and to the processor.
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> +  compatible:
>>>>>>>> +    oneOf:
>>>>>>>> +      - items:
>>>>>>>> +          - enum:
>>>>>>>> +              - atmel,at91sam9g15-pmc
>>>>>>>> +              - atmel,at91sam9g20-pmc
>>>>>>>> +              - atmel,at91sam9g25-pmc
>>>>>>>> +              - atmel,at91sam9g35-pmc
>>>>>>>> +              - atmel,at91sam9x25-pmc
>>>>>>>> +              - atmel,at91sam9x35-pmc
>>>>>>>> +          - enum:
>>>>>>>> +              - atmel,at91sam9260-pmc
>>>>>>>> +              - atmel,at91sam9x5-pmc
>>>>>>>
>>>>>>> I missed it last time - why you have two enums? We never talked about
>>>>>>> this. It's usually wrong... are you sure this is real hardware:
>>>>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>>>>>> ?
>>>>>>
>>>>>> I have 2 enums because there are some hardware covered by:
>>>>>> "vendor-name,hardware-v1-pmc", "syscon" and some covered by:
>>>>>> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".
>>>>>
>>>>> The enum does not say this. At all.
>>>>>
>>>>> So again, answer, do not ignore:
>>>>> is this valid setup:
>>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>>>> ?
>>>>
>>>> Not w/o syscon. This is valid:
>>>
>>> Syscon is not important here, but indeed I missed it.
>>>
>>>>
>>>> compatible = "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc", "syscon";
>>>>
>>>> available in arch/arm/boot/dts/at91sam9g20.dtsi +45
>>>
>>> Nice, so my random choice was actually correct. Ok, so another:
>>>
>>> atmel,at91sam9g15-pmc, atmel,at91sam9260-pmc, syscon
>>>
>>> Is it valid hardware?
>>
>> This one, no. So, I guess, the wrong here is that there could be
>> combinations that are not for actual hardware and yet considered valid by
>> changes in this patch?
> 
> I just don't understand why you have two enums. This is not a pattern
> which is allowed anywhere. It might appear but only as exception or mistake.

I'm not at all an YAML expert and this is how I've managed to make
dt_binding_check/dtbs_check happy.

> 
> 
> Best regards,
> Krzysztof
>
Conor Dooley May 11, 2023, 8:58 a.m. UTC | #9 
On Thu, May 11, 2023 at 06:29:39AM +0000, Claudiu.Beznea@microchip.com wrote:
> On 10.05.2023 13:12, Krzysztof Kozlowski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 10/05/2023 10:31, Claudiu.Beznea@microchip.com wrote:
> >> On 10.05.2023 10:58, Krzysztof Kozlowski wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On 10/05/2023 09:14, Claudiu.Beznea@microchip.com wrote:
> >>>> On 10.05.2023 10:06, Krzysztof Kozlowski wrote:
> >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>>>
> >>>>> On 10/05/2023 09:00, Claudiu.Beznea@microchip.com wrote:
> >>>>>> On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
> >>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>>>>>
> >>>>>>> On 09/05/2023 07:27, Claudiu Beznea wrote:
> >>>>>>>> Convert Atmel PMC documentation to yaml. Along with it clock names
> >>>>>>>> were adapted according to the current available device trees as
> >>>>>>>> different controller versions accept different clocks (some of them
> >>>>>>>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
> >>>>>>>> input clocks uses different clock names).
> >>>>>>>>
> >>>>>>>
> >>>>>>> Thank you for your patch. There is something to discuss/improve.
> >>>>>>>
> >>>>>>>> +title: Atmel Power Management Controller (PMC)
> >>>>>>>> +
> >>>>>>>> +maintainers:
> >>>>>>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
> >>>>>>>> +
> >>>>>>>> +description:
> >>>>>>>> +  The power management controller optimizes power consumption by controlling all
> >>>>>>>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
> >>>>>>>> +  to many of the peripherals and to the processor.
> >>>>>>>> +
> >>>>>>>> +properties:
> >>>>>>>> +  compatible:
> >>>>>>>> +    oneOf:
> >>>>>>>> +      - items:
> >>>>>>>> +          - enum:
> >>>>>>>> +              - atmel,at91sam9g15-pmc
> >>>>>>>> +              - atmel,at91sam9g20-pmc
> >>>>>>>> +              - atmel,at91sam9g25-pmc
> >>>>>>>> +              - atmel,at91sam9g35-pmc
> >>>>>>>> +              - atmel,at91sam9x25-pmc
> >>>>>>>> +              - atmel,at91sam9x35-pmc
> >>>>>>>> +          - enum:
> >>>>>>>> +              - atmel,at91sam9260-pmc
> >>>>>>>> +              - atmel,at91sam9x5-pmc
> >>>>>>>
> >>>>>>> I missed it last time - why you have two enums? We never talked about
> >>>>>>> this. It's usually wrong... are you sure this is real hardware:
> >>>>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
> >>>>>>> ?
> >>>>>>
> >>>>>> I have 2 enums because there are some hardware covered by:
> >>>>>> "vendor-name,hardware-v1-pmc", "syscon" and some covered by:
> >>>>>> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".
> >>>>>
> >>>>> The enum does not say this. At all.
> >>>>>
> >>>>> So again, answer, do not ignore:
> >>>>> is this valid setup:
> >>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
> >>>>> ?
> >>>>
> >>>> Not w/o syscon. This is valid:
> >>>
> >>> Syscon is not important here, but indeed I missed it.
> >>>
> >>>>
> >>>> compatible = "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc", "syscon";
> >>>>
> >>>> available in arch/arm/boot/dts/at91sam9g20.dtsi +45
> >>>
> >>> Nice, so my random choice was actually correct. Ok, so another:
> >>>
> >>> atmel,at91sam9g15-pmc, atmel,at91sam9260-pmc, syscon
> >>>
> >>> Is it valid hardware?
> >>
> >> This one, no. So, I guess, the wrong here is that there could be
> >> combinations that are not for actual hardware and yet considered valid by
> >> changes in this patch?
> > 
> > I just don't understand why you have two enums. This is not a pattern
> > which is allowed anywhere. It might appear but only as exception or mistake.
> 
> I'm not at all an YAML expert and this is how I've managed to make
> dt_binding_check/dtbs_check happy.

Picking one item at random, do the devicetrees contain stuff like:
"atmel,at91sam9g35-pmc", "atmel,at91sam9260-pmc", "syscon"
//AND//
"atmel,at91sam9g35-pmc", "atmel,at91sam9x5-pmc", "syscon"
?

If not, why do you not break it down to something like:
- items:
    - enum:
        - atmel,compatible
        - atmel,with
        - atmel,sam9260's pmc
    - const: atmel,at91sam9260-pmc
    - const: syscon

- items:
    - enum:
        - atmel,compatible
        - atmel,with
        - atmel,sam9x5's pmc
    - const: atmel,at91sam9x5-pmc
    - const: syscon

Cheers,
Conor.
Claudiu Beznea May 12, 2023, 7:59 a.m. UTC | #10 
On 11.05.2023 11:58, Conor Dooley wrote:
> On Thu, May 11, 2023 at 06:29:39AM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 10.05.2023 13:12, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 10/05/2023 10:31, Claudiu.Beznea@microchip.com wrote:
>>>> On 10.05.2023 10:58, Krzysztof Kozlowski wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 10/05/2023 09:14, Claudiu.Beznea@microchip.com wrote:
>>>>>> On 10.05.2023 10:06, Krzysztof Kozlowski wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> On 10/05/2023 09:00, Claudiu.Beznea@microchip.com wrote:
>>>>>>>> On 09.05.2023 09:25, Krzysztof Kozlowski wrote:
>>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>>>
>>>>>>>>> On 09/05/2023 07:27, Claudiu Beznea wrote:
>>>>>>>>>> Convert Atmel PMC documentation to yaml. Along with it clock names
>>>>>>>>>> were adapted according to the current available device trees as
>>>>>>>>>> different controller versions accept different clocks (some of them
>>>>>>>>>> have 3 clocks as input, some has 2 clocks as inputs and some with 2
>>>>>>>>>> input clocks uses different clock names).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thank you for your patch. There is something to discuss/improve.
>>>>>>>>>
>>>>>>>>>> +title: Atmel Power Management Controller (PMC)
>>>>>>>>>> +
>>>>>>>>>> +maintainers:
>>>>>>>>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>>>>>>>>>> +
>>>>>>>>>> +description:
>>>>>>>>>> +  The power management controller optimizes power consumption by controlling all
>>>>>>>>>> +  system and user peripheral clocks. The PMC enables/disables the clock inputs
>>>>>>>>>> +  to many of the peripherals and to the processor.
>>>>>>>>>> +
>>>>>>>>>> +properties:
>>>>>>>>>> +  compatible:
>>>>>>>>>> +    oneOf:
>>>>>>>>>> +      - items:
>>>>>>>>>> +          - enum:
>>>>>>>>>> +              - atmel,at91sam9g15-pmc
>>>>>>>>>> +              - atmel,at91sam9g20-pmc
>>>>>>>>>> +              - atmel,at91sam9g25-pmc
>>>>>>>>>> +              - atmel,at91sam9g35-pmc
>>>>>>>>>> +              - atmel,at91sam9x25-pmc
>>>>>>>>>> +              - atmel,at91sam9x35-pmc
>>>>>>>>>> +          - enum:
>>>>>>>>>> +              - atmel,at91sam9260-pmc
>>>>>>>>>> +              - atmel,at91sam9x5-pmc
>>>>>>>>>
>>>>>>>>> I missed it last time - why you have two enums? We never talked about
>>>>>>>>> this. It's usually wrong... are you sure this is real hardware:
>>>>>>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>>>>>>>> ?
>>>>>>>>
>>>>>>>> I have 2 enums because there are some hardware covered by:
>>>>>>>> "vendor-name,hardware-v1-pmc", "syscon" and some covered by:
>>>>>>>> "vendor-name,hardware-v2-pmc", "vendor-name,hardware-v1-pmc", "syscon".
>>>>>>>
>>>>>>> The enum does not say this. At all.
>>>>>>>
>>>>>>> So again, answer, do not ignore:
>>>>>>> is this valid setup:
>>>>>>> atmel,at91sam9g20-pmc, atmel,at91sam9260-pmc
>>>>>>> ?
>>>>>>
>>>>>> Not w/o syscon. This is valid:
>>>>>
>>>>> Syscon is not important here, but indeed I missed it.
>>>>>
>>>>>>
>>>>>> compatible = "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc", "syscon";
>>>>>>
>>>>>> available in arch/arm/boot/dts/at91sam9g20.dtsi +45
>>>>>
>>>>> Nice, so my random choice was actually correct. Ok, so another:
>>>>>
>>>>> atmel,at91sam9g15-pmc, atmel,at91sam9260-pmc, syscon
>>>>>
>>>>> Is it valid hardware?
>>>>
>>>> This one, no. So, I guess, the wrong here is that there could be
>>>> combinations that are not for actual hardware and yet considered valid by
>>>> changes in this patch?
>>>
>>> I just don't understand why you have two enums. This is not a pattern
>>> which is allowed anywhere. It might appear but only as exception or mistake.
>>
>> I'm not at all an YAML expert and this is how I've managed to make
>> dt_binding_check/dtbs_check happy.
> 
> Picking one item at random, do the devicetrees contain stuff like:
> "atmel,at91sam9g35-pmc", "atmel,at91sam9260-pmc", "syscon"
> //AND//
> "atmel,at91sam9g35-pmc", "atmel,at91sam9x5-pmc", "syscon"
> ?
> 
> If not, why do you not break it down to something like:
> - items:
>     - enum:
>         - atmel,compatible
>         - atmel,with
>         - atmel,sam9260's pmc
>     - const: atmel,at91sam9260-pmc
>     - const: syscon
> 
> - items:
>     - enum:
>         - atmel,compatible
>         - atmel,with
>         - atmel,sam9x5's pmc
>     - const: atmel,at91sam9x5-pmc
>     - const: syscon
> 

I'll check it out, thank you!

> Cheers,
> Conor.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/at91-clock.txt b/Documentation/devicetree/bindings/clock/at91-clock.txt
index 13f45db3b66d..57394785d3b0 100644
--- a/Documentation/devicetree/bindings/clock/at91-clock.txt
+++ b/Documentation/devicetree/bindings/clock/at91-clock.txt
@@ -28,31 +28,3 @@  For example:
 		#clock-cells = <0>;
 	};
 
-Power Management Controller (PMC):
-
-Required properties:
-- compatible : shall be "atmel,<chip>-pmc", "syscon" or
-	"microchip,sam9x60-pmc"
-	<chip> can be: at91rm9200, at91sam9260, at91sam9261,
-	at91sam9263, at91sam9g45, at91sam9n12, at91sam9rl, at91sam9g15,
-	at91sam9g25, at91sam9g35, at91sam9x25, at91sam9x35, at91sam9x5,
-	sama5d2, sama5d3 or sama5d4.
-- #clock-cells : from common clock binding; shall be set to 2. The first entry
-  is the type of the clock (core, system, peripheral or generated) and the
-  second entry its index as provided by the datasheet
-- clocks : Must contain an entry for each entry in clock-names.
-- clock-names: Must include the following entries: "slow_clk", "main_xtal"
-
-Optional properties:
-- atmel,osc-bypass : boolean property. Set this when a clock signal is directly
-  provided on XIN.
-
-For example:
-	pmc: pmc@f0018000 {
-		compatible = "atmel,sama5d4-pmc", "syscon";
-		reg = <0xf0018000 0x120>;
-		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
-		#clock-cells = <2>;
-		clocks = <&clk32k>, <&main_xtal>;
-		clock-names = "slow_clk", "main_xtal";
-	};
diff --git a/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
new file mode 100644
index 000000000000..aa727dee6729
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml
@@ -0,0 +1,152 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/atmel,at91rm9200-pmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Atmel Power Management Controller (PMC)
+
+maintainers:
+  - Claudiu Beznea <claudiu.beznea@microchip.com>
+
+description:
+  The power management controller optimizes power consumption by controlling all
+  system and user peripheral clocks. The PMC enables/disables the clock inputs
+  to many of the peripherals and to the processor.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - atmel,at91sam9g15-pmc
+              - atmel,at91sam9g20-pmc
+              - atmel,at91sam9g25-pmc
+              - atmel,at91sam9g35-pmc
+              - atmel,at91sam9x25-pmc
+              - atmel,at91sam9x35-pmc
+          - enum:
+              - atmel,at91sam9260-pmc
+              - atmel,at91sam9x5-pmc
+          - const: syscon
+      - items:
+          - enum:
+              - atmel,at91rm9200-pmc
+              - atmel,at91sam9260-pmc
+              - atmel,at91sam9g45-pmc
+              - atmel,at91sam9n12-pmc
+              - atmel,at91sam9rl-pmc
+              - atmel,sama5d2-pmc
+              - atmel,sama5d3-pmc
+              - atmel,sama5d4-pmc
+              - microchip,sam9x60-pmc
+              - microchip,sama7g5-pmc
+          - const: syscon
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#clock-cells":
+    description: |
+      - 1st cell is the clock type, one of PMC_TYPE_CORE, PMC_TYPE_SYSTEM,
+        PMC_TYPE_PERIPHERAL, PMC_TYPE_GCK, PMC_TYPE_PROGRAMMABLE (as defined
+        in <dt-bindings/clock/at91.h>)
+      - 2nd cell is the clock identifier as defined in <dt-bindings/clock/at91.h
+        (for core clocks) or as defined in datasheet (for system, peripheral,
+        gck and programmable clocks).
+    const: 2
+
+  clocks:
+    minItems: 2
+    maxItems: 3
+
+  clock-names:
+    minItems: 2
+    maxItems: 3
+
+  atmel,osc-bypass:
+    description: set when a clock signal is directly provided on XIN
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#clock-cells"
+  - clocks
+  - clock-names
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - microchip,sam9x60-pmc
+              - microchip,sama7g5-pmc
+    then:
+      properties:
+        clocks:
+          minItems: 3
+          maxItems: 3
+        clock-names:
+          items:
+            - const: td_slck
+            - const: md_slck
+            - const: main_xtal
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - atmel,at91rm9200-pmc
+              - atmel,at91sam9260-pmc
+              - atmel,at91sam9g20-pmc
+    then:
+      properties:
+        clocks:
+          minItems: 2
+          maxItems: 2
+        clock-names:
+          items:
+            - const: slow_xtal
+            - const: main_xtal
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - atmel,sama5d2-pmc
+              - atmel,sama5d3-pmc
+              - atmel,sama5d4-pmc
+    then:
+      properties:
+        clocks:
+          minItems: 2
+          maxItems: 2
+        clock-names:
+          items:
+            - const: slow_clk
+            - const: main_xtal
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    pmc: clock-controller@f0018000 {
+        compatible = "atmel,sama5d4-pmc", "syscon";
+        reg = <0xf0018000 0x120>;
+        interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+        #clock-cells = <2>;
+        clocks = <&clk32k>, <&main_xtal>;
+        clock-names = "slow_clk", "main_xtal";
+    };
+
+...