Hello folks,
I've been addressed with a several questions from Rob, Arnd, Andy and Stephen
in replies to he i2c, clock, ehb (errors handler block) and syscon patchsets sent
by me in the framework of the Baikal-T1 SoC support integration into the kernel
[1 - 4]. Seeing the questions have similar nature it would be better to settle the
issues all at once, provide fixes for all of them and proceed with the
rest of review.
In general the questions are related with syscon, clocks, i2c0 and ehb DT nodes
residence. Some of them concern the memory mapped registers alignment
(clock/i2c0), some of them are of whether the corresponding device drivers
belong to the soc-subsystem (ehb) or whether some device nodes should be
sub-nodes of the syscon or of anything else like root node. Anyway as I see it
the answers to all the questions are hidden in a way the Baikal-T1 System
Controller is structured. That's why I'll start from describing it first.
Then I'll provide the System Controller DT nodes structure currently implemented
for Baikal-T1 SoC and explain why we chose to develop it in favor of any other.
Finally I'll offer you an alternative structure, which we also had in mind,
but didn't implement it for reasons also given in the text below.
In accordance with Baikal-T1 SoC documentations there is a System Control
Module (we also call it System Controller), which contains two big subsystems:
Clocks Control Unit (CCU) and Boot Controller. But the System Controller
functionality isn't limited with just clocks control and boot procedure
execution. There are settings, which aren't directly connected with those
sub-blocks, but is still exposed through the System Controller registers space
just because, well, because either the system designers wanted them to be
exposed from there or they didn't find any better way to map them. We don't
know for sure. We just know the mapping as it is and can't do anything to
change it. So here is the System Controller structure implemented in the
Baikal-T1 SoC:
System Control Module
|
+--> Clock Control Unit + System-specific settings: 0x1F04D000 @ 4KB
| |
| +-> PLLs 0x1F04D000 - 0x1F04D027
| |
| +-> System Devices Clocks: 0x1F04D028 - 0x1F04D02F (clock dividers)
| | (DDR clock gate + L2 WS/Data/Tag stall cycles configs)
| +-> AXI-bus Clocks: 0x1F04D030 - 0x1F04D05F (clock dividers)
| |
| +-> System Devices Clocks: 0x1F04D060 - 0x1F04D09F (clock dividers)
| |
| +-> I2C0 Indirect Access: 0x1F04D100 - 0x1F04D10F (+ IRQ)
| | (Just three Command/Read/Write registers to access the DW APB I2C controller registers space)
| +-> AXI-bus EHB Error Addr:0x1F04D110 - 0x1F04D117 (+ IRQ)
| |
| +-> CPU Debug register: 0x1F04D118 - 0x1F04D11B
| | (provide the CPU reset flag for 'syscon-reboot' driver)
| +-> PCIe control block: 0x1F04D140 - 0x1F04D14F
| | (PCIe internal Clocks, Reset, PMU, LTSSM and custom settings)
| +-> System Devices Clocks: 0x1F04D150 - 0x1F04D153 (clock divider)
| | (Just watchdog clock divider)
| +-> Reset safe register: 0x1F04D154 - 0x1F04D157
| (register preserved during a soft/watchdog resets for
| 'syscon-reboot-mode' driver)
|
+--> Boot Controller: 0x1F040000 @ 4KB, 0x1BFC0000 @ 4KB, 0x1C000000 @ 16MB,
| | 0x1FC00000 @ 4MB
| |
| +-> Boot Controller settings: 0x1F040000 - 0x1F0400FF
| |
| +-> Synopsis DesignWare APB SSI controller: 0x1F040100 - 0x1F040FFF
| | (with very limited resources though: nor IRQ, no DMA, just 8 bytes FIFO)
| +-> Physically mapped Internal ROM: 0x1BFC0000 - 0x1BFCFFFF
| |
| +-> Physically mapped SPI-flash ROM: 0x1C000000 - 0x1CFFFFFF
| | (This region access is multiplexed with DW APB SSI controller registers)
| +-> Physically mapped Boot ROM: 0x1FC00000 - 0x1FFFFFFF
| (mirror of either Internal or SPI-flash ROMs depending on the boot
| mode)
There is also an APB EHB (APB bus Errors Handler Block), but according to the
Baikal-T1 documentation, it isn't part of the System Control Module (though as
for me it was a tech-writers mistake to represent it as a separate block).
+--> APB bus errors handler: 0x1F059000 @ 4KB, 0x1D000000 @ 32.25MB
|
+-> APB bus EHB registers: 0x1F059000 - 0x1F059FFF (+ IRQ)
| (Error address, type and timeout settings)
+-> Unmapped space for errors injection: 0x1D000000 - 0x1F03FFFF
It's a mystery to me why the SoC designers couldn't create a different registers
space for CCU (PLLs and Clock dividers) and for the custom system settings. But
we have to deal with what we currently have, since it can't be changed. So
as you can see the Boot Controller and Clock Control Unit (CCU) registers space
are separated from each other, and CCU registers (PLLs, AXI-bus and System
Devices Clock dividers) can be unpinned from the custom system settings. In
addition the Boot Controller and CCU sub-blocks registers don't have
intersecting parts, which makes the syscon interface using regmaps with
spin-lock synch-primitive redundant for simultaneous accesses of the registers
of different groups. So I've decided to just redistribute the registers
resources between the corresponding DT nodes not necessarily being sub-nodes of
the System Controller node (I suppose most of your questions were raised due to
this decision). This made the nodes being unrelated to each other, but the nodes
MMIO resources turned to be size-unaligned. Here is a DT nodes structure we've
currently implemented in the framework of Baikal-T1 SoC support in the kernel.
/* System Controller DT Nodes I */
root {
reboot {
compatible = "syscon-reboot";
regmap = <&cpu_ctl>;
};
reboot-mode {
compatible = "syscon-reboot-mode";
regmap = <&wdt_rcr>;
};
apb {
compatible = "simple-bus";
ccu_pll {
compatible = "be,bt1-ccu-pll"; /* PLLs */
};
ccu_axi {
compatible = "be,bt1-ccu-axi"; /* AXI-bus Clock */
clocks = <&ccu_pll X>, <&ccu_pll Y>, ...;
};
ccu_sys {
compatible = "be,bt1-ccu-sys"; /* System Devices Clock */
clocks = <&ccu_pll X>, <&ccu_pll Y>, ...;
};
syscon {
compatible = "simple-mfd";
l2_ctl {
compatible = "be,bt1-l2-ctl", "syscon";
};
ddr_ctl {
compatible = "be,bt1-ddr-ctl", "syscon";
};
axi_ehb {
compatible = "be,bt1-axi-ehb";
};
cpu_ctl {
compatible = "be,bt1-cpu-ctl", "syscon";
};
pcie_ctl {
compatible = "be,bt1-pcie-ctl", "syscon";
};
wdt_rcr {
compatible = "be,bt1-wdt-rcr", "syscon";
};
};
i2c0 {
compatible = "be,bt1-i2c"; /* DW APB i2c with indirect registers access */
};
boot {
compatible = "be,bt1-boot-ctl";
int_rom {
compatible = "be,bt1-int-rom", "mtd-rom";
};
spi_rom {
compatible = "be,bt1-ssi-rom", "mtd-rom";
};
spi0 {
compatible = "be,bt1-boot-ssi";
};
boot_rom {
compatible = "be,bt1-boot-rom", "mtd-rom";
};
};
apb_ehb {
compatible = "be,bt1-apb-ehb";
};
};
};
The corresponding dts-file hasn't been submitted so far. So if you don't like
the structure we've chosen to define the System Controller parts, please justify
why it isn't right and if it's possible point me out at some place of the kernel
documentation so to refer to in future developments. We don't refuse to change it,
I'd just like to know what's wrong with it and how to bypass the same problem in
future.
On the other hand as an alternative, which you'd possibly like much more, if we
got used to the fact that syscon-interface converts the registers resource to
the spin-lock secured regmap object and in order to retain the system controller
structure, the next System Controller DT nodes tree could be implemented:
/* System Controller DT Nodes II */
root {
axi {
compatible = "be,bt1-axi", "simple-bus";
syscon = <&syscon>;
interrupts = <...>;
};
apb {
compatible = "be,bt1-apb", "simple-bus";
reg = <X>, <Y>;
interrupts = <...>;
syscon {
compatible = "be,bt1-sys-con", "syscon", "simple-mfd";
ccu_pll {
compatible = "be,bt1-ccu-pll";
};
ccu_axi {
compatible = "be,bt1-ccu-axi";
};
ccu_sys {
compatible = "be,bt1-ccu-sys";
};
l2_ctl {
compatible = "be,bt1-l2-ctl";
};
i2c0 {
compatible = "be,bt1-sys-i2c";
};
reboot {
compatible = "syscon-reboot";
};
reboot-mode {
compatible = "syscon-reboot-mode";
};
boot {
compatible = "be,bt1-boot-ctl";
int_rom {
compatible = "be,bt1-int-rom", "mtd-rom";
};
spi_rom {
compatible = "be,bt1-ssi-rom", "mtd-rom";
};
spi0 {
compatible = "be,bt1-boot-ssi";
};
boot_rom {
compatible = "be,bt1-boot-rom", "mtd-rom";
};
};
};
};
};
I suppose you'd like it better than "DT Nodes I", since in accordance with
Arnd Bergmann suggestion [1] the APB/AXI EHB would be moved to the drivers/bus
subsystem. In the framework of the "be,bt1-sys-con" MFD driver I would have
to implement an extra regmap to provide the indirectly accessible i2c0
registers, which would satisfy the Andy Shevchenko comment [2] regarding our
DW APB i2c patchset. I wouldn't need to have the "syscon-reboot-mode"
patchset in order to be able to declare the reboot-mode node in any place of
the DTS tree, which Rob Herring already nacked [3], but didn't explain why.
Finally I wouldn't have the unaligned registers space defined for Clock Control
Unit nodes, but would use the syscon-regmap to access them from the system
clock drivers. This would fix the issue raised by Stephen Boyd [4] in the
framework of the Baikal-T1 CCU drivers patchset.
So if you like the "DT Nodes II" structure better than the version I or have
any idea of how to improve it could you tell us in reply to this email while
it's still possible to change the System Controller nodes structure at this
point since we are at v1-v2 stages in each patchset and non of the drivers
have been fully accepted so far.
[1] soc: Add Baikal-T1 SoC APB/AXI EHB and L2-cache drivers
https://lkml.org/lkml/2020/3/6/669
[2] i2c: designware: Add Baikal-T1 SoC DW I2C specifics support
https://lkml.org/lkml/2020/3/6/542
[3] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings
https://lkml.org/lkml/2020/3/12/1010
[4] dt-bindings: clk: Add Baikal-T1 System Devices CCU bindings
https://lkml.org/lkml/2020/3/9/1249
Regards,
-Sergey
