| Message ID | 1599742438-16811-3-git-send-email-skakit@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add wakeup support over UART RX | expand |
Hi, On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> wrote: > > Add a suitable sleep configuration for uart3 to support Bluetooth wakeup. > > If QUP function is selected in sleep state, UART RTS/RFR is pulled high > during suspend and BT SoC not able to send wakeup bytes. So, configure > GPIO mode in sleep state to keep it low during suspend. > > Signed-off-by: satya priya <skakit@codeaurora.org> > Reviewed-by: Akash Asthana <akashast@codeaurora.org> > --- > Changes in V2: > - This patch adds sleep state for BT UART. Newly added in V2. > > Changes in V3: > - Remove "output-high" for TX from both sleep and default states > as it is not required. Configure pull-up for TX in sleep state. > > Changes in V4: > - As per Matthias's comment, removed drive-strength for sleep state > and fixed nit-pick. > > Changes in V5: > - As per Matthias's comments, moved pinmux change for sleep state, > pinctrl and interrupt config to the board specific file. > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) Similar comment to patch #1 in that this applies only to the IDP board but that's not obvious from ${SUBJECT} > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > index 04888df..e529a41 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > @@ -344,6 +344,10 @@ > }; > > &uart3 { > + pinctrl-names = "default", "sleep"; > + pinctrl-1 = <&qup_uart3_sleep>; > + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; You need a: /delete-property/interrupts; ...or, alternatively, a patch before this one that converts all the UARTs in sc7180 to just use interrupts-extended. > status = "okay"; Slight nit is that usually I see the status line first. All the other instances in this file have it that way. Can you match? -Doug
El Thu, Sep 10, 2020 at 06:23:56PM +0530 satya priya ha dit: > Add a suitable sleep configuration for uart3 to support Bluetooth wakeup. > > If QUP function is selected in sleep state, UART RTS/RFR is pulled high > during suspend and BT SoC not able to send wakeup bytes. So, configure > GPIO mode in sleep state to keep it low during suspend. > > Signed-off-by: satya priya <skakit@codeaurora.org> > Reviewed-by: Akash Asthana <akashast@codeaurora.org> Was this change really reviewed (privately) by Akash or are you still carrying forward this tag from v2? The configuration and the comments have change substantially since v2, IMO you should drop the tag unless Akash really reviewed the current version or something close to it. > --- > Changes in V2: > - This patch adds sleep state for BT UART. Newly added in V2. > > Changes in V3: > - Remove "output-high" for TX from both sleep and default states > as it is not required. Configure pull-up for TX in sleep state. > > Changes in V4: > - As per Matthias's comment, removed drive-strength for sleep state > and fixed nit-pick. > > Changes in V5: > - As per Matthias's comments, moved pinmux change for sleep state, > pinctrl and interrupt config to the board specific file. > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > index 04888df..e529a41 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > @@ -344,6 +344,10 @@ > }; > > &uart3 { > + pinctrl-names = "default", "sleep"; > + pinctrl-1 = <&qup_uart3_sleep>; > + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; As Doug already said, you need to delete the 'interrupts' property now that we have 'interrupts-extended'.
On Thu 10 Sep 12:53 UTC 2020, satya priya wrote: > Add a suitable sleep configuration for uart3 to support Bluetooth wakeup. > > If QUP function is selected in sleep state, UART RTS/RFR is pulled high > during suspend and BT SoC not able to send wakeup bytes. So, configure > GPIO mode in sleep state to keep it low during suspend. > But patch 4 says that you change this behavior, is that patch really needed if we switch the pins to GPIO, or if this patch really needed if we merge patch 4? Could it be that in lower power states we drop the power to the uart block and rely on the PDC to wait for the BT chip to start sending the wakeup bytes on the rx pin? This commit will become the reference for all other platforms where we enable the same functionality, so better document it properly. > Signed-off-by: satya priya <skakit@codeaurora.org> > Reviewed-by: Akash Asthana <akashast@codeaurora.org> > --- > Changes in V2: > - This patch adds sleep state for BT UART. Newly added in V2. > > Changes in V3: > - Remove "output-high" for TX from both sleep and default states > as it is not required. Configure pull-up for TX in sleep state. > > Changes in V4: > - As per Matthias's comment, removed drive-strength for sleep state > and fixed nit-pick. > > Changes in V5: > - As per Matthias's comments, moved pinmux change for sleep state, > pinctrl and interrupt config to the board specific file. > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > index 04888df..e529a41 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > @@ -344,6 +344,10 @@ > }; > > &uart3 { > + pinctrl-names = "default", "sleep"; > + pinctrl-1 = <&qup_uart3_sleep>; > + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; > status = "okay"; > > bluetooth: wcn3990-bt { > @@ -545,3 +549,47 @@ > }; > }; > > +&tlmm { > + qup_uart3_sleep: qup-uart3-sleep { > + pinmux { > + pins = "gpio38", "gpio39", > + "gpio40", "gpio41"; > + function = "gpio"; > + }; > + > + pinconf-cts { > + /* > + * Configure a pull-down on CTS to match the pull of > + * the Bluetooth module. > + */ > + pins = "gpio38"; > + bias-pull-down; > + }; > + > + pinconf-rts { > + /* > + * Configure pull-down on RTS to make sure that the BT SoC can > + * wake up the system by sending wakeup bytes during suspend. So "request to send" is active low and pulling it low will indicate to the BT chip that it's allowed to wake us up by pulling rx low? I would like this comment to really describe what's actually going on. > + */ > + pins = "gpio39"; > + bias-pull-down; > + }; > + > + pinconf-tx { > + /* Configure pull-up on TX when it isn't actively driven */ Sure, but why? Wouldn't that be to prevent the BT chip from receiving garbage while the SoC is asleep? > + pins = "gpio40"; > + bias-pull-up; > + }; > + > + pinconf-rx { > + /* > + * Configure a pull-up on RX. This is needed to avoid > + * garbage data when the TX pin of the Bluetooth module is > + * in tri-state (module powered off or not driving the > + * signal yet). > + */ It's nice to avoid "garbage data", but isn't the real reason that the floating pin on the other side would cause spurious wakeups? Regards, Bjorn > + pins = "gpio41"; > + bias-pull-up; > + }; > + }; > +}; > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
On 2020-09-10 20:10, Doug Anderson wrote: > Hi, > > On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> > wrote: >> >> Add a suitable sleep configuration for uart3 to support Bluetooth >> wakeup. >> >> If QUP function is selected in sleep state, UART RTS/RFR is pulled >> high >> during suspend and BT SoC not able to send wakeup bytes. So, configure >> GPIO mode in sleep state to keep it low during suspend. >> >> Signed-off-by: satya priya <skakit@codeaurora.org> >> Reviewed-by: Akash Asthana <akashast@codeaurora.org> >> --- >> Changes in V2: >> - This patch adds sleep state for BT UART. Newly added in V2. >> >> Changes in V3: >> - Remove "output-high" for TX from both sleep and default states >> as it is not required. Configure pull-up for TX in sleep state. >> >> Changes in V4: >> - As per Matthias's comment, removed drive-strength for sleep state >> and fixed nit-pick. >> >> Changes in V5: >> - As per Matthias's comments, moved pinmux change for sleep state, >> pinctrl and interrupt config to the board specific file. >> >> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 >> +++++++++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) > > Similar comment to patch #1 in that this applies only to the IDP board > but that's not obvious from ${SUBJECT} > Okay. > >> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> index 04888df..e529a41 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> @@ -344,6 +344,10 @@ >> }; >> >> &uart3 { >> + pinctrl-names = "default", "sleep"; >> + pinctrl-1 = <&qup_uart3_sleep>; >> + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, >> + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; > > You need a: > > /delete-property/interrupts; > > ...or, alternatively, a patch before this one that converts all the > UARTs in sc7180 to just use interrupts-extended. > Sure, I will add this. But I think when both are added, "interrupts-extended" will get priority as per [1] and there wouldn't be any problem. [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > >> status = "okay"; > > Slight nit is that usually I see the status line first. All the other > instances in this file have it that way. Can you match? > Ok, will correct it. > > > > -Doug
Hi Matthias, On 2020-09-10 21:05, Matthias Kaehlcke wrote: > El Thu, Sep 10, 2020 at 06:23:56PM +0530 satya priya ha dit: > >> Add a suitable sleep configuration for uart3 to support Bluetooth >> wakeup. >> >> If QUP function is selected in sleep state, UART RTS/RFR is pulled >> high >> during suspend and BT SoC not able to send wakeup bytes. So, configure >> GPIO mode in sleep state to keep it low during suspend. >> >> Signed-off-by: satya priya <skakit@codeaurora.org> >> Reviewed-by: Akash Asthana <akashast@codeaurora.org> > > Was this change really reviewed (privately) by Akash or are you still > carrying forward this tag from v2? The configuration and the comments > have change substantially since v2, IMO you should drop the tag unless > Akash really reviewed the current version or something close to it. > Okay, will drop the tag. >> --- >> Changes in V2: >> - This patch adds sleep state for BT UART. Newly added in V2. >> >> Changes in V3: >> - Remove "output-high" for TX from both sleep and default states >> as it is not required. Configure pull-up for TX in sleep state. >> >> Changes in V4: >> - As per Matthias's comment, removed drive-strength for sleep state >> and fixed nit-pick. >> >> Changes in V5: >> - As per Matthias's comments, moved pinmux change for sleep state, >> pinctrl and interrupt config to the board specific file. >> >> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 >> +++++++++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> index 04888df..e529a41 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> @@ -344,6 +344,10 @@ >> }; >> >> &uart3 { >> + pinctrl-names = "default", "sleep"; >> + pinctrl-1 = <&qup_uart3_sleep>; >> + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, >> + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; > > As Doug already said, you need to delete the 'interrupts' property now > that we have 'interrupts-extended'. okay. Thanks, Satya Priya
Hi Bjorn, Thanks for reviewing the patches. On 2020-09-11 05:15, Bjorn Andersson wrote: > On Thu 10 Sep 12:53 UTC 2020, satya priya wrote: > >> Add a suitable sleep configuration for uart3 to support Bluetooth >> wakeup. >> >> If QUP function is selected in sleep state, UART RTS/RFR is pulled >> high >> during suspend and BT SoC not able to send wakeup bytes. So, configure >> GPIO mode in sleep state to keep it low during suspend. >> > > But patch 4 says that you change this behavior, is that patch really > needed if we switch the pins to GPIO, or if this patch really needed if > we merge patch 4? > > Could it be that in lower power states we drop the power to the uart > block and rely on the PDC to wait for the BT chip to start sending the > wakeup bytes on the rx pin? > As discussed on V4 patch https://patchwork.kernel.org/patch/11753971/#23602723 The patch 4 is good to have, to make sure the UART_MANUAL_RFR is in ready state to receive the wakeup bytes. > > This commit will become the reference for all other platforms where we > enable the same functionality, so better document it properly. > Okay. >> Signed-off-by: satya priya <skakit@codeaurora.org> >> Reviewed-by: Akash Asthana <akashast@codeaurora.org> >> --- >> Changes in V2: >> - This patch adds sleep state for BT UART. Newly added in V2. >> >> Changes in V3: >> - Remove "output-high" for TX from both sleep and default states >> as it is not required. Configure pull-up for TX in sleep state. >> >> Changes in V4: >> - As per Matthias's comment, removed drive-strength for sleep state >> and fixed nit-pick. >> >> Changes in V5: >> - As per Matthias's comments, moved pinmux change for sleep state, >> pinctrl and interrupt config to the board specific file. >> >> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 >> +++++++++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> index 04888df..e529a41 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> @@ -344,6 +344,10 @@ >> }; >> >> &uart3 { >> + pinctrl-names = "default", "sleep"; >> + pinctrl-1 = <&qup_uart3_sleep>; >> + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, >> + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; >> status = "okay"; >> >> bluetooth: wcn3990-bt { >> @@ -545,3 +549,47 @@ >> }; >> }; >> >> +&tlmm { >> + qup_uart3_sleep: qup-uart3-sleep { >> + pinmux { >> + pins = "gpio38", "gpio39", >> + "gpio40", "gpio41"; >> + function = "gpio"; >> + }; >> + >> + pinconf-cts { >> + /* >> + * Configure a pull-down on CTS to match the pull of >> + * the Bluetooth module. >> + */ >> + pins = "gpio38"; >> + bias-pull-down; >> + }; >> + >> + pinconf-rts { >> + /* >> + * Configure pull-down on RTS to make sure that the BT SoC can >> + * wake up the system by sending wakeup bytes during suspend. > > So "request to send" is active low and pulling it low will indicate to > the BT chip that it's allowed to wake us up by pulling rx low? > > I would like this comment to really describe what's actually going on. > Ok, will modify the rationale. >> + */ >> + pins = "gpio39"; >> + bias-pull-down; >> + }; >> + >> + pinconf-tx { >> + /* Configure pull-up on TX when it isn't actively driven */ > > Sure, but why? Wouldn't that be to prevent the BT chip from receiving > garbage while the SoC is asleep? > yes, this is to prevent the BT chip from receiving garbage, will mention the same. >> + pins = "gpio40"; >> + bias-pull-up; >> + }; >> + >> + pinconf-rx { >> + /* >> + * Configure a pull-up on RX. This is needed to avoid >> + * garbage data when the TX pin of the Bluetooth module is >> + * in tri-state (module powered off or not driving the >> + * signal yet). >> + */ > > It's nice to avoid "garbage data", but isn't the real reason that the > floating pin on the other side would cause spurious wakeups? > yes, we need pull-up on RX to prevent spurious wakeups, will modify this comment to mention it. > Regards, > Bjorn > >> + pins = "gpio41"; >> + bias-pull-up; >> + }; >> + }; >> +}; >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a >> member >> of Code Aurora Forum, hosted by The Linux Foundation >> Thanks, Satya Priya
On Fri 11 Sep 05:15 CDT 2020, skakit@codeaurora.org wrote: > On 2020-09-10 20:10, Doug Anderson wrote: > > Hi, > > > > On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> > > wrote: > > > > > > Add a suitable sleep configuration for uart3 to support Bluetooth > > > wakeup. > > > > > > If QUP function is selected in sleep state, UART RTS/RFR is pulled > > > high > > > during suspend and BT SoC not able to send wakeup bytes. So, configure > > > GPIO mode in sleep state to keep it low during suspend. > > > > > > Signed-off-by: satya priya <skakit@codeaurora.org> > > > Reviewed-by: Akash Asthana <akashast@codeaurora.org> > > > --- > > > Changes in V2: > > > - This patch adds sleep state for BT UART. Newly added in V2. > > > > > > Changes in V3: > > > - Remove "output-high" for TX from both sleep and default states > > > as it is not required. Configure pull-up for TX in sleep state. > > > > > > Changes in V4: > > > - As per Matthias's comment, removed drive-strength for sleep state > > > and fixed nit-pick. > > > > > > Changes in V5: > > > - As per Matthias's comments, moved pinmux change for sleep state, > > > pinctrl and interrupt config to the board specific file. > > > > > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 > > > +++++++++++++++++++++++++++++++++ > > > 1 file changed, 48 insertions(+) > > > > Similar comment to patch #1 in that this applies only to the IDP board > > but that's not obvious from ${SUBJECT} > > > > Okay. > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > > > index 04888df..e529a41 100644 > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > > > @@ -344,6 +344,10 @@ > > > }; > > > > > > &uart3 { > > > + pinctrl-names = "default", "sleep"; > > > + pinctrl-1 = <&qup_uart3_sleep>; > > > + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, > > > + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; > > > > You need a: > > > > /delete-property/interrupts; > > > > ...or, alternatively, a patch before this one that converts all the > > UARTs in sc7180 to just use interrupts-extended. > > > > Sure, I will add this. But I think when both are added, > "interrupts-extended" will get priority as per [1] and there wouldn't be any > problem. > You're indeed correct, please stick with what you have. Regards, Bjorn > [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > > > > > > status = "okay"; > > > > Slight nit is that usually I see the status line first. All the other > > instances in this file have it that way. Can you match? > > > > Ok, will correct it. > > > > > > > > > -Doug
Hi, On Fri, Sep 11, 2020 at 6:52 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > > > > &uart3 { > > > > + pinctrl-names = "default", "sleep"; > > > > + pinctrl-1 = <&qup_uart3_sleep>; > > > > + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, > > > > + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; > > > > > > You need a: > > > > > > /delete-property/interrupts; > > > > > > ...or, alternatively, a patch before this one that converts all the > > > UARTs in sc7180 to just use interrupts-extended. > > > > > > > Sure, I will add this. But I think when both are added, > > "interrupts-extended" will get priority as per [1] and there wouldn't be any > > problem. > > > > You're indeed correct, please stick with what you have. IMO this is ugly, but I won't fight too hard on it if you guys really want to do it that way. Reading the documentation it sounds as if allowing for both is intended to be used in cases where the same device tree might be used on old software (that didn't understand interrupts-extended) and on new software (that does). The interrupts-extended property by far predates any sc7180 support, though, so I can't imagine anyone really needing to use that these days. Is it really that bad to add the /delete-property/ to end up with a cleaner final device tree? -Doug
diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts index 04888df..e529a41 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts @@ -344,6 +344,10 @@ }; &uart3 { + pinctrl-names = "default", "sleep"; + pinctrl-1 = <&qup_uart3_sleep>; + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; status = "okay"; bluetooth: wcn3990-bt { @@ -545,3 +549,47 @@ }; }; +&tlmm { + qup_uart3_sleep: qup-uart3-sleep { + pinmux { + pins = "gpio38", "gpio39", + "gpio40", "gpio41"; + function = "gpio"; + }; + + pinconf-cts { + /* + * Configure a pull-down on CTS to match the pull of + * the Bluetooth module. + */ + pins = "gpio38"; + bias-pull-down; + }; + + pinconf-rts { + /* + * Configure pull-down on RTS to make sure that the BT SoC can + * wake up the system by sending wakeup bytes during suspend. + */ + pins = "gpio39"; + bias-pull-down; + }; + + pinconf-tx { + /* Configure pull-up on TX when it isn't actively driven */ + pins = "gpio40"; + bias-pull-up; + }; + + pinconf-rx { + /* + * Configure a pull-up on RX. This is needed to avoid + * garbage data when the TX pin of the Bluetooth module is + * in tri-state (module powered off or not driving the + * signal yet). + */ + pins = "gpio41"; + bias-pull-up; + }; + }; +};