Re: Proper DT bindings for Broadcom's DMU node

[Florian Fainelli <f.fainelli@gmail.com>]

On 8/20/20 5:38 AM, Rafał Miłecki wrote:
> 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";

This should actually be a simple-bus no?

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

It does not seem to me that there is an use case for syscon except for
the USB2 PHY PLL which should arguably be a clock provider and the "DMP"
register base used by the USB 3.0 PHY which should be a reset provider.
-- 
Florian