| Message ID | 20250103215636.19967-4-heylenay@4d2.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add clock controller support for SpacemiT K1 | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote: > Add documentation to describe Spacemit K1 system controller registers. > > Signed-off-by: Haylen Chu <heylenay@4d2.org> > --- > .../soc/spacemit/spacemit,k1-syscon.yaml | 52 +++++++++++++++++++ > 1 file changed, 52 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > > diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > new file mode 100644 > index 000000000000..79c4a74ff30e > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > @@ -0,0 +1,52 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Spacemit K1 SoC System Controller > + > +maintainers: > + - Haylen Chu <heylenay@4d2.org> > + > +description: > + The Spacemit K1 SoC system controller provides access to shared register files > + for related SoC modules, such as clock controller and reset controller. > + > +properties: > + compatible: > + items: > + - enum: > + - spacemit,k1-apbc-syscon > + - spacemit,k1-apbs-syscon > + - spacemit,k1-apmu-syscon > + - spacemit,k1-mpmu-syscon > + - const: syscon > + - const: simple-mfd > + > + reg: > + maxItems: 1 > + > + clock-controller: > + $ref: /schemas/clock/spacemit,k1-ccu.yaml# > + type: object So now we see the full picture and it leads to questions. 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a correct combination? 2. Why having this split in the first place? Please confirm that clock controller is really, really a separate device and its child in datasheet. IOW, fake child for your Linux is a no-go. Fake child while devices are independent is another no-go. Actual answer for 1+2 above would be to fold the child into parent, assuming clock controller split is fake in terms of datasheet. If it is real device, then allOf:if:then: narrowing the compatibles of child might not be worth the complexity. 3. Why using different naming, look: spacemit,k1-XXXX-syscon spacemit,k1-ccu-XXXX Best regards, Krzysztof
On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote: > On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote: > > Add documentation to describe Spacemit K1 system controller registers. > > > > Signed-off-by: Haylen Chu <heylenay@4d2.org> > > --- > > .../soc/spacemit/spacemit,k1-syscon.yaml | 52 +++++++++++++++++++ > > 1 file changed, 52 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > > > > diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > > new file mode 100644 > > index 000000000000..79c4a74ff30e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > > @@ -0,0 +1,52 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Spacemit K1 SoC System Controller > > + > > +maintainers: > > + - Haylen Chu <heylenay@4d2.org> > > + > > +description: > > + The Spacemit K1 SoC system controller provides access to shared register files > > + for related SoC modules, such as clock controller and reset controller. > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - spacemit,k1-apbc-syscon > > + - spacemit,k1-apbs-syscon > > + - spacemit,k1-apmu-syscon > > + - spacemit,k1-mpmu-syscon > > + - const: syscon > > + - const: simple-mfd > > + > > + reg: > > + maxItems: 1 > > + > > + clock-controller: > > + $ref: /schemas/clock/spacemit,k1-ccu.yaml# > > + type: object > > So now we see the full picture and it leads to questions. > > 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a > correct combination? > > 2. Why having this split in the first place? Please confirm that clock > controller is really, really a separate device and its child in > datasheet. IOW, fake child for your Linux is a no-go. Fake child while > devices are independent is another no-go. These syscons are introduced because the clock controllers share registers with reset controllers. Folding them into the parents results in devicetree nodes act as both reset and clock controllers, like what has been done for Rockchip SoCs. Such folding isn't practical for the MPMU region either, since watchdog and other misc bits (e.g. PLL lock status) locates in it. If you're more comfortable with reset and clock controllers folded together and eliminating most of these syscons, I'm willing to make the change. > Actual answer for 1+2 above would be to fold the child into parent, > assuming clock controller split is fake in terms of datasheet. > > If it is real device, then allOf:if:then: narrowing the compatibles of > child might not be worth the complexity. > > 3. Why using different naming, look: > > spacemit,k1-XXXX-syscon > spacemit,k1-ccu-XXXX I didn't consider about consistency when naming them. Talked to Yixun, I'll unify them as spacemit,k1-syscon-* and spacemit,k1-ccu-*, keeping synchronized with other K1 peripherals. > > Best regards, > Krzysztof > Thanks, Haylen Chu
On 11/02/2025 06:15, Haylen Chu wrote: > On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote: >> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote: >>> Add documentation to describe Spacemit K1 system controller registers. >>> >>> Signed-off-by: Haylen Chu <heylenay@4d2.org> >>> --- >>> .../soc/spacemit/spacemit,k1-syscon.yaml | 52 +++++++++++++++++++ >>> 1 file changed, 52 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >>> new file mode 100644 >>> index 000000000000..79c4a74ff30e >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >>> @@ -0,0 +1,52 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Spacemit K1 SoC System Controller >>> + >>> +maintainers: >>> + - Haylen Chu <heylenay@4d2.org> >>> + >>> +description: >>> + The Spacemit K1 SoC system controller provides access to shared register files >>> + for related SoC modules, such as clock controller and reset controller. >>> + >>> +properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - spacemit,k1-apbc-syscon >>> + - spacemit,k1-apbs-syscon >>> + - spacemit,k1-apmu-syscon >>> + - spacemit,k1-mpmu-syscon >>> + - const: syscon >>> + - const: simple-mfd >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clock-controller: >>> + $ref: /schemas/clock/spacemit,k1-ccu.yaml# >>> + type: object >> >> So now we see the full picture and it leads to questions. >> >> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a >> correct combination? >> >> 2. Why having this split in the first place? Please confirm that clock >> controller is really, really a separate device and its child in >> datasheet. IOW, fake child for your Linux is a no-go. Fake child while >> devices are independent is another no-go. > > These syscons are introduced because the clock controllers share > registers with reset controllers. Folding them into the parents results So a fake split... > in devicetree nodes act as both reset and clock controllers, like what Which is correct hardware representation, isn't it? > has been done for Rockchip SoCs. Such folding isn't practical for the > MPMU region either, since watchdog and other misc bits (e.g. PLL lock > status) locates in it. Hm? Why? You have a device which is reset and clock controller, so why one device node is not practical? Other vendors do not have problem with this. > > If you're more comfortable with reset and clock controllers folded > together and eliminating most of these syscons, I'm willing to make the > change. This is expected. Best regards, Krzysztof
On Tue, Feb 11, 2025 at 09:03:20AM +0100, Krzysztof Kozlowski wrote: > On 11/02/2025 06:15, Haylen Chu wrote: > > On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote: > >> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote: > >>> Add documentation to describe Spacemit K1 system controller registers. > >>> > >>> Signed-off-by: Haylen Chu <heylenay@4d2.org> > >>> --- > >>> .../soc/spacemit/spacemit,k1-syscon.yaml | 52 +++++++++++++++++++ > >>> 1 file changed, 52 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > >>> > >>> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > >>> new file mode 100644 > >>> index 000000000000..79c4a74ff30e > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > >>> @@ -0,0 +1,52 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: Spacemit K1 SoC System Controller > >>> + > >>> +maintainers: > >>> + - Haylen Chu <heylenay@4d2.org> > >>> + > >>> +description: > >>> + The Spacemit K1 SoC system controller provides access to shared register files > >>> + for related SoC modules, such as clock controller and reset controller. > >>> + > >>> +properties: > >>> + compatible: > >>> + items: > >>> + - enum: > >>> + - spacemit,k1-apbc-syscon > >>> + - spacemit,k1-apbs-syscon > >>> + - spacemit,k1-apmu-syscon > >>> + - spacemit,k1-mpmu-syscon > >>> + - const: syscon > >>> + - const: simple-mfd > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + clock-controller: > >>> + $ref: /schemas/clock/spacemit,k1-ccu.yaml# > >>> + type: object > >> > >> So now we see the full picture and it leads to questions. > >> > >> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a > >> correct combination? > >> > >> 2. Why having this split in the first place? Please confirm that clock > >> controller is really, really a separate device and its child in > >> datasheet. IOW, fake child for your Linux is a no-go. Fake child while > >> devices are independent is another no-go. > > > > These syscons are introduced because the clock controllers share > > registers with reset controllers. Folding them into the parents results > > So a fake split... > > > in devicetree nodes act as both reset and clock controllers, like what > > Which is correct hardware representation, isn't it? > > > has been done for Rockchip SoCs. Such folding isn't practical for the > > MPMU region either, since watchdog and other misc bits (e.g. PLL lock > > status) locates in it. I have to correct that the watchdog doesn't stay in the MPMU region, I misremembered it. > Hm? Why? You have a device which is reset and clock controller, so why > one device node is not practical? Other vendors do not have problem with > this. Merging reset and clock controllers together is fine to me. What I want to mention is that APMU and MPMU, abbreviated from Application/Main Power Management Unit, contain not only clock/reset-related registers but also power management ones[1]. Additionally, the PLL lock status bits locate at MPMU, split from the PLL configuration registers as you've already seen in the binding of spacemit,k1-ccu-apbs where I refer to it with a phandle. Since reset/clock and power management registers interleave in the MMIO region, do you think syscons are acceptable in this situation or it should be handled in another way? The reset and clock controllers could still be folded together as they share the same registers. The device tree will look like, syscon_mpmu: system-controller@d4050000 { compatible = "spacemit,mpmu-syscon", "syscon", "simple-mfd"; reg = <0xd4050000 0x10000>; cru_mpmu: clock-controller { compatible = "spacemit,k1-cru-mpmu"; #clock-cells = <1>; #reset-cells = <1>; }; power_mpmu: power-controller { compatible = "spacemit,k1-powerdomain-mpmu"; /* ... */ #power-domain-cells = <0>; }; }; For the other two clock controllers (APBS and APBC), syscons are really unnecessary and it's simple to fold them. > > > > If you're more comfortable with reset and clock controllers folded > > together and eliminating most of these syscons, I'm willing to make the > > change. > > This is expected. Thanks for the explanation. > > > Best regards, > Krzysztof Best regards, Haylen Chu [1]: https://developer.spacemit.com/documentation?token=T7TnwVZz1iPBk1kKwAPc6lyKnNb#part958
On 13/02/2025 12:14, Haylen Chu wrote: > On Tue, Feb 11, 2025 at 09:03:20AM +0100, Krzysztof Kozlowski wrote: >> On 11/02/2025 06:15, Haylen Chu wrote: >>> On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote: >>>> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote: >>>>> Add documentation to describe Spacemit K1 system controller registers. >>>>> >>>>> Signed-off-by: Haylen Chu <heylenay@4d2.org> >>>>> --- >>>>> .../soc/spacemit/spacemit,k1-syscon.yaml | 52 +++++++++++++++++++ >>>>> 1 file changed, 52 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >>>>> new file mode 100644 >>>>> index 000000000000..79c4a74ff30e >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml >>>>> @@ -0,0 +1,52 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml# >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>> + >>>>> +title: Spacemit K1 SoC System Controller >>>>> + >>>>> +maintainers: >>>>> + - Haylen Chu <heylenay@4d2.org> >>>>> + >>>>> +description: >>>>> + The Spacemit K1 SoC system controller provides access to shared register files >>>>> + for related SoC modules, such as clock controller and reset controller. >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + items: >>>>> + - enum: >>>>> + - spacemit,k1-apbc-syscon >>>>> + - spacemit,k1-apbs-syscon >>>>> + - spacemit,k1-apmu-syscon >>>>> + - spacemit,k1-mpmu-syscon >>>>> + - const: syscon >>>>> + - const: simple-mfd >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + clock-controller: >>>>> + $ref: /schemas/clock/spacemit,k1-ccu.yaml# >>>>> + type: object >>>> >>>> So now we see the full picture and it leads to questions. >>>> >>>> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a >>>> correct combination? >>>> >>>> 2. Why having this split in the first place? Please confirm that clock >>>> controller is really, really a separate device and its child in >>>> datasheet. IOW, fake child for your Linux is a no-go. Fake child while >>>> devices are independent is another no-go. >>> >>> These syscons are introduced because the clock controllers share >>> registers with reset controllers. Folding them into the parents results >> >> So a fake split... >> >>> in devicetree nodes act as both reset and clock controllers, like what >> >> Which is correct hardware representation, isn't it? >> >>> has been done for Rockchip SoCs. Such folding isn't practical for the >>> MPMU region either, since watchdog and other misc bits (e.g. PLL lock >>> status) locates in it. > > I have to correct that the watchdog doesn't stay in the MPMU region, I > misremembered it. > >> Hm? Why? You have a device which is reset and clock controller, so why >> one device node is not practical? Other vendors do not have problem with >> this. > > Merging reset and clock controllers together is fine to me. What I want > to mention is that APMU and MPMU, abbreviated from Application/Main Power > Management Unit, contain not only clock/reset-related registers but also > power management ones[1]. Additionally, the PLL lock status bits locate > at MPMU, split from the PLL configuration registers as you've already > seen in the binding of spacemit,k1-ccu-apbs where I refer to it with a > phandle. You need to define what is the device here. Don't create fake nodes just for your drivers. If registers are interleaved and manual says "this is block APMU/MPMU" then you have one device, so one node with 'reg'. If subblocks are re-usable hardware (unlikely) or at least separate/distinguishable, you could have children. If subblocks are re-usable but addresses are interleaved, then children should not have 'reg'. If children do not have any resources as an effect, this is strong indication these are not re-usable, separate subblocks. > > Since reset/clock and power management registers interleave in the MMIO > region, do you think syscons are acceptable in this situation or it > should be handled in another way? The reset and clock controllers could > still be folded together as they share the same registers. The device > tree will look like, > > syscon_mpmu: system-controller@d4050000 { > compatible = "spacemit,mpmu-syscon", "syscon", "simple-mfd"; > reg = <0xd4050000 0x10000>; > > cru_mpmu: clock-controller { > compatible = "spacemit,k1-cru-mpmu"; > #clock-cells = <1>; > #reset-cells = <1>; > }; > > power_mpmu: power-controller { > compatible = "spacemit,k1-powerdomain-mpmu"; > /* ... */ > #power-domain-cells = <0>; > }; Based on above, I do not see any need for children device nodes. It's fake split to match driver design. > }; > > For the other two clock controllers (APBS and APBC), syscons are really > unnecessary and it's simple to fold them. I don't follow. Do we talk about children or syscon compatible? Best regards, Krzysztof
On Thu, Feb 13, 2025 at 07:07:55PM +0100, Krzysztof Kozlowski wrote: > On 13/02/2025 12:14, Haylen Chu wrote: > > On Tue, Feb 11, 2025 at 09:03:20AM +0100, Krzysztof Kozlowski wrote: > >> On 11/02/2025 06:15, Haylen Chu wrote: > >>> On Sat, Jan 04, 2025 at 11:07:58AM +0100, Krzysztof Kozlowski wrote: > >>>> On Fri, Jan 03, 2025 at 09:56:35PM +0000, Haylen Chu wrote: > >>>>> Add documentation to describe Spacemit K1 system controller registers. > >>>>> > >>>>> Signed-off-by: Haylen Chu <heylenay@4d2.org> > >>>>> --- > >>>>> .../soc/spacemit/spacemit,k1-syscon.yaml | 52 +++++++++++++++++++ > >>>>> 1 file changed, 52 insertions(+) > >>>>> create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > >>>>> new file mode 100644 > >>>>> index 000000000000..79c4a74ff30e > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml > >>>>> @@ -0,0 +1,52 @@ > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>>> +%YAML 1.2 > >>>>> +--- > >>>>> +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml# > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>>> + > >>>>> +title: Spacemit K1 SoC System Controller > >>>>> + > >>>>> +maintainers: > >>>>> + - Haylen Chu <heylenay@4d2.org> > >>>>> + > >>>>> +description: > >>>>> + The Spacemit K1 SoC system controller provides access to shared register files > >>>>> + for related SoC modules, such as clock controller and reset controller. > >>>>> + > >>>>> +properties: > >>>>> + compatible: > >>>>> + items: > >>>>> + - enum: > >>>>> + - spacemit,k1-apbc-syscon > >>>>> + - spacemit,k1-apbs-syscon > >>>>> + - spacemit,k1-apmu-syscon > >>>>> + - spacemit,k1-mpmu-syscon > >>>>> + - const: syscon > >>>>> + - const: simple-mfd > >>>>> + > >>>>> + reg: > >>>>> + maxItems: 1 > >>>>> + > >>>>> + clock-controller: > >>>>> + $ref: /schemas/clock/spacemit,k1-ccu.yaml# > >>>>> + type: object > >>>> > >>>> So now we see the full picture and it leads to questions. > >>>> > >>>> 1. Why spacemit,k1-apbc-syscon with spacemit,k1-ccu-apmu child is a > >>>> correct combination? > >>>> > >>>> 2. Why having this split in the first place? Please confirm that clock > >>>> controller is really, really a separate device and its child in > >>>> datasheet. IOW, fake child for your Linux is a no-go. Fake child while > >>>> devices are independent is another no-go. > >>> > >>> These syscons are introduced because the clock controllers share > >>> registers with reset controllers. Folding them into the parents results > >> > >> So a fake split... > >> > >>> in devicetree nodes act as both reset and clock controllers, like what > >> > >> Which is correct hardware representation, isn't it? > >> > >>> has been done for Rockchip SoCs. Such folding isn't practical for the > >>> MPMU region either, since watchdog and other misc bits (e.g. PLL lock > >>> status) locates in it. > > > > I have to correct that the watchdog doesn't stay in the MPMU region, I > > misremembered it. > > > >> Hm? Why? You have a device which is reset and clock controller, so why > >> one device node is not practical? Other vendors do not have problem with > >> this. > > > > Merging reset and clock controllers together is fine to me. What I want > > to mention is that APMU and MPMU, abbreviated from Application/Main Power > > Management Unit, contain not only clock/reset-related registers but also > > power management ones[1]. Additionally, the PLL lock status bits locate > > at MPMU, split from the PLL configuration registers as you've already > > seen in the binding of spacemit,k1-ccu-apbs where I refer to it with a > > phandle. > > You need to define what is the device here. Don't create fake nodes just > for your drivers. If registers are interleaved and manual says "this is > block APMU/MPMU" then you have one device, so one node with 'reg'. > > If subblocks are re-usable hardware (unlikely) or at least > separate/distinguishable, you could have children. If subblocks are > re-usable but addresses are interleaved, then children should not have > 'reg'. If children do not have any resources as an effect, this is > strong indication these are not re-usable, separate subblocks. > > > > > Since reset/clock and power management registers interleave in the MMIO > > region, do you think syscons are acceptable in this situation or it > > should be handled in another way? The reset and clock controllers could > > still be folded together as they share the same registers. The device > > tree will look like, > > > > syscon_mpmu: system-controller@d4050000 { > > compatible = "spacemit,mpmu-syscon", "syscon", "simple-mfd"; > > reg = <0xd4050000 0x10000>; > > > > cru_mpmu: clock-controller { > > compatible = "spacemit,k1-cru-mpmu"; > > #clock-cells = <1>; > > #reset-cells = <1>; > > }; > > > > power_mpmu: power-controller { > > compatible = "spacemit,k1-powerdomain-mpmu"; > > /* ... */ > > #power-domain-cells = <0>; > > }; > > Based on above, I do not see any need for children device nodes. It's > fake split to match driver design. Okay, I'll make APMU/MPMU act as a whole device without split children and drop bindings for the childern (spacemit,k1-ccu-mpmu) in the next revision. Do I get the point? > > }; > > > > For the other two clock controllers (APBS and APBC), syscons are really > > unnecessary and it's simple to fold them. > > > I don't follow. Do we talk about children or syscon compatible? APBS region contains only clock (PLL) bits and APBC region contains only reset and clock bits, so I was thinking about dropping the syscon nodes and changing their compatible to spacemit,k1-plls and spacemit,k1-cru-apbc. In summary, my plan is, - For MPMU, APMU and APBC region, keep the binding in soc/spacemit. They'll be reset, clock and power controllers, with compatible "spacemit,k1-syscon-*". - For APBS region, write a new binding clock/spacemit,k1-plls, as it contains only PLL-related bits. It acts as clock controller. - All split children will be eliminated, there'll be only four device nodes, one for each region, matching the datasheet. - Put all clock-related binding definition of SpacemiT K1 in dt-bindings/clock/spacemit,k1-ccu.h Is it fine for you? > > Best regards, > Krzysztof Thanks, Haylen Chu
I have a general proposal on how to represent this, but I'd
like to know whether it makes sense. It might be what Krzysztof
is suggesting, but in any case, I hope this representation would
work, because it could simplify the code, and compartmentalizes
things.
Part of what motivates this is that I've been looking at the
downstream reset code this week. It contains a large number of
register offset definitions identical to what's used for the
clock driver. The reset driver uses exactly the same registers
as the clock driver does. Downstream they are separate drivers,
but the clock driver exports a shared spinlock for both drivers
to use.
These really need to be incorporated into the same driver for
upstream.
The clock code defines four distinct "units" (a term I'll use
from here on; there might be a better name):
MPMU Main Power Management Unit
APMU Application Power Management Unit
APBC APB Clock
APBS APB Spare
The reset code defines some of those, but doesn't use APBS.
It also defines three more:
APBC2 Another APB Clock
RCPU Real-time CPU?
RCPU2 Another Real-time CPU
Each of these "units" has a distinct I/O memory region that
contains registers that manage the clocks and reset signals.
I suggest a single "k1-clocks" device be created, which has
access to all of the I/O address ranges. And then within
the DT node for that device there is a sub-node for the
clocks, and another sub-node for the resets. Each of these
uses 2 cells for addressing. The first indicates which
"unit" contains the clock or reset registers, and the
second indicates the clock or reset line implemented by
that unit.
I might not have this exactly right, but below I show some
DTS code that I hope demonstrates what I mean.
Could this approach, or something close, work?
-Alex
/* SpacemiT clock/reset unit numbers */
#define SPACEMIT_CRST_RCPU 0
#define SPACEMIT_CRST_RCPU2 1
#define SPACEMIT_CRST_APBC 2
/* . . . */
#define SPACEMIT_CRST_APBC2 6
/* SpacemiT RCPU unit reset IDs */
#define SPACEMIT_RCPU_HDMIAUDIO 0
#define SPACEMIT_RCPU_CAN 1
/* . . . */
#define SPACEMIT_RCPU_UART1 6
/* SpacemiT RCPU2 unit reset IDs */
#define SPACEMIT_RCPU2_PWM0 0
#define SPACEMIT_RCPU2_PWM1 1
/* . . . */
#define SPACEMIT_RCPU2_PWM9 9
/* SpacemiT APBC unit reset/clock IDs */
#define SPACEMIT_APBC_UART1 0
#define SPACEMIT_APBC_UART2 1
#define SPACEMIT_APBC_GPIO 2
/* . . . */
#define SPACEMIT_APBC_TWSI0 27
/* APBC reset/clock IDs */
#define SPACEMIT_APBC_UART1 0
#define SPACEMIT_APBC_UART2 1
#define SPACEMIT_APBC_GPIO 2
/* . . . */
clocks {
compatible = "spacemit,k1-clocks";
reg = <0x0 0xc0880000 0x0 0x2050>,
<0x0 0xc0888000 0x0 0x30>,
<0x0 0xd4015000 0x0 0x1000>,
<0x0 0xd4050000 0x0 0x209c>,
<0x0 0xd4090000 0x0 0x1000>,
<0x0 0xd4282800 0x0 0x400>,
<0x0 0xf0610000 0x0 0x20>;
reg-names = "rcpu",
"rcpu2",
"apbc",
"mpmu",
"apbs",
"apmu",
"apbc2";
/*
* The two reset cells indicate:
* unit number (e.g. SPACEMIT_CRST_RCPU)
* reset within that unit (e.g. SPACEMIT_RCPU_CAN)
*/
k1_reset: spacemit-k1-reset {
#reset-cells = <2>;
};
/*
* The two clock cells indicate:
* unit number (e.g. SPACEMIT_CRST_APBC)
* clock within that unit (e.g. SPACEMIT_APBC_UART1)
*/
k1_clock: spacemit-k1-clock {
#clock-cells = <2>;
};
/* . . . */
};
/* . . . */
uart8: serial@d4017700 {
compatible = "spacemit,k1-uart",
"intel,xscale-uart";
reg = <0x0 0xd4017700 0x0 0x100>;
interrupts = <50>;
clocks = <&k1_clock SPACEMIT_CRST_APBC SPACEMIT_APBC_UART8>;
<&k1_clock SPACEMIT_CRST_APBC SPACEMIT_APBC_UART8_BUS>;
clock-names = "core",
"bus";
reg-shift = <2>;
reg-io-width = <4>;
resets = <&k1_reset SPACEMIT_CRST_APBC SPACEMIT_APBC_UART8>;
status = "disabled";
};
On 15/02/2025 09:41, Haylen Chu wrote: > >>> }; >>> >>> For the other two clock controllers (APBS and APBC), syscons are really >>> unnecessary and it's simple to fold them. >> >> >> I don't follow. Do we talk about children or syscon compatible? > > APBS region contains only clock (PLL) bits and APBC region contains only > reset and clock bits, so I was thinking about dropping the syscon nodes > and changing their compatible to spacemit,k1-plls and > spacemit,k1-cru-apbc. > > In summary, my plan is, > > - For MPMU, APMU and APBC region, keep the binding in soc/spacemit. > They'll be reset, clock and power controllers, with compatible > "spacemit,k1-syscon-*". > - For APBS region, write a new binding clock/spacemit,k1-plls, as it > contains only PLL-related bits. It acts as clock controller. > - All split children will be eliminated, there'll be only four device > nodes, one for each region, matching the datasheet. > - Put all clock-related binding definition of SpacemiT K1 in > dt-bindings/clock/spacemit,k1-ccu.h > > Is it fine for you? > That did not explain hardware to me. You assume that some way, maybe through magical crystal ball, I know your hardware and will tell you what to do. No. I have dozens of other patches in my inbox. It's you who should explain the hardware in simple, concise way so we can judge whether DT description is correct. Again: define what is the actual device, what is its address space, what are its possible *separate* and *distinctive* children. Best regards, Krzysztof
On 22/02/2025 00:40, Alex Elder wrote: > I have a general proposal on how to represent this, but I'd > like to know whether it makes sense. It might be what Krzysztof > is suggesting, but in any case, I hope this representation would > work, because it could simplify the code, and compartmentalizes > things. > > Part of what motivates this is that I've been looking at the > downstream reset code this week. It contains a large number of > register offset definitions identical to what's used for the > clock driver. The reset driver uses exactly the same registers > as the clock driver does. Downstream they are separate drivers, > but the clock driver exports a shared spinlock for both drivers > to use. > > These really need to be incorporated into the same driver for > upstream. Why? First, it is not related to the topic here at all. You can design drivers as you wish and still nothing to do with discussion about binding. Second, different subsystems justify different drivers and Linux handles this well already. No need for custom spinlock - regmap already does it. > > The clock code defines four distinct "units" (a term I'll use > from here on; there might be a better name): > MPMU Main Power Management Unit > APMU Application Power Management Unit > APBC APB Clock > APBS APB Spare > > The reset code defines some of those, but doesn't use APBS. > It also defines three more: > APBC2 Another APB Clock > RCPU Real-time CPU? > RCPU2 Another Real-time CPU > > Each of these "units" has a distinct I/O memory region that > contains registers that manage the clocks and reset signals. So there are children - mpmu, apmu, apbclock, apbspare, apbclock2, rcpu 1+2? But previous statements were saying these are intermixed? " I'll make APMU/MPMU act as a whole device" > > I suggest a single "k1-clocks" device be created, which has For four devices? Or for one device? No, it's again going to wrong direction. I already said: "You need to define what is the device here. Don't create fake nodes ust for your drivers. If registers are interleaved and manual says "this is block APMU/MPMU" then you have one device, so one node with 'reg'." So what is the device here? Can you people actually answer? > access to all of the I/O address ranges. And then within > the DT node for that device there is a sub-node for the Uh, confusing. You said there is one device for all the clocks, so if there is one device so also one device node. No children. Maybe you have more devices but none of you is explaining the hardware that way. Mixing talk about drivers is really not helping. > Best regards, Krzysztof
On Sat, Feb 22, 2025 at 10:59:09AM +0100, Krzysztof Kozlowski wrote: > On 22/02/2025 00:40, Alex Elder wrote: > > I have a general proposal on how to represent this, but I'd > > like to know whether it makes sense. It might be what Krzysztof > > is suggesting, but in any case, I hope this representation would > > work, because it could simplify the code, and compartmentalizes > > things. > > > > Part of what motivates this is that I've been looking at the > > downstream reset code this week. It contains a large number of > > register offset definitions identical to what's used for the > > clock driver. The reset driver uses exactly the same registers > > as the clock driver does. Downstream they are separate drivers, > > but the clock driver exports a shared spinlock for both drivers > > to use. > > > > These really need to be incorporated into the same driver for > > upstream. > > Why? First, it is not related to the topic here at all. You can design > drivers as you wish and still nothing to do with discussion about binding. > Second, different subsystems justify different drivers and Linux handles > this well already. No need for custom spinlock - regmap already does it. > > > > > > The clock code defines four distinct "units" (a term I'll use > > from here on; there might be a better name): > > MPMU Main Power Management Unit > > APMU Application Power Management Unit > > APBC APB Clock > > APBS APB Spare > > > > The reset code defines some of those, but doesn't use APBS. > > It also defines three more: > > APBC2 Another APB Clock > > RCPU Real-time CPU? > > RCPU2 Another Real-time CPU > > > > Each of these "units" has a distinct I/O memory region that > > contains registers that manage the clocks and reset signals. > > So there are children - mpmu, apmu, apbclock, apbspare, apbclock2, rcpu > 1+2? But previous statements were saying these are intermixed? > > " I'll make APMU/MPMU act as a whole device" My reply seems somehow misleading. The statement means I will merge the children with the syscon into one devicetree node, which applies for both APMU and MPMU. I wasn't going to say that APMU and MPMU are intermixed. As Alex said, all these units have their own distinct and separate MMIO regions. > > > > I suggest a single "k1-clocks" device be created, which has > > For four devices? Or for one device? By Alex's example, I think he means a device node taking all these distinct MMIO regions as resource. clock { compatible = "spacemit,k1-clocks"; reg = <0x0 0xc0880000 0x0 0x2050>, <0x0 0xc0888000 0x0 0x30>, <0x0 0xd4015000 0x0 0x1000>, <0x0 0xd4050000 0x0 0x209c>, <0x0 0xd4090000 0x0 0x1000>, <0x0 0xd4282800 0x0 0x400>, <0x0 0xf0610000 0x0 0x20>; reg-names = "rcpu", "rcpu2", "apbc", "mpmu", "apbs", "apmu", "apbc2"; /* ... */ }; > No, it's again going to wrong direction. I already said: > > "You need to define what is the device here. Don't create fake nodes ust > for your drivers. If registers are interleaved and manual says "this is > block APMU/MPMU" then you have one device, so one node with 'reg'." > > So what is the device here? Can you people actually answer? > I'm not sure about the apbc2, rcpu and rcpu2 regions; they aren't related to the thread, either. For APBC, MPMU, APBS and APMU, I'm pretty sure they're standalone blocks with distinct and separate MMIO regions, this could be confirmed by the address mapping[1]. > > > access to all of the I/O address ranges. And then within > > the DT node for that device there is a sub-node for the > > Uh, confusing. You said there is one device for all the clocks, so if > there is one device so also one device node. No children. > > Maybe you have more devices but none of you is explaining the hardware > that way. Mixing talk about drivers is really not helping. > > > > > Best regards, > Krzysztof Best regards, Haylen Chu [1]: https://developer.spacemit.com/documentation?token=LzJyw97BCipK1dkUygrcbT0NnMg
On Sat, Feb 22, 2025 at 10:52:02AM +0100, Krzysztof Kozlowski wrote: > On 15/02/2025 09:41, Haylen Chu wrote: > > > >>> }; > >>> > >>> For the other two clock controllers (APBS and APBC), syscons are really > >>> unnecessary and it's simple to fold them. > >> > >> > >> I don't follow. Do we talk about children or syscon compatible? > > > > APBS region contains only clock (PLL) bits and APBC region contains only > > reset and clock bits, so I was thinking about dropping the syscon nodes > > and changing their compatible to spacemit,k1-plls and > > spacemit,k1-cru-apbc. > > > > In summary, my plan is, > > > > - For MPMU, APMU and APBC region, keep the binding in soc/spacemit. > > They'll be reset, clock and power controllers, with compatible > > "spacemit,k1-syscon-*". > > - For APBS region, write a new binding clock/spacemit,k1-plls, as it > > contains only PLL-related bits. It acts as clock controller. > > - All split children will be eliminated, there'll be only four device > > nodes, one for each region, matching the datasheet. > > - Put all clock-related binding definition of SpacemiT K1 in > > dt-bindings/clock/spacemit,k1-ccu.h > > > > Is it fine for you? > > > > That did not explain hardware to me. Sorry if my replies haven't made things clear. I'm goint to make a (hopefully) more clear conclusion, > You assume that some way, maybe > through magical crystal ball, I know your hardware and will tell you > what to do. > > No. > > I have dozens of other patches in my inbox. It's you who should explain > the hardware in simple, concise way so we can judge whether DT > description is correct. > > Again: define what is the actual device, what is its address space, what > are its possible *separate* and *distinctive* children. The series covers four seperate blocks, - Application Power Manage Unit, APMU - Main Power Manage Unit, MPMU - APB Bus Clock Unit, APBC - APB Spare, APBS they're clearly separate blocks and have their own distinct, separate address spaces, confirmed by the Address Mapping section in the TRM[1]. These four blocks provide hardware bits for three purposes: power management, reset signals and clocks. Not every block is capable of all the three functionalities, - APMU, MPMU: power, reset, clock - APBC: clock, reset - APBS: clock Reset and clock bits, if present, always stay in the same register. Power management bits stay in others. These two types of registers interleave if present in the same block (APMU and MPMU case). These blocks have no child: power, clock and reset definitions differ from block to block, no reusable nodes could be split from them. Hope this conclusion will help the reviewing. Please tell if something is unclear. > > Best regards, > Krzysztof Thanks, Haylen Chu [1]: https://developer.spacemit.com/documentation?token=LzJyw97BCipK1dkUygrcbT0NnMg
On 22/02/2025 11:48, Haylen Chu wrote: > On Sat, Feb 22, 2025 at 10:59:09AM +0100, Krzysztof Kozlowski wrote: >> On 22/02/2025 00:40, Alex Elder wrote: >>> I have a general proposal on how to represent this, but I'd >>> like to know whether it makes sense. It might be what Krzysztof >>> is suggesting, but in any case, I hope this representation would >>> work, because it could simplify the code, and compartmentalizes >>> things. >>> >>> Part of what motivates this is that I've been looking at the >>> downstream reset code this week. It contains a large number of >>> register offset definitions identical to what's used for the >>> clock driver. The reset driver uses exactly the same registers >>> as the clock driver does. Downstream they are separate drivers, >>> but the clock driver exports a shared spinlock for both drivers >>> to use. >>> >>> These really need to be incorporated into the same driver for >>> upstream. >> >> Why? First, it is not related to the topic here at all. You can design >> drivers as you wish and still nothing to do with discussion about binding. >> Second, different subsystems justify different drivers and Linux handles >> this well already. No need for custom spinlock - regmap already does it. >> >> >>> >>> The clock code defines four distinct "units" (a term I'll use >>> from here on; there might be a better name): >>> MPMU Main Power Management Unit >>> APMU Application Power Management Unit >>> APBC APB Clock >>> APBS APB Spare >>> >>> The reset code defines some of those, but doesn't use APBS. >>> It also defines three more: >>> APBC2 Another APB Clock >>> RCPU Real-time CPU? >>> RCPU2 Another Real-time CPU >>> >>> Each of these "units" has a distinct I/O memory region that >>> contains registers that manage the clocks and reset signals. >> >> So there are children - mpmu, apmu, apbclock, apbspare, apbclock2, rcpu >> 1+2? But previous statements were saying these are intermixed? >> >> " I'll make APMU/MPMU act as a whole device" > > My reply seems somehow misleading. The statement means I will merge the > children with the syscon into one devicetree node, which applies for > both APMU and MPMU. I wasn't going to say that APMU and MPMU are > intermixed. > > As Alex said, all these units have their own distinct and separate MMIO > regions. > >>> >>> I suggest a single "k1-clocks" device be created, which has >> >> For four devices? Or for one device? > > By Alex's example, I think he means a device node taking all these > distinct MMIO regions as resource. You still do not answer about the hardware: how many devices is there? > > clock { > compatible = "spacemit,k1-clocks"; > > reg = <0x0 0xc0880000 0x0 0x2050>, > <0x0 0xc0888000 0x0 0x30>, > <0x0 0xd4015000 0x0 0x1000>, > <0x0 0xd4050000 0x0 0x209c>, > <0x0 0xd4090000 0x0 0x1000>, > <0x0 0xd4282800 0x0 0x400>, > <0x0 0xf0610000 0x0 0x20>; > reg-names = "rcpu", > "rcpu2", > "apbc", > "mpmu", > "apbs", > "apmu", > "apbc2"; > > /* ... */ > }; > >> No, it's again going to wrong direction. I already said: >> >> "You need to define what is the device here. Don't create fake nodes ust >> for your drivers. If registers are interleaved and manual says "this is >> block APMU/MPMU" then you have one device, so one node with 'reg'." >> >> So what is the device here? Can you people actually answer? >> > > I'm not sure about the apbc2, rcpu and rcpu2 regions; they aren't > related to the thread, either. For APBC, MPMU, APBS and APMU, I'm pretty > sure they're standalone blocks with distinct and separate MMIO regions, > this could be confirmed by the address mapping[1]. They were brought here to discuss for some reason. Long discussions, long emails, unrelated topics like hardware or different devices - all this is not making it easier for me to understand. Best regards, Krzysztof
On Sat, Feb 22, 2025 at 12:50:13PM +0100, Krzysztof Kozlowski wrote: > On 22/02/2025 11:48, Haylen Chu wrote: > > On Sat, Feb 22, 2025 at 10:59:09AM +0100, Krzysztof Kozlowski wrote: > >> On 22/02/2025 00:40, Alex Elder wrote: > >>> I have a general proposal on how to represent this, but I'd > >>> like to know whether it makes sense. It might be what Krzysztof > >>> is suggesting, but in any case, I hope this representation would > >>> work, because it could simplify the code, and compartmentalizes > >>> things. > >>> > >>> Part of what motivates this is that I've been looking at the > >>> downstream reset code this week. It contains a large number of > >>> register offset definitions identical to what's used for the > >>> clock driver. The reset driver uses exactly the same registers > >>> as the clock driver does. Downstream they are separate drivers, > >>> but the clock driver exports a shared spinlock for both drivers > >>> to use. > >>> > >>> These really need to be incorporated into the same driver for > >>> upstream. > >> > >> Why? First, it is not related to the topic here at all. You can design > >> drivers as you wish and still nothing to do with discussion about binding. > >> Second, different subsystems justify different drivers and Linux handles > >> this well already. No need for custom spinlock - regmap already does it. > >> > >> > >>> > >>> The clock code defines four distinct "units" (a term I'll use > >>> from here on; there might be a better name): > >>> MPMU Main Power Management Unit > >>> APMU Application Power Management Unit > >>> APBC APB Clock > >>> APBS APB Spare > >>> > >>> The reset code defines some of those, but doesn't use APBS. > >>> It also defines three more: > >>> APBC2 Another APB Clock > >>> RCPU Real-time CPU? > >>> RCPU2 Another Real-time CPU > >>> > >>> Each of these "units" has a distinct I/O memory region that > >>> contains registers that manage the clocks and reset signals. > >> > >> So there are children - mpmu, apmu, apbclock, apbspare, apbclock2, rcpu > >> 1+2? But previous statements were saying these are intermixed? > >> > >> " I'll make APMU/MPMU act as a whole device" > > > > My reply seems somehow misleading. The statement means I will merge the > > children with the syscon into one devicetree node, which applies for > > both APMU and MPMU. I wasn't going to say that APMU and MPMU are > > intermixed. > > > > As Alex said, all these units have their own distinct and separate MMIO > > regions. > > > >>> > >>> I suggest a single "k1-clocks" device be created, which has > >> > >> For four devices? Or for one device? > > > > By Alex's example, I think he means a device node taking all these > > distinct MMIO regions as resource. > > > You still do not answer about the hardware: how many devices is there? In my understanding, the series covers four devices, APBC, APMU, MPMU and APBS, each comes with its separate MMIO region and is clearly described in the datasheet. I stated this in the later part of the reply, > > For APBC, MPMU, APBS and APMU, I'm pretty > > sure they're standalone blocks with distinct and separate MMIO regions, > > this could be confirmed by the address mapping[1]. Thus I don't agree on Alex's solution, since it creates fake devices not mentioned by the datasheet (spacemit,k1-clocks and all its children in the example devicetree). > > > > clock { > > compatible = "spacemit,k1-clocks"; > > > > reg = <0x0 0xc0880000 0x0 0x2050>, > > <0x0 0xc0888000 0x0 0x30>, > > <0x0 0xd4015000 0x0 0x1000>, > > <0x0 0xd4050000 0x0 0x209c>, > > <0x0 0xd4090000 0x0 0x1000>, > > <0x0 0xd4282800 0x0 0x400>, > > <0x0 0xf0610000 0x0 0x20>; > > reg-names = "rcpu", > > "rcpu2", > > "apbc", > > "mpmu", > > "apbs", > > "apmu", > > "apbc2"; > > > > /* ... */ > > }; > > > >> No, it's again going to wrong direction. I already said: > >> > >> "You need to define what is the device here. Don't create fake nodes ust > >> for your drivers. If registers are interleaved and manual says "this is > >> block APMU/MPMU" then you have one device, so one node with 'reg'." > >> > >> So what is the device here? Can you people actually answer? > >> > > > > I'm not sure about the apbc2, rcpu and rcpu2 regions; they aren't > > related to the thread, either. For APBC, MPMU, APBS and APMU, I'm pretty > > sure they're standalone blocks with distinct and separate MMIO regions, > > this could be confirmed by the address mapping[1]. > > They were brought here to discuss for some reason. Long discussions, > long emails, unrelated topics like hardware or different devices - all > this is not making it easier for me to understand. > > Best regards, > Krzysztof By the way, I made a summary on the hardware covered by this series in one of my earlier reply[1]. Could you please comment further on my proposal[2] according it, or pointing out anything that's unclear or missing? It will be helpful for things to improve. Thanks, Haylen Chu [1]: https://lore.kernel.org/all/Z7m2oNXbwJ06KtLQ@ketchup/ [2]: https://lore.kernel.org/all/Z7BTVu10EKHMqOnJ@ketchup/
On 24/02/2025 11:17, Haylen Chu wrote: > On Sat, Feb 22, 2025 at 12:50:13PM +0100, Krzysztof Kozlowski wrote: >> On 22/02/2025 11:48, Haylen Chu wrote: >>> On Sat, Feb 22, 2025 at 10:59:09AM +0100, Krzysztof Kozlowski wrote: >>>> On 22/02/2025 00:40, Alex Elder wrote: >>>>> I have a general proposal on how to represent this, but I'd >>>>> like to know whether it makes sense. It might be what Krzysztof >>>>> is suggesting, but in any case, I hope this representation would >>>>> work, because it could simplify the code, and compartmentalizes >>>>> things. >>>>> >>>>> Part of what motivates this is that I've been looking at the >>>>> downstream reset code this week. It contains a large number of >>>>> register offset definitions identical to what's used for the >>>>> clock driver. The reset driver uses exactly the same registers >>>>> as the clock driver does. Downstream they are separate drivers, >>>>> but the clock driver exports a shared spinlock for both drivers >>>>> to use. >>>>> >>>>> These really need to be incorporated into the same driver for >>>>> upstream. >>>> >>>> Why? First, it is not related to the topic here at all. You can design >>>> drivers as you wish and still nothing to do with discussion about binding. >>>> Second, different subsystems justify different drivers and Linux handles >>>> this well already. No need for custom spinlock - regmap already does it. >>>> >>>> >>>>> >>>>> The clock code defines four distinct "units" (a term I'll use >>>>> from here on; there might be a better name): >>>>> MPMU Main Power Management Unit >>>>> APMU Application Power Management Unit >>>>> APBC APB Clock >>>>> APBS APB Spare >>>>> >>>>> The reset code defines some of those, but doesn't use APBS. >>>>> It also defines three more: >>>>> APBC2 Another APB Clock >>>>> RCPU Real-time CPU? >>>>> RCPU2 Another Real-time CPU >>>>> >>>>> Each of these "units" has a distinct I/O memory region that >>>>> contains registers that manage the clocks and reset signals. >>>> >>>> So there are children - mpmu, apmu, apbclock, apbspare, apbclock2, rcpu >>>> 1+2? But previous statements were saying these are intermixed? >>>> >>>> " I'll make APMU/MPMU act as a whole device" >>> >>> My reply seems somehow misleading. The statement means I will merge the >>> children with the syscon into one devicetree node, which applies for >>> both APMU and MPMU. I wasn't going to say that APMU and MPMU are >>> intermixed. >>> >>> As Alex said, all these units have their own distinct and separate MMIO >>> regions. >>> >>>>> >>>>> I suggest a single "k1-clocks" device be created, which has >>>> >>>> For four devices? Or for one device? >>> >>> By Alex's example, I think he means a device node taking all these >>> distinct MMIO regions as resource. >> >> >> You still do not answer about the hardware: how many devices is there? > > In my understanding, the series covers four devices, APBC, APMU, MPMU > and APBS, each comes with its separate MMIO region and is clearly > described in the datasheet. I stated this in the later part of the > reply, Ack > >>> For APBC, MPMU, APBS and APMU, I'm pretty >>> sure they're standalone blocks with distinct and separate MMIO regions, >>> this could be confirmed by the address mapping[1]. > > Thus I don't agree on Alex's solution, since it creates fake devices not > mentioned by the datasheet (spacemit,k1-clocks and all its children in > the example devicetree). Ack > >>> >>> clock { >>> compatible = "spacemit,k1-clocks"; >>> >>> reg = <0x0 0xc0880000 0x0 0x2050>, >>> <0x0 0xc0888000 0x0 0x30>, >>> <0x0 0xd4015000 0x0 0x1000>, >>> <0x0 0xd4050000 0x0 0x209c>, >>> <0x0 0xd4090000 0x0 0x1000>, >>> <0x0 0xd4282800 0x0 0x400>, >>> <0x0 0xf0610000 0x0 0x20>; >>> reg-names = "rcpu", >>> "rcpu2", >>> "apbc", >>> "mpmu", >>> "apbs", >>> "apmu", >>> "apbc2"; >>> >>> /* ... */ >>> }; >>> >>>> No, it's again going to wrong direction. I already said: >>>> >>>> "You need to define what is the device here. Don't create fake nodes ust >>>> for your drivers. If registers are interleaved and manual says "this is >>>> block APMU/MPMU" then you have one device, so one node with 'reg'." >>>> >>>> So what is the device here? Can you people actually answer? >>>> >>> >>> I'm not sure about the apbc2, rcpu and rcpu2 regions; they aren't >>> related to the thread, either. For APBC, MPMU, APBS and APMU, I'm pretty >>> sure they're standalone blocks with distinct and separate MMIO regions, >>> this could be confirmed by the address mapping[1]. >> >> They were brought here to discuss for some reason. Long discussions, >> long emails, unrelated topics like hardware or different devices - all >> this is not making it easier for me to understand. >> >> Best regards, >> Krzysztof > > By the way, I made a summary on the hardware covered by this series in > one of my earlier reply[1]. Could you please comment further on my > proposal[2] according it, or pointing out anything that's unclear or > missing? It will be helpful for things to improve. Thanks, it looks good. Best regards, Krzysztof
On 2/25/25 2:12 AM, Krzysztof Kozlowski wrote: >>> They were brought here to discuss for some reason. Long discussions, >>> long emails, unrelated topics like hardware or different devices - all >>> this is not making it easier for me to understand. >>> >>> Best regards, >>> Krzysztof >> By the way, I made a summary on the hardware covered by this series in >> one of my earlier reply[1]. Could you please comment further on my >> proposal[2] according it, or pointing out anything that's unclear or >> missing? It will be helpful for things to improve. > Thanks, it looks good. Sorry about my earlier message; I hadn't reviewed the discussion carefully enough, and what I said didn't help. It seems like Haylen is on the right track now, and I'll wait for the next version of the series to comment again. -Alex
diff --git a/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml new file mode 100644 index 000000000000..79c4a74ff30e --- /dev/null +++ b/Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml @@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/soc/spacemit/spacemit,k1-syscon.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Spacemit K1 SoC System Controller + +maintainers: + - Haylen Chu <heylenay@4d2.org> + +description: + The Spacemit K1 SoC system controller provides access to shared register files + for related SoC modules, such as clock controller and reset controller. + +properties: + compatible: + items: + - enum: + - spacemit,k1-apbc-syscon + - spacemit,k1-apbs-syscon + - spacemit,k1-apmu-syscon + - spacemit,k1-mpmu-syscon + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + clock-controller: + $ref: /schemas/clock/spacemit,k1-ccu.yaml# + type: object + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + system-controller@d4050000 { + compatible = "spacemit,k1-mpmu-syscon", "syscon", "simple-mfd"; + reg = <0xd4050000 0x209c>; + + clock-controller { + compatible = "spacemit,k1-ccu-mpmu"; + clocks = <&osc_32k>, <&vctcxo_1m>, <&vctcxo_3m>, <&vctcxo_24m>; + clock-names = "osc", "vctcxo_1m", "vctcxo_3m", "vctcxo_24m"; + #clock-cells = <1>; + }; + };
Add documentation to describe Spacemit K1 system controller registers. Signed-off-by: Haylen Chu <heylenay@4d2.org> --- .../soc/spacemit/spacemit,k1-syscon.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/spacemit/spacemit,k1-syscon.yaml