[RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Arend van Spriel]

The Broadcom bcm43xx sdio devices are fullmac devices that may be
integrated in ARM platforms. Currently, the brcmfmac driver for
these devices support use of platform data. This patch specifies
the bindings that allow this platform data to be expressed in the
devicetree.

Cc: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
Cc: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Hante Meuleman <meuleman-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Pieter-Paul Giesberts <pieterpg-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Arend van Spriel <arend-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
This devicetree binding proposal is intended for platforms with
Broadcom wireless device in MMC sdio slot. These devices may
have their own interrupt and power line. Also the SDIO drive
strength is often hardware dependent and expressed in this
binding.

Not sure if this should go in staging or not. Feel free to
comment on this proposal.

Regards,
Arend
---
 .../staging/net/wireless/brcm,bcm43xx-fmac.txt     |   37 ++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt

diff --git a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
new file mode 100644
index 0000000..535f343
--- /dev/null
+++ b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
@@ -0,0 +1,37 @@
+Broadcom BCM43xx Fullmac wireless SDIO devices
+
+This node provides properties for controlling the Broadcom wireless device. The
+node is expected to be specified as a child node to the MMC controller that
+connects the device to the system.
+
+Required properties:
+
+ - compatible : Should be "brcm,bcm43xx-fmac".
+ - wlan-supply : phandle for fixed regulator used to control power for
+	the device/module.
+
+Optional properties:
+ - drive-strength : drive strength used for SDIO pins on device (default = 6mA).
+ - interrupt-parent : the phandle for the interrupt controller to which the
+	device interrupt (HOST_WAKE) is connected.
+ - interrupts : interrupt specifier encoded according the interrupt controller
+	specified by interrupt-parent property.
+
+Example:
+
+mmc3: mmc@01c20000 {
+	pinctrl-0 = <&mmc3_pins>;
+	pinctrl-1 = <&wifi_host_wake>;
+	vmmc-supply = <&mmc3_supply>;
+	bus-width = <4>;
+
+	bcm4335: bcm4335@0 {
+		compatible = "brcm,bcm43xx-fmac";
+		wlan-supply = <&wlan-reg>;
+		drive-strength = <4>;
+		interrupt-parent = <&gpx2>;
+		interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "HOST_WAKE";
+	};
+};
+
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Chen-Yu Tsai]

On Tue, Feb 11, 2014 at 3:17 AM, Arend van Spriel <arend@broadcom.com> wrote:
> The Broadcom bcm43xx sdio devices are fullmac devices that may be
> integrated in ARM platforms. Currently, the brcmfmac driver for
> these devices support use of platform data. This patch specifies
> the bindings that allow this platform data to be expressed in the
> devicetree.
>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> ---
> This devicetree binding proposal is intended for platforms with
> Broadcom wireless device in MMC sdio slot. These devices may
> have their own interrupt and power line. Also the SDIO drive
> strength is often hardware dependent and expressed in this
> binding.

Hi,

I believe the interrupts and drive-strength properties are Ok.
However, letting the platform driver power up the wifi chip does
not guarantee proper ordering, does it? I mean, what if the MMC
bus was probed before the (non-removable) chip was switched on?

Being able to tie that to the mmc controller would be better IMO.
I think this was discussed in Olof's "mmc: add support for
power-on sequencing through DT" thread.


Thanks!
ChenYu

>
> Not sure if this should go in staging or not. Feel free to
> comment on this proposal.
>
> Regards,
> Arend

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Tomasz Figa]

Hi Arend,

On 10.02.2014 20:17, Arend van Spriel wrote:
> The Broadcom bcm43xx sdio devices are fullmac devices that may be
> integrated in ARM platforms. Currently, the brcmfmac driver for
> these devices support use of platform data. This patch specifies
> the bindings that allow this platform data to be expressed in the
> devicetree.
>
> Cc: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> Cc: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Hante Meuleman <meuleman-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Pieter-Paul Giesberts <pieterpg-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Arend van Spriel <arend-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
> This devicetree binding proposal is intended for platforms with
> Broadcom wireless device in MMC sdio slot. These devices may
> have their own interrupt and power line. Also the SDIO drive
> strength is often hardware dependent and expressed in this
> binding.
>
> Not sure if this should go in staging or not. Feel free to
> comment on this proposal.
>
> Regards,
> Arend
> ---
>   .../staging/net/wireless/brcm,bcm43xx-fmac.txt     |   37 ++++++++++++++++++++
>   1 file changed, 37 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>
> diff --git a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
> new file mode 100644
> index 0000000..535f343
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
> @@ -0,0 +1,37 @@
> +Broadcom BCM43xx Fullmac wireless SDIO devices
> +
> +This node provides properties for controlling the Broadcom wireless device. The
> +node is expected to be specified as a child node to the MMC controller that
> +connects the device to the system.
> +
> +Required properties:
> +
> + - compatible : Should be "brcm,bcm43xx-fmac".
> + - wlan-supply : phandle for fixed regulator used to control power for
> +	the device/module.

The BCM43xx WLAN chips I used to work always have been controlled by a 
simple power enable GPIO of the chip itself. Has this changed in newer 
chips?

If you need to simply toggle a GPIO to control the power, you don't need 
to use the regulator API at all, controlling the GPIO directly.

> +
> +Optional properties:
> + - drive-strength : drive strength used for SDIO pins on device (default = 6mA).

This should be a part of the MMC binding, I think. Probably also moved 
under MMC controller's node, since it's a board-specific property 
altering the parameters of the MMC controller, not the WLAN chip.

> + - interrupt-parent : the phandle for the interrupt controller to which the
> +	device interrupt (HOST_WAKE) is connected.
> + - interrupts : interrupt specifier encoded according the interrupt controller
> +	specified by interrupt-parent property.

I would also add a clock here, since the BCM43xx chips usually need a 
32k clock to operate (or at least the ones I used to work with did). It 
can be optional, as not all systems can control this clock.

> +
> +Example:
> +
> +mmc3: mmc@01c20000 {
> +	pinctrl-0 = <&mmc3_pins>;
> +	pinctrl-1 = <&wifi_host_wake>;

WLAN_HOST_WAKE pin (aka the OOB interrupt) is specific to the WLAN chip, 
so this should be rather configured in a pinctrl state of the WLAN chip 
itself.

> +	vmmc-supply = <&mmc3_supply>;
> +	bus-width = <4>;
> +
> +	bcm4335: bcm4335@0 {

nit: Why @0, if there is no reg property under this node?

Best regards,
Tomasz

> +		compatible = "brcm,bcm43xx-fmac";
> +		wlan-supply = <&wlan-reg>;
> +		drive-strength = <4>;
> +		interrupt-parent = <&gpx2>;
> +		interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "HOST_WAKE";
> +	};
> +};
> +
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Chen-Yu Tsai]

Hi,

On Thu, Feb 13, 2014 at 5:13 PM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Arend,
>
>
> On 10.02.2014 20:17, Arend van Spriel wrote:
>>
>> The Broadcom bcm43xx sdio devices are fullmac devices that may be
>> integrated in ARM platforms. Currently, the brcmfmac driver for
>> these devices support use of platform data. This patch specifies
>> the bindings that allow this platform data to be expressed in the
>> devicetree.
>>
>> Cc: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> Cc: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Reviewed-by: Hante Meuleman <meuleman-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Reviewed-by: Pieter-Paul Giesberts <pieterpg-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Arend van Spriel <arend-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> ---
>> This devicetree binding proposal is intended for platforms with
>> Broadcom wireless device in MMC sdio slot. These devices may
>> have their own interrupt and power line. Also the SDIO drive
>> strength is often hardware dependent and expressed in this
>> binding.
>>
>> Not sure if this should go in staging or not. Feel free to
>> comment on this proposal.
>>
>> Regards,
>> Arend
>> ---
>>   .../staging/net/wireless/brcm,bcm43xx-fmac.txt     |   37
>> ++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>> b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>> new file mode 100644
>> index 0000000..535f343
>> --- /dev/null
>> +++
>> b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>> @@ -0,0 +1,37 @@
>> +Broadcom BCM43xx Fullmac wireless SDIO devices
>> +
>> +This node provides properties for controlling the Broadcom wireless
>> device. The
>> +node is expected to be specified as a child node to the MMC controller
>> that
>> +connects the device to the system.
>> +
>> +Required properties:
>> +
>> + - compatible : Should be "brcm,bcm43xx-fmac".
>> + - wlan-supply : phandle for fixed regulator used to control power for
>> +       the device/module.
>
>
> The BCM43xx WLAN chips I used to work always have been controlled by a
> simple power enable GPIO of the chip itself. Has this changed in newer
> chips?

Actually some (I only have one kind) of the chips have 2 GPIO lines,
but they are almost always tied together in the module or on the
board.

> If you need to simply toggle a GPIO to control the power, you don't need to
> use the regulator API at all, controlling the GPIO directly.
>
>
>> +
>> +Optional properties:
>> + - drive-strength : drive strength used for SDIO pins on device (default
>> = 6mA).
>
>
> This should be a part of the MMC binding, I think. Probably also moved under
> MMC controller's node, since it's a board-specific property altering the
> parameters of the MMC controller, not the WLAN chip.

AFAIK, this controls the drive strength for MMC on the WLAN chip's MMC
controller.

>> + - interrupt-parent : the phandle for the interrupt controller to which
>> the
>> +       device interrupt (HOST_WAKE) is connected.
>> + - interrupts : interrupt specifier encoded according the interrupt
>> controller
>> +       specified by interrupt-parent property.
>
>
> I would also add a clock here, since the BCM43xx chips usually need a 32k
> clock to operate (or at least the ones I used to work with did). It can be
> optional, as not all systems can control this clock.

2, the main oscillator (MHz range) and the low power clock (32.768 KHz).
In the other thread, someone mentioned they had to enable the main
oscillator separately.

>> +
>> +Example:
>> +
>> +mmc3: mmc@01c20000 {
>> +       pinctrl-0 = <&mmc3_pins>;
>> +       pinctrl-1 = <&wifi_host_wake>;
>
>
> WLAN_HOST_WAKE pin (aka the OOB interrupt) is specific to the WLAN chip, so
> this should be rather configured in a pinctrl state of the WLAN chip itself.

AFAIK, the pinctrl in tied to the device node, and is selected when the device
is registered. The MMC subsystem currently does not register child nodes, so
this would be useless.

brcmfmac actually has to walk the whole DT to find the node with the right
compatible.

>> +       vmmc-supply = <&mmc3_supply>;
>> +       bus-width = <4>;
>> +
>> +       bcm4335: bcm4335@0 {
>
>
> nit: Why @0, if there is no reg property under this node?

Off chance that you have 2 or more nodes with the same name?
The node name should be more generic, too.

This brings about a different problem, how should driver in its current
state, differentiate between the two? And brcmfmac only supports one set
of platform data ATM.


Cheers
ChenYu


> Best regards,
> Tomasz
>
>
>> +               compatible = "brcm,bcm43xx-fmac";
>> +               wlan-supply = <&wlan-reg>;
>> +               drive-strength = <4>;
>> +               interrupt-parent = <&gpx2>;
>> +               interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
>> +               interrupt-names = "HOST_WAKE";
>> +       };
>> +};
>> +
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Arend van Spriel]

On 02/13/14 10:28, Chen-Yu Tsai wrote:
> Hi,
>
> On Thu, Feb 13, 2014 at 5:13 PM, Tomasz Figa<tomasz.figa@gmail.com>  wrote:
>> Hi Arend,
>>
>>
>> On 10.02.2014 20:17, Arend van Spriel wrote:

[...]

>>> +
>>> +Example:
>>> +
>>> +mmc3: mmc@01c20000 {
>>> +       pinctrl-0 =<&mmc3_pins>;
>>> +       pinctrl-1 =<&wifi_host_wake>;
>>
>>
>> WLAN_HOST_WAKE pin (aka the OOB interrupt) is specific to the WLAN chip, so
>> this should be rather configured in a pinctrl state of the WLAN chip itself.

Hi Chen-Yu,

picking up this thread.

> AFAIK, the pinctrl in tied to the device node, and is selected when the device
> is registered. The MMC subsystem currently does not register child nodes, so
> this would be useless.

So if MMC does not register child nodes, brcmfmac will not be probed 
with of_node set? Have there been patches submitted for this in mmc 
subsystem recently.

> brcmfmac actually has to walk the whole DT to find the node with the right
> compatible.

Is it just me or should this be avoided? What if there are multiple entries?

Regards,
Arend

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Arend van Spriel]

On 02/13/2014 10:13 AM, Tomasz Figa wrote:
> Hi Arend,
> 
> On 10.02.2014 20:17, Arend van Spriel wrote:
>> The Broadcom bcm43xx sdio devices are fullmac devices that may be
>> integrated in ARM platforms. Currently, the brcmfmac driver for
>> these devices support use of platform data. This patch specifies
>> the bindings that allow this platform data to be expressed in the
>> devicetree.
>>
>> Cc: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> Cc: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Reviewed-by: Hante Meuleman <meuleman-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Reviewed-by: Pieter-Paul Giesberts <pieterpg-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Arend van Spriel <arend-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> ---
>> This devicetree binding proposal is intended for platforms with
>> Broadcom wireless device in MMC sdio slot. These devices may
>> have their own interrupt and power line. Also the SDIO drive
>> strength is often hardware dependent and expressed in this
>> binding.
>>
>> Not sure if this should go in staging or not. Feel free to
>> comment on this proposal.
>>
>> Regards,
>> Arend
>> ---
>>   .../staging/net/wireless/brcm,bcm43xx-fmac.txt     |   37
>> ++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>
>>
>> diff --git
>> a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>> b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>
>> new file mode 100644
>> index 0000000..535f343
>> --- /dev/null
>> +++
>> b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>
>> @@ -0,0 +1,37 @@
>> +Broadcom BCM43xx Fullmac wireless SDIO devices
>> +
>> +This node provides properties for controlling the Broadcom wireless
>> device. The
>> +node is expected to be specified as a child node to the MMC
>> controller that
>> +connects the device to the system.
>> +
>> +Required properties:
>> +
>> + - compatible : Should be "brcm,bcm43xx-fmac".
>> + - wlan-supply : phandle for fixed regulator used to control power for
>> +    the device/module.
> 
> The BCM43xx WLAN chips I used to work always have been controlled by a
> simple power enable GPIO of the chip itself. Has this changed in newer
> chips?
> 
> If you need to simply toggle a GPIO to control the power, you don't need
> to use the regulator API at all, controlling the GPIO directly.

Not sure I understand. Do you really mean 'chip' or 'wifi module' here.
The chip needs to be powered and for that it is hooked up to a
host/module provided GPIO (at least that is my understanding).

>> +
>> +Optional properties:
>> + - drive-strength : drive strength used for SDIO pins on device
>> (default = 6mA).
> 
> This should be a part of the MMC binding, I think. Probably also moved
> under MMC controller's node, since it's a board-specific property
> altering the parameters of the MMC controller, not the WLAN chip.

It is an electrical interfacing parameter between MMC controller and the
device. The specified drive-strength here is used to configure the PMU
on the chip so it is really related to the the chip.

>> + - interrupt-parent : the phandle for the interrupt controller to
>> which the
>> +    device interrupt (HOST_WAKE) is connected.
>> + - interrupts : interrupt specifier encoded according the interrupt
>> controller
>> +    specified by interrupt-parent property.
> 
> I would also add a clock here, since the BCM43xx chips usually need a
> 32k clock to operate (or at least the ones I used to work with did). It
> can be optional, as not all systems can control this clock.
> 
>> +
>> +Example:
>> +
>> +mmc3: mmc@01c20000 {
>> +    pinctrl-0 = <&mmc3_pins>;
>> +    pinctrl-1 = <&wifi_host_wake>;
> 
> WLAN_HOST_WAKE pin (aka the OOB interrupt) is specific to the WLAN chip,
> so this should be rather configured in a pinctrl state of the WLAN chip
> itself.

You mean that pinctrl-1 should move inside the "brcm,bcm43xx-fmac" node,
right?

>> +    vmmc-supply = <&mmc3_supply>;
>> +    bus-width = <4>;
>> +
>> +    bcm4335: bcm4335@0 {
> 
> nit: Why @0, if there is no reg property under this node?

I am not fluent with devicetree specifications (yet) nor know all the
conventions.

> Best regards,
> Tomasz

Thanks,
Arend

>> +        compatible = "brcm,bcm43xx-fmac";
>> +        wlan-supply = <&wlan-reg>;
>> +        drive-strength = <4>;
>> +        interrupt-parent = <&gpx2>;
>> +        interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
>> +        interrupt-names = "HOST_WAKE";
>> +    };
>> +};
>> +
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Tomasz Figa]

On 13.02.2014 13:07, Arend van Spriel wrote:
> On 02/13/2014 10:13 AM, Tomasz Figa wrote:
>> Hi Arend,
>>
>> On 10.02.2014 20:17, Arend van Spriel wrote:
>>> The Broadcom bcm43xx sdio devices are fullmac devices that may be
>>> integrated in ARM platforms. Currently, the brcmfmac driver for
>>> these devices support use of platform data. This patch specifies
>>> the bindings that allow this platform data to be expressed in the
>>> devicetree.
>>>
>>> Cc: Chen-Yu Tsai <wens@csie.org>
>>> Cc: Tomasz Figa <tomasz.figa@gmail.com>
>>> Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
>>> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>>> Signed-off-by: Arend van Spriel <arend@broadcom.com>
>>> ---
>>> This devicetree binding proposal is intended for platforms with
>>> Broadcom wireless device in MMC sdio slot. These devices may
>>> have their own interrupt and power line. Also the SDIO drive
>>> strength is often hardware dependent and expressed in this
>>> binding.
>>>
>>> Not sure if this should go in staging or not. Feel free to
>>> comment on this proposal.
>>>
>>> Regards,
>>> Arend
>>> ---
>>>    .../staging/net/wireless/brcm,bcm43xx-fmac.txt     |   37
>>> ++++++++++++++++++++
>>>    1 file changed, 37 insertions(+)
>>>    create mode 100644
>>> Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>>
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>> b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>>
>>> new file mode 100644
>>> index 0000000..535f343
>>> --- /dev/null
>>> +++
>>> b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>>
>>> @@ -0,0 +1,37 @@
>>> +Broadcom BCM43xx Fullmac wireless SDIO devices
>>> +
>>> +This node provides properties for controlling the Broadcom wireless
>>> device. The
>>> +node is expected to be specified as a child node to the MMC
>>> controller that
>>> +connects the device to the system.
>>> +
>>> +Required properties:
>>> +
>>> + - compatible : Should be "brcm,bcm43xx-fmac".
>>> + - wlan-supply : phandle for fixed regulator used to control power for
>>> +    the device/module.
>>
>> The BCM43xx WLAN chips I used to work always have been controlled by a
>> simple power enable GPIO of the chip itself. Has this changed in newer
>> chips?
>>
>> If you need to simply toggle a GPIO to control the power, you don't need
>> to use the regulator API at all, controlling the GPIO directly.
>
> Not sure I understand. Do you really mean 'chip' or 'wifi module' here.
> The chip needs to be powered and for that it is hooked up to a
> host/module provided GPIO (at least that is my understanding).

Your binding asks for a regulator, not a simple GPIO, which is all I 
believe you need for BCM43xx power handling.

Mark (added to Cc), could we get your opinion on this?

>
>>> +
>>> +Optional properties:
>>> + - drive-strength : drive strength used for SDIO pins on device
>>> (default = 6mA).
>>
>> This should be a part of the MMC binding, I think. Probably also moved
>> under MMC controller's node, since it's a board-specific property
>> altering the parameters of the MMC controller, not the WLAN chip.
>
> It is an electrical interfacing parameter between MMC controller and the
> device. The specified drive-strength here is used to configure the PMU
> on the chip so it is really related to the the chip.
>

Aha, so I misunderstood the description. Anyway, if it's a 
device-specific property, it should have vendor prefix ("brcm,").

>>> + - interrupt-parent : the phandle for the interrupt controller to
>>> which the
>>> +    device interrupt (HOST_WAKE) is connected.
>>> + - interrupts : interrupt specifier encoded according the interrupt
>>> controller
>>> +    specified by interrupt-parent property.
>>
>> I would also add a clock here, since the BCM43xx chips usually need a
>> 32k clock to operate (or at least the ones I used to work with did). It
>> can be optional, as not all systems can control this clock.
>>
>>> +
>>> +Example:
>>> +
>>> +mmc3: mmc@01c20000 {
>>> +    pinctrl-0 = <&mmc3_pins>;
>>> +    pinctrl-1 = <&wifi_host_wake>;
>>
>> WLAN_HOST_WAKE pin (aka the OOB interrupt) is specific to the WLAN chip,
>> so this should be rather configured in a pinctrl state of the WLAN chip
>> itself.
>
> You mean that pinctrl-1 should move inside the "brcm,bcm43xx-fmac" node,
> right?

Yes.

By the way, if not moved to device node, shouldn't it rather be just 
another entry of pinctrl-0? Also isn't pinctrl-names property missing here?

>
>>> +    vmmc-supply = <&mmc3_supply>;
>>> +    bus-width = <4>;
>>> +
>>> +    bcm4335: bcm4335@0 {
>>
>> nit: Why @0, if there is no reg property under this node?
>
> I am not fluent with devicetree specifications (yet) nor know all the
> conventions.

The "@" suffix is reserved for bus constructs, where the unit-address 
value after "@" corresponds to first address in reg property of the 
node. Here you could just simply use "bcm4335" or if you somehow manage 
to have two such chips (which I don't think is likely to happen) then 
you could use "bcm4335-1".

Best regards,
Tomasz

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

On Thu, Feb 13, 2014 at 01:35:41PM +0100, Tomasz Figa wrote:
> On 13.02.2014 13:07, Arend van Spriel wrote:
> >On 02/13/2014 10:13 AM, Tomasz Figa wrote:

> >>The BCM43xx WLAN chips I used to work always have been controlled by a
> >>simple power enable GPIO of the chip itself. Has this changed in newer
> >>chips?

> >>If you need to simply toggle a GPIO to control the power, you don't need
> >>to use the regulator API at all, controlling the GPIO directly.

> >Not sure I understand. Do you really mean 'chip' or 'wifi module' here.
> >The chip needs to be powered and for that it is hooked up to a
> >host/module provided GPIO (at least that is my understanding).

> Your binding asks for a regulator, not a simple GPIO, which is all I
> believe you need for BCM43xx power handling.

> Mark (added to Cc), could we get your opinion on this?

If it's a GPIO connected directly to the device it should be using
gpiolib, the fact that it happens to have an effect on power rather than
(say) just being a reset isn't terribly relevant to anything outside the
driver.  If it's an external regulator that happens to be controlled
using a GPIO on the current system then a regulator is better since
another system might use a regulator with a different control interface.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Ulf Hansson]

On 13 February 2014 17:22, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Feb 13, 2014 at 01:35:41PM +0100, Tomasz Figa wrote:
>> On 13.02.2014 13:07, Arend van Spriel wrote:
>> >On 02/13/2014 10:13 AM, Tomasz Figa wrote:
>
>> >>The BCM43xx WLAN chips I used to work always have been controlled by a
>> >>simple power enable GPIO of the chip itself. Has this changed in newer
>> >>chips?
>
>> >>If you need to simply toggle a GPIO to control the power, you don't need
>> >>to use the regulator API at all, controlling the GPIO directly.
>
>> >Not sure I understand. Do you really mean 'chip' or 'wifi module' here.
>> >The chip needs to be powered and for that it is hooked up to a
>> >host/module provided GPIO (at least that is my understanding).
>
>> Your binding asks for a regulator, not a simple GPIO, which is all I
>> believe you need for BCM43xx power handling.
>
>> Mark (added to Cc), could we get your opinion on this?
>
> If it's a GPIO connected directly to the device it should be using
> gpiolib, the fact that it happens to have an effect on power rather than
> (say) just being a reset isn't terribly relevant to anything outside the
> driver.  If it's an external regulator that happens to be controlled
> using a GPIO on the current system then a regulator is better since
> another system might use a regulator with a different control interface.

>From an SDIO point of view, we need to find the corresponding OCR mask
the mmc host driver supports. The OCR mask gives the voltage levels
the host supports and it's being used during SDIO initialization while
negotiating the voltage level with the SDIO card.

Using a gpio regulator would simplify here, since the driver don't
need specific "SDIO hacks" but can use the mmc_regulator_get_supply()
API, which calculates the OCR mask, based upon the regulator's
supported voltages.

Kind regards
Ulf Hansson

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On Mon, Mar 31, 2014 at 10:24:00AM +0200, Ulf Hansson wrote:

> Using a gpio regulator would simplify here, since the driver don't
> need specific "SDIO hacks" but can use the mmc_regulator_get_supply()
> API, which calculates the OCR mask, based upon the regulator's
> supported voltages.

If that makes life easier the driver could also just instantiate a fixed
regulator itself rather than requiring that one be put in the DT?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Stephen Warren]

On 02/10/2014 12:17 PM, Arend van Spriel wrote:
> The Broadcom bcm43xx sdio devices are fullmac devices that may be
> integrated in ARM platforms. Currently, the brcmfmac driver for
> these devices support use of platform data. This patch specifies
> the bindings that allow this platform data to be expressed in the
> devicetree.

> diff --git a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt

> + - compatible : Should be "brcm,bcm43xx-fmac".
> + - wlan-supply : phandle for fixed regulator used to control power for
> +	the device/module.

Ignoring the fact that perhaps this should just be a GPIO instead and
assuming it actually make sense for this to be a regulator:

Why "fixed regulator" not just "the regulator". There shouldn't be any
requirement for the power supply to the device to be fixed; the driver
should (a) set the voltage (which will be a no-op for a fixed regulator
already providing that voltage), then (b) enable the regulator. That
would allow a PMIC with programmable voltage to be feeding the device.

Now, if this property was really intended to control some enable GPIO on
the device, as others have said, this shouldn't be a regulator property
but rather a GPIO property. However, there is definitely some power
supply fed to the device, so you definitely need /some/ supply property
here.

Aren't there other enable GPIOs required? These should be specified in DT.

Doesn't the WiFi chip/module require a (32KHz?) clock? If so, that needs
to be represented in DT. Preferably write the binding to require
clock-names (name-based lookup) rather than just clocks (index-based
lookup).

> +Optional properties:
> + - drive-strength : drive strength used for SDIO pins on device (default = 6mA).

As mentioned elsewhere, since that's a binding-specific property, rename
it brcm,drive-strength.

> + - interrupt-parent : the phandle for the interrupt controller to which the
> +	device interrupt (HOST_WAKE) is connected.

That's such a common property, individual bindings don't typically
mention it.

> + - interrupts : interrupt specifier encoded according the interrupt controller
> +	specified by interrupt-parent property.

The description of the property should say which interrupt (name and/or
description) it's describing, even if there's only 1.


> +mmc3: mmc@01c20000 {
> +	pinctrl-0 = <&mmc3_pins>;
> +	pinctrl-1 = <&wifi_host_wake>;
> +	vmmc-supply = <&mmc3_supply>;
> +	bus-width = <4>;

None of that is really relevant to this binding, and may vary from SDIO
controller to SDIO controller, so may end up being wrong.

I'm not sure whether it makes sense to show the example inside some
arbitrary SDIO controller node. Perhaps /just/ put the WiFi node in the
example? The text above should be enough to describe that the node
should be inside an SDIO controller.

> +	bcm4335: bcm4335@0 {
> +		compatible = "brcm,bcm43xx-fmac";
> +		wlan-supply = <&wlan-reg>;
> +		drive-strength = <4>;
> +		interrupt-parent = <&gpx2>;
> +		interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "HOST_WAKE";

interrupt-names wasn't documented in the list of properties above.
Entries in *-names properties are usually lower-case.

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Arend van Spriel]

On 02/25/2014 11:51 PM, Stephen Warren wrote:
> On 02/10/2014 12:17 PM, Arend van Spriel wrote:
>> The Broadcom bcm43xx sdio devices are fullmac devices that may be
>> integrated in ARM platforms. Currently, the brcmfmac driver for
>> these devices support use of platform data. This patch specifies
>> the bindings that allow this platform data to be expressed in the
>> devicetree.
> 
>> diff --git a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
> 
>> + - compatible : Should be "brcm,bcm43xx-fmac".
>> + - wlan-supply : phandle for fixed regulator used to control power for
>> +	the device/module.
> 
> Ignoring the fact that perhaps this should just be a GPIO instead and
> assuming it actually make sense for this to be a regulator:
> 
> Why "fixed regulator" not just "the regulator". There shouldn't be any
> requirement for the power supply to the device to be fixed; the driver
> should (a) set the voltage (which will be a no-op for a fixed regulator
> already providing that voltage), then (b) enable the regulator. That
> would allow a PMIC with programmable voltage to be feeding the device.
> 
> Now, if this property was really intended to control some enable GPIO on
> the device, as others have said, this shouldn't be a regulator property
> but rather a GPIO property. However, there is definitely some power
> supply fed to the device, so you definitely need /some/ supply property
> here.
> 
> Aren't there other enable GPIOs required? These should be specified in DT.
> 
> Doesn't the WiFi chip/module require a (32KHz?) clock? If so, that needs
> to be represented in DT. Preferably write the binding to require
> clock-names (name-based lookup) rather than just clocks (index-based
> lookup).

Hi Stephen,

Thanks for these comments. While I agree with most of them, I am having
some difficulty with the DT concept. Essentially, a DT node describes a
part of the system. My scope for this change is probably limited wearing
my brcmfmac glasses. Am I correct in assuming that a DT node may be
processed/used by multiple drivers. As an example, the 32 kHz clock is
not something brcmfmac cares about. It simple needs to be available and
hooked up to the wlan device. The DT should have another node for this
clock which a (common) clock driver picks up. So having it referenced in
this node is purely informational, right?

Regards,
Arend

>> +Optional properties:
>> + - drive-strength : drive strength used for SDIO pins on device (default = 6mA).
> 
> As mentioned elsewhere, since that's a binding-specific property, rename
> it brcm,drive-strength.
> 
>> + - interrupt-parent : the phandle for the interrupt controller to which the
>> +	device interrupt (HOST_WAKE) is connected.
> 
> That's such a common property, individual bindings don't typically
> mention it.
> 
>> + - interrupts : interrupt specifier encoded according the interrupt controller
>> +	specified by interrupt-parent property.
> 
> The description of the property should say which interrupt (name and/or
> description) it's describing, even if there's only 1.
> 
> 
>> +mmc3: mmc@01c20000 {
>> +	pinctrl-0 = <&mmc3_pins>;
>> +	pinctrl-1 = <&wifi_host_wake>;
>> +	vmmc-supply = <&mmc3_supply>;
>> +	bus-width = <4>;
> 
> None of that is really relevant to this binding, and may vary from SDIO
> controller to SDIO controller, so may end up being wrong.
> 
> I'm not sure whether it makes sense to show the example inside some
> arbitrary SDIO controller node. Perhaps /just/ put the WiFi node in the
> example? The text above should be enough to describe that the node
> should be inside an SDIO controller.
> 
>> +	bcm4335: bcm4335@0 {
>> +		compatible = "brcm,bcm43xx-fmac";
>> +		wlan-supply = <&wlan-reg>;
>> +		drive-strength = <4>;
>> +		interrupt-parent = <&gpx2>;
>> +		interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
>> +		interrupt-names = "HOST_WAKE";
> 
> interrupt-names wasn't documented in the list of properties above.
> Entries in *-names properties are usually lower-case.
> 

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Tomasz Figa]

Hi Arend,

On 13.03.2014 11:16, Arend van Spriel wrote:
> On 02/25/2014 11:51 PM, Stephen Warren wrote:
>> On 02/10/2014 12:17 PM, Arend van Spriel wrote:
>>> The Broadcom bcm43xx sdio devices are fullmac devices that may be
>>> integrated in ARM platforms. Currently, the brcmfmac driver for
>>> these devices support use of platform data. This patch specifies
>>> the bindings that allow this platform data to be expressed in the
>>> devicetree.
>>
>>> diff --git a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>
>>> + - compatible : Should be "brcm,bcm43xx-fmac".
>>> + - wlan-supply : phandle for fixed regulator used to control power for
>>> +	the device/module.
>>
>> Ignoring the fact that perhaps this should just be a GPIO instead and
>> assuming it actually make sense for this to be a regulator:
>>
>> Why "fixed regulator" not just "the regulator". There shouldn't be any
>> requirement for the power supply to the device to be fixed; the driver
>> should (a) set the voltage (which will be a no-op for a fixed regulator
>> already providing that voltage), then (b) enable the regulator. That
>> would allow a PMIC with programmable voltage to be feeding the device.
>>
>> Now, if this property was really intended to control some enable GPIO on
>> the device, as others have said, this shouldn't be a regulator property
>> but rather a GPIO property. However, there is definitely some power
>> supply fed to the device, so you definitely need /some/ supply property
>> here.
>>
>> Aren't there other enable GPIOs required? These should be specified in DT.
>>
>> Doesn't the WiFi chip/module require a (32KHz?) clock? If so, that needs
>> to be represented in DT. Preferably write the binding to require
>> clock-names (name-based lookup) rather than just clocks (index-based
>> lookup).
>
> Hi Stephen,
>
> Thanks for these comments. While I agree with most of them, I am having
> some difficulty with the DT concept. Essentially, a DT node describes a
> part of the system.

That's correct. A DT node represents a component of a system and its 
contents should contain all resources and other device-specific data 
required for this device to operate or optional.

> My scope for this change is probably limited wearing
> my brcmfmac glasses. Am I correct in assuming that a DT node may be
> processed/used by multiple drivers.

It may be, but it is usually not. The typical use case for such scheme 
is a bus-like topology, where devices on the bus are sub-nodes of the 
bus controller node and may contain some bus-specific information, such 
as chip select (e.g. SPI), address (e.g. I2C) or maximum bus speed.

> As an example, the 32 kHz clock is
> not something brcmfmac cares about. It simple needs to be available and
> hooked up to the wlan device.

Not really. The driver should care about any resources needed for the 
device to operate. In this case, a 32 kHz clock even if wired to the 
chip, sometimes is not operational until it gets ungated. This is not an 
artificial example, as on many boards I used to work with the 32 kHz 
clock was driven by a PMIC with clock gating control through I2C, gated 
by default.

Moreover, (well, 32 kHz might not be the best example) from power saving 
reasons, it might be a good idea to let the driver control the clock and 
gate it whenever it is not necessary.

> The DT should have another node for this
> clock which a (common) clock driver picks up. So having it referenced in
> this node is purely informational, right?

You are confusing here provider with consumer. The bcm43xx chip is 
clearly a consumer of a 32 kHz clock and so its DT node should specify this.

A DT node for a clock, would be a clock provider node and that would be 
handled by common clock framework in case of Linux indeed. A clock 
provider node doesn't have to be limited to a single clock, though. In 
the case I mentioned above, PMIC node would be a clock provider and PMIC 
driver would register necessary clocks in common clock framework.

Best regards,
Tomasz

Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Arend van Spriel]

On 03/13/2014 11:42 AM, Tomasz Figa wrote:
> Hi Arend,
> 
> On 13.03.2014 11:16, Arend van Spriel wrote:
>> On 02/25/2014 11:51 PM, Stephen Warren wrote:
>>> On 02/10/2014 12:17 PM, Arend van Spriel wrote:
>>>> The Broadcom bcm43xx sdio devices are fullmac devices that may be
>>>> integrated in ARM platforms. Currently, the brcmfmac driver for
>>>> these devices support use of platform data. This patch specifies
>>>> the bindings that allow this platform data to be expressed in the
>>>> devicetree.
>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>>> b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
>>>>
>>>
>>>> + - compatible : Should be "brcm,bcm43xx-fmac".
>>>> + - wlan-supply : phandle for fixed regulator used to control power for
>>>> +    the device/module.
>>>
>>> Ignoring the fact that perhaps this should just be a GPIO instead and
>>> assuming it actually make sense for this to be a regulator:
>>>
>>> Why "fixed regulator" not just "the regulator". There shouldn't be any
>>> requirement for the power supply to the device to be fixed; the driver
>>> should (a) set the voltage (which will be a no-op for a fixed regulator
>>> already providing that voltage), then (b) enable the regulator. That
>>> would allow a PMIC with programmable voltage to be feeding the device.
>>>
>>> Now, if this property was really intended to control some enable GPIO on
>>> the device, as others have said, this shouldn't be a regulator property
>>> but rather a GPIO property. However, there is definitely some power
>>> supply fed to the device, so you definitely need /some/ supply property
>>> here.
>>>
>>> Aren't there other enable GPIOs required? These should be specified
>>> in DT.
>>>
>>> Doesn't the WiFi chip/module require a (32KHz?) clock? If so, that needs
>>> to be represented in DT. Preferably write the binding to require
>>> clock-names (name-based lookup) rather than just clocks (index-based
>>> lookup).
>>
>> Hi Stephen,
>>
>> Thanks for these comments. While I agree with most of them, I am having
>> some difficulty with the DT concept. Essentially, a DT node describes a
>> part of the system.
> 
> That's correct. A DT node represents a component of a system and its
> contents should contain all resources and other device-specific data
> required for this device to operate or optional.
> 
>> My scope for this change is probably limited wearing
>> my brcmfmac glasses. Am I correct in assuming that a DT node may be
>> processed/used by multiple drivers.
> 
> It may be, but it is usually not. The typical use case for such scheme
> is a bus-like topology, where devices on the bus are sub-nodes of the
> bus controller node and may contain some bus-specific information, such
> as chip select (e.g. SPI), address (e.g. I2C) or maximum bus speed.
> 
>> As an example, the 32 kHz clock is
>> not something brcmfmac cares about. It simple needs to be available and
>> hooked up to the wlan device.
> 
> Not really. The driver should care about any resources needed for the
> device to operate. In this case, a 32 kHz clock even if wired to the
> chip, sometimes is not operational until it gets ungated. This is not an
> artificial example, as on many boards I used to work with the 32 kHz
> clock was driven by a PMIC with clock gating control through I2C, gated
> by default.
> 
> Moreover, (well, 32 kHz might not be the best example) from power saving
> reasons, it might be a good idea to let the driver control the clock and
> gate it whenever it is not necessary.

Hi Tomasz,

Thanks. That clarifies things.

>> The DT should have another node for this
>> clock which a (common) clock driver picks up. So having it referenced in
>> this node is purely informational, right?
> 
> You are confusing here provider with consumer. The bcm43xx chip is
> clearly a consumer of a 32 kHz clock and so its DT node should specify
> this.
> 
> A DT node for a clock, would be a clock provider node and that would be
> handled by common clock framework in case of Linux indeed. A clock
> provider node doesn't have to be limited to a single clock, though. In
> the case I mentioned above, PMIC node would be a clock provider and PMIC
> driver would register necessary clocks in common clock framework.

I see. I figured the provider driver would not do that when the device
tree did not contain a consumer. Either, it is now clear what is
required from brcmfmac driver regarding clocks and gpios. Just still not
sure about the wlan-supply property. Does it depend on the specific
platform whether it is a gpio or regulator, ie. should I support both
(mutual exclusive or not).

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html