Re: [PATCH 3/5] dt-bindings: clk: Add Baikal-T1 System Devices CCU bindings

[Sergey Semin]

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