* [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support @ 2017-05-08 16:21 Ulf Hansson 2017-05-08 16:21 ` Ulf Hansson ` (8 more replies) 0 siblings, 9 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson @ 2017-05-08 16:21 ` Ulf Hansson 2017-05-08 16:21 ` Ulf Hansson ` (7 subsequent siblings) 8 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: Wei Xu, linux-arm-kernel Cc: Ulf Hansson, Daniel Lezcano, devicetree, Rob Herring, linux-mmc 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us @ 2017-05-08 16:21 ` Ulf Hansson 0 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
[parent not found: <1494260477-25163-2-git-send-email-ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]
* Re: [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us 2017-05-08 16:21 ` Ulf Hansson @ 2017-05-12 20:03 ` Rob Herring -1 siblings, 0 replies; 59+ messages in thread From: Rob Herring @ 2017-05-12 20:03 UTC (permalink / raw) To: Ulf Hansson Cc: Wei Xu, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Daniel Lezcano, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-mmc-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us @ 2017-05-12 20:03 ` Rob Herring 0 siblings, 0 replies; 59+ messages in thread From: Rob Herring @ 2017-05-12 20:03 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us 2017-05-12 20:03 ` Rob Herring @ 2017-05-15 11:08 ` Ulf Hansson -1 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-15 11:08 UTC (permalink / raw) To: Rob Herring Cc: Wei Xu, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Daniel Lezcano, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-mmc-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us @ 2017-05-15 11:08 ` Ulf Hansson 0 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-15 11:08 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
[parent not found: <CAPDyKFqhbGqanoQqFsLrLaBXsQ5WdFrrunkvB2eOoJqH+W6jdQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us 2017-05-15 11:08 ` Ulf Hansson @ 2017-05-15 16:16 ` Rob Herring -1 siblings, 0 replies; 59+ messages in thread From: Rob Herring @ 2017-05-15 16:16 UTC (permalink / raw) To: Ulf Hansson Cc: Wei Xu, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Daniel Lezcano, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-mmc-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us @ 2017-05-15 16:16 ` Rob Herring 0 siblings, 0 replies; 59+ messages in thread From: Rob Herring @ 2017-05-15 16:16 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us 2017-05-15 16:16 ` Rob Herring @ 2017-05-16 7:06 ` Ulf Hansson -1 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-16 7:06 UTC (permalink / raw) To: Rob Herring Cc: Wei Xu, linux-arm-kernel, Daniel Lezcano, devicetree, linux-mmc 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us @ 2017-05-16 7:06 ` Ulf Hansson 0 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-16 7:06 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/8] mmc: pwrseq_simple: Parse DTS for the power-off-delay-us property 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson @ 2017-05-08 16:21 ` Ulf Hansson 2017-05-08 16:21 ` Ulf Hansson ` (7 subsequent siblings) 8 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: Wei Xu, linux-arm-kernel; +Cc: Ulf Hansson, Daniel Lezcano, linux-mmc 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 2/8] mmc: pwrseq_simple: Parse DTS for the power-off-delay-us property @ 2017-05-08 16:21 ` Ulf Hansson 0 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 3/8] mfd: dts: hi655x: Add clock binding for the pmic 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson 2017-05-08 16:21 ` Ulf Hansson 2017-05-08 16:21 ` Ulf Hansson @ 2017-05-08 16:21 ` Ulf Hansson 2017-05-08 16:21 ` [PATCH 4/8] arm64: dts: hikey: Add clock for the pmic mfd Ulf Hansson ` (5 subsequent siblings) 8 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 4/8] arm64: dts: hikey: Add clock for the pmic mfd 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson ` (2 preceding siblings ...) 2017-05-08 16:21 ` [PATCH 3/8] mfd: dts: hi655x: Add clock binding for the pmic Ulf Hansson @ 2017-05-08 16:21 ` Ulf Hansson 2017-05-08 16:21 ` [PATCH 5/8] arm64: dts: hi6220: Move the fixed_5v_hub regulator to the hikey dts Ulf Hansson ` (4 subsequent siblings) 8 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 5/8] arm64: dts: hi6220: Move the fixed_5v_hub regulator to the hikey dts 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson ` (3 preceding siblings ...) 2017-05-08 16:21 ` [PATCH 4/8] arm64: dts: hikey: Add clock for the pmic mfd Ulf Hansson @ 2017-05-08 16:21 ` Ulf Hansson 2017-05-08 16:21 ` [PATCH 6/8] arm64: dts: hikey: Add the SYS_5V and the VDD_3V3 regulators Ulf Hansson ` (3 subsequent siblings) 8 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 = <®_5v_hub>; hisilicon,peripheral-syscon = <&sys_ctrl>; }; -- 2.7.4 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 6/8] arm64: dts: hikey: Add the SYS_5V and the VDD_3V3 regulators 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson ` (4 preceding siblings ...) 2017-05-08 16:21 ` [PATCH 5/8] arm64: dts: hi6220: Move the fixed_5v_hub regulator to the hikey dts Ulf Hansson @ 2017-05-08 16:21 ` Ulf Hansson 2017-05-08 16:21 ` [PATCH 7/8] arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts Ulf Hansson ` (2 subsequent siblings) 8 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 = <®_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 = <®_sys_5v>; }; soc { -- 2.7.4 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 7/8] arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson ` (5 preceding siblings ...) 2017-05-08 16:21 ` [PATCH 6/8] arm64: dts: hikey: Add the SYS_5V and the VDD_3V3 regulators Ulf Hansson @ 2017-05-08 16:21 ` Ulf Hansson 2017-05-23 11:56 ` Arnd Bergmann 2017-05-08 16:21 ` [PATCH 8/8] arm64: dts: hikey: Fix WiFi support Ulf Hansson 2017-05-22 8:40 ` [PATCH 0/8] arm64: hi6220-hikey: " Ulf Hansson 8 siblings, 1 reply; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 7/8] arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts 2017-05-08 16:21 ` [PATCH 7/8] arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts Ulf Hansson @ 2017-05-23 11:56 ` Arnd Bergmann 0 siblings, 0 replies; 59+ messages in thread From: Arnd Bergmann @ 2017-05-23 11:56 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson ` (6 preceding siblings ...) 2017-05-08 16:21 ` [PATCH 7/8] arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts Ulf Hansson @ 2017-05-08 16:21 ` Ulf Hansson 2017-05-31 18:14 ` John Stultz 2017-05-22 8:40 ` [PATCH 0/8] arm64: hi6220-hikey: " Ulf Hansson 8 siblings, 1 reply; 59+ messages in thread From: Ulf Hansson @ 2017-05-08 16:21 UTC (permalink / raw) To: linux-arm-kernel 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 = <®_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 = <®_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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-05-08 16:21 ` [PATCH 8/8] arm64: dts: hikey: Fix WiFi support Ulf Hansson @ 2017-05-31 18:14 ` John Stultz 2017-05-31 18:36 ` Daniel Lezcano 0 siblings, 1 reply; 59+ messages in thread From: John Stultz @ 2017-05-31 18:14 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-05-31 18:14 ` John Stultz @ 2017-05-31 18:36 ` Daniel Lezcano [not found] ` <CALAqxLU2zz17sHMFKOe4p248Bu4fRiU_dKoBBPbY38gDrpb_mw@mail.gmail.com> 0 siblings, 1 reply; 59+ messages in thread From: Daniel Lezcano @ 2017-05-31 18:36 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
[parent not found: <CALAqxLU2zz17sHMFKOe4p248Bu4fRiU_dKoBBPbY38gDrpb_mw@mail.gmail.com>]
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support [not found] ` <CALAqxLU2zz17sHMFKOe4p248Bu4fRiU_dKoBBPbY38gDrpb_mw@mail.gmail.com> @ 2017-06-05 15:15 ` Ulf Hansson 2017-06-05 17:32 ` John Stultz 2017-06-05 21:10 ` [PATCH 8/8] arm64: dts: hikey: Fix WiFi support John Stultz 0 siblings, 2 replies; 59+ messages in thread From: Ulf Hansson @ 2017-06-05 15:15 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-05 15:15 ` Ulf Hansson @ 2017-06-05 17:32 ` John Stultz 2017-06-05 18:13 ` Daniel Lezcano 2017-06-05 21:10 ` [PATCH 8/8] arm64: dts: hikey: Fix WiFi support John Stultz 1 sibling, 1 reply; 59+ messages in thread From: John Stultz @ 2017-06-05 17:32 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-05 17:32 ` John Stultz @ 2017-06-05 18:13 ` Daniel Lezcano 0 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2017-06-05 18:13 UTC (permalink / raw) To: catalin.marinas, will.deacon Cc: daniel.lezcano, John Stultz, Ulf Hansson, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-05 18:13 ` Daniel Lezcano 0 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2017-06-05 18:13 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-05 18:13 ` Daniel Lezcano @ 2017-06-06 14:17 ` Ulf Hansson -1 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-06-06 14:17 UTC (permalink / raw) To: Daniel Lezcano Cc: Catalin Marinas, Will Deacon, John Stultz, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 > ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-06 14:17 ` Ulf Hansson 0 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-06-06 14:17 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-06 14:17 ` Ulf Hansson @ 2017-06-09 15:46 ` Daniel Lezcano -1 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2017-06-09 15:46 UTC (permalink / raw) To: Ulf Hansson Cc: Catalin Marinas, Will Deacon, Olof Johansson, Arnd Bergmann, John Stultz, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-09 15:46 ` Daniel Lezcano 0 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2017-06-09 15:46 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-09 15:46 ` Daniel Lezcano @ 2017-06-09 20:06 ` Arnd Bergmann -1 siblings, 0 replies; 59+ messages in thread From: Arnd Bergmann @ 2017-06-09 20:06 UTC (permalink / raw) To: Daniel Lezcano Cc: Ulf Hansson, Catalin Marinas, Will Deacon, Olof Johansson, John Stultz, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-09 20:06 ` Arnd Bergmann 0 siblings, 0 replies; 59+ messages in thread From: Arnd Bergmann @ 2017-06-09 20:06 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-09 20:06 ` Arnd Bergmann @ 2017-06-09 20:15 ` John Stultz -1 siblings, 0 replies; 59+ messages in thread From: John Stultz @ 2017-06-09 20:15 UTC (permalink / raw) To: Arnd Bergmann Cc: Daniel Lezcano, Ulf Hansson, Catalin Marinas, Will Deacon, Olof Johansson, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-09 20:15 ` John Stultz 0 siblings, 0 replies; 59+ messages in thread From: John Stultz @ 2017-06-09 20:15 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-09 20:15 ` John Stultz @ 2017-06-09 20:48 ` Arnd Bergmann -1 siblings, 0 replies; 59+ messages in thread From: Arnd Bergmann @ 2017-06-09 20:48 UTC (permalink / raw) To: John Stultz Cc: Daniel Lezcano, Ulf Hansson, Catalin Marinas, Will Deacon, Olof Johansson, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-09 20:48 ` Arnd Bergmann 0 siblings, 0 replies; 59+ messages in thread From: Arnd Bergmann @ 2017-06-09 20:48 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-09 20:48 ` Arnd Bergmann @ 2017-06-12 9:38 ` Daniel Lezcano -1 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2017-06-12 9:38 UTC (permalink / raw) To: Arnd Bergmann Cc: John Stultz, Ulf Hansson, Catalin Marinas, Will Deacon, Olof Johansson, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-12 9:38 ` Daniel Lezcano 0 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2017-06-12 9:38 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-12 9:38 ` Daniel Lezcano @ 2017-06-12 21:12 ` Arnd Bergmann -1 siblings, 0 replies; 59+ messages in thread From: Arnd Bergmann @ 2017-06-12 21:12 UTC (permalink / raw) To: Daniel Lezcano Cc: John Stultz, Ulf Hansson, Catalin Marinas, Will Deacon, Olof Johansson, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-12 21:12 ` Arnd Bergmann 0 siblings, 0 replies; 59+ messages in thread From: Arnd Bergmann @ 2017-06-12 21:12 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-12 21:12 ` Arnd Bergmann @ 2017-06-13 12:48 ` Daniel Lezcano -1 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2017-06-13 12:48 UTC (permalink / raw) To: Arnd Bergmann Cc: John Stultz, Ulf Hansson, Catalin Marinas, Will Deacon, Olof Johansson, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2017-06-13 12:48 ` Daniel Lezcano 0 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2017-06-13 12:48 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2017-06-12 21:12 ` Arnd Bergmann @ 2018-02-16 17:35 ` Daniel Lezcano -1 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2018-02-16 17:35 UTC (permalink / raw) To: Arnd Bergmann Cc: John Stultz, Ulf Hansson, Catalin Marinas, Will Deacon, Olof Johansson, Wei Xu, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE), open list 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2018-02-16 17:35 ` Daniel Lezcano 0 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2018-02-16 17:35 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2018-02-16 17:35 ` Daniel Lezcano @ 2018-02-21 10:30 ` Riku Voipio -1 siblings, 0 replies; 59+ messages in thread From: Riku Voipio @ 2018-02-21 10:30 UTC (permalink / raw) To: Daniel Lezcano Cc: Arnd Bergmann, Ulf Hansson, Catalin Marinas, Will Deacon, open list, Wei Xu, John Stultz, Olof Johansson, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE) 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2018-02-21 10:30 ` Riku Voipio 0 siblings, 0 replies; 59+ messages in thread From: Riku Voipio @ 2018-02-21 10:30 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk 2018-02-21 10:30 ` Riku Voipio @ 2018-02-21 10:34 ` Daniel Lezcano -1 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2018-02-21 10:34 UTC (permalink / raw) To: Riku Voipio Cc: Arnd Bergmann, Ulf Hansson, Catalin Marinas, Will Deacon, open list, Wei Xu, John Stultz, Olof Johansson, moderated list:ARM64 PORT (AARCH64 ARCHITECTURE) 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk @ 2018-02-21 10:34 ` Daniel Lezcano 0 siblings, 0 replies; 59+ messages in thread From: Daniel Lezcano @ 2018-02-21 10:34 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-05 15:15 ` Ulf Hansson 2017-06-05 17:32 ` John Stultz @ 2017-06-05 21:10 ` John Stultz 2017-06-05 21:29 ` Rob Herring 1 sibling, 1 reply; 59+ messages in thread From: John Stultz @ 2017-06-05 21:10 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-05 21:10 ` [PATCH 8/8] arm64: dts: hikey: Fix WiFi support John Stultz @ 2017-06-05 21:29 ` Rob Herring 2017-06-06 10:08 ` Ulf Hansson 0 siblings, 1 reply; 59+ messages in thread From: Rob Herring @ 2017-06-05 21:29 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-05 21:29 ` Rob Herring @ 2017-06-06 10:08 ` Ulf Hansson 2017-06-06 14:13 ` Ulf Hansson 2017-06-06 15:58 ` John Stultz 0 siblings, 2 replies; 59+ messages in thread From: Ulf Hansson @ 2017-06-06 10:08 UTC (permalink / raw) To: linux-arm-kernel [...] >>> >>> 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-06 10:08 ` Ulf Hansson @ 2017-06-06 14:13 ` Ulf Hansson 2017-06-06 16:24 ` John Stultz 2017-06-06 15:58 ` John Stultz 1 sibling, 1 reply; 59+ messages in thread From: Ulf Hansson @ 2017-06-06 14:13 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-06 14:13 ` Ulf Hansson @ 2017-06-06 16:24 ` John Stultz 2017-06-07 4:24 ` Ulf Hansson 0 siblings, 1 reply; 59+ messages in thread From: John Stultz @ 2017-06-06 16:24 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-06 16:24 ` John Stultz @ 2017-06-07 4:24 ` Ulf Hansson 2017-06-07 5:25 ` John Stultz 0 siblings, 1 reply; 59+ messages in thread From: Ulf Hansson @ 2017-06-07 4:24 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-07 4:24 ` Ulf Hansson @ 2017-06-07 5:25 ` John Stultz 0 siblings, 0 replies; 59+ messages in thread From: John Stultz @ 2017-06-07 5:25 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 8/8] arm64: dts: hikey: Fix WiFi support 2017-06-06 10:08 ` Ulf Hansson 2017-06-06 14:13 ` Ulf Hansson @ 2017-06-06 15:58 ` John Stultz 1 sibling, 0 replies; 59+ messages in thread From: John Stultz @ 2017-06-06 15:58 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson ` (7 preceding siblings ...) 2017-05-08 16:21 ` [PATCH 8/8] arm64: dts: hikey: Fix WiFi support Ulf Hansson @ 2017-05-22 8:40 ` Ulf Hansson 2017-05-23 12:00 ` Arnd Bergmann 8 siblings, 1 reply; 59+ messages in thread From: Ulf Hansson @ 2017-05-22 8:40 UTC (permalink / raw) To: linux-arm-kernel + 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support 2017-05-22 8:40 ` [PATCH 0/8] arm64: hi6220-hikey: " Ulf Hansson @ 2017-05-23 12:00 ` Arnd Bergmann 2017-05-23 12:11 ` Ulf Hansson 0 siblings, 1 reply; 59+ messages in thread From: Arnd Bergmann @ 2017-05-23 12:00 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support 2017-05-23 12:00 ` Arnd Bergmann @ 2017-05-23 12:11 ` Ulf Hansson 0 siblings, 0 replies; 59+ messages in thread From: Ulf Hansson @ 2017-05-23 12:11 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
end of thread, other threads:[~2018-02-21 10:34 UTC | newest] Thread overview: 59+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-08 16:21 [PATCH 0/8] arm64: hi6220-hikey: Fix WiFi support Ulf Hansson 2017-05-08 16:21 ` [PATCH 1/8] mmc: dt: pwrseq-simple: Invent power-off-delay-us Ulf Hansson 2017-05-08 16:21 ` Ulf Hansson [not found] ` <1494260477-25163-2-git-send-email-ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2017-05-12 20:03 ` Rob Herring 2017-05-12 20:03 ` Rob Herring 2017-05-15 11:08 ` Ulf Hansson 2017-05-15 11:08 ` Ulf Hansson [not found] ` <CAPDyKFqhbGqanoQqFsLrLaBXsQ5WdFrrunkvB2eOoJqH+W6jdQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-05-15 16:16 ` Rob Herring 2017-05-15 16:16 ` Rob Herring 2017-05-16 7:06 ` Ulf Hansson 2017-05-16 7:06 ` Ulf Hansson 2017-05-08 16:21 ` [PATCH 2/8] mmc: pwrseq_simple: Parse DTS for the power-off-delay-us property Ulf Hansson 2017-05-08 16:21 ` Ulf Hansson 2017-05-08 16:21 ` [PATCH 3/8] mfd: dts: hi655x: Add clock binding for the pmic Ulf Hansson 2017-05-08 16:21 ` [PATCH 4/8] arm64: dts: hikey: Add clock for the pmic mfd Ulf Hansson 2017-05-08 16:21 ` [PATCH 5/8] arm64: dts: hi6220: Move the fixed_5v_hub regulator to the hikey dts Ulf Hansson 2017-05-08 16:21 ` [PATCH 6/8] arm64: dts: hikey: Add the SYS_5V and the VDD_3V3 regulators Ulf Hansson 2017-05-08 16:21 ` [PATCH 7/8] arm64: dts: hi6220: Move board data from the dwmmc nodes to hikey dts Ulf Hansson 2017-05-23 11:56 ` Arnd Bergmann 2017-05-08 16:21 ` [PATCH 8/8] arm64: dts: hikey: Fix WiFi support Ulf Hansson 2017-05-31 18:14 ` John Stultz 2017-05-31 18:36 ` Daniel Lezcano [not found] ` <CALAqxLU2zz17sHMFKOe4p248Bu4fRiU_dKoBBPbY38gDrpb_mw@mail.gmail.com> 2017-06-05 15:15 ` Ulf Hansson 2017-06-05 17:32 ` John Stultz 2017-06-05 18:13 ` [PATCH] ARM64: Kconfig: Fix the missing hi655x common clk Daniel Lezcano 2017-06-05 18:13 ` Daniel Lezcano 2017-06-06 14:17 ` Ulf Hansson 2017-06-06 14:17 ` Ulf Hansson 2017-06-09 15:46 ` Daniel Lezcano 2017-06-09 15:46 ` Daniel Lezcano 2017-06-09 20:06 ` Arnd Bergmann 2017-06-09 20:06 ` Arnd Bergmann 2017-06-09 20:15 ` John Stultz 2017-06-09 20:15 ` John Stultz 2017-06-09 20:48 ` Arnd Bergmann 2017-06-09 20:48 ` Arnd Bergmann 2017-06-12 9:38 ` Daniel Lezcano 2017-06-12 9:38 ` Daniel Lezcano 2017-06-12 21:12 ` Arnd Bergmann 2017-06-12 21:12 ` Arnd Bergmann 2017-06-13 12:48 ` Daniel Lezcano 2017-06-13 12:48 ` Daniel Lezcano 2018-02-16 17:35 ` Daniel Lezcano 2018-02-16 17:35 ` Daniel Lezcano 2018-02-21 10:30 ` Riku Voipio 2018-02-21 10:30 ` Riku Voipio 2018-02-21 10:34 ` Daniel Lezcano 2018-02-21 10:34 ` Daniel Lezcano 2017-06-05 21:10 ` [PATCH 8/8] arm64: dts: hikey: Fix WiFi support John Stultz 2017-06-05 21:29 ` Rob Herring 2017-06-06 10:08 ` Ulf Hansson 2017-06-06 14:13 ` Ulf Hansson 2017-06-06 16:24 ` John Stultz 2017-06-07 4:24 ` Ulf Hansson 2017-06-07 5:25 ` John Stultz 2017-06-06 15:58 ` John Stultz 2017-05-22 8:40 ` [PATCH 0/8] arm64: hi6220-hikey: " Ulf Hansson 2017-05-23 12:00 ` Arnd Bergmann 2017-05-23 12:11 ` Ulf Hansson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.