[PATCH 00/15] ARM: sunxi: Enable Broadcom-based Bluetooth controllers

[Chen-Yu Tsai]

Hi everyone,

On many Allwinner SBCs / developer boards, there is a WiFi+BT combo
module from AMPAK. Inside is either one or two Broadcom chips, depending
on the model. This series enables the Bluetooth controllers for AMPAK
AP6210, AP6212, and AP6330 found on several boards. More will come later
as other SoCs require changes to some other parts. I did not cover the
SCO PCM connections from the controller to the SoC's I2S interface. It
seems no one is actually doing this, so I was not sure how to proceed.
Any suggestions?

I deliberately left out the netdev mailing list and Dave Miller, as the
only thing that is under net is the binding document. Maybe we should
move that out of Documentation/devicetree/bindings/net/ ?

Also, I'm not subscribed to the linux-bluetooth ML, so please CC me for
any discussions.

Patches 1 through 4 are device tree binding changes:

1 - Make the external clock name unambiguous, and add a second entry for
the LPO clock.

2 - Add regulator supply properties for the VBAT and VDDIO power pins.

3 - Add a compatible string for BCM20702A1.

4 - Add a compatible string for BCM4330.

Patches 5 through 13 are changes to the driver, either improvements,
or updates to handle the updated device tree binding.

5 - Make the driver handle deferred probing for the external clock.

6 - Simplify clock error checking for subsequent clk API calls.

7 - Handle clock-names for the main external clock.

8 - Support a new external clock, the LPO.

9 - Support regulator supplies.

10 - Wait a small amount of time after toggling the GPIO for the device
to settle.

11 - Add support for BCM20702A1, including its default address.

12 - Add BCM4330 compatible string to the driver.

13 - Handle default address for BCM43430A0.

14 - Enable Broadcom-based serdev Bluetooth for multiple Allwinner ARMv7
boards.

15 - Enable Broadcom-based serdev Bluetooth for the Bananapi M64.

checkpatch reports an error for both patch 11 and patch 13:

ERROR: space required after that close brace '}'

I followed the existing code's style. If this is undesirable, I can send
a follow-up patch fixing the entire code block.

The first 13 patches should go through the Bluetooth tree, while we, the
sunxi maintainers, will take the last 2.

Chen-Yu Tsai (14):
dt-bindings: net: broadcom-bluetooth: Fix external clock names
dt-bindings: net: broadcom-bluetooth: Add VBAT and VDDIO supplies
dt-bindings: net: broadcom-bluetooth: Add BCM20702A1 compatible string
dt-bindings: net: broadcom-bluetooth: Add BCM4330 compatible string
Bluetooth: hci_bcm: Handle deferred probing for the clock supply
Bluetooth: hci_bcm: Simplify clk_get error handling
Bluetooth: hci_bcm: Use "txco" and "extclk" to get clock reference
Bluetooth: hci_bcm: Add support for LPO clock
Bluetooth: hci_bcm: Add support for regulator supplies
Bluetooth: hci_bcm: Wait for device to come out of reset after power
on
Bluetooth: hci_bcm: Add compatible string for BCM4330
Bluetooth: btbcm: Add default address for BCM43430A0
ARM: dts: sunxi: Enable Broadcom-based Bluetooth for multiple boards
arm64: dts: allwinner: a64: bananapi-m64: Add Bluetooth device node

Maxime Ripard (1):
Bluetooth: hci_bcm: Add BCM20702A1 variant

.../bindings/net/broadcom-bluetooth.txt | 9 +-
arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 22 ++++
arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 18 +++
.../boot/dts/sun8i-a83t-cubietruck-plus.dts | 18 +++
arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts | 14 +++
.../dts/allwinner/sun50i-a64-bananapi-m64.dts | 14 +++
drivers/bluetooth/btbcm.c | 13 +-
drivers/bluetooth/hci_bcm.c | 112 +++++++++++++++---
8 files changed, 204 insertions(+), 16 deletions(-)

--
2.19.1

[Chen-Yu Tsai]

The Broadcom Bluetooth chips have two power inputs, VBAT and VDDIO.
The former provides overall power for the chip, while the latter powers
the I/O pins and buffers.

This patch adds properties for the two so we can describe the power
supply relationships.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt | 2 ++
1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
index 2535e54219af..f6ff1a60ab3e 100644
--- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
@@ -23,6 +23,8 @@ Optional properties:
- "extclk": deprecated, replaced by "txco"
- "lpo": external low power 32.768 kHz clock
- clock-names: should be "extclk"
+ - vbat-supply: phandle to regulator supply for VBAT
+ - vddio-supply: phandle to regulator supply for VDDIO


Example:
--
2.19.1

[Chen-Yu Tsai]

The BCM4330 is a 802.11 a/b/g/n WiFi + Bluetooth 4.0 chip from Broadcom.
It is found in the Ampak AP6330 WiFi+BT module. The partiular one I have
identifies as BCM4330B1 for Bluetooth and BCM4330/4 for WiFi.

It is unclear if the AP6330 module uses this revision of the BCM4330, or
if there are multiple revisions. The module does not have revision
markings. This patch elects to use just BCM4330 for the compatible
string.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
index cb50931a6506..33423002c8c2 100644
--- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
@@ -11,6 +11,7 @@ Required properties:

- compatible: should contain one of the following:
* "brcm,bcm20702a1"
+ * "brcm,bcm4330-bt"
* "brcm,bcm43438-bt"

Optional properties:
--
2.19.1

[Chen-Yu Tsai]

The Broadcom Bluetooth controllers can take up to two external clocks:
an external frequency reference, substituting the main crystal, and a
LPO clock at 32.768 kHz substituting the internal LPO clock.

In particular, the external LPO clock must be used when the controller
does not have NVRAM connected, and the main reference frequency is not
the default 20 MHz. This is described in detail in the datasheet.

The original "extclk" clock name is ambiguous as to which of these it
refers to, and some designs might even require both.

This patch deprecates the existing name, and adds "txco" and "lpo".

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
index 4194ff7e6ee6..2535e54219af 100644
--- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
@@ -18,7 +18,10 @@ Optional properties:
- shutdown-gpios: GPIO specifier, used to enable the BT module
- device-wakeup-gpios: GPIO specifier, used to wakeup the controller
- host-wakeup-gpios: GPIO specifier, used to wakeup the host processor
- - clocks: clock specifier if external clock provided to the controller
+ - clocks and clock-names: clock specifier if external clocks are provided
+ - "txco": external reference clock
+ - "extclk": deprecated, replaced by "txco"
+ - "lpo": external low power 32.768 kHz clock

- clock-names: should be "extclk"

--
2.19.1

[Chen-Yu Tsai]

The BCM20702A1 is a Bluetooth 4.0 chip from Broadcom. It is found in the
Ampak AP6210 WiFi+BT module, identified from the read verbose config info
command response. However the Bluetooth firmware provided by vendors uses
the name BCM20710. This patch elects to use the chip ID returned by the
chip for the compatible string.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
index f6ff1a60ab3e..cb50931a6506 100644
--- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
@@ -10,6 +10,7 @@ device the slave device is attached to.

Required properties:

- compatible: should contain one of the following:

+ * "brcm,bcm20702a1"

[Chen-Yu Tsai]

The Broadcom Bluetooth controllers support a secondary LPO clock at
32.768 kHz. This external clock provides low power timing, and also
a way to detect the frequency of the main reference clock. On many
designs without NVRAM and a non-default reference clock, this must
be used or the controller will not function correctly.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/hci_bcm.c | 41 ++++++++++++++++++++++++++++++++-----
1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 7556f8fac8aa..ab6e527952a9 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -72,7 +72,8 @@
* @btpu: Apple ACPI method to drive BT_REG_ON pin high ("Bluetooth Power Up")
* @btpd: Apple ACPI method to drive BT_REG_ON pin low ("Bluetooth Power Down")
* @txco_clk: external reference frequency clock used by Bluetooth device
- * @clk_enabled: whether @txco_clk is prepared and enabled
+ * @lpo_clk: external LPO clock used by Bluetooth device
+ * @clk_enabled: whether clocks are prepared and enabled
* @init_speed: default baudrate of Bluetooth device;
* the host UART is initially set to this baudrate so that
* it can configure the Bluetooth device for @oper_speed
@@ -103,6 +104,7 @@ struct bcm_device {
#endif

struct clk *txco_clk;
+ struct clk *lpo_clk;
bool clk_enabled;

u32 init_speed;
@@ -215,21 +217,34 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
int err;

if (powered && !dev->clk_enabled) {
- err = clk_prepare_enable(dev->txco_clk);
+ /* LPO clock needs to be 32.768 kHz */
+ err = clk_set_rate(dev->lpo_clk, 32768);
+ if (err) {
+ dev_err(dev->dev, "Could not set LPO clock rate\n");
+ return err;
+ }
+
+ err = clk_prepare_enable(dev->lpo_clk);
if (err)
return err;
+
+ err = clk_prepare_enable(dev->txco_clk);
+ if (err)
+ goto err_lpo_clk_disable;
}

err = dev->set_shutdown(dev, powered);
if (err)
- goto err_clk_disable;
+ goto err_txco_clk_disable;

err = dev->set_device_wakeup(dev, powered);
if (err)
goto err_revert_shutdown;

- if (!powered && dev->clk_enabled)
+ if (!powered && dev->clk_enabled) {
clk_disable_unprepare(dev->txco_clk);
+ clk_disable_unprepare(dev->lpo_clk);
+ }

dev->clk_enabled = powered;

@@ -237,9 +252,12 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)

err_revert_shutdown:
dev->set_shutdown(dev, !powered);
-err_clk_disable:
+err_txco_clk_disable:
if (powered && !dev->clk_enabled)
clk_disable_unprepare(dev->txco_clk);
+err_lpo_clk_disable:
+ if (powered && !dev->clk_enabled)
+ clk_disable_unprepare(dev->lpo_clk);
return err;
}

@@ -934,6 +952,19 @@ static int bcm_get_resources(struct bcm_device *dev)
if (IS_ERR(dev->txco_clk))
dev->txco_clk = NULL;

+ dev->lpo_clk = devm_clk_get(dev->dev, "lpo");
+ if (IS_ERR(dev->lpo_clk) && PTR_ERR(dev->lpo_clk) == -EPROBE_DEFER)
+ return PTR_ERR(dev->lpo_clk);
+
+ if (IS_ERR(dev->lpo_clk))
+ dev->lpo_clk = NULL;
+
+ /* Check if we accidentally fetched the lpo clock twice */
+ if (dev->lpo_clk && clk_is_match(dev->lpo_clk, dev->txco_clk)) {
+ devm_clk_put(dev->dev, dev->txco_clk);
+ dev->txco_clk = NULL;
+ }
+
dev->device_wakeup = devm_gpiod_get_optional(dev->dev, "device-wakeup",
GPIOD_OUT_LOW);
if (IS_ERR(dev->device_wakeup))
--
2.19.1

[Chen-Yu Tsai]

The BCM4330 chip is a 802.11 a/b/g/n + Bluetooth 4.0 + HS controller.
This patch adds a compatible string match to the serdev driver for the
Bluetooth part of the chip.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/hci_bcm.c | 1 +

1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 73b520622a24..b6b0b2139892 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -1397,6 +1397,7 @@ static void bcm_serdev_remove(struct serdev_device *serdev)
#ifdef CONFIG_OF
static const struct of_device_id bcm_bluetooth_of_match[] = {
{ .compatible = "brcm,bcm20702a1" },
+ { .compatible = "brcm,bcm4330-bt" },
{ .compatible = "brcm,bcm43438-bt" },
{ },
};
--
2.19.1

[Chen-Yu Tsai]

Originally the device tree binding only specified one clock reference,
with the name "extclk". The driver simply retrieves the clock without
bothering to specify a name.

Since we added a second clock to the binding, we need to fetch the
clocks by name now. First we try the new name "txco", then fall back
to the old name "extclk", and finally try retrieving a clock without
using any name, to cover any instances where a bad device tree or
firmware worked by accident.

In the last case, we should take care that we don't get the same
clock twice when we add support for the "lpo" clock.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/hci_bcm.c | 41 +++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 7f21d9ab5029..7556f8fac8aa 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -71,8 +71,8 @@
* @btlp: Apple ACPI method to toggle BT_WAKE pin ("Bluetooth Low Power")

* @btpu: Apple ACPI method to drive BT_REG_ON pin high ("Bluetooth Power Up")
* @btpd: Apple ACPI method to drive BT_REG_ON pin low ("Bluetooth Power Down")

- * @clk: clock used by Bluetooth device
- * @clk_enabled: whether @clk is prepared and enabled
+ * @txco_clk: external reference frequency clock used by Bluetooth device
+ * @clk_enabled: whether @txco_clk is prepared and enabled

* @init_speed: default baudrate of Bluetooth device;
* the host UART is initially set to this baudrate so that
* it can configure the Bluetooth device for @oper_speed

@@ -102,7 +102,7 @@ struct bcm_device {
int gpio_int_idx;
#endif

- struct clk *clk;
+ struct clk *txco_clk;
bool clk_enabled;

u32 init_speed;
@@ -215,7 +215,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)

int err;

if (powered && !dev->clk_enabled) {

- err = clk_prepare_enable(dev->clk);

+ err = clk_prepare_enable(dev->txco_clk);

if (err)
return err;
}
@@ -229,7 +229,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
goto err_revert_shutdown;

if (!powered && dev->clk_enabled)

- clk_disable_unprepare(dev->clk);
+ clk_disable_unprepare(dev->txco_clk);

dev->clk_enabled = powered;

@@ -239,7 +239,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
dev->set_shutdown(dev, !powered);
err_clk_disable:

if (powered && !dev->clk_enabled)

- clk_disable_unprepare(dev->clk);
+ clk_disable_unprepare(dev->txco_clk);
return err;
}

@@ -896,6 +896,25 @@ static int bcm_gpio_set_shutdown(struct bcm_device *dev, bool powered)
return 0;
}

+/* Try a bunch of names for TXCO */
+static struct clk *bcm_get_txco(struct device *dev)
+{
+ struct clk *clk;
+
+ /* New explicit name */
+ clk = devm_clk_get(dev, "txco");
+ if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
+ return clk;
+
+ /* Deprecated name */
+ clk = devm_clk_get(dev, "extclk");
+ if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
+ return clk;
+
+ /* Original code used no name at all */
+ return devm_clk_get(dev, NULL);
+}
+

static int bcm_get_resources(struct bcm_device *dev)

{
const struct dmi_system_id *dmi_id;
@@ -905,15 +924,15 @@ static int bcm_get_resources(struct bcm_device *dev)
if (x86_apple_machine && !bcm_apple_get_resources(dev))
return 0;

- dev->clk = devm_clk_get(dev->dev, NULL);
+ dev->txco_clk = bcm_get_txco(dev->dev);

/* Handle deferred probing */
- if (IS_ERR(dev->clk) && PTR_ERR(dev->clk) == -EPROBE_DEFER)
- return PTR_ERR(dev->clk);
+ if (IS_ERR(dev->txco_clk) && PTR_ERR(dev->txco_clk) == -EPROBE_DEFER)
+ return PTR_ERR(dev->txco_clk);

/* Ignore all other errors as before */
- if (IS_ERR(dev->clk))
- dev->clk = NULL;
+ if (IS_ERR(dev->txco_clk))

+ dev->txco_clk = NULL;

dev->device_wakeup = devm_gpiod_get_optional(dev->dev, "device-wakeup",
GPIOD_OUT_LOW);

--
2.19.1

[Chen-Yu Tsai]

The driver currently checks the clk pointer for an error condition, as
returned by clk_get, before every invocation of the clk consumer API.
This is redundant if the goal is simply to ignore the errors, thereby
making the clk optional. The clk consumer API already checks if the
pointer is NULL or not.

Simplify the code a bit by assigning NULL to the clk pointer if the
error condition is one we want to ignore, which is every error except
deferred probing.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/hci_bcm.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 0a20ad48b511..7f21d9ab5029 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -214,7 +214,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
{
int err;

- if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled) {
+ if (powered && !dev->clk_enabled) {
err = clk_prepare_enable(dev->clk);
if (err)
return err;
@@ -228,7 +228,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
if (err)
goto err_revert_shutdown;

- if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
+ if (!powered && dev->clk_enabled)
clk_disable_unprepare(dev->clk);

dev->clk_enabled = powered;
@@ -238,7 +238,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
err_revert_shutdown:
dev->set_shutdown(dev, !powered);
err_clk_disable:
- if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
+ if (powered && !dev->clk_enabled)
clk_disable_unprepare(dev->clk);
return err;
}
@@ -911,6 +911,10 @@ static int bcm_get_resources(struct bcm_device *dev)

if (IS_ERR(dev->clk) && PTR_ERR(dev->clk) == -EPROBE_DEFER)

return PTR_ERR(dev->clk);

+ /* Ignore all other errors as before */
+ if (IS_ERR(dev->clk))
+ dev->clk = NULL;
+

dev->device_wakeup = devm_gpiod_get_optional(dev->dev, "device-wakeup",
GPIOD_OUT_LOW);

[Chen-Yu Tsai]

This patch adds the Bluetooth node, and the underlying UART node if it's
missing, to the board device tree file for several boards. The LPO clock
is also added to the WiFi side's power sequencing node if it's missing,
to correctly represent the shared connections. There is also a PCM
connection for Bluetooth, but this is not covered in this patch.

These boards all have a WiFi+BT module from AMPAK, which contains one or
two Broadcom chips, depending on the model. The older AP6210 contains
two, while the newer AP6212 and AP6330 contain just one, as they use
two-in-one combo chips.

The Bluetooth side of the module is always connected to a UART on the
same pingroup as the SDIO pins for the WiFi side, in a 4 wire
configuration. Power to the VBAT and VDDIO pins are provided either by
the PMIC, using one or several of its regulator outputs, or other fixed
regulators on the board. The VBAT and VDDIO pins are shared with the
WiFi side, which would correspond to vmmc-supply and vqmmc-supply in the
mmc host node. A clock output from the SoC or the external X-Powers RTC
provides the LPO low power clock at 32.768 kHz.

All the boards covered in this patch are ones that do not require extra
changes to the SoC's dtsi file. For the remaining boards that I have
worked on, properties or device nodes for the LPO clock's source are
missing.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 22 +++++++++++++++++++
arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 18 +++++++++++++++
.../boot/dts/sun8i-a83t-cubietruck-plus.dts | 18 +++++++++++++++
arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts | 14 ++++++++++++
4 files changed, 72 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index 5649161de1d7..ccbf3b7a062b 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -103,6 +103,8 @@
pinctrl-names = "default";
pinctrl-0 = <&mmc3_pwrseq_pin_cubietruck>;
reset-gpios = <&pio 7 9 GPIO_ACTIVE_LOW>; /* PH9 WIFI_EN */
+ clocks = <&ccu CLK_OUT_A>;
+ clock-names = "ext_clock";
};

sound {
@@ -246,6 +248,10 @@
};

&pio {
+ /* Pin outputs low power clock for WiFi and BT */
+ pinctrl-0 = <&clk_out_a_pins_a>;
+ pinctrl-names = "default";
+
ahci_pwr_pin_cubietruck: ahci_pwr_pin@1 {
pins = "PH12";
function = "gpio_out";
@@ -350,6 +356,22 @@
status = "okay";
};

+&uart2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins_a>;
+ status = "okay";
+
+ bluetooth {
+ compatible = "brcm,bcm20702a1";
+ clocks = <&ccu CLK_OUT_A>;
+ clock-names = "lpo";
+ device-wakeup-gpios = <&pio 7 24 GPIO_ACTIVE_LOW>; /* PH24 */
+ host-wakeup-gpios = <&pio 7 25 GPIO_ACTIVE_LOW>; /* PH25 */
+ shutdown-gpios = <&pio 7 18 GPIO_ACTIVE_HIGH>; /* PH18 */
+ max-speed = <1500000>;
+ };
+};
+
&usb_otg {
dr_mode = "otg";
status = "okay";
diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
index 742d2946b08b..c21320c4f4c2 100644
--- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
@@ -363,6 +363,24 @@
status = "okay";
};

+&uart1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+ uart-has-rtscts;
+ status = "okay";
+
+ bluetooth {
+ compatible = "brcm,bcm43438-bt";
+ clocks = <&ac100_rtc 1>;
+ clock-names = "lpo";
+ vbat-supply = <&reg_dldo1>;
+ vddio-supply = <&reg_dldo1>;
+ device-wakeup-gpios = <&pio 7 9 GPIO_ACTIVE_HIGH>; /* PH9 */
+ host-wakeup-gpios = <&r_pio 0 5 GPIO_ACTIVE_HIGH>; /* PL5 */
+ shutdown-gpios = <&r_pio 0 4 GPIO_ACTIVE_HIGH>; /* PL4 */
+ };
+};
+
&usbphy {
usb1_vbus-supply = <&reg_usb1_vbus>;
status = "okay";
diff --git a/arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts b/arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts
index e5f0645e53a7..a5a9f5a0603e 100644
--- a/arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts
@@ -394,6 +394,24 @@
status = "okay";
};

+&uart1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+ uart-has-rtscts;
+ status = "okay";
+
+ bluetooth {
+ compatible = "brcm,bcm4330-bt";
+ clocks = <&ac100_rtc 1>;
+ clock-names = "lpo";
+ vbat-supply = <&reg_dcdc1>;
+ vddio-supply = <&reg_sw>;
+ device-wakeup-gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /* PL10 */
+ host-wakeup-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
+ shutdown-gpios = <&r_pio 0 4 GPIO_ACTIVE_HIGH>; /* PL4 */
+ };
+};
+
&usbphy {
usb1_vbus-supply = <&reg_usb1_vbus>;
usb2_vbus-supply = <&reg_usb2_vbus>;
diff --git a/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts b/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts
index 0dbdb29a8fff..b85acca94441 100644
--- a/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts
+++ b/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts
@@ -91,6 +91,8 @@
wifi_pwrseq: wifi_pwrseq {
compatible = "mmc-pwrseq-simple";
reset-gpios = <&r_pio 0 6 GPIO_ACTIVE_LOW>; /* PL06 */
+ clocks = <&rtc 1>;
+ clock-names = "ext_clock";
};
};

@@ -299,7 +301,19 @@
&uart1 {
pinctrl-names = "default";
pinctrl-0 = <&uart1_pins_a>, <&uart1_pins_cts_rts_a>;
+ uart-has-rtscts;
status = "okay";
+
+ bluetooth {
+ compatible = "brcm,bcm43438-bt";
+ clocks = <&rtc 1>;
+ clock-names = "lpo";
+ vbat-supply = <&reg_dldo1>;
+ vddio-supply = <&reg_aldo3>;
+ device-wakeup-gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /* PL10 */
+ host-wakeup-gpios = <&r_pio 0 9 GPIO_ACTIVE_HIGH>; /* PL9 */
+ shutdown-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
+ };
};

&usb_otg {
--
2.19.1

[Chen-Yu Tsai]

The AP6212 is based on the Broadcom BCM43430 or BCM43438. The WiFi side
identifies as BCM43430, while the Bluetooth side identifies as BCM43438.

The Bluetooth side is connected to UART1 in a 4 wire configuration. Same
as the WiFi side, due to being the same chip and package, DLDO2 provides
overall power via VBAT, and DLDO4 provides I/O power via VDDIO. The RTC
clock output provides the LPO low power clock at 32.768 kHz.

This patch enables Bluetooth on this board, and also adds the missing
LPO clock on the WiFi side. There is also a PCM connection for Bluetooth,
but this is not covered here.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

.../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
index ef1c90401bb2..beca5c4979cf 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
@@ -94,6 +94,8 @@

wifi_pwrseq: wifi_pwrseq {
compatible = "mmc-pwrseq-simple";

reset-gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2 */

+ clocks = <&rtc 1>;
+ clock-names = "ext_clock";
};
};

@@ -335,7 +337,19 @@
&uart1 {
pinctrl-names = "default";

pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+ uart-has-rtscts;

status = "okay";
+
+ bluetooth {
+ compatible = "brcm,bcm43438-bt";
+ clocks = <&rtc 1>;
+ clock-names = "lpo";

+ vbat-supply = <&reg_dldo2>;
+ vddio-supply = <&reg_dldo4>;
+ device-wakeup-gpios = <&r_pio 0 6 GPIO_ACTIVE_HIGH>; /* PL6 */

+ host-wakeup-gpios = <&r_pio 0 5 GPIO_ACTIVE_HIGH>; /* PL5 */
+ shutdown-gpios = <&r_pio 0 4 GPIO_ACTIVE_HIGH>; /* PL4 */
+ };

};

&usb_otg {
--
2.19.1

[Chen-Yu Tsai]

The datasheets for BCM20702 and BCM43438 both have power up time
sequence graphs, however they are slightly different. Both chips
also have an internal power-on-reset, which holds the chip in reset
for a short time after the regulators are enabled.

For the BCM20702, the time period from when the regulators are enabled,
until the chip settles and comes out of sleep state, is 6564 ~ 8171 us.

For the BCM43438, the graph only shows the time period from when the
regulators are enabled until the chip responds by driving the host's
CTS line low, assuming the host has already driven its RTS line low.
This is shown to be 6.5 sleep cycles, with the sleep clock at 32.768
kHz. This is around 2 ms.

Wait a full 10 ms after the regulators are enabled to account for signal
rising times.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/hci_bcm.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 060001f11a98..bf5dee9b04d8 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -256,6 +256,9 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
regulator_bulk_disable(BCM_NUM_SUPPLIES, dev->supplies);
}

+ /* wait for device to power on and come out of reset */
+ usleep_range(10000, 20000);
+
dev->res_enabled = powered;

return 0;
--
2.19.1

[Chen-Yu Tsai]

The BCM43430A0 has the default MAC address 43:43:A0:12:1F:AC if none
is given. This address was found when enabling Bluetooth on a bunch of
boards with the AMPAK AP6210 module, all sharing the same address. It
also contains the sequence 4343A0, which is suspicious as that is also
the name the chip identifies itself as.

Add this to the list of default MAC addresses and leave it to the user
to configure a valid one.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/btbcm.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 22d4b530da03..08e1f3dd4e03 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -34,6 +34,7 @@

#define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00}})
#define BDADDR_BCM20702A1 (&(bdaddr_t) {{0x00, 0x00, 0xa0, 0x02, 0x70, 0x20}})
+#define BDADDR_BCM43430A0 (&(bdaddr_t) {{0xac, 0x1f, 0x12, 0xa0, 0x43, 0x43}})
#define BDADDR_BCM4324B3 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb3, 0x24, 0x43}})
#define BDADDR_BCM4330B1 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb1, 0x30, 0x43}})

@@ -73,11 +74,15 @@ int btbcm_check_bdaddr(struct hci_dev *hdev)
*
* The address 43:30:B1:00:00:00 indicates a BCM4330B1 controller
* with waiting for configuration state.
+ *
+ * The address 43:43:A0:12:1F:AC indicates a BCM43430A0 controller
+ * with no configured address.
*/
if (!bacmp(&bda->bdaddr, BDADDR_BCM20702A0) ||
!bacmp(&bda->bdaddr, BDADDR_BCM20702A1) ||
!bacmp(&bda->bdaddr, BDADDR_BCM4324B3) ||
- !bacmp(&bda->bdaddr, BDADDR_BCM4330B1)) {
+ !bacmp(&bda->bdaddr, BDADDR_BCM4330B1) ||
+ !bacmp(&bda->bdaddr, BDADDR_BCM43430A0)) {
bt_dev_info(hdev, "BCM: Using default device address (%pMR)",
&bda->bdaddr);
set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
--
2.19.1

[Chen-Yu Tsai]

On some systems that actually have the bluetooth controller wired up
with an extra clock signal, it's possible the bluetooth controller
probes before the clock provider. clk_get would return a defer probe
error, which was not handled by this driver.

Handle this properly, so that these systems can work reliably.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/hci_bcm.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index ddbd8c6a0ceb..0a20ad48b511 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -907,6 +907,10 @@ static int bcm_get_resources(struct bcm_device *dev)

dev->clk = devm_clk_get(dev->dev, NULL);

+ /* Handle deferred probing */
+ if (IS_ERR(dev->clk) && PTR_ERR(dev->clk) == -EPROBE_DEFER)
+ return PTR_ERR(dev->clk);

[Chen-Yu Tsai]

The Broadcom Bluetooth chips have two power inputs, VBAT and VDDIO.
The former provides overall power for the chip, while the latter powers
the I/O pins and buffers.

Model these two as regulator supplies, and let the driver manage them
in the same way as it does the clock supply.

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/hci_bcm.c | 39 ++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index ab6e527952a9..060001f11a98 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -31,6 +31,7 @@
#include <linux/property.h>
#include <linux/platform_data/x86/apple.h>
#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
#include <linux/clk.h>
#include <linux/gpio/consumer.h>
#include <linux/tty.h>
@@ -53,6 +54,8 @@

#define BCM_AUTOSUSPEND_DELAY 5000 /* default autosleep delay */

+#define BCM_NUM_SUPPLIES 2
+
/**
* struct bcm_device - device driver resources
* @serdev_hu: HCI UART controller struct
@@ -73,7 +76,8 @@

* @btpd: Apple ACPI method to drive BT_REG_ON pin low ("Bluetooth Power Down")

* @txco_clk: external reference frequency clock used by Bluetooth device

* @lpo_clk: external LPO clock used by Bluetooth device
- * @clk_enabled: whether clocks are prepared and enabled
+ * @supplies: VBAT and VDDIO supplies used by Bluetooth device
+ * @res_enabled: whether clocks and supplies are prepared and enabled

* @init_speed: default baudrate of Bluetooth device;
* the host UART is initially set to this baudrate so that
* it can configure the Bluetooth device for @oper_speed

@@ -105,7 +109,8 @@ struct bcm_device {

struct clk *txco_clk;
struct clk *lpo_clk;
- bool clk_enabled;
+ struct regulator_bulk_data supplies[BCM_NUM_SUPPLIES];
+ bool res_enabled;

u32 init_speed;
u32 oper_speed;
@@ -216,17 +221,21 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
{
int err;

- if (powered && !dev->clk_enabled) {
+ if (powered && !dev->res_enabled) {
+ err = regulator_bulk_enable(BCM_NUM_SUPPLIES, dev->supplies);
+ if (err)
+ return err;

+
/* LPO clock needs to be 32.768 kHz */

err = clk_set_rate(dev->lpo_clk, 32768);

if (err) {

dev_err(dev->dev, "Could not set LPO clock rate\n");

- return err;
+ goto err_regulator_disable;

}

err = clk_prepare_enable(dev->lpo_clk);
if (err)

- return err;
+ goto err_regulator_disable;

err = clk_prepare_enable(dev->txco_clk);
if (err)

@@ -241,23 +250,27 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
if (err)
goto err_revert_shutdown;

- if (!powered && dev->clk_enabled) {
+ if (!powered && dev->res_enabled) {
clk_disable_unprepare(dev->txco_clk);
clk_disable_unprepare(dev->lpo_clk);
+ regulator_bulk_disable(BCM_NUM_SUPPLIES, dev->supplies);
}

- dev->clk_enabled = powered;

+ dev->res_enabled = powered;

return 0;

err_revert_shutdown:
dev->set_shutdown(dev, !powered);
err_txco_clk_disable:
- if (powered && !dev->clk_enabled)
+ if (powered && !dev->res_enabled)
clk_disable_unprepare(dev->txco_clk);
err_lpo_clk_disable:
- if (powered && !dev->clk_enabled)
+ if (powered && !dev->res_enabled)
clk_disable_unprepare(dev->lpo_clk);
+err_regulator_disable:
+ if (powered && !dev->res_enabled)
+ regulator_bulk_disable(BCM_NUM_SUPPLIES, dev->supplies);
return err;
}

@@ -936,6 +949,7 @@ static struct clk *bcm_get_txco(struct device *dev)

static int bcm_get_resources(struct bcm_device *dev)

{
const struct dmi_system_id *dmi_id;
+ int err;

dev->name = dev_name(dev->dev);

@@ -978,6 +992,13 @@ static int bcm_get_resources(struct bcm_device *dev)
dev->set_device_wakeup = bcm_gpio_set_device_wakeup;
dev->set_shutdown = bcm_gpio_set_shutdown;

+ dev->supplies[0].supply = "vbat";
+ dev->supplies[1].supply = "vddio";
+ err = devm_regulator_bulk_get(dev->dev, BCM_NUM_SUPPLIES,
+ dev->supplies);
+ if (err)
+ return err;
+
/* IRQ can be declared in ACPI table as Interrupt or GpioInt */
if (dev->irq <= 0) {
struct gpio_desc *gpio;
--
2.19.1

[Chen-Yu Tsai]

From: Maxime Ripard <maxime...@free-electrons.com>

The BCM20702A1 chip is a single-chip Bluetooth 4.0 controller and
transceiver. It is found in the AMPAK AP6210 WiFi+BT package.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>

Signed-off-by: Chen-Yu Tsai <we...@csie.org>
---

drivers/bluetooth/btbcm.c | 6 ++++++
drivers/bluetooth/hci_bcm.c | 1 +
2 files changed, 7 insertions(+)

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index e3e4d929e74f..22d4b530da03 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -33,6 +33,7 @@
#define VERSION "0.1"

#define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00}})

+#define BDADDR_BCM20702A1 (&(bdaddr_t) {{0x00, 0x00, 0xa0, 0x02, 0x70, 0x20}})

#define BDADDR_BCM4324B3 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb3, 0x24, 0x43}})
#define BDADDR_BCM4330B1 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb1, 0x30, 0x43}})

@@ -64,6 +65,9 @@ int btbcm_check_bdaddr(struct hci_dev *hdev)
* The address 00:20:70:02:A0:00 indicates a BCM20702A0 controller

* with no configured address.

*
+ * The address 20:70:02:A0:00:00 indicates a BCM20702A1 controller

+ * with no configured address.

+ *
* The address 43:24:B3:00:00:00 indicates a BCM4324B3 controller

* with waiting for configuration state.

*
@@ -71,6 +75,7 @@ int btbcm_check_bdaddr(struct hci_dev *hdev)

* with waiting for configuration state.

*/
if (!bacmp(&bda->bdaddr, BDADDR_BCM20702A0) ||

+ !bacmp(&bda->bdaddr, BDADDR_BCM20702A1) ||
!bacmp(&bda->bdaddr, BDADDR_BCM4324B3) ||
!bacmp(&bda->bdaddr, BDADDR_BCM4330B1)) {

bt_dev_info(hdev, "BCM: Using default device address (%pMR)",

@@ -330,6 +335,7 @@ static const struct bcm_subver_table bcm_uart_subver_table[] = {
{ 0x2209, "BCM43430A1" }, /* 001.002.009 */
{ 0x6119, "BCM4345C0" }, /* 003.001.025 */
{ 0x230f, "BCM4356A2" }, /* 001.003.015 */
+ { 0x220e, "BCM20702A1" }, /* 001.002.014 */
{ }
};

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index bf5dee9b04d8..73b520622a24 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -1396,6 +1396,7 @@ static void bcm_serdev_remove(struct serdev_device *serdev)

#ifdef CONFIG_OF
static const struct of_device_id bcm_bluetooth_of_match[] = {

+ { .compatible = "brcm,bcm20702a1" },

[Maxime Ripard]

Hi!

On Wed, Nov 07, 2018 at 06:13:04PM +0800, Chen-Yu Tsai wrote:
> From: Maxime Ripard <maxime...@free-electrons.com>
>
> The BCM20702A1 chip is a single-chip Bluetooth 4.0 controller and
> transceiver. It is found in the AMPAK AP6210 WiFi+BT package.
>
> Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
> Signed-off-by: Chen-Yu Tsai <we...@csie.org>

if you resubmit it, could you do s/free-electrons/bootlin/ ?

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Chen-Yu Tsai]

On Thu, Nov 8, 2018 at 4:36 AM Maxime Ripard <maxime...@bootlin.com> wrote:
>
> Hi!
>
> On Wed, Nov 07, 2018 at 06:13:04PM +0800, Chen-Yu Tsai wrote:
> > From: Maxime Ripard <maxime...@free-electrons.com>
> >
> > The BCM20702A1 chip is a single-chip Bluetooth 4.0 controller and
> > transceiver. It is found in the AMPAK AP6210 WiFi+BT package.
> >
> > Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
> > Signed-off-by: Chen-Yu Tsai <we...@csie.org>
>
> if you resubmit it, could you do s/free-electrons/bootlin/ ?

OK. And for the author as well?

ChenYu

[Maxime Ripard]

Yes, please :)

[Maxime Ripard]

I guess we should make it clear in the comment why it cannot be tied
to both devices.

> ahci_pwr_pin_cubietruck: ahci_pwr_pin@1 {
> pins = "PH12";
> function = "gpio_out";
> @@ -350,6 +356,22 @@
> status = "okay";
> };
>
> +&uart2 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart2_pins_a>;
> + status = "okay";

No RTS/CTS?

[Maxime Ripard]

On Wed, Nov 07, 2018 at 06:13:08PM +0800, Chen-Yu Tsai wrote:
> The AP6212 is based on the Broadcom BCM43430 or BCM43438. The WiFi side
> identifies as BCM43430, while the Bluetooth side identifies as BCM43438.
>
> The Bluetooth side is connected to UART1 in a 4 wire configuration. Same
> as the WiFi side, due to being the same chip and package, DLDO2 provides
> overall power via VBAT, and DLDO4 provides I/O power via VDDIO. The RTC
> clock output provides the LPO low power clock at 32.768 kHz.
>
> This patch enables Bluetooth on this board, and also adds the missing
> LPO clock on the WiFi side. There is also a PCM connection for Bluetooth,
> but this is not covered here.
>
> Signed-off-by: Chen-Yu Tsai <we...@csie.org>

Acked-by: Maxime Ripard <maxime...@bootlin.com>

Thanks for figuring this out!

[Rob Herring]

This line should change?

'clocks' needs to describe how many clocks and the order of them.

'clock-names' needs to list the names. Keep them separate.

Rob

[Rob Herring]

On Wed, 7 Nov 2018 18:12:55 +0800, Chen-Yu Tsai wrote:
> The Broadcom Bluetooth chips have two power inputs, VBAT and VDDIO.
> The former provides overall power for the chip, while the latter powers
> the I/O pins and buffers.
>
> This patch adds properties for the two so we can describe the power
> supply relationships.
>
> Signed-off-by: Chen-Yu Tsai <we...@csie.org>
> ---
> Documentation/devicetree/bindings/net/broadcom-bluetooth.txt | 2 ++
> 1 file changed, 2 insertions(+)
>

Reviewed-by: Rob Herring <ro...@kernel.org>

[Rob Herring]

On Wed, 7 Nov 2018 18:12:56 +0800, Chen-Yu Tsai wrote:
> The BCM20702A1 is a Bluetooth 4.0 chip from Broadcom. It is found in the
> Ampak AP6210 WiFi+BT module, identified from the read verbose config info
> command response. However the Bluetooth firmware provided by vendors uses
> the name BCM20710. This patch elects to use the chip ID returned by the
> chip for the compatible string.
>
> Signed-off-by: Chen-Yu Tsai <we...@csie.org>
> ---
> Documentation/devicetree/bindings/net/broadcom-bluetooth.txt | 1 +
> 1 file changed, 1 insertion(+)
>

Reviewed-by: Rob Herring <ro...@kernel.org>

[Rob Herring]

On Wed, 7 Nov 2018 18:12:57 +0800, Chen-Yu Tsai wrote:
> The BCM4330 is a 802.11 a/b/g/n WiFi + Bluetooth 4.0 chip from Broadcom.
> It is found in the Ampak AP6330 WiFi+BT module. The partiular one I have
> identifies as BCM4330B1 for Bluetooth and BCM4330/4 for WiFi.
>
> It is unclear if the AP6330 module uses this revision of the BCM4330, or
> if there are multiple revisions. The module does not have revision
> markings. This patch elects to use just BCM4330 for the compatible
> string.
>
> Signed-off-by: Chen-Yu Tsai <we...@csie.org>
> ---
> Documentation/devicetree/bindings/net/broadcom-bluetooth.txt | 1 +
> 1 file changed, 1 insertion(+)
>

Reviewed-by: Rob Herring <ro...@kernel.org>

[Chen-Yu Tsai]

Yes. Missed that.

>
> 'clocks' needs to describe how many clocks and the order of them.
>
> 'clock-names' needs to list the names. Keep them separate.

I was under the impression that when clock-names was used, the
order of clocks shouldn't matter.

Also, both clocks are optional. The controller can use a standalone
crystal instead of an external TXCO, which would not get described in
the device tree, and/or not use an LPO clock. How would one describe
a device that has an LPO clock input but not a TXCO clock input?

Last, IMHO listing them with name + description, one item per line
is more readable then having the items on one line, then having the
next line list all their respective names. If ordering and number of
items is important, I could add the requirements to the description
of "clocks and clock-names"?

Thanks
ChenYu

[Chen-Yu Tsai]

I think it's a limitation of the implementation. But it's also a shared output,
so putting it at the source probably makes more sense. So I think it's more like
a preference rather than a hard limitation.

> > ahci_pwr_pin_cubietruck: ahci_pwr_pin@1 {
> > pins = "PH12";
> > function = "gpio_out";
> > @@ -350,6 +356,22 @@
> > status = "okay";
> > };
> >
> > +&uart2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart2_pins_a>;
> > + status = "okay";
>
> No RTS/CTS?

Missed this one. This was your Cubietruck patch squashed in with the rest.

ChenYu

[Rob Herring]

Generally, no. The order should be defined still.

> Also, both clocks are optional. The controller can use a standalone
> crystal instead of an external TXCO, which would not get described in
> the device tree, and/or not use an LPO clock. How would one describe
> a device that has an LPO clock input but not a TXCO clock input?

A crystal would still be a fixed-clock. Does s/w need to know which one it is?

Your's is a case where index alone doesn't work and you need
clock-names. But still, you should define the order for when both
clocks are present so we don't end with both orders for no good
reason.

> Last, IMHO listing them with name + description, one item per line
> is more readable then having the items on one line, then having the
> next line list all their respective names. If ordering and number of
> items is important, I could add the requirements to the description
> of "clocks and clock-names"?

clocks can just say something like "1 or 2 clocks as defined in clock-names".

Rob

[Chen-Yu Tsai]

The datasheet doesn't mention anything s/w related. It does provide
a CLK_REQ signal that the application can use to determine if the
TXCO signal is required or not, but then again one can just enable
it as part of the power sequencing. TXCO is general connected to the
same pin as an XTAL input, so nothing s/w related there.

As for the clock rate, the hardware detects it via some measuring
mechanism that requires the LPO clock be provided. Or the clock rate
is provided through NVRAM, or if it's one of the two standard rates,
using a latched pin. Hence I think if a crystal is used, it doesn't
need to go into the device tree.

> Your's is a case where index alone doesn't work and you need
> clock-names. But still, you should define the order for when both
> clocks are present so we don't end with both orders for no good
> reason.

I see. That makes sense.

> > Last, IMHO listing them with name + description, one item per line
> > is more readable then having the items on one line, then having the
> > next line list all their respective names. If ordering and number of
> > items is important, I could add the requirements to the description
> > of "clocks and clock-names"?
>
> clocks can just say something like "1 or 2 clocks as defined in clock-names".

OK. I'll rework this.

ChenYu