Message-ID: <qjwlppsq4eorzepvjsgjjyyaddouo5w2rjguu5c2mqesd6luwp@f426xeghy2ht>
Date: Mon, 10 Feb 2025 11:35:02 +0100
From: Markus Schneider-Pargmann <msp@...libre.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Andrew Davis <afd@...com>, Lee Jones <lee@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Siddharth Vadapalli <s-vadapalli@...com>, 
	Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>, 
	Tero Kristo <kristo@...nel.org>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/5] dt-bindings: mfd: syscon: Add ti,am62-ddr-pmctrl

On Sun, Feb 09, 2025 at 01:21:27PM +0100, Krzysztof Kozlowski wrote:
> 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@...libre.com>
> >>>>>>>> ---
> >>>>>>>>   Documentation/devicetree/bindings/mfd/syscon.yaml | 2 ++
> >>>>>>>>   1 file changed, 2 insertions(+)
> >>>>>>>
> >>>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.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?

Yes, it is available here: https://www.ti.com/de/lit/pdf/spruj16

14 Registers
14.2 Device Configuration Registers
14.2.1 CTRL_MMR Registers
14.2.1.1 General Purpose Control Registers
14.2.1.1.3 WKUP_CTRL_MMR0 Registers

Each domain has their own set of general purpose control registers,
CTRL_MMR for the main domain, MCU_CTRL_MMR0 for the MCU domain,
WKUP_CTRL_MMR0 for the wakeup domain.

So I understand this to just be a collection of general purpose control
registers. If you go by feature, then many of the registers can be
grouped into units with a specific purpose or controlling a specific
device which are also grouped by the offsets they represent. I assume
this is why the other nodes in this wkup_conf node were created. Also in
my opinion this makes the relation between the original device and this
general purpose control registers better understandable.

For this patch the ddr-pmctrl regsiter is just a single register, but it
has the purpose of controlling the DDR device power management.

The other patch I posted is a collection of registers for the
CANUART_WAKE functionality. CANUART here is a group of devices CAN and
UART devices. Thes also have a specific purpose for a specific part of
the SoC.

Best
Markus

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists