Re: [PATCH 2/9] dt-bindings: pinctrl: stm32: Introduce HDP

[Clement LE GOFFIC <clement.legoffic@foss.st.com>]

On 2/26/25 16:05, Krzysztof Kozlowski wrote:
> On 26/02/2025 11:52, Clement LE GOFFIC wrote:
>> On 2/26/25 08:21, Krzysztof Kozlowski wrote:
>>> On 25/02/2025 16:51, Clement LE GOFFIC wrote:
>>>> On 2/25/25 14:04, Krzysztof Kozlowski wrote:
>>>>> On 25/02/2025 09:48, Clément Le Goffic wrote:
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Clément LE GOFFIC <clement.legoffic@foss.st.com>
>>>>>> +
>>>>>> +description: |
>>>>>
>>>>>
>>>>> Do not need '|' unless you need to preserve formatting.
>>>>
>>>> Ok
>>>>
>>>>>> +  STMicroelectronics's STM32 MPUs integrate a Hardware Debug Port (HDP).
>>>>>> +  It allows to output internal signals on SoC's GPIO.
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: st,stm32mp-hdp
>>>>>
>>>>> There is a mess in STM SoCs. Sometimes you call SoC stm32, sometimes
>>>>> stm32mp and sometimes stm32mpXX.
>>>>>
>>>>> Define for all your STM contributions what is the actual SoC. This
>>>>> feedback was already given to ST.
>>>>>
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  clocks:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +patternProperties:
>>>>>> +  '-pins$':
>>>>>> +    type: object
>>>>>> +    $ref: pinmux-node.yaml#
>>>>>> +
>>>>>> +    properties:
>>>>>> +      function:
>>>>>> +        enum: [ "0", "1", "2", "3", "4", "5", "6", "7",
>>>>>> +                "8", "9", "10", "11", "12", "13", "14",
>>>>>> +                "15" ]
>>>>>
>>>>> Function which has a number is not really useful. What does it even express?
>>>>
>>>> As said in my previous answer, function names are very different from
>>>> one platform to another. Numbers were used as string to be generic.
>>>> I'll consider it in a V2.
>>>
>>> What does it mean "one platform to another"? This is one platform! Is
>>> this some sort of continuation of SoC compatible mess?
>>
>> I may used incorrectly the word platform.
>> This driver is the same for the three SoC families STM32MP13, STM32MP15
> 
> That's driver and it is fine, but we talk about hardware here. The
> binding is for given specific hardware.
> 
>> and STM32MP25 because the hardware is mostly the same.
>>
>> Why mostly ?
>>
>> The peripheral is behaving as a mux, there are 8 HDP ports, for each
>> port there is up to 16 possible hardware signals. Numbered from 0 to 15.
>> Each of this number represent a signal on the port.
>>
>> But the hardware signal behind the number is not the same from one SoC
>> family to another.
>> As example, in STM32MP15 family the HDP is able to output GPU hardware
>> signals because the family has a GPU but in the STM32MP13 family this
>> signal is not present.
> 
> It looks like you have clear mapping between function and port number
> (your header also suggests that), so the function property should follow
> that user-visible function.
> 
> Just like we do for many other architectures - it is not that very, very
> different, I think. all of platform hardwares do not operate on strings
> but some bits in registers (so numbers) but all (ideally) bindings
> operate on strings. You created here exception on basis this is somehow
> special, but the point is: it is not special.
> 
>>
>> The purpose of my helpers was to give a readable name to facilitate the
>> configuration in boards devicetree's. If needed I can get rid of that
>> and use only the number as string.
> 
> If you use "names" you do not need even that helper header.
> 
>>
>>> What are the exact functions written in datasheet?
>>
>> The exact functions name written in the datasheet are the ones of my
>> helper file without the HDP prefix.
> 
> so full strings "pwr_pwrwake_sys" and these should be used.

Ok so in the V2, I'll keep the 'function' property of the pinmux and use 
signal names such as 'pwr_pwrwake_sys' to select signals in the DT.
The signal names are different from one SoC to another (stm32mp131, 
stm32mp151 and stm32mp251) so I'll need compatible data and the 
compatibles will be:

MP15: compatible = "st,stm32mp151-hdp";
MP13: compatible = "st,stm32mp131-hdp";
MP25: compatible = "st,stm32mp251-hdp";


> Best regards,
> Krzysztof