Re: [PATCH 2/5] dt-bindings: mfd: syscon: Add ti,am62-ddr-pmctrl

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 07/02/2025 15:40, Markus Schneider-Pargmann wrote:
> Hi Krzysztof,
> 
> On Mon, Jan 27, 2025 at 01:09:49PM +0100, Krzysztof Kozlowski wrote:
>> On 24/01/2025 23:35, Andrew Davis wrote:
>>> On 1/24/25 10:48 AM, Krzysztof Kozlowski wrote:
>>>> On 24/01/2025 17:05, Markus Schneider-Pargmann wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On Fri, Jan 24, 2025 at 09:22:54AM +0100, Krzysztof Kozlowski wrote:
>>>>>> On Fri, Jan 24, 2025 at 09:19:49AM +0100, Krzysztof Kozlowski wrote:
>>>>>>> On Wed, Jan 22, 2025 at 11:24:33AM +0100, Markus Schneider-Pargmann wrote:
>>>>>>>> Add compatible for ti,am62-ddr-pmctrl to the list. There is a DDR pmctrl
>>>>>>>> register in the wkup-conf register space of am62a and am62p. This
>>>>>>>> register controls DDR power management.
>>>>>>>>
>>>>>>>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
>>>>>>>> ---
>>>>>>>>   Documentation/devicetree/bindings/mfd/syscon.yaml | 2 ++
>>>>>>>>   1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>>
>>>>>> Un-acked, I missed the point that you really speak in commit msg about
>>>>>> register and you really treat one register is a device. I assumed you
>>>>>> only need that register from this device, but no. That obviously is not
>>>>>> what this device is. Device is not a single register among 10000 others.
>>>>>> IOW, You do not have 10000 devices there.
>>>>>
>>>>> Do I understand you correctly that the whole register range of the
>>>>> wkup_conf node as seen in arch/arm64/boot/dts/ti/k3-am62a-wakeup.dtsi
>>>>> should be considered a single syscon device?
>>>>
>>>> I don't have the datasheets (and not my task to actually check this),
>>>> but you should probably follow datasheet. I assume it describes what is
>>>> the device, more or less.
>>>>
>>>> I assume entire wkup_conf is considered a device.
>>>>
>>>>>
>>>>> Unfortunately wkup_conf is modeled as a simple-bus with currently 5
>>>>> subnodes defined of which 4 of them consist of a single register. Most
>>>>> of them are syscon as well. So I think I can't change the simple-bus
>>>>> back to syscon.
>>>>
>>>> Huh... Maybe TI folks will help us understand why such design was chosen.
>>>>
>>>
>>> Many of the devices inside the wkup_conf are already modeled as such.
>>> Clocks and muxes for instance already have drivers and bindings, this
>>> is nothing new to TI.
>>>
>>> If we just use a blank "syscon" over the entire region we would end up
>>> with drivers that use phandles to the top level wkup_conf node and
>>> poke directly the registers they need from that space.
>>>
>>> Would you rather have
>>>
>>> some-device {
>>> 	ti,epwm_tbclk = <&wkup_conf>;
>>> }
>>>
>>> or
>>>
>>> some-device {
>>> 	clocks = <&epwm_tbclk 0>;
>>> }
>>
>> How is this comparable? These are clocks. You would have clocks property
>> in both cases.
>>
>>
>>>
>>> with that epwm_tbclk being a proper clock node inside wkup_conf?
>>> I would much prefer the second, even though the clock node
>>> only uses a single register. And in the first case, we would need
>>> to have the offset into the wkup_conf space hard-coded in the
>>> driver for each new SoC. Eventually all that data would need to be
>>> put in tables and we end up back to machine board files..
>>>
>>> I'm not saying every magic number in all drivers should
>>> be offloaded into DT, but there is a line somewhere between
>>> that and having the DT simply contain the SoC's name compatible
>>
>> That's not the question here.
>>
>>> and all other data going into the kernel. That line might be a
>>> personal preference, so my question back is: what is wrong
>>> if we do want "1000 new syscons per each register" for our
>>> SoCs DT?
>>
>> Because it is false representation of hardware. You do not have 1000
>> devices. You have only one device.
>>
>>
>>>
>>> (and the number is not 1000, scanning the kernel I can see
>>> the largest wkup_conf region node we have today has a grand
>>> total number sub-nodes of 6)
>>
>> But what is being added here is device per each register, not per feature.
> 
> The register layout is like this:

The register layout of what? How is the device called? Is datasheet
available anywhere?

> 
> 0x8010 - 0x803c contains 4 clockselect registers
> 0x80d0 is the DDR16SS_PMCTRL regsiter
> 0x8190 - 0x8600 contains another 7 clockselect registers
> 
> I see the feature here in the block being clockselect registers. But the
> ddr-pmctrl register doesn't fit into this so I opted to describe this
> single register as one node as it looked to me like one feature. Of
> course I would have preferred this to be different but it is not. Would
> you prefer the clockselect registers and the pmctrl register to be
> described as one syscon?
No, because all the time you speak register=device and all the time I
was explaining that this is not correct. Device is the collection of
registers. I already said it - entire block is the syscon, not one
register in the middle, not 2 registers in the middle, not even 4+7
registers in the middle.


Best regards,
Krzysztof