On Mon, Mar 09, 2020 at 07:19:12PM -0700, Stephen Boyd wrote:
> Quoting Sergey.Semin@xxxxxxxxxxxxxxxxxxxx (2020-03-06 05:00:46)
> > diff --git a/Documentation/devicetree/bindings/clock/be,bt1-ccu-sys.yaml b/Documentation/devicetree/bindings/clock/be,bt1-ccu-sys.yaml
> > new file mode 100644
> > index 000000000000..aea09fbafc89
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/be,bt1-ccu-sys.yaml
> > @@ -0,0 +1,169 @@
> [..]
> > + assigned-clock-rates: true
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - "#clock-cells"
> > + - clocks
> > + - clock-names
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/bt1-ccu.h>
> > +
> > + ccu_sys: ccu_sys@1F04D060 {
>
> Node name should be clock-controller@1f04d060.
Ok.
>
> Also, binding looks wrong because that address isn't aligned. Most
> likely it's one hardware block that has many different functionalities
> so splitting it up into different regions isn't doing anything besides
> logically splitting up the register space for software benefits.
As I said in RFC: https://lkml.org/lkml/2020/3/22/393 , CCU (Clock
Control Unit) is part of the Baikal-T1 System Control Module (simply
speaking the system controller). In details why I split the system
controller registers space up, I described in the text. Alternatively I
also suggested there to make the CCU nodes being sub-nodes of the System
Control Module DT node. This would better reflect the hardware blocks,
but instead of having an independent registers MMIO in driver I would
have to use the syscon regmap handler.
As I also said in the RFC, in accordance with the Baikal-T1 CCU documentation
CCU is split up into three blocks: PLLs, AXI-bus clocks and System devices
clocks. That's why the CCU driver expects to find three DT nodes:
CCU PLL (be,bt1-ccu-pll), CCU AXI (be,bt1-ccu-axi) and CCU Sys
(be,bt1-ccu-sys).
>
> > + compatible = "be,bt1-ccu-sys";
> > + reg = <0x1F04D060 0x0A0>,
> > + <0x1F04D150 0x004>;
> > + #clock-cells = <1>;
> > + #reset-cells = <1>;
> > +
> > + clocks = <&osc25>,
> > + <&ccu_pll CCU_SATA_PLL>,
> > + <&ccu_pll CCU_PCIE_PLL>,
> > + <&ccu_pll CCU_ETH_PLL>;
> > + clock-names = "ref_clk", "sata_clk", "pcie_clk",
> > + "eth_clk";
> > +
> > + clock-output-names = "sys_sata_ref_clk", "sys_apb_clk",
> > + "sys_gmac0_csr_clk", "sys_gmac0_tx_clk",
> > + "sys_gmac0_ptp_clk", "sys_gmac1_csr_clk",
> > + "sys_gmac1_tx_clk", "sys_gmac1_ptp_clk",
> > + "sys_xgmac_ref_clk", "sys_xgmac_ptp_clk",
> > + "sys_usb_clk", "sys_pvt_clk",
> > + "sys_hwa_clk", "sys_uart_clk",
> > + "sys_spi_clk", "sys_i2c1_clk",
> > + "sys_i2c2_clk", "sys_gpio_clk",
> > + "sys_timer0_clk", "sys_timer1_clk",
> > + "sys_timer2_clk", "sys_wdt_clk";
> > + };
> > +...
> > diff --git a/include/dt-bindings/reset/bt1-ccu.h b/include/dt-bindings/reset/bt1-ccu.h
> > index 4de5b6bcd433..0bd8fd0edb41 100644
> > --- a/include/dt-bindings/reset/bt1-ccu.h
> > +++ b/include/dt-bindings/reset/bt1-ccu.h
> > @@ -20,4 +20,8 @@
> > #define CCU_AXI_HWA_RST 9
> > #define CCU_AXI_SRAM_RST 10
> >
> > +/* Baikal-T1 System Devices CCU Reset indeces. */
>
> indeces is not a word.
Yeah, it was supposed to be "indices". I'll remove this comment anyway.
Regards,
-Sergey
>
> > +#define CCU_SYS_SATA_REF_RST 0
> > +#define CCU_SYS_APB_RST 1
> > +
