[PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support

[PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support

[Ulf Hansson]

The Hikey board has a Wifi chip attached to the SDIO bus, which is managed by a
dwmmc controller. This is currently broken in that way that not even the SDIO
card is being detected.

To fix this we need to make sure the correct resources are described for the
dwmmc controller, so the mmc core can power on/off the SDIO/WiFi chip
correctly. This is the main issue today.

I have picked up one change from Daniel Lezcano's earlier submission [1] and
folded that into this series, as the Wifi support depends on it. As a matter of
fact I have even split Daniel's change into two halves, one for DT
documentation and one for arm64 hikey dts.

It's important that this series goes together as to conform with TI's HW spec
of the wl18xx Wifi chip, as to prevent it from being damaged. We can either go
via arm-soc, unless Wei Xu the Hikey SoC maintainer, prefers to ack the dts
changes, then I can pick the series via my mmc tree.

Moreover, I would suggest we send this as fixes for the 4.12 rcs to get the
Wifi working for Hikey again.

Kind regards
Ulf Hansson

[1]
https://patchwork.kernel.org/patch/9697189/

Daniel Lezcano (2):
  mfd: dts: hi655x: Add clock binding for the pmic
  arm64: dts: hikey: Add clock for the pmic mfd

Ulf Hansson (6):
  mmc: dt: pwrseq-simple: Invent power-off-delay-us
  mmc: pwrseq_simple: Parse DTS for the power-off-delay-us property
  arm64: dts: hi6220: Move the fixed_5v_hub regulator to the hikey dts
  arm64: dts: hikey: Add the SYS_5V and the VDD_3V3 regulators
  arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts
  arm64: dts: hikey: Fix WiFi support

 .../devicetree/bindings/mfd/hisilicon,hi655x.txt   |  6 ++
 .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt  |  2 +
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     | 78 +++++++++++++++++-----
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 31 +--------
 drivers/mmc/core/pwrseq_simple.c                   |  7 ++
 5 files changed, 79 insertions(+), 45 deletions(-)

-- 
2.7.4

[PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Ulf Hansson]

During power off, after the GPIO pin has been asserted, some devices like
the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
clock gating and turning off regulators as to follow a graceful shutdown
sequence.

Therefore invent an optional power-off-delay-us DT binding for
mmc-pwrseq-simple, to allow us to support this constraint.

Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mmc@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index e254368..9029b45 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -18,6 +18,8 @@ Optional properties:
   "ext_clock" (External clock provided to the card).
 - post-power-on-delay-ms : Delay in ms after powering the card and
 	de-asserting the reset-gpios (if any)
+- power-off-delay-us : Delay in us after asserting the reset-gpios (if any)
+	during power off of the card.
 
 Example:
 
-- 
2.7.4

[PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Ulf Hansson]

During power off, after the GPIO pin has been asserted, some devices like
the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
clock gating and turning off regulators as to follow a graceful shutdown
sequence.

Therefore invent an optional power-off-delay-us DT binding for
mmc-pwrseq-simple, to allow us to support this constraint.

Cc: devicetree at vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mmc at vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index e254368..9029b45 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -18,6 +18,8 @@ Optional properties:
   "ext_clock" (External clock provided to the card).
 - post-power-on-delay-ms : Delay in ms after powering the card and
 	de-asserting the reset-gpios (if any)
+- power-off-delay-us : Delay in us after asserting the reset-gpios (if any)
+	during power off of the card.
 
 Example:
 
-- 
2.7.4

[PATCH 2/8] mmc: pwrseq_simple: Parse DTS for the power-off-delay-us property

If the optional power-off-delay-us property is found, insert the
corresponding delay after asserting the GPIO during power off. This enables
a graceful shutdown sequence for some devices.

Cc: linux-mmc@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/pwrseq_simple.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 1304160..13ef162 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -27,6 +27,7 @@ struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
 	bool clk_enabled;
 	u32 post_power_on_delay_ms;
+	u32 power_off_delay_us;
 	struct clk *ext_clk;
 	struct gpio_descs *reset_gpios;
 };
@@ -78,6 +79,10 @@ static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
 
 	mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
 
+	if (pwrseq->power_off_delay_us)
+		usleep_range(pwrseq->power_off_delay_us,
+			2 * pwrseq->power_off_delay_us);
+
 	if (!IS_ERR(pwrseq->ext_clk) && pwrseq->clk_enabled) {
 		clk_disable_unprepare(pwrseq->ext_clk);
 		pwrseq->clk_enabled = false;
@@ -119,6 +124,8 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 
 	device_property_read_u32(dev, "post-power-on-delay-ms",
 				 &pwrseq->post_power_on_delay_ms);
+	device_property_read_u32(dev, "power-off-delay-us",
+				 &pwrseq->power_off_delay_us);
 
 	pwrseq->pwrseq.dev = dev;
 	pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
-- 
2.7.4

[PATCH 2/8] mmc: pwrseq_simple: Parse DTS for the power-off-delay-us property

[Ulf Hansson]

If the optional power-off-delay-us property is found, insert the
corresponding delay after asserting the GPIO during power off. This enables
a graceful shutdown sequence for some devices.

Cc: linux-mmc at vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/pwrseq_simple.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 1304160..13ef162 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -27,6 +27,7 @@ struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
 	bool clk_enabled;
 	u32 post_power_on_delay_ms;
+	u32 power_off_delay_us;
 	struct clk *ext_clk;
 	struct gpio_descs *reset_gpios;
 };
@@ -78,6 +79,10 @@ static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
 
 	mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
 
+	if (pwrseq->power_off_delay_us)
+		usleep_range(pwrseq->power_off_delay_us,
+			2 * pwrseq->power_off_delay_us);
+
 	if (!IS_ERR(pwrseq->ext_clk) && pwrseq->clk_enabled) {
 		clk_disable_unprepare(pwrseq->ext_clk);
 		pwrseq->clk_enabled = false;
@@ -119,6 +124,8 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 
 	device_property_read_u32(dev, "post-power-on-delay-ms",
 				 &pwrseq->post_power_on_delay_ms);
+	device_property_read_u32(dev, "power-off-delay-us",
+				 &pwrseq->power_off_delay_us);
 
 	pwrseq->pwrseq.dev = dev;
 	pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
-- 
2.7.4

[PATCH 3/8] mfd: dts: hi655x: Add clock binding for the pmic

[Ulf Hansson]

From: Daniel Lezcano <daniel.lezcano@linaro.org>

The hi655x PMIC provides the regulators but also a clock. The latter is
missing in the definition, so extend the documentation to include this as
well.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
[Ulf: Split patch and updated changelog]
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
index 0548569..9630ac0 100644
--- a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
+++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
@@ -16,6 +16,11 @@ Required properties:
 - reg:                  Base address of PMIC on Hi6220 SoC.
 - interrupt-controller: Hi655x has internal IRQs (has own IRQ domain).
 - pmic-gpios:           The GPIO used by PMIC IRQ.
+- #clock-cells:		From common clock binding; shall be set to 0
+
+Optional properties:
+- clock-output-names: From common clock binding to override the
+  default output clock name
 
 Example:
 	pmic: pmic at f8000000 {
@@ -24,4 +29,5 @@ Example:
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		pmic-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+		#clock-cells = <0>;
 	}
-- 
2.7.4

[PATCH 4/8] arm64: dts: hikey: Add clock for the pmic mfd

[Ulf Hansson]

From: Daniel Lezcano <daniel.lezcano@linaro.org>

The hi655x PMIC provides the regulators but also a clock. The latter is
missing so let's add it. This clock is used by WiFi/Bluetooth chip, but
that connection is done in a separate change on top of this one.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
[Ulf: Split patch and updated changelog]
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index dba3c13..e0496f7 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -325,6 +325,7 @@
 	pmic: pmic at f8000000 {
 		compatible = "hisilicon,hi655x-pmic";
 		reg = <0x0 0xf8000000 0x0 0x1000>;
+		#clock-cells = <0>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
 		pmic-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
-- 
2.7.4

[PATCH 5/8] arm64: dts: hi6220: Move the fixed_5v_hub regulator to the hikey dts

[Ulf Hansson]

The regulator is a part of the hikey board, therefore let's move it from
the hi6220 SoC dtsi file into the hikey dts file . Let's also rename the
regulator according to the datasheet (5V_HUB) to better reflect the HW.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 10 ++++++++++
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi      | 12 +-----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index e0496f7..8070c75 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -81,6 +81,16 @@
 		};
 	};
 
+	reg_5v_hub: regulator at 0 {
+		compatible = "regulator-fixed";
+		regulator-name = "5V_HUB";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-boot-on;
+		gpio = <&gpio0 7 0>;
+		regulator-always-on;
+	};
+
 	soc {
 		spi0: spi at f7106000 {
 			status = "ok";
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 1e5129b..951152d 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -725,20 +725,10 @@
 			status = "disabled";
 		};
 
-		fixed_5v_hub: regulator at 0 {
-			compatible = "regulator-fixed";
-			regulator-name = "fixed_5v_hub";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			regulator-boot-on;
-			gpio = <&gpio0 7 0>;
-			regulator-always-on;
-		};
-
 		usb_phy: usbphy {
 			compatible = "hisilicon,hi6220-usb-phy";
 			#phy-cells = <0>;
-			phy-supply = <&fixed_5v_hub>;
+			phy-supply = <&reg_5v_hub>;
 			hisilicon,peripheral-syscon = <&sys_ctrl>;
 		};
 
-- 
2.7.4

[PATCH 6/8] arm64: dts: hikey: Add the SYS_5V and the VDD_3V3 regulators

[Ulf Hansson]

Add these regulators to better describe the HW, but also because those is
needed in following changes.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index 8070c75..6dab03a 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -81,7 +81,26 @@
 		};
 	};
 
-	reg_5v_hub: regulator at 0 {
+	reg_sys_5v: regulator at 0 {
+		compatible = "regulator-fixed";
+		regulator-name = "SYS_5V";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
+
+	reg_vdd_3v3: regulator at 1 {
+		compatible = "regulator-fixed";
+		regulator-name = "VDD_3V3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		regulator-always-on;
+		vin-supply = <&reg_sys_5v>;
+	};
+
+	reg_5v_hub: regulator at 2 {
 		compatible = "regulator-fixed";
 		regulator-name = "5V_HUB";
 		regulator-min-microvolt = <5000000>;
@@ -89,6 +108,7 @@
 		regulator-boot-on;
 		gpio = <&gpio0 7 0>;
 		regulator-always-on;
+		vin-supply = <&reg_sys_5v>;
 	};
 
 	soc {
-- 
2.7.4

[PATCH 7/8] arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts

[Ulf Hansson]

Move the board specific descriptions for the dwmmc nodes in the hi6220 SoC
dtsi, into the hikey dts as it's there these belongs.

While changing this, let's take the opportunity to drop the use of the
"ti,non-removable" binding for one of the dwmmc device nodes, as it's not a
valid binding and not used. Drop also the unnecessary use of "num-slots =
<0x1>" for all of the dwmmc nodes, as there is no need to set this since
when default number of slots is one.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 23 ++++++++++++++++++++++-
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi      | 19 -------------------
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index 6dab03a..d4b3879 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -281,8 +281,29 @@
 
 		/* GPIO blocks 16 thru 19 do not appear to be routed to pins */
 
+		dwmmc_0: dwmmc0 at f723d000 {
+			cap-mmc-highspeed;
+			non-removable;
+			bus-width = <0x8>;
+			vmmc-supply = <&ldo19>;
+		};
+
+		dwmmc_1: dwmmc1 at f723e000 {
+			card-detect-delay = <200>;
+			cap-sd-highspeed;
+			sd-uhs-sdr12;
+			sd-uhs-sdr25;
+			sd-uhs-sdr50;
+			vqmmc-supply = <&ldo7>;
+			vmmc-supply = <&ldo10>;
+			bus-width = <0x4>;
+			disable-wp;
+			cd-gpios = <&gpio1 0 1>;
+		};
+
 		dwmmc_2: dwmmc2 at f723f000 {
-			ti,non-removable;
+			broken-cd;
+			bus-width = <0x4>;
 			non-removable;
 			/* WL_EN */
 			vmmc-supply = <&wlan_en_reg>;
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 951152d..5013e4b 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -756,17 +756,12 @@
 
 		dwmmc_0: dwmmc0 at f723d000 {
 			compatible = "hisilicon,hi6220-dw-mshc";
-			num-slots = <0x1>;
-			cap-mmc-highspeed;
-			non-removable;
 			reg = <0x0 0xf723d000 0x0 0x1000>;
 			interrupts = <0x0 0x48 0x4>;
 			clocks = <&sys_ctrl 2>, <&sys_ctrl 1>;
 			clock-names = "ciu", "biu";
 			resets = <&sys_ctrl PERIPH_RSTDIS0_MMC0>;
 			reset-names = "reset";
-			bus-width = <0x8>;
-			vmmc-supply = <&ldo19>;
 			pinctrl-names = "default";
 			pinctrl-0 = <&emmc_pmx_func &emmc_clk_cfg_func
 				     &emmc_cfg_func &emmc_rst_cfg_func>;
@@ -774,13 +769,7 @@
 
 		dwmmc_1: dwmmc1 at f723e000 {
 			compatible = "hisilicon,hi6220-dw-mshc";
-			num-slots = <0x1>;
-			card-detect-delay = <200>;
 			hisilicon,peripheral-syscon = <&ao_ctrl>;
-			cap-sd-highspeed;
-			sd-uhs-sdr12;
-			sd-uhs-sdr25;
-			sd-uhs-sdr50;
 			reg = <0x0 0xf723e000 0x0 0x1000>;
 			interrupts = <0x0 0x49 0x4>;
 			#address-cells = <0x1>;
@@ -789,11 +778,6 @@
 			clock-names = "ciu", "biu";
 			resets = <&sys_ctrl PERIPH_RSTDIS0_MMC1>;
 			reset-names = "reset";
-			vqmmc-supply = <&ldo7>;
-			vmmc-supply = <&ldo10>;
-			bus-width = <0x4>;
-			disable-wp;
-			cd-gpios = <&gpio1 0 1>;
 			pinctrl-names = "default", "idle";
 			pinctrl-0 = <&sd_pmx_func &sd_clk_cfg_func &sd_cfg_func>;
 			pinctrl-1 = <&sd_pmx_idle &sd_clk_cfg_idle &sd_cfg_idle>;
@@ -801,15 +785,12 @@
 
 		dwmmc_2: dwmmc2 at f723f000 {
 			compatible = "hisilicon,hi6220-dw-mshc";
-			num-slots = <0x1>;
 			reg = <0x0 0xf723f000 0x0 0x1000>;
 			interrupts = <0x0 0x4a 0x4>;
 			clocks = <&sys_ctrl HI6220_MMC2_CIUCLK>, <&sys_ctrl HI6220_MMC2_CLK>;
 			clock-names = "ciu", "biu";
 			resets = <&sys_ctrl PERIPH_RSTDIS0_MMC2>;
 			reset-names = "reset";
-			bus-width = <0x4>;
-			broken-cd;
 			pinctrl-names = "default", "idle";
 			pinctrl-0 = <&sdio_pmx_func &sdio_clk_cfg_func &sdio_cfg_func>;
 			pinctrl-1 = <&sdio_pmx_idle &sdio_clk_cfg_idle &sdio_cfg_idle>;
-- 
2.7.4

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[Ulf Hansson]

The description of the connection between the dwmmc (SDIO) controller and
the Wifi chip, which is attached to the SDIO bus is wrong. Currently the
SDIO card can't be detected and thus the Wifi doesn't work.

Let's fix this by assigning the correct vmmc supply, which is the always on
regulator VDD_3V3 and remove the WLAN enable regulator altogether. Then to
properly deal with the power on/off sequence, add a mmc-pwrseq node to
describe the resources needed to detect the SDIO card.

Except for the WLAN enable GPIO and its corresponding assert/de-assert
delays, the mmc-pwrseq node also contains a handle to a clock provided by
the hi655x pmic. This clock is also needed to be able to turn on the WiFi
chip.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index d4b3879..f72698b 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -111,6 +111,15 @@
 		vin-supply = <&reg_sys_5v>;
 	};
 
+	wl1835_pwrseq: wl1835-pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		/* WLAN_EN GPIO */
+		reset-gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
+		clocks = <&pmic>;
+		clock-names = "ext_clock";
+		power-off-delay-us = <10>;
+	};
+
 	soc {
 		spi0: spi at f7106000 {
 			status = "ok";
@@ -302,11 +311,10 @@
 		};
 
 		dwmmc_2: dwmmc2 at f723f000 {
-			broken-cd;
 			bus-width = <0x4>;
 			non-removable;
-			/* WL_EN */
-			vmmc-supply = <&wlan_en_reg>;
+			vmmc-supply = <&reg_vdd_3v3>;
+			mmc-pwrseq = <&wl1835_pwrseq>;
 
 			#address-cells = <0x1>;
 			#size-cells = <0x0>;
@@ -318,18 +326,6 @@
 				interrupts = <3 IRQ_TYPE_EDGE_RISING>;
 			};
 		};
-
-		wlan_en_reg: regulator at 1 {
-			compatible = "regulator-fixed";
-			regulator-name = "wlan-en-regulator";
-			regulator-min-microvolt = <1800000>;
-			regulator-max-microvolt = <1800000>;
-			/* WLAN_EN GPIO */
-			gpio = <&gpio0 5 0>;
-			/* WLAN card specific delay */
-			startup-delay-us = <70000>;
-			enable-active-high;
-		};
 	};
 
 	leds {
-- 
2.7.4

Re: [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Rob Herring]

On Mon, May 08, 2017 at 06:21:10PM +0200, Ulf Hansson wrote:
> During power off, after the GPIO pin has been asserted, some devices like
> the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
> clock gating and turning off regulators as to follow a graceful shutdown
> sequence.
> 
> Therefore invent an optional power-off-delay-us DT binding for
> mmc-pwrseq-simple, to allow us to support this constraint.

Do you really need this to be programmable per device. A delay is not 
going to hurt devices that don't need it.

Sorry, but this is exactly what I don't like about "simple" bindings: 
adding one property at a time.

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

[PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Rob Herring]

On Mon, May 08, 2017 at 06:21:10PM +0200, Ulf Hansson wrote:
> During power off, after the GPIO pin has been asserted, some devices like
> the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
> clock gating and turning off regulators as to follow a graceful shutdown
> sequence.
> 
> Therefore invent an optional power-off-delay-us DT binding for
> mmc-pwrseq-simple, to allow us to support this constraint.

Do you really need this to be programmable per device. A delay is not 
going to hurt devices that don't need it.

Sorry, but this is exactly what I don't like about "simple" bindings: 
adding one property at a time.

Rob

Re: [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Ulf Hansson]

On 12 May 2017 at 22:03, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, May 08, 2017 at 06:21:10PM +0200, Ulf Hansson wrote:
>> During power off, after the GPIO pin has been asserted, some devices like
>> the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
>> clock gating and turning off regulators as to follow a graceful shutdown
>> sequence.
>>
>> Therefore invent an optional power-off-delay-us DT binding for
>> mmc-pwrseq-simple, to allow us to support this constraint.
>
> Do you really need this to be programmable per device. A delay is not
> going to hurt devices that don't need it.

Well, that depends on what "hurt" means. The device would still be
properly shut down, only that it would take unnecessary longer to do
so.

I think the problem here, is that this delay may also affect system
suspend/resume time of the device, if the device powers off/on in this
sequence.

>
> Sorry, but this is exactly what I don't like about "simple" bindings:
> adding one property at a time.

I understand you opinion, which in the end is a matter of taste/flavor.

However, for me this just follows the existing approach - and suddenly
say no to this, doesn't really seems right either.

Kind regards
Uffe
--
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

[PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Ulf Hansson]

On 12 May 2017 at 22:03, Rob Herring <robh@kernel.org> wrote:
> On Mon, May 08, 2017 at 06:21:10PM +0200, Ulf Hansson wrote:
>> During power off, after the GPIO pin has been asserted, some devices like
>> the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
>> clock gating and turning off regulators as to follow a graceful shutdown
>> sequence.
>>
>> Therefore invent an optional power-off-delay-us DT binding for
>> mmc-pwrseq-simple, to allow us to support this constraint.
>
> Do you really need this to be programmable per device. A delay is not
> going to hurt devices that don't need it.

Well, that depends on what "hurt" means. The device would still be
properly shut down, only that it would take unnecessary longer to do
so.

I think the problem here, is that this delay may also affect system
suspend/resume time of the device, if the device powers off/on in this
sequence.

>
> Sorry, but this is exactly what I don't like about "simple" bindings:
> adding one property at a time.

I understand you opinion, which in the end is a matter of taste/flavor.

However, for me this just follows the existing approach - and suddenly
say no to this, doesn't really seems right either.

Kind regards
Uffe

Re: [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Rob Herring]

On Mon, May 15, 2017 at 6:08 AM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 12 May 2017 at 22:03, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Mon, May 08, 2017 at 06:21:10PM +0200, Ulf Hansson wrote:
>>> During power off, after the GPIO pin has been asserted, some devices like
>>> the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
>>> clock gating and turning off regulators as to follow a graceful shutdown
>>> sequence.
>>>
>>> Therefore invent an optional power-off-delay-us DT binding for
>>> mmc-pwrseq-simple, to allow us to support this constraint.
>>
>> Do you really need this to be programmable per device. A delay is not
>> going to hurt devices that don't need it.
>
> Well, that depends on what "hurt" means. The device would still be
> properly shut down, only that it would take unnecessary longer to do
> so.
>
> I think the problem here, is that this delay may also affect system
> suspend/resume time of the device, if the device powers off/on in this
> sequence.

I was assuming that given you changed the units the time was small
enough to not be significant.

>> Sorry, but this is exactly what I don't like about "simple" bindings:
>> adding one property at a time.
>
> I understand you opinion, which in the end is a matter of taste/flavor.

It's more than that. The problem is you would end up with a different
binding if everything is defined up front versus reviewing one
addition at a time.

To give a trivial example here, now we have power on and off times in
different units and if I was reviewing them together I would say make
them both usec. That example is mostly taste, but different units also
makes it more error prone for the dts writer.

> However, for me this just follows the existing approach - and suddenly
> say no to this, doesn't really seems right either.

I never said no.

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

[PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Rob Herring]

On Mon, May 15, 2017 at 6:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 May 2017 at 22:03, Rob Herring <robh@kernel.org> wrote:
>> On Mon, May 08, 2017 at 06:21:10PM +0200, Ulf Hansson wrote:
>>> During power off, after the GPIO pin has been asserted, some devices like
>>> the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
>>> clock gating and turning off regulators as to follow a graceful shutdown
>>> sequence.
>>>
>>> Therefore invent an optional power-off-delay-us DT binding for
>>> mmc-pwrseq-simple, to allow us to support this constraint.
>>
>> Do you really need this to be programmable per device. A delay is not
>> going to hurt devices that don't need it.
>
> Well, that depends on what "hurt" means. The device would still be
> properly shut down, only that it would take unnecessary longer to do
> so.
>
> I think the problem here, is that this delay may also affect system
> suspend/resume time of the device, if the device powers off/on in this
> sequence.

I was assuming that given you changed the units the time was small
enough to not be significant.

>> Sorry, but this is exactly what I don't like about "simple" bindings:
>> adding one property at a time.
>
> I understand you opinion, which in the end is a matter of taste/flavor.

It's more than that. The problem is you would end up with a different
binding if everything is defined up front versus reviewing one
addition at a time.

To give a trivial example here, now we have power on and off times in
different units and if I was reviewing them together I would say make
them both usec. That example is mostly taste, but different units also
makes it more error prone for the dts writer.

> However, for me this just follows the existing approach - and suddenly
> say no to this, doesn't really seems right either.

I never said no.

Rob

Re: [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Ulf Hansson]

On 15 May 2017 at 18:16, Rob Herring <robh@kernel.org> wrote:
> On Mon, May 15, 2017 at 6:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 12 May 2017 at 22:03, Rob Herring <robh@kernel.org> wrote:
>>> On Mon, May 08, 2017 at 06:21:10PM +0200, Ulf Hansson wrote:
>>>> During power off, after the GPIO pin has been asserted, some devices like
>>>> the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
>>>> clock gating and turning off regulators as to follow a graceful shutdown
>>>> sequence.
>>>>
>>>> Therefore invent an optional power-off-delay-us DT binding for
>>>> mmc-pwrseq-simple, to allow us to support this constraint.
>>>
>>> Do you really need this to be programmable per device. A delay is not
>>> going to hurt devices that don't need it.
>>
>> Well, that depends on what "hurt" means. The device would still be
>> properly shut down, only that it would take unnecessary longer to do
>> so.
>>
>> I think the problem here, is that this delay may also affect system
>> suspend/resume time of the device, if the device powers off/on in this
>> sequence.
>
> I was assuming that given you changed the units the time was small
> enough to not be significant.

That's right. We are in the range of < 50us, which is suitable for the
Wl18xx chip.

However, the problem occurs when some other device needs a longer
delay and then we may reach a threshold that isn't acceptable.

To me it's better to allow it to be described in DT - then only
influencing those devices that really needs it.

>
>>> Sorry, but this is exactly what I don't like about "simple" bindings:
>>> adding one property at a time.
>>
>> I understand you opinion, which in the end is a matter of taste/flavor.
>
> It's more than that. The problem is you would end up with a different
> binding if everything is defined up front versus reviewing one
> addition at a time.
>
> To give a trivial example here, now we have power on and off times in
> different units and if I was reviewing them together I would say make
> them both usec. That example is mostly taste, but different units also
> makes it more error prone for the dts writer.

Okay, I see your a point.

>
>> However, for me this just follows the existing approach - and suddenly
>> say no to this, doesn't really seems right either.
>
> I never said no.

Alright. Is that a yes then? :-)

If not, what do you prefer me to do?

Kind regards
Uffe

[PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us

[Ulf Hansson]

On 15 May 2017 at 18:16, Rob Herring <robh@kernel.org> wrote:
> On Mon, May 15, 2017 at 6:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 12 May 2017 at 22:03, Rob Herring <robh@kernel.org> wrote:
>>> On Mon, May 08, 2017 at 06:21:10PM +0200, Ulf Hansson wrote:
>>>> During power off, after the GPIO pin has been asserted, some devices like
>>>> the Wifi chip from TI, Wl18xx, needs a delay before the host continues with
>>>> clock gating and turning off regulators as to follow a graceful shutdown
>>>> sequence.
>>>>
>>>> Therefore invent an optional power-off-delay-us DT binding for
>>>> mmc-pwrseq-simple, to allow us to support this constraint.
>>>
>>> Do you really need this to be programmable per device. A delay is not
>>> going to hurt devices that don't need it.
>>
>> Well, that depends on what "hurt" means. The device would still be
>> properly shut down, only that it would take unnecessary longer to do
>> so.
>>
>> I think the problem here, is that this delay may also affect system
>> suspend/resume time of the device, if the device powers off/on in this
>> sequence.
>
> I was assuming that given you changed the units the time was small
> enough to not be significant.

That's right. We are in the range of < 50us, which is suitable for the
Wl18xx chip.

However, the problem occurs when some other device needs a longer
delay and then we may reach a threshold that isn't acceptable.

To me it's better to allow it to be described in DT - then only
influencing those devices that really needs it.

>
>>> Sorry, but this is exactly what I don't like about "simple" bindings:
>>> adding one property at a time.
>>
>> I understand you opinion, which in the end is a matter of taste/flavor.
>
> It's more than that. The problem is you would end up with a different
> binding if everything is defined up front versus reviewing one
> addition at a time.
>
> To give a trivial example here, now we have power on and off times in
> different units and if I was reviewing them together I would say make
> them both usec. That example is mostly taste, but different units also
> makes it more error prone for the dts writer.

Okay, I see your a point.

>
>> However, for me this just follows the existing approach - and suddenly
>> say no to this, doesn't really seems right either.
>
> I never said no.

Alright. Is that a yes then? :-)

If not, what do you prefer me to do?

Kind regards
Uffe

[PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support

[Ulf Hansson]

+ Arnd, Rob

On 8 May 2017 at 18:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The Hikey board has a Wifi chip attached to the SDIO bus, which is managed by a
> dwmmc controller. This is currently broken in that way that not even the SDIO
> card is being detected.
>
> To fix this we need to make sure the correct resources are described for the
> dwmmc controller, so the mmc core can power on/off the SDIO/WiFi chip
> correctly. This is the main issue today.
>
> I have picked up one change from Daniel Lezcano's earlier submission [1] and
> folded that into this series, as the Wifi support depends on it. As a matter of
> fact I have even split Daniel's change into two halves, one for DT
> documentation and one for arm64 hikey dts.
>
> It's important that this series goes together as to conform with TI's HW spec
> of the wl18xx Wifi chip, as to prevent it from being damaged. We can either go
> via arm-soc, unless Wei Xu the Hikey SoC maintainer, prefers to ack the dts
> changes, then I can pick the series via my mmc tree.
>
> Moreover, I would suggest we send this as fixes for the 4.12 rcs to get the
> Wifi working for Hikey again.
>
> Kind regards
> Ulf Hansson
>
> [1]
> https://patchwork.kernel.org/patch/9697189/
>
> Daniel Lezcano (2):
>   mfd: dts: hi655x: Add clock binding for the pmic
>   arm64: dts: hikey: Add clock for the pmic mfd
>
> Ulf Hansson (6):
>   mmc: dt: pwrseq-simple: Invent power-off-delay-us
>   mmc: pwrseq_simple: Parse DTS for the power-off-delay-us property
>   arm64: dts: hi6220: Move the fixed_5v_hub regulator to the hikey dts
>   arm64: dts: hikey: Add the SYS_5V and the VDD_3V3 regulators
>   arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts
>   arm64: dts: hikey: Fix WiFi support
>
>  .../devicetree/bindings/mfd/hisilicon,hi655x.txt   |  6 ++
>  .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt  |  2 +
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     | 78 +++++++++++++++++-----
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 31 +--------
>  drivers/mmc/core/pwrseq_simple.c                   |  7 ++
>  5 files changed, 79 insertions(+), 45 deletions(-)
>
> --
> 2.7.4
>

Wei Xu,

This series fixes my wifi issues on the Hikey board and currently all
but the hikey dts changes seems to be okay/acked.

Rob and me had some discussions regarding the new mmc DT binding,
however in the last response from Rob he indicates that he is not
objecting to adding the new binding.

So, unless I hear some objection from any of you, I am going to queue
this up for fixes via my mmc tree within a day or so.

Kind regards
Uffe

[PATCH 7/8] arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts

[Arnd Bergmann]

On Mon, May 8, 2017 at 6:21 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Move the board specific descriptions for the dwmmc nodes in the hi6220 SoC
> dtsi, into the hikey dts as it's there these belongs.
>
> While changing this, let's take the opportunity to drop the use of the
> "ti,non-removable" binding for one of the dwmmc device nodes, as it's not a
> valid binding and not used. Drop also the unnecessary use of "num-slots =
> <0x1>" for all of the dwmmc nodes, as there is no need to set this since
> when default number of slots is one.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 23 ++++++++++++++++++++++-
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      | 19 -------------------
>  2 files changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> index 6dab03a..d4b3879 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> @@ -281,8 +281,29 @@
>
>                 /* GPIO blocks 16 thru 19 do not appear to be routed to pins */
>
> +               dwmmc_0: dwmmc0 at f723d000 {
> +                       cap-mmc-highspeed;
> +                       non-removable;
> +                       bus-width = <0x8>;
> +                       vmmc-supply = <&ldo19>;
> +               };
> +
> +               dwmmc_1: dwmmc1 at f723e000 {
> +                       card-detect-delay = <200>;
> +                       cap-sd-highspeed;
> +                       sd-uhs-sdr12;
> +                       sd-uhs-sdr25;
> +                       sd-uhs-sdr50;
> +                       vqmmc-supply = <&ldo7>;
> +                       vmmc-supply = <&ldo10>;
> +                       bus-width = <0x4>;
> +                       disable-wp;
> +                       cd-gpios = <&gpio1 0 1>;
> +               };
> +
>                 dwmmc_2: dwmmc2 at f723f000 {
> -                       ti,non-removable;
> +                       broken-cd;
> +                       bus-width = <0x4>;
>                         non-removable;
>                         /* WL_EN */

I see you are just following the style that is used for all other nodes
in this file, but it's worth pointing out that it's really silly to use both
a label and the full path in both the .dtsi file for the SoC and the
board specific .dts file.

If you already have labels for each node, they should be used.

(no need to respin the patch for that, just pointing it out to
the maintainers for future updates)

        Arnd

[PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support

[Arnd Bergmann]

On Mon, May 22, 2017 at 10:40 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> + Arnd, Rob
>
> On 8 May 2017 at 18:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> The Hikey board has a Wifi chip attached to the SDIO bus, which is managed by a
>> dwmmc controller. This is currently broken in that way that not even the SDIO
>> card is being detected.
>>
>> To fix this we need to make sure the correct resources are described for the
>> dwmmc controller, so the mmc core can power on/off the SDIO/WiFi chip
>> correctly. This is the main issue today.
>>
>> I have picked up one change from Daniel Lezcano's earlier submission [1] and
>> folded that into this series, as the Wifi support depends on it. As a matter of
>> fact I have even split Daniel's change into two halves, one for DT
>> documentation and one for arm64 hikey dts.
>>
>> It's important that this series goes together as to conform with TI's HW spec
>> of the wl18xx Wifi chip, as to prevent it from being damaged. We can either go
>> via arm-soc, unless Wei Xu the Hikey SoC maintainer, prefers to ack the dts
>> changes, then I can pick the series via my mmc tree.
>>
>> Moreover, I would suggest we send this as fixes for the 4.12 rcs to get the
>> Wifi working for Hikey again.
>
> This series fixes my wifi issues on the Hikey board and currently all
> but the hikey dts changes seems to be okay/acked.
>
> Rob and me had some discussions regarding the new mmc DT binding,
> however in the last response from Rob he indicates that he is not
> objecting to adding the new binding.
>
> So, unless I hear some objection from any of you, I am going to queue
> this up for fixes via my mmc tree within a day or so.

Hi Ulf,

I looked at all the changes, and they seem reasonable to me. Please
add my

Acked-by: Arnd Bergmann <arnd@arndb.de>

and put them into a separate branch that you merge into your mmc
git, so we can merge the changes with the other updates in case
there are conflicts.

I see that the only code change is in patch 2. Are you able to put
that patch last in the series without introducing bisection problems?
That would be preferred, as it allows us to keep a pure 'DT' branch
if we end up merging your other seven patches in.

       Arnd

[PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support

[Ulf Hansson]

On 23 May 2017 at 14:00, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, May 22, 2017 at 10:40 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> + Arnd, Rob
>>
>> On 8 May 2017 at 18:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> The Hikey board has a Wifi chip attached to the SDIO bus, which is managed by a
>>> dwmmc controller. This is currently broken in that way that not even the SDIO
>>> card is being detected.
>>>
>>> To fix this we need to make sure the correct resources are described for the
>>> dwmmc controller, so the mmc core can power on/off the SDIO/WiFi chip
>>> correctly. This is the main issue today.
>>>
>>> I have picked up one change from Daniel Lezcano's earlier submission [1] and
>>> folded that into this series, as the Wifi support depends on it. As a matter of
>>> fact I have even split Daniel's change into two halves, one for DT
>>> documentation and one for arm64 hikey dts.
>>>
>>> It's important that this series goes together as to conform with TI's HW spec
>>> of the wl18xx Wifi chip, as to prevent it from being damaged. We can either go
>>> via arm-soc, unless Wei Xu the Hikey SoC maintainer, prefers to ack the dts
>>> changes, then I can pick the series via my mmc tree.
>>>
>>> Moreover, I would suggest we send this as fixes for the 4.12 rcs to get the
>>> Wifi working for Hikey again.
>>
>> This series fixes my wifi issues on the Hikey board and currently all
>> but the hikey dts changes seems to be okay/acked.
>>
>> Rob and me had some discussions regarding the new mmc DT binding,
>> however in the last response from Rob he indicates that he is not
>> objecting to adding the new binding.
>>
>> So, unless I hear some objection from any of you, I am going to queue
>> this up for fixes via my mmc tree within a day or so.
>
> Hi Ulf,
>
> I looked at all the changes, and they seem reasonable to me. Please
> add my
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>

Thanks Arnd!

> and put them into a separate branch that you merge into your mmc
> git, so we can merge the changes with the other updates in case
> there are conflicts.

I do that!

However, these will be sent as fixes for 4.12 rcs. I guess you can
pull the changes from there later instead.

>
> I see that the only code change is in patch 2. Are you able to put
> that patch last in the series without introducing bisection problems?
> That would be preferred, as it allows us to keep a pure 'DT' branch
> if we end up merging your other seven patches in.

Unfortunate not. Because of one may then end up running an incorrect
power off sequence, potentially damaging the Wifi chip.

Kind regards
Uffe

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[John Stultz]

On Mon, May 8, 2017 at 9:21 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The description of the connection between the dwmmc (SDIO) controller and
> the Wifi chip, which is attached to the SDIO bus is wrong. Currently the
> SDIO card can't be detected and thus the Wifi doesn't work.
>
> Let's fix this by assigning the correct vmmc supply, which is the always on
> regulator VDD_3V3 and remove the WLAN enable regulator altogether. Then to
> properly deal with the power on/off sequence, add a mmc-pwrseq node to
> describe the resources needed to detect the SDIO card.
>
> Except for the WLAN enable GPIO and its corresponding assert/de-assert
> delays, the mmc-pwrseq node also contains a handle to a clock provided by
> the hi655x pmic. This clock is also needed to be able to turn on the WiFi
> chip.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>


Ulf,
  So oddly, this patch seems to have broken wifi on HiKey when it
landed in 4.12-rc3. I checked my config and CONFIG_PWRSEQ_SIMPLE is
enabled.

Reverting this patch seems to get things working again for me.

thanks
-john

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[Daniel Lezcano]

On 31/05/2017 20:14, John Stultz wrote:
> On Mon, May 8, 2017 at 9:21 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> The description of the connection between the dwmmc (SDIO) controller and
>> the Wifi chip, which is attached to the SDIO bus is wrong. Currently the
>> SDIO card can't be detected and thus the Wifi doesn't work.
>>
>> Let's fix this by assigning the correct vmmc supply, which is the always on
>> regulator VDD_3V3 and remove the WLAN enable regulator altogether. Then to
>> properly deal with the power on/off sequence, add a mmc-pwrseq node to
>> describe the resources needed to detect the SDIO card.
>>
>> Except for the WLAN enable GPIO and its corresponding assert/de-assert
>> delays, the mmc-pwrseq node also contains a handle to a clock provided by
>> the hi655x pmic. This clock is also needed to be able to turn on the WiFi
>> chip.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> 
> Ulf,
>   So oddly, this patch seems to have broken wifi on HiKey when it
> landed in 4.12-rc3. I checked my config and CONFIG_PWRSEQ_SIMPLE is
> enabled.

Hi John,

how is it possible the WiFi stops working with this series? This series
provides the missing bits to enable the WiFi with a vanilla kernel.

May be the WiFi is reset with this kernel and when it starts again the
firmware fails to load because it is not up-to-date?

Can you check in dmesg if there is a firmware error message?




> Reverting this patch seems to get things working again for me.
> 
> thanks
> -john
> 


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[Ulf Hansson]

On 1 June 2017 at 22:57, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, May 31, 2017 at 11:36 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 31/05/2017 20:14, John Stultz wrote:
>>> On Mon, May 8, 2017 at 9:21 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> The description of the connection between the dwmmc (SDIO) controller and
>>>> the Wifi chip, which is attached to the SDIO bus is wrong. Currently the
>>>> SDIO card can't be detected and thus the Wifi doesn't work.
>>>>
>>>> Let's fix this by assigning the correct vmmc supply, which is the always on
>>>> regulator VDD_3V3 and remove the WLAN enable regulator altogether. Then to
>>>> properly deal with the power on/off sequence, add a mmc-pwrseq node to
>>>> describe the resources needed to detect the SDIO card.
>>>>
>>>> Except for the WLAN enable GPIO and its corresponding assert/de-assert
>>>> delays, the mmc-pwrseq node also contains a handle to a clock provided by
>>>> the hi655x pmic. This clock is also needed to be able to turn on the WiFi
>>>> chip.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>>
>>> Ulf,
>>>   So oddly, this patch seems to have broken wifi on HiKey when it
>>> landed in 4.12-rc3. I checked my config and CONFIG_PWRSEQ_SIMPLE is
>>> enabled.
>>
>> Hi John,
>>
>> how is it possible the WiFi stops working with this series? This series
>> provides the missing bits to enable the WiFi with a vanilla kernel.
>>
>> May be the WiFi is reset with this kernel and when it starts again the
>> firmware fails to load because it is not up-to-date?
>>
>> Can you check in dmesg if there is a firmware error message?
>
> So, it seems to me to be connected to some of the tweaks to the mmc2 dts node.
>
> When it works I see:
>  dwmmc_k3 f723f000.dwmmc2: Using internal DMA controller.
>  dwmmc_k3 f723f000.dwmmc2: Version ID is 250a
>  dwmmc_k3 f723f000.dwmmc2: DW MMC controller at irq 43,32 bit host
> data width,128 deep fifo
>  mmc_host mmc2: card is non-removable.
>  mmc_host mmc2: Bus speed (slot 0) = 24800000Hz (slot req 400000Hz,
> actual 400000HZ div = 31)
>  dwmmc_k3 f723f000.dwmmc2: 1 slots initialized
>  dwmmc_k3 f723f000.dwmmc2: card claims to support voltages below defined range
>  mmc_host mmc2: Bus speed (slot 0) = 24800000Hz (slot req 25000000Hz,
> actual 24800000HZ div = 0)
>  mmc2: new SDIO card at address 0001
>  wl18xx_driver wl18xx.4.auto: Direct firmware load for
> ti-connectivity/wl18xx-conf.bin failed with error -2
>  wl18xx_driver wl18xx.4.auto: Falling back to user helper
>
> When it fails:
>  dwmmc_k3 f723f000.dwmmc2: fifo-depth property not found, using value
> of FIFOTH register as default
>  dwmmc_k3 f723f000.dwmmc2: IDMAC supports 32-bit address mode.
>  dwmmc_k3 f723f000.dwmmc2: Using internal DMA controller.
>  dwmmc_k3 f723f000.dwmmc2: Version ID is 250a
>  dwmmc_k3 f723f000.dwmmc2: DW MMC controller at irq 43,32 bit host
> data width,128 deep fifo
>
> is seen repeatedly.
>
>
> I'm using my branch here (using the hikey_defconfig included):
> https://git.linaro.org/people/john.stultz/android-dev.git dev/hikey-mainline-WIP

John, thanks for the report!

After a some investigation, I realized that the mmc pwrseq_simple
driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
Hence the SDIO card will not be detected.

To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
kernel config. This is actually also the case when using the arm64
defconfig for a plain 4.12 rc3 and later. We should probably make this
driver enabled per default when building the arm64 defconfig.

Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
set? Just to make sure it works also with those boot binaries you are
using...

>
> With the last patch included or removed to generate the working and
> broken dmesgs respectively.
>
> thanks
> -john

Kind regards
Uffe

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[John Stultz]

On Mon, Jun 5, 2017 at 8:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 1 June 2017 at 22:57, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, May 31, 2017 at 11:36 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> On 31/05/2017 20:14, John Stultz wrote:
>>>> On Mon, May 8, 2017 at 9:21 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> The description of the connection between the dwmmc (SDIO) controller and
>>>>> the Wifi chip, which is attached to the SDIO bus is wrong. Currently the
>>>>> SDIO card can't be detected and thus the Wifi doesn't work.
>>>>>
>>>>> Let's fix this by assigning the correct vmmc supply, which is the always on
>>>>> regulator VDD_3V3 and remove the WLAN enable regulator altogether. Then to
>>>>> properly deal with the power on/off sequence, add a mmc-pwrseq node to
>>>>> describe the resources needed to detect the SDIO card.
>>>>>
>>>>> Except for the WLAN enable GPIO and its corresponding assert/de-assert
>>>>> delays, the mmc-pwrseq node also contains a handle to a clock provided by
>>>>> the hi655x pmic. This clock is also needed to be able to turn on the WiFi
>>>>> chip.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>>
>>>> Ulf,
>>>>   So oddly, this patch seems to have broken wifi on HiKey when it
>>>> landed in 4.12-rc3. I checked my config and CONFIG_PWRSEQ_SIMPLE is
>>>> enabled.
>>>
>>> Hi John,
>>>
>>> how is it possible the WiFi stops working with this series? This series
>>> provides the missing bits to enable the WiFi with a vanilla kernel.
>>>
>>> May be the WiFi is reset with this kernel and when it starts again the
>>> firmware fails to load because it is not up-to-date?
>>>
>>> Can you check in dmesg if there is a firmware error message?
>>
>> So, it seems to me to be connected to some of the tweaks to the mmc2 dts node.
>>
>> When it works I see:
>>  dwmmc_k3 f723f000.dwmmc2: Using internal DMA controller.
>>  dwmmc_k3 f723f000.dwmmc2: Version ID is 250a
>>  dwmmc_k3 f723f000.dwmmc2: DW MMC controller at irq 43,32 bit host
>> data width,128 deep fifo
>>  mmc_host mmc2: card is non-removable.
>>  mmc_host mmc2: Bus speed (slot 0) = 24800000Hz (slot req 400000Hz,
>> actual 400000HZ div = 31)
>>  dwmmc_k3 f723f000.dwmmc2: 1 slots initialized
>>  dwmmc_k3 f723f000.dwmmc2: card claims to support voltages below defined range
>>  mmc_host mmc2: Bus speed (slot 0) = 24800000Hz (slot req 25000000Hz,
>> actual 24800000HZ div = 0)
>>  mmc2: new SDIO card at address 0001
>>  wl18xx_driver wl18xx.4.auto: Direct firmware load for
>> ti-connectivity/wl18xx-conf.bin failed with error -2
>>  wl18xx_driver wl18xx.4.auto: Falling back to user helper
>>
>> When it fails:
>>  dwmmc_k3 f723f000.dwmmc2: fifo-depth property not found, using value
>> of FIFOTH register as default
>>  dwmmc_k3 f723f000.dwmmc2: IDMAC supports 32-bit address mode.
>>  dwmmc_k3 f723f000.dwmmc2: Using internal DMA controller.
>>  dwmmc_k3 f723f000.dwmmc2: Version ID is 250a
>>  dwmmc_k3 f723f000.dwmmc2: DW MMC controller at irq 43,32 bit host
>> data width,128 deep fifo
>>
>> is seen repeatedly.
>>
>>
>> I'm using my branch here (using the hikey_defconfig included):
>> https://git.linaro.org/people/john.stultz/android-dev.git dev/hikey-mainline-WIP
>
> John, thanks for the report!
>
> After a some investigation, I realized that the mmc pwrseq_simple
> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
> Hence the SDIO card will not be detected.
>
> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
> kernel config. This is actually also the case when using the arm64
> defconfig for a plain 4.12 rc3 and later. We should probably make this
> driver enabled per default when building the arm64 defconfig.

Ah! Thanks for this!

Would it make sense to have the hi6220 clk select this if its
required? I had thought the Hi655x was a different SoC, not just a
PMIC.  Expecting users to be able to figure any of this out doesn't
seem reasonable.


> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
> set? Just to make sure it works also with those boot binaries you are
> using...

Yep. With that it seems to work alright. I'll let you know if I see
any further problems as I continue to use it.

Thanks again for the help sorting this out!
-john

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

With the addition of the hi655x common clock, the config option is missing
for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
the hi655x clock driver misses when initializing the power sequence via DT.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 73272f4..6dfe72c 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -79,6 +79,7 @@ config ARCH_LG1K
 config ARCH_HISI
 	bool "Hisilicon SoC Family"
 	select ARM_TIMER_SP804
+	select COMMON_CLK_HI655X
 	select HISILICON_IRQ_MBIGEN if PCI
 	select PINCTRL
 	help
-- 
2.7.4

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

With the addition of the hi655x common clock, the config option is missing
for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
the hi655x clock driver misses when initializing the power sequence via DT.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 73272f4..6dfe72c 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -79,6 +79,7 @@ config ARCH_LG1K
 config ARCH_HISI
 	bool "Hisilicon SoC Family"
 	select ARM_TIMER_SP804
+	select COMMON_CLK_HI655X
 	select HISILICON_IRQ_MBIGEN if PCI
 	select PINCTRL
 	help
-- 
2.7.4

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[John Stultz]

On Mon, Jun 5, 2017 at 8:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 1 June 2017 at 22:57, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, May 31, 2017 at 11:36 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> On 31/05/2017 20:14, John Stultz wrote:
>>>> On Mon, May 8, 2017 at 9:21 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> The description of the connection between the dwmmc (SDIO) controller and
>>>>> the Wifi chip, which is attached to the SDIO bus is wrong. Currently the
>>>>> SDIO card can't be detected and thus the Wifi doesn't work.
>>>>>
>>>>> Let's fix this by assigning the correct vmmc supply, which is the always on
>>>>> regulator VDD_3V3 and remove the WLAN enable regulator altogether. Then to
>>>>> properly deal with the power on/off sequence, add a mmc-pwrseq node to
>>>>> describe the resources needed to detect the SDIO card.
>>>>>
>>>>> Except for the WLAN enable GPIO and its corresponding assert/de-assert
>>>>> delays, the mmc-pwrseq node also contains a handle to a clock provided by
>>>>> the hi655x pmic. This clock is also needed to be able to turn on the WiFi
>>>>> chip.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>>
>>>> Ulf,
>>>>   So oddly, this patch seems to have broken wifi on HiKey when it
>>>> landed in 4.12-rc3. I checked my config and CONFIG_PWRSEQ_SIMPLE is
>>>> enabled.
>>>
>>> Hi John,
>>>
>>> how is it possible the WiFi stops working with this series? This series
>>> provides the missing bits to enable the WiFi with a vanilla kernel.
>>>
>>> May be the WiFi is reset with this kernel and when it starts again the
>>> firmware fails to load because it is not up-to-date?
>>>
>>> Can you check in dmesg if there is a firmware error message?
>>
>> So, it seems to me to be connected to some of the tweaks to the mmc2 dts node.
>>
>> When it works I see:
>>  dwmmc_k3 f723f000.dwmmc2: Using internal DMA controller.
>>  dwmmc_k3 f723f000.dwmmc2: Version ID is 250a
>>  dwmmc_k3 f723f000.dwmmc2: DW MMC controller at irq 43,32 bit host
>> data width,128 deep fifo
>>  mmc_host mmc2: card is non-removable.
>>  mmc_host mmc2: Bus speed (slot 0) = 24800000Hz (slot req 400000Hz,
>> actual 400000HZ div = 31)
>>  dwmmc_k3 f723f000.dwmmc2: 1 slots initialized
>>  dwmmc_k3 f723f000.dwmmc2: card claims to support voltages below defined range
>>  mmc_host mmc2: Bus speed (slot 0) = 24800000Hz (slot req 25000000Hz,
>> actual 24800000HZ div = 0)
>>  mmc2: new SDIO card at address 0001
>>  wl18xx_driver wl18xx.4.auto: Direct firmware load for
>> ti-connectivity/wl18xx-conf.bin failed with error -2
>>  wl18xx_driver wl18xx.4.auto: Falling back to user helper
>>
>> When it fails:
>>  dwmmc_k3 f723f000.dwmmc2: fifo-depth property not found, using value
>> of FIFOTH register as default
>>  dwmmc_k3 f723f000.dwmmc2: IDMAC supports 32-bit address mode.
>>  dwmmc_k3 f723f000.dwmmc2: Using internal DMA controller.
>>  dwmmc_k3 f723f000.dwmmc2: Version ID is 250a
>>  dwmmc_k3 f723f000.dwmmc2: DW MMC controller at irq 43,32 bit host
>> data width,128 deep fifo
>>
>> is seen repeatedly.
>>
>>
>> I'm using my branch here (using the hikey_defconfig included):
>> https://git.linaro.org/people/john.stultz/android-dev.git dev/hikey-mainline-WIP
>
> John, thanks for the report!
>
> After a some investigation, I realized that the mmc pwrseq_simple
> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
> Hence the SDIO card will not be detected.
>
> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
> kernel config. This is actually also the case when using the arm64
> defconfig for a plain 4.12 rc3 and later. We should probably make this
> driver enabled per default when building the arm64 defconfig.
>
> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
> set? Just to make sure it works also with those boot binaries you are
> using...

So unfortunately, now I'm seeing a side-effect from enabling
CONFIG_COMMON_CLK_HI655X.

Using the serial-dev driver for the TI bluetooth, it seems I'm seeing
failures when CONFIG_COMMON_CLK_HI655X is enabled, but configuring it
out, bluetooth works (and wifi then fails).

Looking through the dmesg logs, the bluetooth driver seems to
initialize up properly and load the firmware, but with CLK_HI655X
enabled, we see:

Bluetooth: hci0 command 0xff05 tx timeout
Bluetooth: hci0: send command failed\x0a
Bluetooth: hci0 command 0xff36 tx timeout
Bluetooth: hci0 command 0x1003 tx timeout
Bluetooth: hci0 command 0x1001 tx timeout
Bluetooth: hci0 command 0x1009 tx timeout
...

This effectively seems to make bluetooth an wifi functionality
exclusive.  So I don't think this is a sufficient solution (over
reverting the original patch that changes the dts - which allows both
to function).

thanks
-john

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[Rob Herring]

On Mon, Jun 5, 2017 at 4:10 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Mon, Jun 5, 2017 at 8:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 1 June 2017 at 22:57, John Stultz <john.stultz@linaro.org> wrote:
>>> On Wed, May 31, 2017 at 11:36 AM, Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>> On 31/05/2017 20:14, John Stultz wrote:
>>>>> On Mon, May 8, 2017 at 9:21 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> The description of the connection between the dwmmc (SDIO) controller and
>>>>>> the Wifi chip, which is attached to the SDIO bus is wrong. Currently the
>>>>>> SDIO card can't be detected and thus the Wifi doesn't work.
>>>>>>
>>>>>> Let's fix this by assigning the correct vmmc supply, which is the always on
>>>>>> regulator VDD_3V3 and remove the WLAN enable regulator altogether. Then to
>>>>>> properly deal with the power on/off sequence, add a mmc-pwrseq node to
>>>>>> describe the resources needed to detect the SDIO card.
>>>>>>
>>>>>> Except for the WLAN enable GPIO and its corresponding assert/de-assert
>>>>>> delays, the mmc-pwrseq node also contains a handle to a clock provided by
>>>>>> the hi655x pmic. This clock is also needed to be able to turn on the WiFi
>>>>>> chip.
>>>>>>
>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>
>>>>>
>>>>> Ulf,
>>>>>   So oddly, this patch seems to have broken wifi on HiKey when it
>>>>> landed in 4.12-rc3. I checked my config and CONFIG_PWRSEQ_SIMPLE is
>>>>> enabled.
>>>>
>>>> Hi John,
>>>>
>>>> how is it possible the WiFi stops working with this series? This series
>>>> provides the missing bits to enable the WiFi with a vanilla kernel.
>>>>
>>>> May be the WiFi is reset with this kernel and when it starts again the
>>>> firmware fails to load because it is not up-to-date?
>>>>
>>>> Can you check in dmesg if there is a firmware error message?
>>>
>>> So, it seems to me to be connected to some of the tweaks to the mmc2 dts node.
>>>
>>> When it works I see:
>>>  dwmmc_k3 f723f000.dwmmc2: Using internal DMA controller.
>>>  dwmmc_k3 f723f000.dwmmc2: Version ID is 250a
>>>  dwmmc_k3 f723f000.dwmmc2: DW MMC controller at irq 43,32 bit host
>>> data width,128 deep fifo
>>>  mmc_host mmc2: card is non-removable.
>>>  mmc_host mmc2: Bus speed (slot 0) = 24800000Hz (slot req 400000Hz,
>>> actual 400000HZ div = 31)
>>>  dwmmc_k3 f723f000.dwmmc2: 1 slots initialized
>>>  dwmmc_k3 f723f000.dwmmc2: card claims to support voltages below defined range
>>>  mmc_host mmc2: Bus speed (slot 0) = 24800000Hz (slot req 25000000Hz,
>>> actual 24800000HZ div = 0)
>>>  mmc2: new SDIO card at address 0001
>>>  wl18xx_driver wl18xx.4.auto: Direct firmware load for
>>> ti-connectivity/wl18xx-conf.bin failed with error -2
>>>  wl18xx_driver wl18xx.4.auto: Falling back to user helper
>>>
>>> When it fails:
>>>  dwmmc_k3 f723f000.dwmmc2: fifo-depth property not found, using value
>>> of FIFOTH register as default
>>>  dwmmc_k3 f723f000.dwmmc2: IDMAC supports 32-bit address mode.
>>>  dwmmc_k3 f723f000.dwmmc2: Using internal DMA controller.
>>>  dwmmc_k3 f723f000.dwmmc2: Version ID is 250a
>>>  dwmmc_k3 f723f000.dwmmc2: DW MMC controller at irq 43,32 bit host
>>> data width,128 deep fifo
>>>
>>> is seen repeatedly.
>>>
>>>
>>> I'm using my branch here (using the hikey_defconfig included):
>>> https://git.linaro.org/people/john.stultz/android-dev.git dev/hikey-mainline-WIP
>>
>> John, thanks for the report!
>>
>> After a some investigation, I realized that the mmc pwrseq_simple
>> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
>> Hence the SDIO card will not be detected.
>>
>> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
>> kernel config. This is actually also the case when using the arm64
>> defconfig for a plain 4.12 rc3 and later. We should probably make this
>> driver enabled per default when building the arm64 defconfig.
>>
>> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
>> set? Just to make sure it works also with those boot binaries you are
>> using...
>
> So unfortunately, now I'm seeing a side-effect from enabling
> CONFIG_COMMON_CLK_HI655X.
>
> Using the serial-dev driver for the TI bluetooth, it seems I'm seeing
> failures when CONFIG_COMMON_CLK_HI655X is enabled, but configuring it
> out, bluetooth works (and wifi then fails).
>
> Looking through the dmesg logs, the bluetooth driver seems to
> initialize up properly and load the firmware, but with CLK_HI655X
> enabled, we see:
>
> Bluetooth: hci0 command 0xff05 tx timeout
> Bluetooth: hci0: send command failed\x0a
> Bluetooth: hci0 command 0xff36 tx timeout
> Bluetooth: hci0 command 0x1003 tx timeout
> Bluetooth: hci0 command 0x1001 tx timeout
> Bluetooth: hci0 command 0x1009 tx timeout
> ...
>
> This effectively seems to make bluetooth an wifi functionality
> exclusive.  So I don't think this is a sufficient solution (over
> reverting the original patch that changes the dts - which allows both
> to function).

If the clock to the TI chip is described in DT for WiFi, then the BT
side needs it as well (as does the driver) for proper refcounting. The
same would apply to regulators as well. I looked at the regulators for
HiKey and the 2 supplies (Vbat and i/o) appear to both be always on.

Rob

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[Ulf Hansson]

[...]

>>>
>>> John, thanks for the report!
>>>
>>> After a some investigation, I realized that the mmc pwrseq_simple
>>> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
>>> Hence the SDIO card will not be detected.
>>>
>>> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
>>> kernel config. This is actually also the case when using the arm64
>>> defconfig for a plain 4.12 rc3 and later. We should probably make this
>>> driver enabled per default when building the arm64 defconfig.
>>>
>>> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
>>> set? Just to make sure it works also with those boot binaries you are
>>> using...
>>
>> So unfortunately, now I'm seeing a side-effect from enabling
>> CONFIG_COMMON_CLK_HI655X.
>>
>> Using the serial-dev driver for the TI bluetooth, it seems I'm seeing
>> failures when CONFIG_COMMON_CLK_HI655X is enabled, but configuring it
>> out, bluetooth works (and wifi then fails).
>>
>> Looking through the dmesg logs, the bluetooth driver seems to
>> initialize up properly and load the firmware, but with CLK_HI655X
>> enabled, we see:
>>
>> Bluetooth: hci0 command 0xff05 tx timeout
>> Bluetooth: hci0: send command failed\x0a
>> Bluetooth: hci0 command 0xff36 tx timeout
>> Bluetooth: hci0 command 0x1003 tx timeout
>> Bluetooth: hci0 command 0x1001 tx timeout
>> Bluetooth: hci0 command 0x1009 tx timeout
>> ...

As Rob said below, this is because the blue-tooth driver doesn't
properly deal with the power on/off sequence.

I guess it works for you because your versions of the boot binaries
turns all needed resources on. That isn't the case for me, blue-tooth
is neither working before or after this change, but wifi is.

>>
>> This effectively seems to make bluetooth an wifi functionality
>> exclusive.  So I don't think this is a sufficient solution (over
>> reverting the original patch that changes the dts - which allows both
>> to function).

Since the issues you are reporting about is depending on the
boot-binaries and not a really bugs in the kernel, may I suggest that
we invest our efforts in fixing the blue-tooth driver instead, as it's
there the real problem is!?

>
> If the clock to the TI chip is described in DT for WiFi, then the BT
> side needs it as well (as does the driver) for proper refcounting. The
> same would apply to regulators as well. I looked at the regulators for
> HiKey and the 2 supplies (Vbat and i/o) appear to both be always on.
>
> Rob

Correct.

I am working on patch, I keep you on cc.

Kind regards
Uffe

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[Ulf Hansson]

On 6 June 2017 at 12:08, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> [...]
>
>>>>
>>>> John, thanks for the report!
>>>>
>>>> After a some investigation, I realized that the mmc pwrseq_simple
>>>> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
>>>> Hence the SDIO card will not be detected.
>>>>
>>>> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
>>>> kernel config. This is actually also the case when using the arm64
>>>> defconfig for a plain 4.12 rc3 and later. We should probably make this
>>>> driver enabled per default when building the arm64 defconfig.
>>>>
>>>> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
>>>> set? Just to make sure it works also with those boot binaries you are
>>>> using...
>>>
>>> So unfortunately, now I'm seeing a side-effect from enabling
>>> CONFIG_COMMON_CLK_HI655X.
>>>
>>> Using the serial-dev driver for the TI bluetooth, it seems I'm seeing
>>> failures when CONFIG_COMMON_CLK_HI655X is enabled, but configuring it
>>> out, bluetooth works (and wifi then fails).
>>>
>>> Looking through the dmesg logs, the bluetooth driver seems to
>>> initialize up properly and load the firmware, but with CLK_HI655X
>>> enabled, we see:
>>>
>>> Bluetooth: hci0 command 0xff05 tx timeout
>>> Bluetooth: hci0: send command failed\x0a
>>> Bluetooth: hci0 command 0xff36 tx timeout
>>> Bluetooth: hci0 command 0x1003 tx timeout
>>> Bluetooth: hci0 command 0x1001 tx timeout
>>> Bluetooth: hci0 command 0x1009 tx timeout
>>> ...
>
> As Rob said below, this is because the blue-tooth driver doesn't
> properly deal with the power on/off sequence.
>
> I guess it works for you because your versions of the boot binaries
> turns all needed resources on. That isn't the case for me, blue-tooth
> is neither working before or after this change, but wifi is.
>
>>>
>>> This effectively seems to make bluetooth an wifi functionality
>>> exclusive.  So I don't think this is a sufficient solution (over
>>> reverting the original patch that changes the dts - which allows both
>>> to function).
>
> Since the issues you are reporting about is depending on the
> boot-binaries and not a really bugs in the kernel, may I suggest that
> we invest our efforts in fixing the blue-tooth driver instead, as it's
> there the real problem is!?
>
>>
>> If the clock to the TI chip is described in DT for WiFi, then the BT
>> side needs it as well (as does the driver) for proper refcounting. The
>> same would apply to regulators as well. I looked at the regulators for
>> HiKey and the 2 supplies (Vbat and i/o) appear to both be always on.
>>
>> Rob
>
> Correct.
>
> I am working on patch, I keep you on cc.

Rob, John,

I have now looked into this a bit more. So I have some local patches,
which in principle adds the external clock to the bluetooth node for
the hikey dts, and adapts the uart bluetooth driver
(drivers/bluetooth/hci_ll.c) to also take the ext clock into
consideration while powering on/off the chip. I am working on a
vanilla kernel, thus not John's tree.

However, I can't get the uart1 amba device to be added because of this
error during boot:
"OF: amba_device_add() failed (-19) for /soc/uart at f7111000"

The reason why it fails is because amba_device_try_add() fails to read
the amba periphid of the uart1 device. I haven't yet been able to
figure out why.

Is this problem something you have seen before? I tried out v4.11,
v4.12-rc3 and John's tree (dev/hikey-mainline-WIP which is based upon
4.12.rc3), they all suffer from the same problem, not being able to
add the uart1 device.

The consequence then becomes that the bluetooth node (which is a child
node for the uart1 node), added in the below commit by Rob, never gets
parsed and thus the device don't become added. In other words, I
haven't been able to test my changes since I can't even get the
bluetooth device to be added.

John, are you using the pcm bluetooth interface or the uart?

commit 019aa56b7df8a796b2c01a56269a370ad3442ec7
Author: Rob Herring <robh@kernel.org>
Date:   Thu Apr 13 10:03:53 2017 -0500

    arm64: dts: hikey: add WL1835 Bluetooth device node

    This adds the serial slave device for the WL1835 Bluetooth interface.

    Signed-off-by: Rob Herring <robh@kernel.org>
    Cc: Wei Xu <xuwei5@hisilicon.com>
    Cc: Mark Rutland <mark.rutland@arm.com>
    Signed-off-by: Marcel Holtmann <marcel@holtmann.org>

Kind regards
Uffe

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Ulf Hansson]

On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> With the addition of the hi655x common clock, the config option is missing
> for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
> the hi655x clock driver misses when initializing the power sequence via DT.
>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
work for Hikey.

Kind regards
Uffe

> ---
>  arch/arm64/Kconfig.platforms | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 73272f4..6dfe72c 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -79,6 +79,7 @@ config ARCH_LG1K
>  config ARCH_HISI
>         bool "Hisilicon SoC Family"
>         select ARM_TIMER_SP804
> +       select COMMON_CLK_HI655X
>         select HISILICON_IRQ_MBIGEN if PCI
>         select PINCTRL
>         help
> --
> 2.7.4
>

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Ulf Hansson]

On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> With the addition of the hi655x common clock, the config option is missing
> for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
> the hi655x clock driver misses when initializing the power sequence via DT.
>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
work for Hikey.

Kind regards
Uffe

> ---
>  arch/arm64/Kconfig.platforms | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 73272f4..6dfe72c 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -79,6 +79,7 @@ config ARCH_LG1K
>  config ARCH_HISI
>         bool "Hisilicon SoC Family"
>         select ARM_TIMER_SP804
> +       select COMMON_CLK_HI655X
>         select HISILICON_IRQ_MBIGEN if PCI
>         select PINCTRL
>         help
> --
> 2.7.4
>

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[John Stultz]

On Tue, Jun 6, 2017 at 3:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> [...]
>
>>>>
>>>> John, thanks for the report!
>>>>
>>>> After a some investigation, I realized that the mmc pwrseq_simple
>>>> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
>>>> Hence the SDIO card will not be detected.
>>>>
>>>> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
>>>> kernel config. This is actually also the case when using the arm64
>>>> defconfig for a plain 4.12 rc3 and later. We should probably make this
>>>> driver enabled per default when building the arm64 defconfig.
>>>>
>>>> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
>>>> set? Just to make sure it works also with those boot binaries you are
>>>> using...
>>>
>>> So unfortunately, now I'm seeing a side-effect from enabling
>>> CONFIG_COMMON_CLK_HI655X.
>>>
>>> Using the serial-dev driver for the TI bluetooth, it seems I'm seeing
>>> failures when CONFIG_COMMON_CLK_HI655X is enabled, but configuring it
>>> out, bluetooth works (and wifi then fails).
>>>
>>> Looking through the dmesg logs, the bluetooth driver seems to
>>> initialize up properly and load the firmware, but with CLK_HI655X
>>> enabled, we see:
>>>
>>> Bluetooth: hci0 command 0xff05 tx timeout
>>> Bluetooth: hci0: send command failed\x0a
>>> Bluetooth: hci0 command 0xff36 tx timeout
>>> Bluetooth: hci0 command 0x1003 tx timeout
>>> Bluetooth: hci0 command 0x1001 tx timeout
>>> Bluetooth: hci0 command 0x1009 tx timeout
>>> ...
>
> As Rob said below, this is because the blue-tooth driver doesn't
> properly deal with the power on/off sequence.
>
> I guess it works for you because your versions of the boot binaries
> turns all needed resources on. That isn't the case for me, blue-tooth
> is neither working before or after this change, but wifi is.
>
>>>
>>> This effectively seems to make bluetooth an wifi functionality
>>> exclusive.  So I don't think this is a sufficient solution (over
>>> reverting the original patch that changes the dts - which allows both
>>> to function).
>
> Since the issues you are reporting about is depending on the
> boot-binaries and not a really bugs in the kernel, may I suggest that
> we invest our efforts in fixing the blue-tooth driver instead, as it's
> there the real problem is!?

I'd agree a fast fix to the bluetooth node would be preferred. But to
the issue of bugs in the kernel vs bootloader, to users it doesn't
matter. It worked before, and now it doesn't. That's a regression, so
either we need a fix or a revert.

>> If the clock to the TI chip is described in DT for WiFi, then the BT
>> side needs it as well (as does the driver) for proper refcounting. The
>> same would apply to regulators as well. I looked at the regulators for
>> HiKey and the 2 supplies (Vbat and i/o) appear to both be always on.
>>
>> Rob
>
> Correct.
>
> I am working on patch, I keep you on cc.

Nice, I'll be happy to test!

thanks
-john

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[John Stultz]

On Tue, Jun 6, 2017 at 7:13 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 6 June 2017 at 12:08, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> [...]
>>
>>>>>
>>>>> John, thanks for the report!
>>>>>
>>>>> After a some investigation, I realized that the mmc pwrseq_simple
>>>>> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
>>>>> Hence the SDIO card will not be detected.
>>>>>
>>>>> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
>>>>> kernel config. This is actually also the case when using the arm64
>>>>> defconfig for a plain 4.12 rc3 and later. We should probably make this
>>>>> driver enabled per default when building the arm64 defconfig.
>>>>>
>>>>> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
>>>>> set? Just to make sure it works also with those boot binaries you are
>>>>> using...
>>>>
>>>> So unfortunately, now I'm seeing a side-effect from enabling
>>>> CONFIG_COMMON_CLK_HI655X.
>>>>
>>>> Using the serial-dev driver for the TI bluetooth, it seems I'm seeing
>>>> failures when CONFIG_COMMON_CLK_HI655X is enabled, but configuring it
>>>> out, bluetooth works (and wifi then fails).
>>>>
>>>> Looking through the dmesg logs, the bluetooth driver seems to
>>>> initialize up properly and load the firmware, but with CLK_HI655X
>>>> enabled, we see:
>>>>
>>>> Bluetooth: hci0 command 0xff05 tx timeout
>>>> Bluetooth: hci0: send command failed\x0a
>>>> Bluetooth: hci0 command 0xff36 tx timeout
>>>> Bluetooth: hci0 command 0x1003 tx timeout
>>>> Bluetooth: hci0 command 0x1001 tx timeout
>>>> Bluetooth: hci0 command 0x1009 tx timeout
>>>> ...
>>
>> As Rob said below, this is because the blue-tooth driver doesn't
>> properly deal with the power on/off sequence.
>>
>> I guess it works for you because your versions of the boot binaries
>> turns all needed resources on. That isn't the case for me, blue-tooth
>> is neither working before or after this change, but wifi is.
>>
>>>>
>>>> This effectively seems to make bluetooth an wifi functionality
>>>> exclusive.  So I don't think this is a sufficient solution (over
>>>> reverting the original patch that changes the dts - which allows both
>>>> to function).
>>
>> Since the issues you are reporting about is depending on the
>> boot-binaries and not a really bugs in the kernel, may I suggest that
>> we invest our efforts in fixing the blue-tooth driver instead, as it's
>> there the real problem is!?
>>
>>>
>>> If the clock to the TI chip is described in DT for WiFi, then the BT
>>> side needs it as well (as does the driver) for proper refcounting. The
>>> same would apply to regulators as well. I looked at the regulators for
>>> HiKey and the 2 supplies (Vbat and i/o) appear to both be always on.
>>>
>>> Rob
>>
>> Correct.
>>
>> I am working on patch, I keep you on cc.
>
> Rob, John,
>
> I have now looked into this a bit more. So I have some local patches,
> which in principle adds the external clock to the bluetooth node for
> the hikey dts, and adapts the uart bluetooth driver
> (drivers/bluetooth/hci_ll.c) to also take the ext clock into
> consideration while powering on/off the chip. I am working on a
> vanilla kernel, thus not John's tree.
>
> However, I can't get the uart1 amba device to be added because of this
> error during boot:
> "OF: amba_device_add() failed (-19) for /soc/uart at f7111000"
>
> The reason why it fails is because amba_device_try_add() fails to read
> the amba periphid of the uart1 device. I haven't yet been able to
> figure out why.
>
> Is this problem something you have seen before? I tried out v4.11,
> v4.12-rc3 and John's tree (dev/hikey-mainline-WIP which is based upon
> 4.12.rc3), they all suffer from the same problem, not being able to
> add the uart1 device.
>
> The consequence then becomes that the bluetooth node (which is a child
> node for the uart1 node), added in the below commit by Rob, never gets
> parsed and thus the device don't become added. In other words, I
> haven't been able to test my changes since I can't even get the
> bluetooth device to be added.
>
> John, are you using the pcm bluetooth interface or the uart?

UART.

I'm not sure why initializing the UART fails for you.  I suspect again
it might be related to differences in the bootloader, as I'm not sure
if uboot has had nearly the amount of usage as UEFI.

If you want to send a patch my way, I'm happy to test it, or you can
grab debian images that use UEFI here:
https://www.96boards.org/documentation/ConsumerEdition/HiKey/Downloads/Debian.md/

thanks
-john

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[Ulf Hansson]

On 6 June 2017 at 18:24, John Stultz <john.stultz@linaro.org> wrote:
> On Tue, Jun 6, 2017 at 7:13 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 6 June 2017 at 12:08, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> [...]
>>>
>>>>>>
>>>>>> John, thanks for the report!
>>>>>>
>>>>>> After a some investigation, I realized that the mmc pwrseq_simple
>>>>>> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
>>>>>> Hence the SDIO card will not be detected.
>>>>>>
>>>>>> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
>>>>>> kernel config. This is actually also the case when using the arm64
>>>>>> defconfig for a plain 4.12 rc3 and later. We should probably make this
>>>>>> driver enabled per default when building the arm64 defconfig.
>>>>>>
>>>>>> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
>>>>>> set? Just to make sure it works also with those boot binaries you are
>>>>>> using...
>>>>>
>>>>> So unfortunately, now I'm seeing a side-effect from enabling
>>>>> CONFIG_COMMON_CLK_HI655X.
>>>>>
>>>>> Using the serial-dev driver for the TI bluetooth, it seems I'm seeing
>>>>> failures when CONFIG_COMMON_CLK_HI655X is enabled, but configuring it
>>>>> out, bluetooth works (and wifi then fails).
>>>>>
>>>>> Looking through the dmesg logs, the bluetooth driver seems to
>>>>> initialize up properly and load the firmware, but with CLK_HI655X
>>>>> enabled, we see:
>>>>>
>>>>> Bluetooth: hci0 command 0xff05 tx timeout
>>>>> Bluetooth: hci0: send command failed\x0a
>>>>> Bluetooth: hci0 command 0xff36 tx timeout
>>>>> Bluetooth: hci0 command 0x1003 tx timeout
>>>>> Bluetooth: hci0 command 0x1001 tx timeout
>>>>> Bluetooth: hci0 command 0x1009 tx timeout
>>>>> ...
>>>
>>> As Rob said below, this is because the blue-tooth driver doesn't
>>> properly deal with the power on/off sequence.
>>>
>>> I guess it works for you because your versions of the boot binaries
>>> turns all needed resources on. That isn't the case for me, blue-tooth
>>> is neither working before or after this change, but wifi is.
>>>
>>>>>
>>>>> This effectively seems to make bluetooth an wifi functionality
>>>>> exclusive.  So I don't think this is a sufficient solution (over
>>>>> reverting the original patch that changes the dts - which allows both
>>>>> to function).
>>>
>>> Since the issues you are reporting about is depending on the
>>> boot-binaries and not a really bugs in the kernel, may I suggest that
>>> we invest our efforts in fixing the blue-tooth driver instead, as it's
>>> there the real problem is!?
>>>
>>>>
>>>> If the clock to the TI chip is described in DT for WiFi, then the BT
>>>> side needs it as well (as does the driver) for proper refcounting. The
>>>> same would apply to regulators as well. I looked at the regulators for
>>>> HiKey and the 2 supplies (Vbat and i/o) appear to both be always on.
>>>>
>>>> Rob
>>>
>>> Correct.
>>>
>>> I am working on patch, I keep you on cc.
>>
>> Rob, John,
>>
>> I have now looked into this a bit more. So I have some local patches,
>> which in principle adds the external clock to the bluetooth node for
>> the hikey dts, and adapts the uart bluetooth driver
>> (drivers/bluetooth/hci_ll.c) to also take the ext clock into
>> consideration while powering on/off the chip. I am working on a
>> vanilla kernel, thus not John's tree.
>>
>> However, I can't get the uart1 amba device to be added because of this
>> error during boot:
>> "OF: amba_device_add() failed (-19) for /soc/uart at f7111000"
>>
>> The reason why it fails is because amba_device_try_add() fails to read
>> the amba periphid of the uart1 device. I haven't yet been able to
>> figure out why.
>>
>> Is this problem something you have seen before? I tried out v4.11,
>> v4.12-rc3 and John's tree (dev/hikey-mainline-WIP which is based upon
>> 4.12.rc3), they all suffer from the same problem, not being able to
>> add the uart1 device.
>>
>> The consequence then becomes that the bluetooth node (which is a child
>> node for the uart1 node), added in the below commit by Rob, never gets
>> parsed and thus the device don't become added. In other words, I
>> haven't been able to test my changes since I can't even get the
>> bluetooth device to be added.
>>
>> John, are you using the pcm bluetooth interface or the uart?
>
> UART.
>
> I'm not sure why initializing the UART fails for you.  I suspect again
> it might be related to differences in the bootloader, as I'm not sure
> if uboot has had nearly the amount of usage as UEFI.

You are probably right, but that also makes me worried, as we have
likely other similar issues where UEFI just magically solves things
for the kernel.

>
> If you want to send a patch my way, I'm happy to test it, or you can
> grab debian images that use UEFI here:
> https://www.96boards.org/documentation/ConsumerEdition/HiKey/Downloads/Debian.md/

Seems like I need to give UEFI a try again, however this time I would
really appreciate your help in testing as I am running out of
bandwidth for this task.

Just about to post the patches....

>
> thanks
> -john

Kind regards
Uffe

[PATCH 8/8] arm64: dts: hikey: Fix WiFi support

[John Stultz]

On Tue, Jun 6, 2017 at 9:24 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 6 June 2017 at 18:24, John Stultz <john.stultz@linaro.org> wrote:
>> On Tue, Jun 6, 2017 at 7:13 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 6 June 2017 at 12:08, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> [...]
>>>>
>>>>>>>
>>>>>>> John, thanks for the report!
>>>>>>>
>>>>>>> After a some investigation, I realized that the mmc pwrseq_simple
>>>>>>> driver returns -EPROBE_DEFER as it's not able to get the "ext" clock.
>>>>>>> Hence the SDIO card will not be detected.
>>>>>>>
>>>>>>> To fix the problem, you need to enable CONFIG_COMMON_CLK_HI655X in the
>>>>>>> kernel config. This is actually also the case when using the arm64
>>>>>>> defconfig for a plain 4.12 rc3 and later. We should probably make this
>>>>>>> driver enabled per default when building the arm64 defconfig.
>>>>>>>
>>>>>>> Can you run a re-test at your side with the CONFIG_COMMON_CLK_HI655X
>>>>>>> set? Just to make sure it works also with those boot binaries you are
>>>>>>> using...
>>>>>>
>>>>>> So unfortunately, now I'm seeing a side-effect from enabling
>>>>>> CONFIG_COMMON_CLK_HI655X.
>>>>>>
>>>>>> Using the serial-dev driver for the TI bluetooth, it seems I'm seeing
>>>>>> failures when CONFIG_COMMON_CLK_HI655X is enabled, but configuring it
>>>>>> out, bluetooth works (and wifi then fails).
>>>>>>
>>>>>> Looking through the dmesg logs, the bluetooth driver seems to
>>>>>> initialize up properly and load the firmware, but with CLK_HI655X
>>>>>> enabled, we see:
>>>>>>
>>>>>> Bluetooth: hci0 command 0xff05 tx timeout
>>>>>> Bluetooth: hci0: send command failed\x0a
>>>>>> Bluetooth: hci0 command 0xff36 tx timeout
>>>>>> Bluetooth: hci0 command 0x1003 tx timeout
>>>>>> Bluetooth: hci0 command 0x1001 tx timeout
>>>>>> Bluetooth: hci0 command 0x1009 tx timeout
>>>>>> ...
>>>>
>>>> As Rob said below, this is because the blue-tooth driver doesn't
>>>> properly deal with the power on/off sequence.
>>>>
>>>> I guess it works for you because your versions of the boot binaries
>>>> turns all needed resources on. That isn't the case for me, blue-tooth
>>>> is neither working before or after this change, but wifi is.
>>>>
>>>>>>
>>>>>> This effectively seems to make bluetooth an wifi functionality
>>>>>> exclusive.  So I don't think this is a sufficient solution (over
>>>>>> reverting the original patch that changes the dts - which allows both
>>>>>> to function).
>>>>
>>>> Since the issues you are reporting about is depending on the
>>>> boot-binaries and not a really bugs in the kernel, may I suggest that
>>>> we invest our efforts in fixing the blue-tooth driver instead, as it's
>>>> there the real problem is!?
>>>>
>>>>>
>>>>> If the clock to the TI chip is described in DT for WiFi, then the BT
>>>>> side needs it as well (as does the driver) for proper refcounting. The
>>>>> same would apply to regulators as well. I looked at the regulators for
>>>>> HiKey and the 2 supplies (Vbat and i/o) appear to both be always on.
>>>>>
>>>>> Rob
>>>>
>>>> Correct.
>>>>
>>>> I am working on patch, I keep you on cc.
>>>
>>> Rob, John,
>>>
>>> I have now looked into this a bit more. So I have some local patches,
>>> which in principle adds the external clock to the bluetooth node for
>>> the hikey dts, and adapts the uart bluetooth driver
>>> (drivers/bluetooth/hci_ll.c) to also take the ext clock into
>>> consideration while powering on/off the chip. I am working on a
>>> vanilla kernel, thus not John's tree.
>>>
>>> However, I can't get the uart1 amba device to be added because of this
>>> error during boot:
>>> "OF: amba_device_add() failed (-19) for /soc/uart at f7111000"
>>>
>>> The reason why it fails is because amba_device_try_add() fails to read
>>> the amba periphid of the uart1 device. I haven't yet been able to
>>> figure out why.
>>>
>>> Is this problem something you have seen before? I tried out v4.11,
>>> v4.12-rc3 and John's tree (dev/hikey-mainline-WIP which is based upon
>>> 4.12.rc3), they all suffer from the same problem, not being able to
>>> add the uart1 device.
>>>
>>> The consequence then becomes that the bluetooth node (which is a child
>>> node for the uart1 node), added in the below commit by Rob, never gets
>>> parsed and thus the device don't become added. In other words, I
>>> haven't been able to test my changes since I can't even get the
>>> bluetooth device to be added.
>>>
>>> John, are you using the pcm bluetooth interface or the uart?
>>
>> UART.
>>
>> I'm not sure why initializing the UART fails for you.  I suspect again
>> it might be related to differences in the bootloader, as I'm not sure
>> if uboot has had nearly the amount of usage as UEFI.
>
> You are probably right, but that also makes me worried, as we have
> likely other similar issues where UEFI just magically solves things
> for the kernel.

Yea. If I recall there were a few other clks (like with the gpu) where
the initialization was moved to UEFI.


>> If you want to send a patch my way, I'm happy to test it, or you can
>> grab debian images that use UEFI here:
>> https://www.96boards.org/documentation/ConsumerEdition/HiKey/Downloads/Debian.md/
>
> Seems like I need to give UEFI a try again, however this time I would
> really appreciate your help in testing as I am running out of
> bandwidth for this task.
>
> Just about to post the patches....

Sure. I'm happy to test and help tinker.

Sorry for my nit-picking of this issue is causing a burden. But
chasing down these sort of things in every release is a load on my
side too. :)

thanks
-john

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> > With the addition of the hi655x common clock, the config option is missing
> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
> > the hi655x clock driver misses when initializing the power sequence via DT.
> >
> > Cc: John Stultz <john.stultz@linaro.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Wei Xu <xuwei5@hisilicon.com>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
> work for Hikey.
> 

I'm wondering if I submitted this patch for the right path.

Shall it go through arm-soc ?

+Olof, +Arnd

> 
> > ---
> >  arch/arm64/Kconfig.platforms | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> > index 73272f4..6dfe72c 100644
> > --- a/arch/arm64/Kconfig.platforms
> > +++ b/arch/arm64/Kconfig.platforms
> > @@ -79,6 +79,7 @@ config ARCH_LG1K
> >  config ARCH_HISI
> >         bool "Hisilicon SoC Family"
> >         select ARM_TIMER_SP804
> > +       select COMMON_CLK_HI655X
> >         select HISILICON_IRQ_MBIGEN if PCI
> >         select PINCTRL
> >         help
> > --
> > 2.7.4
> >

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> > With the addition of the hi655x common clock, the config option is missing
> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
> > the hi655x clock driver misses when initializing the power sequence via DT.
> >
> > Cc: John Stultz <john.stultz@linaro.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Wei Xu <xuwei5@hisilicon.com>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
> work for Hikey.
> 

I'm wondering if I submitted this patch for the right path.

Shall it go through arm-soc ?

+Olof, +Arnd

> 
> > ---
> >  arch/arm64/Kconfig.platforms | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> > index 73272f4..6dfe72c 100644
> > --- a/arch/arm64/Kconfig.platforms
> > +++ b/arch/arm64/Kconfig.platforms
> > @@ -79,6 +79,7 @@ config ARCH_LG1K
> >  config ARCH_HISI
> >         bool "Hisilicon SoC Family"
> >         select ARM_TIMER_SP804
> > +       select COMMON_CLK_HI655X
> >         select HISILICON_IRQ_MBIGEN if PCI
> >         select PINCTRL
> >         help
> > --
> > 2.7.4
> >

-- 

 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Arnd Bergmann]

On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> > With the addition of the hi655x common clock, the config option is missing
>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
>> > the hi655x clock driver misses when initializing the power sequence via DT.
>> >
>> > Cc: John Stultz <john.stultz@linaro.org>
>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> > Cc: Wei Xu <xuwei5@hisilicon.com>
>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
>> work for Hikey.
>>
>
> I'm wondering if I submitted this patch for the right path.
>
> Shall it go through arm-soc ?

Yes, but I'm not sure this is the right patch either. We tend to not
use 'select' for user-visible drivers, and most hisilicon platforms
won't need this driver.

I think it would be more consistent to add this to the defconfig
and regard it as a user error when the driver is disabled on a
machine that needs it.

       Arnd

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Arnd Bergmann]

On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> > With the addition of the hi655x common clock, the config option is missing
>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
>> > the hi655x clock driver misses when initializing the power sequence via DT.
>> >
>> > Cc: John Stultz <john.stultz@linaro.org>
>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> > Cc: Wei Xu <xuwei5@hisilicon.com>
>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
>> work for Hikey.
>>
>
> I'm wondering if I submitted this patch for the right path.
>
> Shall it go through arm-soc ?

Yes, but I'm not sure this is the right patch either. We tend to not
use 'select' for user-visible drivers, and most hisilicon platforms
won't need this driver.

I think it would be more consistent to add this to the defconfig
and regard it as a user error when the driver is disabled on a
machine that needs it.

       Arnd

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[John Stultz]

On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>> > With the addition of the hi655x common clock, the config option is missing
>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
>>> > the hi655x clock driver misses when initializing the power sequence via DT.
>>> >
>>> > Cc: John Stultz <john.stultz@linaro.org>
>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>> > Cc: Wei Xu <xuwei5@hisilicon.com>
>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
>>> work for Hikey.
>>>
>>
>> I'm wondering if I submitted this patch for the right path.
>>
>> Shall it go through arm-soc ?
>
> Yes, but I'm not sure this is the right patch either. We tend to not
> use 'select' for user-visible drivers, and most hisilicon platforms
> won't need this driver.
>
> I think it would be more consistent to add this to the defconfig
> and regard it as a user error when the driver is disabled on a
> machine that needs it.

Maybe the select is not exactly in the right place, but I don't really
feel like a pmic on an SoC is a "user-visible driver". I deal with the
board often and when the new dependency was made on the clk, I would
have never have found it on my own w/o Ulf and Daniel pointing out
what I needed to enable.

thanks
-john

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[John Stultz]

On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>> > With the addition of the hi655x common clock, the config option is missing
>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
>>> > the hi655x clock driver misses when initializing the power sequence via DT.
>>> >
>>> > Cc: John Stultz <john.stultz@linaro.org>
>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>> > Cc: Wei Xu <xuwei5@hisilicon.com>
>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
>>> work for Hikey.
>>>
>>
>> I'm wondering if I submitted this patch for the right path.
>>
>> Shall it go through arm-soc ?
>
> Yes, but I'm not sure this is the right patch either. We tend to not
> use 'select' for user-visible drivers, and most hisilicon platforms
> won't need this driver.
>
> I think it would be more consistent to add this to the defconfig
> and regard it as a user error when the driver is disabled on a
> machine that needs it.

Maybe the select is not exactly in the right place, but I don't really
feel like a pmic on an SoC is a "user-visible driver". I deal with the
board often and when the new dependency was made on the clk, I would
have never have found it on my own w/o Ulf and Daniel pointing out
what I needed to enable.

thanks
-john

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Arnd Bergmann]

On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
>>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>> > With the addition of the hi655x common clock, the config option is missing
>>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
>>>> > the hi655x clock driver misses when initializing the power sequence via DT.
>>>> >
>>>> > Cc: John Stultz <john.stultz@linaro.org>
>>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>>> > Cc: Wei Xu <xuwei5@hisilicon.com>
>>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>
>>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
>>>> work for Hikey.
>>>>
>>>
>>> I'm wondering if I submitted this patch for the right path.
>>>
>>> Shall it go through arm-soc ?
>>
>> Yes, but I'm not sure this is the right patch either. We tend to not
>> use 'select' for user-visible drivers, and most hisilicon platforms
>> won't need this driver.
>>
>> I think it would be more consistent to add this to the defconfig
>> and regard it as a user error when the driver is disabled on a
>> machine that needs it.
>
> Maybe the select is not exactly in the right place, but I don't really
> feel like a pmic on an SoC is a "user-visible driver". I deal with the
> board often and when the new dependency was made on the clk, I would
> have never have found it on my own w/o Ulf and Daniel pointing out
> what I needed to enable.

What I meant is that the Kconfig option is user-visible. On a very high
level, this is a result of arch/arm64/Kconfig.platforms listing only
very broad categories of SoCs, in many cases only the manufacturers
of very different chip families, which then control the visibility of the
individual Kconfig items for things like pinctrl or clk.

I now see that MFD_HI655X_PMIC is the top-level driver that you
have to select before enabling COMMON_CLK_HI655X, so the
patch is actually broken unless it actually selects both.

How about simply adding a 'default MFD_HI655X_PMIC' to
COMMON_CLK_HI655X to enable it unless it is explicitly
turned off?

      Arnd

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Arnd Bergmann]

On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
>>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>> > With the addition of the hi655x common clock, the config option is missing
>>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
>>>> > the hi655x clock driver misses when initializing the power sequence via DT.
>>>> >
>>>> > Cc: John Stultz <john.stultz@linaro.org>
>>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>>> > Cc: Wei Xu <xuwei5@hisilicon.com>
>>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>
>>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
>>>> work for Hikey.
>>>>
>>>
>>> I'm wondering if I submitted this patch for the right path.
>>>
>>> Shall it go through arm-soc ?
>>
>> Yes, but I'm not sure this is the right patch either. We tend to not
>> use 'select' for user-visible drivers, and most hisilicon platforms
>> won't need this driver.
>>
>> I think it would be more consistent to add this to the defconfig
>> and regard it as a user error when the driver is disabled on a
>> machine that needs it.
>
> Maybe the select is not exactly in the right place, but I don't really
> feel like a pmic on an SoC is a "user-visible driver". I deal with the
> board often and when the new dependency was made on the clk, I would
> have never have found it on my own w/o Ulf and Daniel pointing out
> what I needed to enable.

What I meant is that the Kconfig option is user-visible. On a very high
level, this is a result of arch/arm64/Kconfig.platforms listing only
very broad categories of SoCs, in many cases only the manufacturers
of very different chip families, which then control the visibility of the
individual Kconfig items for things like pinctrl or clk.

I now see that MFD_HI655X_PMIC is the top-level driver that you
have to select before enabling COMMON_CLK_HI655X, so the
patch is actually broken unless it actually selects both.

How about simply adding a 'default MFD_HI655X_PMIC' to
COMMON_CLK_HI655X to enable it unless it is explicitly
turned off?

      Arnd

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
> > On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
> >> <daniel.lezcano@linaro.org> wrote:
> >>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
> >>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>>> > With the addition of the hi655x common clock, the config option is missing
> >>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
> >>>> > the hi655x clock driver misses when initializing the power sequence via DT.
> >>>> >
> >>>> > Cc: John Stultz <john.stultz@linaro.org>
> >>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >>>> > Cc: Wei Xu <xuwei5@hisilicon.com>
> >>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>>
> >>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>>>
> >>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
> >>>> work for Hikey.
> >>>>
> >>>
> >>> I'm wondering if I submitted this patch for the right path.
> >>>
> >>> Shall it go through arm-soc ?
> >>
> >> Yes, but I'm not sure this is the right patch either. We tend to not
> >> use 'select' for user-visible drivers, and most hisilicon platforms
> >> won't need this driver.
> >>
> >> I think it would be more consistent to add this to the defconfig
> >> and regard it as a user error when the driver is disabled on a
> >> machine that needs it.
> >
> > Maybe the select is not exactly in the right place, but I don't really
> > feel like a pmic on an SoC is a "user-visible driver". I deal with the
> > board often and when the new dependency was made on the clk, I would
> > have never have found it on my own w/o Ulf and Daniel pointing out
> > what I needed to enable.
> 
> What I meant is that the Kconfig option is user-visible. On a very high
> level, this is a result of arch/arm64/Kconfig.platforms listing only
> very broad categories of SoCs, in many cases only the manufacturers
> of very different chip families, which then control the visibility of the
> individual Kconfig items for things like pinctrl or clk.
> 
> I now see that MFD_HI655X_PMIC is the top-level driver that you
> have to select before enabling COMMON_CLK_HI655X, so the
> patch is actually broken unless it actually selects both.
> 
> How about simply adding a 'default MFD_HI655X_PMIC' to
> COMMON_CLK_HI655X to enable it unless it is explicitly
> turned off?

Actually, I share John's opinion.

Ideally when we choose a platform, all the relevants devices configuration
options should be selected automatically from a single topmost node of a tree
(platform selection) to all the nodes corresponding to the devices, leaving the
user to select one simple option without knowledge of the SoC hardware
internals.

If the user is expert in the platform and knows exactly what he does, then he
can select an _EXPERT_ like option and be able to disable some drivers.

It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
MFD_HI655X_PMIC is enabled?



-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
> > On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
> >> <daniel.lezcano@linaro.org> wrote:
> >>> On Tue, Jun 06, 2017 at 04:17:40PM +0200, Ulf Hansson wrote:
> >>>> On 5 June 2017 at 20:13, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>>> > With the addition of the hi655x common clock, the config option is missing
> >>>> > for the ARM64's hi6220 platform. That leads to a non functionnal WiFi because
> >>>> > the hi655x clock driver misses when initializing the power sequence via DT.
> >>>> >
> >>>> > Cc: John Stultz <john.stultz@linaro.org>
> >>>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >>>> > Cc: Wei Xu <xuwei5@hisilicon.com>
> >>>> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>>
> >>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>>>
> >>>> Would be nice to get this trivial fix in for 4.12 rcs to make the WiFi
> >>>> work for Hikey.
> >>>>
> >>>
> >>> I'm wondering if I submitted this patch for the right path.
> >>>
> >>> Shall it go through arm-soc ?
> >>
> >> Yes, but I'm not sure this is the right patch either. We tend to not
> >> use 'select' for user-visible drivers, and most hisilicon platforms
> >> won't need this driver.
> >>
> >> I think it would be more consistent to add this to the defconfig
> >> and regard it as a user error when the driver is disabled on a
> >> machine that needs it.
> >
> > Maybe the select is not exactly in the right place, but I don't really
> > feel like a pmic on an SoC is a "user-visible driver". I deal with the
> > board often and when the new dependency was made on the clk, I would
> > have never have found it on my own w/o Ulf and Daniel pointing out
> > what I needed to enable.
> 
> What I meant is that the Kconfig option is user-visible. On a very high
> level, this is a result of arch/arm64/Kconfig.platforms listing only
> very broad categories of SoCs, in many cases only the manufacturers
> of very different chip families, which then control the visibility of the
> individual Kconfig items for things like pinctrl or clk.
> 
> I now see that MFD_HI655X_PMIC is the top-level driver that you
> have to select before enabling COMMON_CLK_HI655X, so the
> patch is actually broken unless it actually selects both.
> 
> How about simply adding a 'default MFD_HI655X_PMIC' to
> COMMON_CLK_HI655X to enable it unless it is explicitly
> turned off?

Actually, I share John's opinion.

Ideally when we choose a platform, all the relevants devices configuration
options should be selected automatically from a single topmost node of a tree
(platform selection) to all the nodes corresponding to the devices, leaving the
user to select one simple option without knowledge of the SoC hardware
internals.

If the user is expert in the platform and knows exactly what he does, then he
can select an _EXPERT_ like option and be able to disable some drivers.

It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
MFD_HI655X_PMIC is enabled?



-- 

 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Arnd Bergmann]

On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>> > On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>> >> <daniel.lezcano@linaro.org> wrote:
>> >>
>> >> Yes, but I'm not sure this is the right patch either. We tend to not
>> >> use 'select' for user-visible drivers, and most hisilicon platforms
>> >> won't need this driver.
>> >>
>> >> I think it would be more consistent to add this to the defconfig
>> >> and regard it as a user error when the driver is disabled on a
>> >> machine that needs it.
>> >
>> > Maybe the select is not exactly in the right place, but I don't really
>> > feel like a pmic on an SoC is a "user-visible driver". I deal with the
>> > board often and when the new dependency was made on the clk, I would
>> > have never have found it on my own w/o Ulf and Daniel pointing out
>> > what I needed to enable.
>>
>> What I meant is that the Kconfig option is user-visible. On a very high
>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>> very broad categories of SoCs, in many cases only the manufacturers
>> of very different chip families, which then control the visibility of the
>> individual Kconfig items for things like pinctrl or clk.
>>
>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>> have to select before enabling COMMON_CLK_HI655X, so the
>> patch is actually broken unless it actually selects both.
>>
>> How about simply adding a 'default MFD_HI655X_PMIC' to
>> COMMON_CLK_HI655X to enable it unless it is explicitly
>> turned off?
>
> Actually, I share John's opinion.
>
> Ideally when we choose a platform, all the relevants devices configuration
> options should be selected automatically from a single topmost node of a tree
> (platform selection) to all the nodes corresponding to the devices, leaving the
> user to select one simple option without knowledge of the SoC hardware
> internals.
>
> If the user is expert in the platform and knows exactly what he does, then he
> can select an _EXPERT_ like option and be able to disable some drivers.
>
> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
> MFD_HI655X_PMIC is enabled?

I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
a dependency on COMMON_CLK and will again cause a warning on
machines that disable that during compile testing.

Using 'select' for user-selectable options generally leads to problems,
and you are better off avoiding it. If you want to make the symbol impossible
to turn off for non-EXPERT configurations, you can write it like

config COMMON_CLK_HI655X
        tristate "Clock driver for Hi655x" if EXPERT
        depends on (MFD_HI655X_PMIC || COMPILE_TEST)
        depends on REGMAP
        default MFD_HI655X_PMIC

That way the option is completely hidden for non-EXPERT,
but still has the right default otherwise, and the dependencies
are tracked right for compile-testing.

     Arnd

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Arnd Bergmann]

On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>> > On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>> >> <daniel.lezcano@linaro.org> wrote:
>> >>
>> >> Yes, but I'm not sure this is the right patch either. We tend to not
>> >> use 'select' for user-visible drivers, and most hisilicon platforms
>> >> won't need this driver.
>> >>
>> >> I think it would be more consistent to add this to the defconfig
>> >> and regard it as a user error when the driver is disabled on a
>> >> machine that needs it.
>> >
>> > Maybe the select is not exactly in the right place, but I don't really
>> > feel like a pmic on an SoC is a "user-visible driver". I deal with the
>> > board often and when the new dependency was made on the clk, I would
>> > have never have found it on my own w/o Ulf and Daniel pointing out
>> > what I needed to enable.
>>
>> What I meant is that the Kconfig option is user-visible. On a very high
>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>> very broad categories of SoCs, in many cases only the manufacturers
>> of very different chip families, which then control the visibility of the
>> individual Kconfig items for things like pinctrl or clk.
>>
>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>> have to select before enabling COMMON_CLK_HI655X, so the
>> patch is actually broken unless it actually selects both.
>>
>> How about simply adding a 'default MFD_HI655X_PMIC' to
>> COMMON_CLK_HI655X to enable it unless it is explicitly
>> turned off?
>
> Actually, I share John's opinion.
>
> Ideally when we choose a platform, all the relevants devices configuration
> options should be selected automatically from a single topmost node of a tree
> (platform selection) to all the nodes corresponding to the devices, leaving the
> user to select one simple option without knowledge of the SoC hardware
> internals.
>
> If the user is expert in the platform and knows exactly what he does, then he
> can select an _EXPERT_ like option and be able to disable some drivers.
>
> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
> MFD_HI655X_PMIC is enabled?

I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
a dependency on COMMON_CLK and will again cause a warning on
machines that disable that during compile testing.

Using 'select' for user-selectable options generally leads to problems,
and you are better off avoiding it. If you want to make the symbol impossible
to turn off for non-EXPERT configurations, you can write it like

config COMMON_CLK_HI655X
        tristate "Clock driver for Hi655x" if EXPERT
        depends on (MFD_HI655X_PMIC || COMPILE_TEST)
        depends on REGMAP
        default MFD_HI655X_PMIC

That way the option is completely hidden for non-EXPERT,
but still has the right default otherwise, and the dependencies
are tracked right for compile-testing.

     Arnd

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On 12/06/2017 23:12, Arnd Bergmann wrote:
> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>
>>>>> Yes, but I'm not sure this is the right patch either. We tend to not
>>>>> use 'select' for user-visible drivers, and most hisilicon platforms
>>>>> won't need this driver.
>>>>>
>>>>> I think it would be more consistent to add this to the defconfig
>>>>> and regard it as a user error when the driver is disabled on a
>>>>> machine that needs it.
>>>>
>>>> Maybe the select is not exactly in the right place, but I don't really
>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the
>>>> board often and when the new dependency was made on the clk, I would
>>>> have never have found it on my own w/o Ulf and Daniel pointing out
>>>> what I needed to enable.
>>>
>>> What I meant is that the Kconfig option is user-visible. On a very high
>>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>>> very broad categories of SoCs, in many cases only the manufacturers
>>> of very different chip families, which then control the visibility of the
>>> individual Kconfig items for things like pinctrl or clk.
>>>
>>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>>> have to select before enabling COMMON_CLK_HI655X, so the
>>> patch is actually broken unless it actually selects both.
>>>
>>> How about simply adding a 'default MFD_HI655X_PMIC' to
>>> COMMON_CLK_HI655X to enable it unless it is explicitly
>>> turned off?
>>
>> Actually, I share John's opinion.
>>
>> Ideally when we choose a platform, all the relevants devices configuration
>> options should be selected automatically from a single topmost node of a tree
>> (platform selection) to all the nodes corresponding to the devices, leaving the
>> user to select one simple option without knowledge of the SoC hardware
>> internals.
>>
>> If the user is expert in the platform and knows exactly what he does, then he
>> can select an _EXPERT_ like option and be able to disable some drivers.
>>
>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
>> MFD_HI655X_PMIC is enabled?
> 
> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
> a dependency on COMMON_CLK and will again cause a warning on
> machines that disable that during compile testing.

This issue is related to the missing stubs in the includes.

> Using 'select' for user-selectable options generally leads to problems,
> and you are better off avoiding it. If you want to make the symbol impossible
> to turn off for non-EXPERT configurations, you can write it like
> 
> config COMMON_CLK_HI655X
>         tristate "Clock driver for Hi655x" if EXPERT
>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
>         depends on REGMAP
>         default MFD_HI655X_PMIC
> 
> That way the option is completely hidden for non-EXPERT,
> but still has the right default otherwise, and the dependencies
> are tracked right for compile-testing.

Ok.

Thanks!

  -- Daniel


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On 12/06/2017 23:12, Arnd Bergmann wrote:
> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>
>>>>> Yes, but I'm not sure this is the right patch either. We tend to not
>>>>> use 'select' for user-visible drivers, and most hisilicon platforms
>>>>> won't need this driver.
>>>>>
>>>>> I think it would be more consistent to add this to the defconfig
>>>>> and regard it as a user error when the driver is disabled on a
>>>>> machine that needs it.
>>>>
>>>> Maybe the select is not exactly in the right place, but I don't really
>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the
>>>> board often and when the new dependency was made on the clk, I would
>>>> have never have found it on my own w/o Ulf and Daniel pointing out
>>>> what I needed to enable.
>>>
>>> What I meant is that the Kconfig option is user-visible. On a very high
>>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>>> very broad categories of SoCs, in many cases only the manufacturers
>>> of very different chip families, which then control the visibility of the
>>> individual Kconfig items for things like pinctrl or clk.
>>>
>>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>>> have to select before enabling COMMON_CLK_HI655X, so the
>>> patch is actually broken unless it actually selects both.
>>>
>>> How about simply adding a 'default MFD_HI655X_PMIC' to
>>> COMMON_CLK_HI655X to enable it unless it is explicitly
>>> turned off?
>>
>> Actually, I share John's opinion.
>>
>> Ideally when we choose a platform, all the relevants devices configuration
>> options should be selected automatically from a single topmost node of a tree
>> (platform selection) to all the nodes corresponding to the devices, leaving the
>> user to select one simple option without knowledge of the SoC hardware
>> internals.
>>
>> If the user is expert in the platform and knows exactly what he does, then he
>> can select an _EXPERT_ like option and be able to disable some drivers.
>>
>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
>> MFD_HI655X_PMIC is enabled?
> 
> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
> a dependency on COMMON_CLK and will again cause a warning on
> machines that disable that during compile testing.

This issue is related to the missing stubs in the includes.

> Using 'select' for user-selectable options generally leads to problems,
> and you are better off avoiding it. If you want to make the symbol impossible
> to turn off for non-EXPERT configurations, you can write it like
> 
> config COMMON_CLK_HI655X
>         tristate "Clock driver for Hi655x" if EXPERT
>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
>         depends on REGMAP
>         default MFD_HI655X_PMIC
> 
> That way the option is completely hidden for non-EXPERT,
> but still has the right default otherwise, and the dependencies
> are tracked right for compile-testing.

Ok.

Thanks!

  -- Daniel


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On 12/06/2017 23:12, Arnd Bergmann wrote:
> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>
>>>>> Yes, but I'm not sure this is the right patch either. We tend to not
>>>>> use 'select' for user-visible drivers, and most hisilicon platforms
>>>>> won't need this driver.
>>>>>
>>>>> I think it would be more consistent to add this to the defconfig
>>>>> and regard it as a user error when the driver is disabled on a
>>>>> machine that needs it.
>>>>
>>>> Maybe the select is not exactly in the right place, but I don't really
>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the
>>>> board often and when the new dependency was made on the clk, I would
>>>> have never have found it on my own w/o Ulf and Daniel pointing out
>>>> what I needed to enable.
>>>
>>> What I meant is that the Kconfig option is user-visible. On a very high
>>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>>> very broad categories of SoCs, in many cases only the manufacturers
>>> of very different chip families, which then control the visibility of the
>>> individual Kconfig items for things like pinctrl or clk.
>>>
>>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>>> have to select before enabling COMMON_CLK_HI655X, so the
>>> patch is actually broken unless it actually selects both.
>>>
>>> How about simply adding a 'default MFD_HI655X_PMIC' to
>>> COMMON_CLK_HI655X to enable it unless it is explicitly
>>> turned off?
>>
>> Actually, I share John's opinion.
>>
>> Ideally when we choose a platform, all the relevants devices configuration
>> options should be selected automatically from a single topmost node of a tree
>> (platform selection) to all the nodes corresponding to the devices, leaving the
>> user to select one simple option without knowledge of the SoC hardware
>> internals.
>>
>> If the user is expert in the platform and knows exactly what he does, then he
>> can select an _EXPERT_ like option and be able to disable some drivers.
>>
>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
>> MFD_HI655X_PMIC is enabled?
> 
> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
> a dependency on COMMON_CLK and will again cause a warning on
> machines that disable that during compile testing.
> 
> Using 'select' for user-selectable options generally leads to problems,
> and you are better off avoiding it. If you want to make the symbol impossible
> to turn off for non-EXPERT configurations, you can write it like
> 
> config COMMON_CLK_HI655X
>         tristate "Clock driver for Hi655x" if EXPERT
>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
>         depends on REGMAP
>         default MFD_HI655X_PMIC
> 
> That way the option is completely hidden for non-EXPERT,
> but still has the right default otherwise, and the dependencies
> are tracked right for compile-testing.

What about the options:

CONFIG_HI3660_MBOX
CONFIG_HI6220_MBOX

CONFIG_STUB_CLK_HI6220
CONFIG_STUB_CLK_HI3660

?

Would make sense to do something like:



diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index b9546ab..3a07dfe 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -517,7 +517,6 @@ CONFIG_COMMON_CLK_CS2000_CP=y
 CONFIG_COMMON_CLK_S2MPS11=y
 CONFIG_CLK_QORIQ=y
 CONFIG_COMMON_CLK_PWM=y
-CONFIG_STUB_CLK_HI3660=y
 CONFIG_COMMON_CLK_QCOM=y
 CONFIG_QCOM_CLK_SMD_RPM=y
 CONFIG_IPQ_GCC_8074=y
@@ -529,8 +528,6 @@ CONFIG_HWSPINLOCK_QCOM=y
 CONFIG_ARM_MHU=y
 CONFIG_PLATFORM_MHU=y
 CONFIG_BCM2835_MBOX=y
-CONFIG_HI3660_MBOX=y
-CONFIG_HI6220_MBOX=y
 CONFIG_ROCKCHIP_IOMMU=y
 CONFIG_ARM_SMMU=y
 CONFIG_ARM_SMMU_V3=y
diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
index 1bd4355..becdb1d 100644
--- a/drivers/clk/hisilicon/Kconfig
+++ b/drivers/clk/hisilicon/Kconfig
@@ -44,14 +44,17 @@ config RESET_HISI
 	  Build reset controller driver for HiSilicon device chipsets.

 config STUB_CLK_HI6220
-	bool "Hi6220 Stub Clock Driver"
-	depends on COMMON_CLK_HI6220 && MAILBOX
-	default ARCH_HISI
+	bool "Hi6220 Stub Clock Driver" if EXPERT
+	depends on (COMMON_CLK_HI6220 || COMPILE_TEST)
+	depends on MAILBOX
+	default COMMON_CLK_HI6220
 	help
 	  Build the Hisilicon Hi6220 stub clock driver.

 config STUB_CLK_HI3660
-	bool "Hi3660 Stub Clock Driver"
-	depends on COMMON_CLK_HI3660 && MAILBOX
+	bool "Hi3660 Stub Clock Driver" if EXPERT
+	depends on (COMMON_CLK_HI3660 || COMPILE_TEST)
+	depends on MAILBOX
+	default COMMON_CLK_HI3660
 	help
 	  Build the Hisilicon Hi3660 stub clock driver.
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index de8390d4..8d1726c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -109,16 +109,19 @@ config TI_MESSAGE_MANAGER
 	  platform has support for the hardware block.

 config HI3660_MBOX
-	tristate "Hi3660 Mailbox"
-	depends on ARCH_HISI && OF
+	tristate "Hi3660 Mailbox" if EXPERT
+	depends on (ARCH_HISI || COMPILE_TEST)
+	depends on OF
+	default ARCH_HISI
 	help
 	  An implementation of the hi3660 mailbox. It is used to send message
 	  between application processors and other processors/MCU/DSP. Select
 	  Y here if you want to use Hi3660 mailbox controller.

 config HI6220_MBOX
-	tristate "Hi6220 Mailbox"
-	depends on ARCH_HISI
+	tristate "Hi6220 Mailbox" if EXPERT
+	depends on (ARCH_HISI || COMPILE_TEST)
+	default ARCH_HISI
 	help
 	  An implementation of the hi6220 mailbox. It is used to send message
 	  between application processors and MCU. Say Y here if you want to




-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On 12/06/2017 23:12, Arnd Bergmann wrote:
> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>
>>>>> Yes, but I'm not sure this is the right patch either. We tend to not
>>>>> use 'select' for user-visible drivers, and most hisilicon platforms
>>>>> won't need this driver.
>>>>>
>>>>> I think it would be more consistent to add this to the defconfig
>>>>> and regard it as a user error when the driver is disabled on a
>>>>> machine that needs it.
>>>>
>>>> Maybe the select is not exactly in the right place, but I don't really
>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the
>>>> board often and when the new dependency was made on the clk, I would
>>>> have never have found it on my own w/o Ulf and Daniel pointing out
>>>> what I needed to enable.
>>>
>>> What I meant is that the Kconfig option is user-visible. On a very high
>>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>>> very broad categories of SoCs, in many cases only the manufacturers
>>> of very different chip families, which then control the visibility of the
>>> individual Kconfig items for things like pinctrl or clk.
>>>
>>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>>> have to select before enabling COMMON_CLK_HI655X, so the
>>> patch is actually broken unless it actually selects both.
>>>
>>> How about simply adding a 'default MFD_HI655X_PMIC' to
>>> COMMON_CLK_HI655X to enable it unless it is explicitly
>>> turned off?
>>
>> Actually, I share John's opinion.
>>
>> Ideally when we choose a platform, all the relevants devices configuration
>> options should be selected automatically from a single topmost node of a tree
>> (platform selection) to all the nodes corresponding to the devices, leaving the
>> user to select one simple option without knowledge of the SoC hardware
>> internals.
>>
>> If the user is expert in the platform and knows exactly what he does, then he
>> can select an _EXPERT_ like option and be able to disable some drivers.
>>
>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
>> MFD_HI655X_PMIC is enabled?
> 
> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
> a dependency on COMMON_CLK and will again cause a warning on
> machines that disable that during compile testing.
> 
> Using 'select' for user-selectable options generally leads to problems,
> and you are better off avoiding it. If you want to make the symbol impossible
> to turn off for non-EXPERT configurations, you can write it like
> 
> config COMMON_CLK_HI655X
>         tristate "Clock driver for Hi655x" if EXPERT
>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
>         depends on REGMAP
>         default MFD_HI655X_PMIC
> 
> That way the option is completely hidden for non-EXPERT,
> but still has the right default otherwise, and the dependencies
> are tracked right for compile-testing.

What about the options:

CONFIG_HI3660_MBOX
CONFIG_HI6220_MBOX

CONFIG_STUB_CLK_HI6220
CONFIG_STUB_CLK_HI3660

?

Would make sense to do something like:



diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index b9546ab..3a07dfe 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -517,7 +517,6 @@ CONFIG_COMMON_CLK_CS2000_CP=y
 CONFIG_COMMON_CLK_S2MPS11=y
 CONFIG_CLK_QORIQ=y
 CONFIG_COMMON_CLK_PWM=y
-CONFIG_STUB_CLK_HI3660=y
 CONFIG_COMMON_CLK_QCOM=y
 CONFIG_QCOM_CLK_SMD_RPM=y
 CONFIG_IPQ_GCC_8074=y
@@ -529,8 +528,6 @@ CONFIG_HWSPINLOCK_QCOM=y
 CONFIG_ARM_MHU=y
 CONFIG_PLATFORM_MHU=y
 CONFIG_BCM2835_MBOX=y
-CONFIG_HI3660_MBOX=y
-CONFIG_HI6220_MBOX=y
 CONFIG_ROCKCHIP_IOMMU=y
 CONFIG_ARM_SMMU=y
 CONFIG_ARM_SMMU_V3=y
diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
index 1bd4355..becdb1d 100644
--- a/drivers/clk/hisilicon/Kconfig
+++ b/drivers/clk/hisilicon/Kconfig
@@ -44,14 +44,17 @@ config RESET_HISI
 	  Build reset controller driver for HiSilicon device chipsets.

 config STUB_CLK_HI6220
-	bool "Hi6220 Stub Clock Driver"
-	depends on COMMON_CLK_HI6220 && MAILBOX
-	default ARCH_HISI
+	bool "Hi6220 Stub Clock Driver" if EXPERT
+	depends on (COMMON_CLK_HI6220 || COMPILE_TEST)
+	depends on MAILBOX
+	default COMMON_CLK_HI6220
 	help
 	  Build the Hisilicon Hi6220 stub clock driver.

 config STUB_CLK_HI3660
-	bool "Hi3660 Stub Clock Driver"
-	depends on COMMON_CLK_HI3660 && MAILBOX
+	bool "Hi3660 Stub Clock Driver" if EXPERT
+	depends on (COMMON_CLK_HI3660 || COMPILE_TEST)
+	depends on MAILBOX
+	default COMMON_CLK_HI3660
 	help
 	  Build the Hisilicon Hi3660 stub clock driver.
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index de8390d4..8d1726c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -109,16 +109,19 @@ config TI_MESSAGE_MANAGER
 	  platform has support for the hardware block.

 config HI3660_MBOX
-	tristate "Hi3660 Mailbox"
-	depends on ARCH_HISI && OF
+	tristate "Hi3660 Mailbox" if EXPERT
+	depends on (ARCH_HISI || COMPILE_TEST)
+	depends on OF
+	default ARCH_HISI
 	help
 	  An implementation of the hi3660 mailbox. It is used to send message
 	  between application processors and other processors/MCU/DSP. Select
 	  Y here if you want to use Hi3660 mailbox controller.

 config HI6220_MBOX
-	tristate "Hi6220 Mailbox"
-	depends on ARCH_HISI
+	tristate "Hi6220 Mailbox" if EXPERT
+	depends on (ARCH_HISI || COMPILE_TEST)
+	default ARCH_HISI
 	help
 	  An implementation of the hi6220 mailbox. It is used to send message
 	  between application processors and MCU. Say Y here if you want to




-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Riku Voipio]

On 16 February 2018 at 19:35, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 12/06/2017 23:12, Arnd Bergmann wrote:
>> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>
>>>>>> Yes, but I'm not sure this is the right patch either. We tend to not
>>>>>> use 'select' for user-visible drivers, and most hisilicon platforms
>>>>>> won't need this driver.
>>>>>>
>>>>>> I think it would be more consistent to add this to the defconfig
>>>>>> and regard it as a user error when the driver is disabled on a
>>>>>> machine that needs it.
>>>>>
>>>>> Maybe the select is not exactly in the right place, but I don't really
>>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the
>>>>> board often and when the new dependency was made on the clk, I would
>>>>> have never have found it on my own w/o Ulf and Daniel pointing out
>>>>> what I needed to enable.
>>>>
>>>> What I meant is that the Kconfig option is user-visible. On a very high
>>>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>>>> very broad categories of SoCs, in many cases only the manufacturers
>>>> of very different chip families, which then control the visibility of the
>>>> individual Kconfig items for things like pinctrl or clk.
>>>>
>>>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>>>> have to select before enabling COMMON_CLK_HI655X, so the
>>>> patch is actually broken unless it actually selects both.
>>>>
>>>> How about simply adding a 'default MFD_HI655X_PMIC' to
>>>> COMMON_CLK_HI655X to enable it unless it is explicitly
>>>> turned off?
>>>
>>> Actually, I share John's opinion.
>>>
>>> Ideally when we choose a platform, all the relevants devices configuration
>>> options should be selected automatically from a single topmost node of a tree
>>> (platform selection) to all the nodes corresponding to the devices, leaving the
>>> user to select one simple option without knowledge of the SoC hardware
>>> internals.
>>>
>>> If the user is expert in the platform and knows exactly what he does, then he
>>> can select an _EXPERT_ like option and be able to disable some drivers.
>>>
>>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
>>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
>>> MFD_HI655X_PMIC is enabled?
>>
>> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
>> a dependency on COMMON_CLK and will again cause a warning on
>> machines that disable that during compile testing.
>>
>> Using 'select' for user-selectable options generally leads to problems,
>> and you are better off avoiding it. If you want to make the symbol impossible
>> to turn off for non-EXPERT configurations, you can write it like
>>
>> config COMMON_CLK_HI655X
>>         tristate "Clock driver for Hi655x" if EXPERT
>>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
>>         depends on REGMAP
>>         default MFD_HI655X_PMIC
>>
>> That way the option is completely hidden for non-EXPERT,
>> but still has the right default otherwise, and the dependencies
>> are tracked right for compile-testing.
>
> What about the options:

First, as distros, automatic selection down from selecting ARCH_X is
preferred over
defconfigs. However, we also prefer to build everything possible as
modules, so "default Y"
is sometimes too strong.

> CONFIG_HI3660_MBOX
> CONFIG_HI6220_MBOX

These are tristate and platorms can boot without them.

> CONFIG_STUB_CLK_HI6220
> CONFIG_STUB_CLK_HI3660

These are bool, so default Y is ok.

> Would make sense to do something like:

>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index b9546ab..3a07dfe 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -517,7 +517,6 @@ CONFIG_COMMON_CLK_CS2000_CP=y
>  CONFIG_COMMON_CLK_S2MPS11=y
>  CONFIG_CLK_QORIQ=y
>  CONFIG_COMMON_CLK_PWM=y
> -CONFIG_STUB_CLK_HI3660=y
>  CONFIG_COMMON_CLK_QCOM=y
>  CONFIG_QCOM_CLK_SMD_RPM=y
>  CONFIG_IPQ_GCC_8074=y
> @@ -529,8 +528,6 @@ CONFIG_HWSPINLOCK_QCOM=y
>  CONFIG_ARM_MHU=y
>  CONFIG_PLATFORM_MHU=y
>  CONFIG_BCM2835_MBOX=y
> -CONFIG_HI3660_MBOX=y
> -CONFIG_HI6220_MBOX=y
>  CONFIG_ROCKCHIP_IOMMU=y
>  CONFIG_ARM_SMMU=y
>  CONFIG_ARM_SMMU_V3=y
> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
> index 1bd4355..becdb1d 100644
> --- a/drivers/clk/hisilicon/Kconfig
> +++ b/drivers/clk/hisilicon/Kconfig
> @@ -44,14 +44,17 @@ config RESET_HISI
>           Build reset controller driver for HiSilicon device chipsets.
>
>  config STUB_CLK_HI6220
> -       bool "Hi6220 Stub Clock Driver"
> -       depends on COMMON_CLK_HI6220 && MAILBOX
> -       default ARCH_HISI
> +       bool "Hi6220 Stub Clock Driver" if EXPERT
> +       depends on (COMMON_CLK_HI6220 || COMPILE_TEST)
> +       depends on MAILBOX
> +       default COMMON_CLK_HI6220
>         help
>           Build the Hisilicon Hi6220 stub clock driver.
>
>  config STUB_CLK_HI3660
> -       bool "Hi3660 Stub Clock Driver"
> -       depends on COMMON_CLK_HI3660 && MAILBOX
> +       bool "Hi3660 Stub Clock Driver" if EXPERT
> +       depends on (COMMON_CLK_HI3660 || COMPILE_TEST)
> +       depends on MAILBOX
> +       default COMMON_CLK_HI3660
>         help
>           Build the Hisilicon Hi3660 stub clock driver.
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index de8390d4..8d1726c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -109,16 +109,19 @@ config TI_MESSAGE_MANAGER
>           platform has support for the hardware block.
>
>  config HI3660_MBOX
> -       tristate "Hi3660 Mailbox"
> -       depends on ARCH_HISI && OF
> +       tristate "Hi3660 Mailbox" if EXPERT
> +       depends on (ARCH_HISI || COMPILE_TEST)
> +       depends on OF
> +       default ARCH_HISI
>         help
>           An implementation of the hi3660 mailbox. It is used to send message
>           between application processors and other processors/MCU/DSP. Select
>           Y here if you want to use Hi3660 mailbox controller.

Which kernel tree is this from? I don't see this driver in mainline.

>
>  config HI6220_MBOX
> -       tristate "Hi6220 Mailbox"
> -       depends on ARCH_HISI
> +       tristate "Hi6220 Mailbox" if EXPERT
> +       depends on (ARCH_HISI || COMPILE_TEST)
> +       default ARCH_HISI
>         help
>           An implementation of the hi6220 mailbox. It is used to send message
>           between application processors and MCU. Say Y here if you want to
>
>
>
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Riku Voipio]

On 16 February 2018 at 19:35, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 12/06/2017 23:12, Arnd Bergmann wrote:
>> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>
>>>>>> Yes, but I'm not sure this is the right patch either. We tend to not
>>>>>> use 'select' for user-visible drivers, and most hisilicon platforms
>>>>>> won't need this driver.
>>>>>>
>>>>>> I think it would be more consistent to add this to the defconfig
>>>>>> and regard it as a user error when the driver is disabled on a
>>>>>> machine that needs it.
>>>>>
>>>>> Maybe the select is not exactly in the right place, but I don't really
>>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the
>>>>> board often and when the new dependency was made on the clk, I would
>>>>> have never have found it on my own w/o Ulf and Daniel pointing out
>>>>> what I needed to enable.
>>>>
>>>> What I meant is that the Kconfig option is user-visible. On a very high
>>>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>>>> very broad categories of SoCs, in many cases only the manufacturers
>>>> of very different chip families, which then control the visibility of the
>>>> individual Kconfig items for things like pinctrl or clk.
>>>>
>>>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>>>> have to select before enabling COMMON_CLK_HI655X, so the
>>>> patch is actually broken unless it actually selects both.
>>>>
>>>> How about simply adding a 'default MFD_HI655X_PMIC' to
>>>> COMMON_CLK_HI655X to enable it unless it is explicitly
>>>> turned off?
>>>
>>> Actually, I share John's opinion.
>>>
>>> Ideally when we choose a platform, all the relevants devices configuration
>>> options should be selected automatically from a single topmost node of a tree
>>> (platform selection) to all the nodes corresponding to the devices, leaving the
>>> user to select one simple option without knowledge of the SoC hardware
>>> internals.
>>>
>>> If the user is expert in the platform and knows exactly what he does, then he
>>> can select an _EXPERT_ like option and be able to disable some drivers.
>>>
>>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
>>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
>>> MFD_HI655X_PMIC is enabled?
>>
>> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
>> a dependency on COMMON_CLK and will again cause a warning on
>> machines that disable that during compile testing.
>>
>> Using 'select' for user-selectable options generally leads to problems,
>> and you are better off avoiding it. If you want to make the symbol impossible
>> to turn off for non-EXPERT configurations, you can write it like
>>
>> config COMMON_CLK_HI655X
>>         tristate "Clock driver for Hi655x" if EXPERT
>>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
>>         depends on REGMAP
>>         default MFD_HI655X_PMIC
>>
>> That way the option is completely hidden for non-EXPERT,
>> but still has the right default otherwise, and the dependencies
>> are tracked right for compile-testing.
>
> What about the options:

First, as distros, automatic selection down from selecting ARCH_X is
preferred over
defconfigs. However, we also prefer to build everything possible as
modules, so "default Y"
is sometimes too strong.

> CONFIG_HI3660_MBOX
> CONFIG_HI6220_MBOX

These are tristate and platorms can boot without them.

> CONFIG_STUB_CLK_HI6220
> CONFIG_STUB_CLK_HI3660

These are bool, so default Y is ok.

> Would make sense to do something like:

>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index b9546ab..3a07dfe 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -517,7 +517,6 @@ CONFIG_COMMON_CLK_CS2000_CP=y
>  CONFIG_COMMON_CLK_S2MPS11=y
>  CONFIG_CLK_QORIQ=y
>  CONFIG_COMMON_CLK_PWM=y
> -CONFIG_STUB_CLK_HI3660=y
>  CONFIG_COMMON_CLK_QCOM=y
>  CONFIG_QCOM_CLK_SMD_RPM=y
>  CONFIG_IPQ_GCC_8074=y
> @@ -529,8 +528,6 @@ CONFIG_HWSPINLOCK_QCOM=y
>  CONFIG_ARM_MHU=y
>  CONFIG_PLATFORM_MHU=y
>  CONFIG_BCM2835_MBOX=y
> -CONFIG_HI3660_MBOX=y
> -CONFIG_HI6220_MBOX=y
>  CONFIG_ROCKCHIP_IOMMU=y
>  CONFIG_ARM_SMMU=y
>  CONFIG_ARM_SMMU_V3=y
> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
> index 1bd4355..becdb1d 100644
> --- a/drivers/clk/hisilicon/Kconfig
> +++ b/drivers/clk/hisilicon/Kconfig
> @@ -44,14 +44,17 @@ config RESET_HISI
>           Build reset controller driver for HiSilicon device chipsets.
>
>  config STUB_CLK_HI6220
> -       bool "Hi6220 Stub Clock Driver"
> -       depends on COMMON_CLK_HI6220 && MAILBOX
> -       default ARCH_HISI
> +       bool "Hi6220 Stub Clock Driver" if EXPERT
> +       depends on (COMMON_CLK_HI6220 || COMPILE_TEST)
> +       depends on MAILBOX
> +       default COMMON_CLK_HI6220
>         help
>           Build the Hisilicon Hi6220 stub clock driver.
>
>  config STUB_CLK_HI3660
> -       bool "Hi3660 Stub Clock Driver"
> -       depends on COMMON_CLK_HI3660 && MAILBOX
> +       bool "Hi3660 Stub Clock Driver" if EXPERT
> +       depends on (COMMON_CLK_HI3660 || COMPILE_TEST)
> +       depends on MAILBOX
> +       default COMMON_CLK_HI3660
>         help
>           Build the Hisilicon Hi3660 stub clock driver.
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index de8390d4..8d1726c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -109,16 +109,19 @@ config TI_MESSAGE_MANAGER
>           platform has support for the hardware block.
>
>  config HI3660_MBOX
> -       tristate "Hi3660 Mailbox"
> -       depends on ARCH_HISI && OF
> +       tristate "Hi3660 Mailbox" if EXPERT
> +       depends on (ARCH_HISI || COMPILE_TEST)
> +       depends on OF
> +       default ARCH_HISI
>         help
>           An implementation of the hi3660 mailbox. It is used to send message
>           between application processors and other processors/MCU/DSP. Select
>           Y here if you want to use Hi3660 mailbox controller.

Which kernel tree is this from? I don't see this driver in mainline.

>
>  config HI6220_MBOX
> -       tristate "Hi6220 Mailbox"
> -       depends on ARCH_HISI
> +       tristate "Hi6220 Mailbox" if EXPERT
> +       depends on (ARCH_HISI || COMPILE_TEST)
> +       default ARCH_HISI
>         help
>           An implementation of the hi6220 mailbox. It is used to send message
>           between application processors and MCU. Say Y here if you want to
>
>
>
>
> --
>  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On 21/02/2018 11:30, Riku Voipio wrote:
> On 16 February 2018 at 19:35, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> On 12/06/2017 23:12, Arnd Bergmann wrote:
>>> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>>>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>>
>>>>>>> Yes, but I'm not sure this is the right patch either. We tend to not
>>>>>>> use 'select' for user-visible drivers, and most hisilicon platforms
>>>>>>> won't need this driver.
>>>>>>>
>>>>>>> I think it would be more consistent to add this to the defconfig
>>>>>>> and regard it as a user error when the driver is disabled on a
>>>>>>> machine that needs it.
>>>>>>
>>>>>> Maybe the select is not exactly in the right place, but I don't really
>>>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the
>>>>>> board often and when the new dependency was made on the clk, I would
>>>>>> have never have found it on my own w/o Ulf and Daniel pointing out
>>>>>> what I needed to enable.
>>>>>
>>>>> What I meant is that the Kconfig option is user-visible. On a very high
>>>>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>>>>> very broad categories of SoCs, in many cases only the manufacturers
>>>>> of very different chip families, which then control the visibility of the
>>>>> individual Kconfig items for things like pinctrl or clk.
>>>>>
>>>>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>>>>> have to select before enabling COMMON_CLK_HI655X, so the
>>>>> patch is actually broken unless it actually selects both.
>>>>>
>>>>> How about simply adding a 'default MFD_HI655X_PMIC' to
>>>>> COMMON_CLK_HI655X to enable it unless it is explicitly
>>>>> turned off?
>>>>
>>>> Actually, I share John's opinion.
>>>>
>>>> Ideally when we choose a platform, all the relevants devices configuration
>>>> options should be selected automatically from a single topmost node of a tree
>>>> (platform selection) to all the nodes corresponding to the devices, leaving the
>>>> user to select one simple option without knowledge of the SoC hardware
>>>> internals.
>>>>
>>>> If the user is expert in the platform and knows exactly what he does, then he
>>>> can select an _EXPERT_ like option and be able to disable some drivers.
>>>>
>>>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
>>>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
>>>> MFD_HI655X_PMIC is enabled?
>>>
>>> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
>>> a dependency on COMMON_CLK and will again cause a warning on
>>> machines that disable that during compile testing.
>>>
>>> Using 'select' for user-selectable options generally leads to problems,
>>> and you are better off avoiding it. If you want to make the symbol impossible
>>> to turn off for non-EXPERT configurations, you can write it like
>>>
>>> config COMMON_CLK_HI655X
>>>         tristate "Clock driver for Hi655x" if EXPERT
>>>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
>>>         depends on REGMAP
>>>         default MFD_HI655X_PMIC
>>>
>>> That way the option is completely hidden for non-EXPERT,
>>> but still has the right default otherwise, and the dependencies
>>> are tracked right for compile-testing.
>>
>> What about the options:
> 
> First, as distros, automatic selection down from selecting ARCH_X is
> preferred over
> defconfigs. However, we also prefer to build everything possible as
> modules, so "default Y"
> is sometimes too strong.
> 
>> CONFIG_HI3660_MBOX
>> CONFIG_HI6220_MBOX
> 
> These are tristate and platorms can boot without them.
> 
>> CONFIG_STUB_CLK_HI6220
>> CONFIG_STUB_CLK_HI3660
> 
> These are bool, so default Y is ok.
> 
>> Would make sense to do something like:
> 
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index b9546ab..3a07dfe 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -517,7 +517,6 @@ CONFIG_COMMON_CLK_CS2000_CP=y
>>  CONFIG_COMMON_CLK_S2MPS11=y
>>  CONFIG_CLK_QORIQ=y
>>  CONFIG_COMMON_CLK_PWM=y
>> -CONFIG_STUB_CLK_HI3660=y
>>  CONFIG_COMMON_CLK_QCOM=y
>>  CONFIG_QCOM_CLK_SMD_RPM=y
>>  CONFIG_IPQ_GCC_8074=y
>> @@ -529,8 +528,6 @@ CONFIG_HWSPINLOCK_QCOM=y
>>  CONFIG_ARM_MHU=y
>>  CONFIG_PLATFORM_MHU=y
>>  CONFIG_BCM2835_MBOX=y
>> -CONFIG_HI3660_MBOX=y
>> -CONFIG_HI6220_MBOX=y
>>  CONFIG_ROCKCHIP_IOMMU=y
>>  CONFIG_ARM_SMMU=y
>>  CONFIG_ARM_SMMU_V3=y
>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> index 1bd4355..becdb1d 100644
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
>> @@ -44,14 +44,17 @@ config RESET_HISI
>>           Build reset controller driver for HiSilicon device chipsets.
>>
>>  config STUB_CLK_HI6220
>> -       bool "Hi6220 Stub Clock Driver"
>> -       depends on COMMON_CLK_HI6220 && MAILBOX
>> -       default ARCH_HISI
>> +       bool "Hi6220 Stub Clock Driver" if EXPERT
>> +       depends on (COMMON_CLK_HI6220 || COMPILE_TEST)
>> +       depends on MAILBOX
>> +       default COMMON_CLK_HI6220
>>         help
>>           Build the Hisilicon Hi6220 stub clock driver.
>>
>>  config STUB_CLK_HI3660
>> -       bool "Hi3660 Stub Clock Driver"
>> -       depends on COMMON_CLK_HI3660 && MAILBOX
>> +       bool "Hi3660 Stub Clock Driver" if EXPERT
>> +       depends on (COMMON_CLK_HI3660 || COMPILE_TEST)
>> +       depends on MAILBOX
>> +       default COMMON_CLK_HI3660
>>         help
>>           Build the Hisilicon Hi3660 stub clock driver.
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index de8390d4..8d1726c 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -109,16 +109,19 @@ config TI_MESSAGE_MANAGER
>>           platform has support for the hardware block.
>>
>>  config HI3660_MBOX
>> -       tristate "Hi3660 Mailbox"
>> -       depends on ARCH_HISI && OF
>> +       tristate "Hi3660 Mailbox" if EXPERT
>> +       depends on (ARCH_HISI || COMPILE_TEST)
>> +       depends on OF
>> +       default ARCH_HISI
>>         help
>>           An implementation of the hi3660 mailbox. It is used to send message
>>           between application processors and other processors/MCU/DSP. Select
>>           Y here if you want to use Hi3660 mailbox controller.
> 
> Which kernel tree is this from? I don't see this driver in mainline.

Yes, that's right. The HI6220 part is ok but the HI3660 is still not
mainline yet.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] ARM64: Kconfig: Fix the missing hi655x common clk

[Daniel Lezcano]

On 21/02/2018 11:30, Riku Voipio wrote:
> On 16 February 2018 at 19:35, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> On 12/06/2017 23:12, Arnd Bergmann wrote:
>>> On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote:
>>>>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano
>>>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>>
>>>>>>> Yes, but I'm not sure this is the right patch either. We tend to not
>>>>>>> use 'select' for user-visible drivers, and most hisilicon platforms
>>>>>>> won't need this driver.
>>>>>>>
>>>>>>> I think it would be more consistent to add this to the defconfig
>>>>>>> and regard it as a user error when the driver is disabled on a
>>>>>>> machine that needs it.
>>>>>>
>>>>>> Maybe the select is not exactly in the right place, but I don't really
>>>>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the
>>>>>> board often and when the new dependency was made on the clk, I would
>>>>>> have never have found it on my own w/o Ulf and Daniel pointing out
>>>>>> what I needed to enable.
>>>>>
>>>>> What I meant is that the Kconfig option is user-visible. On a very high
>>>>> level, this is a result of arch/arm64/Kconfig.platforms listing only
>>>>> very broad categories of SoCs, in many cases only the manufacturers
>>>>> of very different chip families, which then control the visibility of the
>>>>> individual Kconfig items for things like pinctrl or clk.
>>>>>
>>>>> I now see that MFD_HI655X_PMIC is the top-level driver that you
>>>>> have to select before enabling COMMON_CLK_HI655X, so the
>>>>> patch is actually broken unless it actually selects both.
>>>>>
>>>>> How about simply adding a 'default MFD_HI655X_PMIC' to
>>>>> COMMON_CLK_HI655X to enable it unless it is explicitly
>>>>> turned off?
>>>>
>>>> Actually, I share John's opinion.
>>>>
>>>> Ideally when we choose a platform, all the relevants devices configuration
>>>> options should be selected automatically from a single topmost node of a tree
>>>> (platform selection) to all the nodes corresponding to the devices, leaving the
>>>> user to select one simple option without knowledge of the SoC hardware
>>>> internals.
>>>>
>>>> If the user is expert in the platform and knows exactly what he does, then he
>>>> can select an _EXPERT_ like option and be able to disable some drivers.
>>>>
>>>> It is how I tend to write the Kconfig options, so the 'default MFD_HI655X_PMIC'
>>>> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when
>>>> MFD_HI655X_PMIC is enabled?
>>>
>>> I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains
>>> a dependency on COMMON_CLK and will again cause a warning on
>>> machines that disable that during compile testing.
>>>
>>> Using 'select' for user-selectable options generally leads to problems,
>>> and you are better off avoiding it. If you want to make the symbol impossible
>>> to turn off for non-EXPERT configurations, you can write it like
>>>
>>> config COMMON_CLK_HI655X
>>>         tristate "Clock driver for Hi655x" if EXPERT
>>>         depends on (MFD_HI655X_PMIC || COMPILE_TEST)
>>>         depends on REGMAP
>>>         default MFD_HI655X_PMIC
>>>
>>> That way the option is completely hidden for non-EXPERT,
>>> but still has the right default otherwise, and the dependencies
>>> are tracked right for compile-testing.
>>
>> What about the options:
> 
> First, as distros, automatic selection down from selecting ARCH_X is
> preferred over
> defconfigs. However, we also prefer to build everything possible as
> modules, so "default Y"
> is sometimes too strong.
> 
>> CONFIG_HI3660_MBOX
>> CONFIG_HI6220_MBOX
> 
> These are tristate and platorms can boot without them.
> 
>> CONFIG_STUB_CLK_HI6220
>> CONFIG_STUB_CLK_HI3660
> 
> These are bool, so default Y is ok.
> 
>> Would make sense to do something like:
> 
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index b9546ab..3a07dfe 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -517,7 +517,6 @@ CONFIG_COMMON_CLK_CS2000_CP=y
>>  CONFIG_COMMON_CLK_S2MPS11=y
>>  CONFIG_CLK_QORIQ=y
>>  CONFIG_COMMON_CLK_PWM=y
>> -CONFIG_STUB_CLK_HI3660=y
>>  CONFIG_COMMON_CLK_QCOM=y
>>  CONFIG_QCOM_CLK_SMD_RPM=y
>>  CONFIG_IPQ_GCC_8074=y
>> @@ -529,8 +528,6 @@ CONFIG_HWSPINLOCK_QCOM=y
>>  CONFIG_ARM_MHU=y
>>  CONFIG_PLATFORM_MHU=y
>>  CONFIG_BCM2835_MBOX=y
>> -CONFIG_HI3660_MBOX=y
>> -CONFIG_HI6220_MBOX=y
>>  CONFIG_ROCKCHIP_IOMMU=y
>>  CONFIG_ARM_SMMU=y
>>  CONFIG_ARM_SMMU_V3=y
>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> index 1bd4355..becdb1d 100644
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
>> @@ -44,14 +44,17 @@ config RESET_HISI
>>           Build reset controller driver for HiSilicon device chipsets.
>>
>>  config STUB_CLK_HI6220
>> -       bool "Hi6220 Stub Clock Driver"
>> -       depends on COMMON_CLK_HI6220 && MAILBOX
>> -       default ARCH_HISI
>> +       bool "Hi6220 Stub Clock Driver" if EXPERT
>> +       depends on (COMMON_CLK_HI6220 || COMPILE_TEST)
>> +       depends on MAILBOX
>> +       default COMMON_CLK_HI6220
>>         help
>>           Build the Hisilicon Hi6220 stub clock driver.
>>
>>  config STUB_CLK_HI3660
>> -       bool "Hi3660 Stub Clock Driver"
>> -       depends on COMMON_CLK_HI3660 && MAILBOX
>> +       bool "Hi3660 Stub Clock Driver" if EXPERT
>> +       depends on (COMMON_CLK_HI3660 || COMPILE_TEST)
>> +       depends on MAILBOX
>> +       default COMMON_CLK_HI3660
>>         help
>>           Build the Hisilicon Hi3660 stub clock driver.
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index de8390d4..8d1726c 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -109,16 +109,19 @@ config TI_MESSAGE_MANAGER
>>           platform has support for the hardware block.
>>
>>  config HI3660_MBOX
>> -       tristate "Hi3660 Mailbox"
>> -       depends on ARCH_HISI && OF
>> +       tristate "Hi3660 Mailbox" if EXPERT
>> +       depends on (ARCH_HISI || COMPILE_TEST)
>> +       depends on OF
>> +       default ARCH_HISI
>>         help
>>           An implementation of the hi3660 mailbox. It is used to send message
>>           between application processors and other processors/MCU/DSP. Select
>>           Y here if you want to use Hi3660 mailbox controller.
> 
> Which kernel tree is this from? I don't see this driver in mainline.

Yes, that's right. The HI6220 part is ok but the HI3660 is still not
mainline yet.


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog