On Tue, Feb 18, 2025 at 10:41:31PM +0100, Alexander Sverdlin wrote: > Thank you for your feedback Rob! > > On Tue, 2025-02-18 at 15:06 -0600, Rob Herring wrote: > > > QUESTION: > > > > > > I'm unsure about reg properties in the subnodes (child devices) of > > > RTCSYS: > > > - they will not be used anyway by the drivers because they genuinely > > > overlap (the whole point of going MFD) -- therefore the drivers will do > > > syscon_node_to_regmap(pdev->dev.parent->of_node) > > > - as I understood from the history of MFD dt bindings' submissions, regs > > > are encouraged, if can be specified > > > - overlapping regs cause dt_binding_check warnings: > > > Documentation/devicetree/bindings/mfd/sophgo,cv1800b-rtcsys.example.dts:34.19-39.15: Warning (unique_unit_address_if_enabled): /example-0/rtcsys@5025000/mcu@0: duplicate unit-address (also used in > > > node /example-0/rtcsys@5025000/pmu@0) > > > Documentation/devicetree/bindings/mfd/sophgo,cv1800b-rtcsys.example.dts:34.19-39.15: Warning (unique_unit_address_if_enabled): /example-0/rtcsys@5025000/mcu@0: duplicate unit-address (also used in > > > node /example-0/rtcsys@5025000/rtc@0) > > > > > > Shall I remove the MMIO resources from the actual devices or rather ignore the warnings? > > > > Ignore the warnings is not an option. > > > > Removing makes since if the registers and bitfields are completely mixed > > up. If they are, then I find it hard to believe the child nodes are > > separate blocks. And if they aren't, then it should all be just 1 node. > > The HW vendor calls it "RTC". But this "RTC" is also responsible for the > whole power sequencing and [chip-wide] power management. And afterwards > they've put SRAM controller registers and remoteproc (independent 8051 core) > controller into the same address space (interleaved). I have hard times > to apply any strict logic here. > > > You don't have to have child nodes to have separate drivers. > > But if I don't utilize "simple-mfd" and children nodes, then I'd need > some MFD core driver registering the "cells" even though, there will be > no other functions in it? > > On the other hand, maybe this is the way forward if we are unsure as > of now, which cells do we want to implement at all as a separate driver > and which ones are we going to combine in a single driver?.. Those are all OS questions which have little to do with the DT binding. Design the binding to best match the h/w. > > > .../bindings/mfd/sophgo,cv1800b-rtcsys.yaml | 222 ++++++++++++++++++ > > > 1 file changed, 222 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/mfd/sophgo,cv1800b-rtcsys.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/sophgo,cv1800b-rtcsys.yaml b/Documentation/devicetree/bindings/mfd/sophgo,cv1800b-rtcsys.yaml > > > new file mode 100644 > > > index 000000000000..2dc7c2df15c1 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mfd/sophgo,cv1800b-rtcsys.yaml > > > @@ -0,0 +1,222 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/mfd/sophgo,cv1800b-rtcsys.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Cvitek CV18xx/Sophgo SG200x Real Time Clock module > > > + > > > +maintainers: > > > + - Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx> > > > + - sophgo@xxxxxxxxxxxxxxx > > > + > > > +description: > > > + The RTC (Real Time Clock) is an independently powered module in the chip. It > > > + contains a 32KHz oscillator and a Power-On-Reset (POR) sub-module, which can > > > + be used for time display and scheduled alarm produce. In addition, the > > > + hardware state machine provides triggering and timing control for chip > > > + power-on, power-off and reset. > > > + > > > + Furthermore, the 8051 subsystem is located within RTCSYS and is independently > > > + powered. System software can use the 8051 to manage wake conditions and wake > > > + the system while the system is asleep, and communicate with external devices > > > + through peripheral controllers. > > > + > > > + Technical Reference Manual available at > > > + https://github.com/sophgo/sophgo-doc/releases/download/sg2000-trm-v1.01/sg2000_trm_en.pdf > > > + > > [...] > > > > > > + "^sram@[0-9a-f]+$": > > > + type: object > > > + additionalProperties: false > > > + > > > + description: > > > + Provide 2KB of SRAM, which can host software code or temporary data. > > > + > > > + properties: > > > + compatible: > > > + items: > > > + - enum: > > > + - sophgo,cv1800b-rtc-sram > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clocks: > > > + maxItems: 1 > > > + > > > + required: > > > + - compatible > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - "#address-cells" > > > + - "#size-cells" > > > + - ranges > > > + > > > +additionalProperties: > > > + type: object > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/clock/sophgo,cv1800.h> > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > + > > > + rtcsys@5025000 { > > > + compatible = "sophgo,cv1800b-rtcsys", "simple-mfd", "syscon"; > > > + reg = <0x5025000 0x2000>; > > > + #address-cells = <1>; > > > + #size-cells = <1>; > > > + ranges = <0 0x5025000 0x2000>; > > > + > > > + mcu@0 { > > > + compatible = "sophgo,cv1800b-rtc-dw8051"; > > > + reg = <0x0 0x1000>; > > > + clocks = <&clk CLK_SRC_RTC_SYS_0>; > > > + sram = <&rtc_sram>; > > > + }; > > > + > > > + pmu@0 { > > > + compatible = "sophgo,cv1800b-rtc-pmu"; > > > + reg = <0x0 0x2000>; > > > + interrupts = <18 IRQ_TYPE_LEVEL_HIGH>, > > > + <19 IRQ_TYPE_LEVEL_HIGH>; > > > + interrupt-names = "longpress", "vbat"; > > > + }; > > > + > > > + rtc@0 { > > > + compatible = "sophgo,cv1800b-rtc"; > > > + reg = <0 0x2000>; > > > + interrupts = <17 IRQ_TYPE_LEVEL_HIGH>; > > > + interrupt-names = "alarm"; > > > + clocks = <&clk CLK_RTC_25M>; > > > + }; > > > + > > > + rtc_sram: sram@0 { > > > + compatible = "sophgo,cv1800b-rtc-sram"; > > > + reg = <0x0 0x1000>; > > > > How does the SRAM overlap registers? > > Those are not SRAM cells mapped into this address space, > but rather several control registers controlling reset, > power and clock of the SRAM. The 'sram' property points to regions of SRAM memory. So what you have is incorrect use of the property. Rob
