diff mbox series

[V3,1/2] dt-bindings: pinctrl: document Broadcom Northstar pin mux controller

Message ID 20180926193103.21241-1-zajec5@gmail.com (mailing list archive)
State New, archived
Headers show
Series [V3,1/2] dt-bindings: pinctrl: document Broadcom Northstar pin mux controller | expand

Commit Message

Rafał Miłecki Sept. 26, 2018, 7:31 p.m. UTC
From: Rafał Miłecki <rafal@milecki.pl>

Northstar has mux controller just like Northstar Plus and Northstar2.
It's a bit different though (different registers & pins) so it requires
its own binding.

It's needed to allow other block bindings specify required mux setup.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
V2: Use "cru_gpio_control"
    Add more functions & groups
    Include Florian's Reviewed-by
V3: Use 3 different bindings as available pins depend on the chipset.
    Strings match Documentation/devicetree/bindings/arm/bcm/brcm,bcm4708.txt
---
 .../bindings/pinctrl/brcm,bcm4708-pinmux.txt       | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt

Comments

Linus Walleij Oct. 10, 2018, 7:16 a.m. UTC | #1
On Wed, Sep 26, 2018 at 9:31 PM Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
>
> Northstar has mux controller just like Northstar Plus and Northstar2.
> It's a bit different though (different registers & pins) so it requires
> its own binding.
>
> It's needed to allow other block bindings specify required mux setup.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> V2: Use "cru_gpio_control"
>     Add more functions & groups
>     Include Florian's Reviewed-by
> V3: Use 3 different bindings as available pins depend on the chipset.
>     Strings match Documentation/devicetree/bindings/arm/bcm/brcm,bcm4708.txt

Patch applied.

Yours,
Linus Walleij
Rob Herring (Arm) Oct. 11, 2018, 10:22 p.m. UTC | #2
On Wed, Sep 26, 2018 at 09:31:02PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Northstar has mux controller just like Northstar Plus and Northstar2.
> It's a bit different though (different registers & pins) so it requires
> its own binding.
> 
> It's needed to allow other block bindings specify required mux setup.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> V2: Use "cru_gpio_control"
>     Add more functions & groups
>     Include Florian's Reviewed-by
> V3: Use 3 different bindings as available pins depend on the chipset.
>     Strings match Documentation/devicetree/bindings/arm/bcm/brcm,bcm4708.txt
> ---
>  .../bindings/pinctrl/brcm,bcm4708-pinmux.txt       | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt
> new file mode 100644
> index 000000000000..af906f196e8c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt
> @@ -0,0 +1,42 @@
> +Broadcom Northstar pins mux controller
> +
> +Some of Northstar SoCs's pins can be used for various purposes thanks to the mux
> +controller. This binding allows describing mux controller and listing available
> +functions. They can be referenced later by other bindings to let system
> +configure controller correctly.
> +
> +A list of pins varies across chipsets so few bindings are available.
> +
> +Required properties:
> +- compatible: must be one of:
> +	"brcm,bcm4708-pinmux"
> +	"brcm,bcm4709-pinmux"
> +	"brcm,bcm53012-pinmux"
> +- reg: iomem address range of CRU (Central Resource Unit) pin registers

Perhaps 'cru' in the compatible if that's what the h/w is called?

Also, if this is a sub-block, then it should be a child of the block 
which should be defined here.

> +- reg-names: "cru_gpio_control" - the only needed & supported reg right now
> +
> +Functions and their groups available for all chipsets:
> +- "spi": "spi_grp"
> +- "i2c": "i2c_grp"
> +- "pwm": "pwm0_grp", "pwm1_grp", "pwm2_grp", "pwm3_grp"
> +- "uart1": "uart1_grp"
> +
> +Additionally available on BCM4709 and BCM53012:
> +- "mdio": "mdio_grp"
> +- "uart2": "uart2_grp"
> +- "sdio": "sdio_pwr_grp", "sdio_1p8v_grp"
> +
> +For documentation of subnodes see:
> +Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +Example:
> +	pinctrl@1800c1c0 {
> +		compatible = "brcm,bcm4708-pinmux";
> +		reg = <0x1800c1c0 0x24>;
> +		reg-names = "cru_gpio_control";
> +
> +		spi {

You'll find this now causes dtc warnings. 'spi' is reserved for SPI 
controller nodes. So 'spi-pins' perhaps.

> +			function = "spi";
> +			groups = "spi_grp";
> +		};
> +	};
> -- 
> 2.13.7
>
Linus Walleij Oct. 12, 2018, 9:13 a.m. UTC | #3
Rafał, could you patch the bindings with Rob comments so I don't have
to pull the patch out of my tree or revert it?

Yours,
Linus Walleij
Rafał Miłecki Oct. 15, 2018, 8:56 a.m. UTC | #4
On 2018-10-12 00:22, Rob Herring wrote:
> On Wed, Sep 26, 2018 at 09:31:02PM +0200, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> Northstar has mux controller just like Northstar Plus and Northstar2.
>> It's a bit different though (different registers & pins) so it 
>> requires
>> its own binding.
>> 
>> It's needed to allow other block bindings specify required mux setup.
>> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> V2: Use "cru_gpio_control"
>>     Add more functions & groups
>>     Include Florian's Reviewed-by
>> V3: Use 3 different bindings as available pins depend on the chipset.
>>     Strings match 
>> Documentation/devicetree/bindings/arm/bcm/brcm,bcm4708.txt
>> ---
>>  .../bindings/pinctrl/brcm,bcm4708-pinmux.txt       | 42 
>> ++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt 
>> b/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt
>> new file mode 100644
>> index 000000000000..af906f196e8c
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt
>> @@ -0,0 +1,42 @@
>> +Broadcom Northstar pins mux controller
>> +
>> +Some of Northstar SoCs's pins can be used for various purposes thanks 
>> to the mux
>> +controller. This binding allows describing mux controller and listing 
>> available
>> +functions. They can be referenced later by other bindings to let 
>> system
>> +configure controller correctly.
>> +
>> +A list of pins varies across chipsets so few bindings are available.
>> +
>> +Required properties:
>> +- compatible: must be one of:
>> +	"brcm,bcm4708-pinmux"
>> +	"brcm,bcm4709-pinmux"
>> +	"brcm,bcm53012-pinmux"
>> +- reg: iomem address range of CRU (Central Resource Unit) pin 
>> registers
> 
> Perhaps 'cru' in the compatible if that's what the h/w is called?
> 
> Also, if this is a sub-block, then it should be a child of the block
> which should be defined here.

It took me some time to do some extra research on the whole CRU thing.

Valuable resources:
[1] bcm5301x_dmu.c (from the SDK)
[2] bcm5301x_pcie.c (from the SDK)
[3] https://patchwork.kernel.org/patch/7888651/
[4] https://patchwork.kernel.org/patch/5051471/

First of all CRU seems to be a sub-block of the DMU (which stands for
"Device Management Unit" according to the [1]). DMU seems to be
independent block at 0x1800c000 with a size of 0x1000.

It isn't actually clear what the CRU stands for. In Broadcom's case:
1. According to the [3] it's "Clock and Reset Unit"
2. According to the [4] it's a "central resource unit"
Other vendors seem to use CRU name for "Clock and Reset Unit".

In any case, you're right Rob, it's a sub-block, a set of random
registers that control SoC.

So I think that:
1) We should have a node for DMU and CRU
2) We should not include "cru" in bindings as pinmuxing seems to be part
    of SoC that just happens to be controlled using CRU registers
Linus Walleij Oct. 16, 2018, 7:41 a.m. UTC | #5
On Mon, Oct 15, 2018 at 10:56 AM Rafał Miłecki <rafal@milecki.pl> wrote:

> > Perhaps 'cru' in the compatible if that's what the h/w is called?
> >
> > Also, if this is a sub-block, then it should be a child of the block
> > which should be defined here.
>
> It took me some time to do some extra research on the whole CRU thing.
>
> Valuable resources:
> [1] bcm5301x_dmu.c (from the SDK)
> [2] bcm5301x_pcie.c (from the SDK)
> [3] https://patchwork.kernel.org/patch/7888651/
> [4] https://patchwork.kernel.org/patch/5051471/
>
> First of all CRU seems to be a sub-block of the DMU (which stands for
> "Device Management Unit" according to the [1]). DMU seems to be
> independent block at 0x1800c000 with a size of 0x1000.
>
> It isn't actually clear what the CRU stands for. In Broadcom's case:
> 1. According to the [3] it's "Clock and Reset Unit"
> 2. According to the [4] it's a "central resource unit"
> Other vendors seem to use CRU name for "Clock and Reset Unit".
>
> In any case, you're right Rob, it's a sub-block, a set of random
> registers that control SoC.
>
> So I think that:
> 1) We should have a node for DMU and CRU
> 2) We should not include "cru" in bindings as pinmuxing seems to be part
>     of SoC that just happens to be controlled using CRU registers

OK do you want me to revert the patches or do you want to
fix this stuff on top of the patches that are already in the tree?
I'm fine either way.

Yours,
Linus Walleij
Rafał Miłecki Oct. 16, 2018, 9:06 a.m. UTC | #6
On Tue, 16 Oct 2018 at 09:41, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Oct 15, 2018 at 10:56 AM Rafał Miłecki <rafal@milecki.pl> wrote:
>
> > > Perhaps 'cru' in the compatible if that's what the h/w is called?
> > >
> > > Also, if this is a sub-block, then it should be a child of the block
> > > which should be defined here.
> >
> > It took me some time to do some extra research on the whole CRU thing.
> >
> > Valuable resources:
> > [1] bcm5301x_dmu.c (from the SDK)
> > [2] bcm5301x_pcie.c (from the SDK)
> > [3] https://patchwork.kernel.org/patch/7888651/
> > [4] https://patchwork.kernel.org/patch/5051471/
> >
> > First of all CRU seems to be a sub-block of the DMU (which stands for
> > "Device Management Unit" according to the [1]). DMU seems to be
> > independent block at 0x1800c000 with a size of 0x1000.
> >
> > It isn't actually clear what the CRU stands for. In Broadcom's case:
> > 1. According to the [3] it's "Clock and Reset Unit"
> > 2. According to the [4] it's a "central resource unit"
> > Other vendors seem to use CRU name for "Clock and Reset Unit".
> >
> > In any case, you're right Rob, it's a sub-block, a set of random
> > registers that control SoC.
> >
> > So I think that:
> > 1) We should have a node for DMU and CRU
> > 2) We should not include "cru" in bindings as pinmuxing seems to be part
> >     of SoC that just happens to be controlled using CRU registers
>
> OK do you want me to revert the patches or do you want to
> fix this stuff on top of the patches that are already in the tree?
> I'm fine either way.

I'm fine either way, thanks for taking care of my work!
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt b/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt
new file mode 100644
index 000000000000..af906f196e8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt
@@ -0,0 +1,42 @@ 
+Broadcom Northstar pins mux controller
+
+Some of Northstar SoCs's pins can be used for various purposes thanks to the mux
+controller. This binding allows describing mux controller and listing available
+functions. They can be referenced later by other bindings to let system
+configure controller correctly.
+
+A list of pins varies across chipsets so few bindings are available.
+
+Required properties:
+- compatible: must be one of:
+	"brcm,bcm4708-pinmux"
+	"brcm,bcm4709-pinmux"
+	"brcm,bcm53012-pinmux"
+- reg: iomem address range of CRU (Central Resource Unit) pin registers
+- reg-names: "cru_gpio_control" - the only needed & supported reg right now
+
+Functions and their groups available for all chipsets:
+- "spi": "spi_grp"
+- "i2c": "i2c_grp"
+- "pwm": "pwm0_grp", "pwm1_grp", "pwm2_grp", "pwm3_grp"
+- "uart1": "uart1_grp"
+
+Additionally available on BCM4709 and BCM53012:
+- "mdio": "mdio_grp"
+- "uart2": "uart2_grp"
+- "sdio": "sdio_pwr_grp", "sdio_1p8v_grp"
+
+For documentation of subnodes see:
+Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+
+Example:
+	pinctrl@1800c1c0 {
+		compatible = "brcm,bcm4708-pinmux";
+		reg = <0x1800c1c0 0x24>;
+		reg-names = "cru_gpio_control";
+
+		spi {
+			function = "spi";
+			groups = "spi_grp";
+		};
+	};