Re: Proper DT bindings for Broadcom's DMU node

["Rafał Miłecki" <zajec5@gmail.com>]

Hi Rob, could you help me/us with Northstar bindings design, please?

On 04.05.2020 17:24, Rafał Miłecki wrote:
> I need some help with designing proper bindings for Broadcom's DMU
> block. We already have it partially covered but it's inconsistent, some
> cleanups were rejected and so I want to redesign it as it should be.
> 
> DMU ("Device Management Unit") is a block that can be found on Broadcom
> iProc / Northstar devices. On Northstar it's mapped at:
> ranges = <0x1800c000 0x1000>
> 
> It contains:
> 1. Few random registers, some of them shared by various hardware
>     blocks (and possibly hard to abstract?)
> 2. At least one sub-block with even more random registers
> 
> Some of known DMU registers are:
> reg = <0x100 0x14>    CRU LCPLL control0
> reg = <0x140 0x24>    CRU GENPLL
> reg = <0x164 0x04>    CRU_USB2_CONTROL
> reg = <0x180 0x04>    CRU_CLKSET_KEY
> reg = <0x184 0x04>    CRU_RESET
> reg = <0x1c0 0x24>    pinctrl
> reg = <0x2a0 0x04>    CRU_STRAPS_CTRL
> reg = <0x2c0 0x04>    PVTMON control0
> (Broadcom never released a proper documentation)
> 
> 
> As you can see there are a few CRU registers (depending on a source it's
> a "Clock and Reset Unit" or "Central Resource Unit"). It's said to be
> separated block and was described by Scott (from Broadcom) as: "unit
> with a lot of random registers to perform various operations".
> 
> As I said, there are also some shared registers:
> 
> CRU_CLKSET_KEY is accessed by:
> 1. USB 2.0 PHY driver for (un)locking DMU PLL settings
> 2. GMAC for changing 2.66G line rate to 2Gbps
> 
> CRU_STRAPS_CTRL needs to be accessed by:
> 1. USB 3.0 PHY driver for PHY connected to MDIO
> 2. PCIE driver for PHY connected to MDIO
> 
> 
> My initial idea was to have something like:
> 
> dmu@1800c000 {
>      compatible = "simple-bus";
>      ranges = <0 0x1800c000 0x1000>;
>      #address-cells = <1>;
>      #size-cells = <1>;
> 
>      cru@100 {
>          compatible = "simple-bus";
>          reg = <0x100 0x1a4>;
> 
>          lcpll { ... };
>          genpll { ... };
>          reset { ... };
>      };
> };
> 
> but Rob noticed that "simple-bus" requires everything in DMU to have
> sub-nodes [0] [1].

"simple-bus" apparently is a no-way as some single registers may need to
be referenced using syscon.


> I thought it can be solved by using compatible = "syscon", "simple-mfd"
> and I even got one patch for that accepted [2] [3] (pinctrl). It seems
> it slipped through and was possibly a mistake. Another similar patch was
> rejected [4] [5] (bcm-ns-usb2-phy).
> 
> What I tried to achieve was something like this:
> 
> dmu@1800c000 {
>      compatible = "simple-bus";
>      ranges = <0 0x1800c000 0x1000>;
>      #address-cells = <1>;
>      #size-cells = <1>;
> 
>      cru: syscon@100 {
>          compatible = "syscon", "simple-mfd";
>          reg = <0x100 0x1a4>;
>          ranges;
>          #address-cells = <1>;
>          #size-cells = <1>;
> 
>          lcpll0@0 {
>              #clock-cells = <1>;
>              compatible = "brcm,nsp-lcpll0";
>              reg = <0x0 0x14>;
>          };
> 
>          genpll@40 {
>              #clock-cells = <1>;
>              compatible = "brcm,nsp-genpll";
>              reg = <0x40 0x24>;
>          };
> 
>          pin-controller@c0 {
>              compatible = "brcm,bcm4708-pinmux";
>              reg = <0xc0 0x24>;
>              reg-names = "cru_gpio_control";
>          };
> 
>          thermal@1c0 {
>              compatible = "brcm,ns-thermal";
>              reg = <0x1c0 0x10>;
>              #thermal-sensor-cells = <0>;
>          };
>      };
> };
> 
> cru-reset@??? {
>      compatible = "brcm,ns-cru-reset";
>      syscon-cru = <&cru>; /* CRU_RESET */
>      #reset-cells = <1>;
> };
> 
> usb2-phy@??? {
>      compatible = "brcm,ns-usb2-phy";
>      syscon-cru = <&cru>; /* CRU_CLKSET_KEY */
>      #phy-cells = <0>;
> };
> 
> (apparently it wasn't a good idea)

Here I tried "syscon", "simple-mfd" which lets me:
1. Have subnodes for all small hardware subblocks
2. Reference single registers using syscon
but it appears I can't mix those two.


> So my question is: how to properly handle this? I'm not sure what's the
> proper "compatible" string to use. Is my idea of:
> 1. Using sub-node for registers explicitly used by one driver
> 2. Using syscon for shared registers
> OK?
> 
> 
> [0] https://www.spinics.net/lists/arm-kernel/msg682838.html
> [1] https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20181015093013.31651-1-zajec5@gmail.com/
> [2] https://spinics.net/lists/linux-gpio/msg35285.html
> [3] https://patchwork.kernel.org/patch/10735931/
> [4] https://lkml.org/lkml/2019/1/15/913
> [5] https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20190108123907.19816-1-zajec5@gmail.com/