Proper DT bindings for Broadcom's DMU node

Proper DT bindings for Broadcom's DMU node

[Rafał Miłecki]

Hi,

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


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)

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/

Re: Proper DT bindings for Broadcom's DMU node

[Ray Jui]

Hi Rafal,

On 5/4/2020 8:24 AM, Rafał Miłecki wrote:
> Hi,
> 
> 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].
> 
> 
> 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)
> 
> 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?

The above two proposed scheme from you to handle CRU in NS makes sense
to me and they also model the ASIC architecture correctly.

Thanks,

Ray

> 
> 
> [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/
> 

Re: Proper DT bindings for Broadcom's DMU node

[Rafał Miłecki]

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/

Re: Proper DT bindings for Broadcom's DMU node

[Florian Fainelli]

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