Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
Hi Matthias,
On 2020-08-27 20:53, Matthias Kaehlcke wrote:
Hi Satya,
On Thu, Aug 27, 2020 at 08:37:33PM +0530, ska...@codeaurora.org wrote:
Hi Matthias,
On 2020-08-26 22:10, Matthias Kaehlcke wrote:
> Hi Satya,
>
> On Wed, Aug 26, 2020 at 09:35:15PM +0530, ska...@codeaurora.org wrote:
> > Hi Matthias,
> >
> > On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> > > On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
> > > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > > > > On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > > > > > Add sleep pin ctrl for BT uart, and also change the bias
> > > > > > configuration to match Bluetooth module.
> > > > > >
> > > > > > Signed-off-by: satya priya
> > > > > > Reviewed-by: Akash Asthana
> > > > > > ---
> > > > > > 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.
> > > > > >
> > > > > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > > > > +++--
> > > > > > 1 file changed, 45 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > index d8b5507..806f626 100644
> > > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > @@ -473,20 +473,20 @@
> > > > > >
> > > > > > _uart3_default {
> > > > > > pinconf-cts {
> > > > > > - /*
> > > > > > - * Configure a pull-down on 38 (CTS) to match the pull of
> > > > > > - * the Bluetooth module.
> > > > > > - */
> > > > > > + /* Configure no pull on 38 (CTS) to match Bluetooth
module */
> > > > > > pins = "gpio38";
> > > > > > - bias-pull-down;
> > > > > > - output-high;
> > > > > > + bias-disable;
> > > > >
> > > > > I think it should be ok in functional terms, but I don't like the
> > > > > rationale
> > > > > and also doubt the change is really needed.
> > > > >
> > > > > If the pull is removed to match the Bluetooth module, then that sounds
> > > > > as
> > > > > if the signal was floating on the the BT side, which I think is not
the
> > > > > case.
> > > > > Yes, according to the datasheet there is no pull when the BT
controller
> > > > > is
> > > > > active, but then it drives the signal actively to either high or low.
> > > > > There
> > > > > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > > > > the
> > > > > signal was really floating on the BT side we would definitely not want
> > > > > this.
> > > > >
> > > > > In a reply to v2 you said:
> > > > >
> > > > > > Recently on cherokee we worked with BT team and came to an agreement
> > > > > > to
> > > > > > keep no-pull from our side in order to not conflict with their pull
in
> > > > > > any state.
> > > > >
> > > > > What are these conflicting pull states?
> > > > >
> > > > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset
and
> > > > > boot mode, and no pull in active mode. In reset and boot mode the host
> > > > > config with a pull down would match, and no pull in active mode
doesn't
> > > > > conflict with the pull-down on the host UART. My understanding is that
> > > > > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > > > > RTS the pull on the host side shouldn't matter.
> > > > >
> > > >
> > > > yes, I agree with you, the pinconf pulls are weak. As this is driven
> > > > by BT
> > > > SoC (pull on HOST side shouldn't matter), we are not mentioning any
> > > > bias
> > > > configuration from our side and simply putting it as no-pull, just
> > > > to not
> > > > conflict in any case. It seems that the rationale mentioned is a bit
> > > > confusing i will change it to clearly specify why we are configuring
> > > > no-pull.
> > > >
> > > > > Is this change actually related with wakeup support? I have the
> > > > > impression
> > > > > that multiple things are conflated in this patch. If some of the
changes
> > > > > are just fixing/improving other things they should be in a separate
> > > > > patch,
> > > > > which could be part of this series, otherwise it's really hard to
> > > > > distinguish between the pieces that are actually relevant for wakeup
and
> > > > > the rest.
> > > > >
> > > > > Independently of whether the changes are done in a single or multiple
> > > > > patches, the commit log should include details on why the changes are
> > > > > necessary, especially when there are not explantatory comments in the
> > > > > DT/code itself (e.g. the removal of 'output-high', which seems correct
> > > > > to me, but no reason is given why it is done).
> > > > >
> > > >
> > > > This change is not related to wakeup
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
Hi Satya,
On Thu, Aug 27, 2020 at 08:37:33PM +0530, ska...@codeaurora.org wrote:
> Hi Matthias,
>
> On 2020-08-26 22:10, Matthias Kaehlcke wrote:
> > Hi Satya,
> >
> > On Wed, Aug 26, 2020 at 09:35:15PM +0530, ska...@codeaurora.org wrote:
> > > Hi Matthias,
> > >
> > > On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> > > > On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
> > > > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > > > > > On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > > > > > > Add sleep pin ctrl for BT uart, and also change the bias
> > > > > > > configuration to match Bluetooth module.
> > > > > > >
> > > > > > > Signed-off-by: satya priya
> > > > > > > Reviewed-by: Akash Asthana
> > > > > > > ---
> > > > > > > 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.
> > > > > > >
> > > > > > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > > > > > +++--
> > > > > > > 1 file changed, 45 insertions(+), 9 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > index d8b5507..806f626 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > @@ -473,20 +473,20 @@
> > > > > > >
> > > > > > > _uart3_default {
> > > > > > > pinconf-cts {
> > > > > > > - /*
> > > > > > > - * Configure a pull-down on 38 (CTS) to match the pull
> > > > > > > of
> > > > > > > - * the Bluetooth module.
> > > > > > > - */
> > > > > > > + /* Configure no pull on 38 (CTS) to match Bluetooth
> > > > > > > module */
> > > > > > > pins = "gpio38";
> > > > > > > - bias-pull-down;
> > > > > > > - output-high;
> > > > > > > + bias-disable;
> > > > > >
> > > > > > I think it should be ok in functional terms, but I don't like the
> > > > > > rationale
> > > > > > and also doubt the change is really needed.
> > > > > >
> > > > > > If the pull is removed to match the Bluetooth module, then that
> > > > > > sounds
> > > > > > as
> > > > > > if the signal was floating on the the BT side, which I think is not
> > > > > > the
> > > > > > case.
> > > > > > Yes, according to the datasheet there is no pull when the BT
> > > > > > controller
> > > > > > is
> > > > > > active, but then it drives the signal actively to either high or
> > > > > > low.
> > > > > > There
> > > > > > seems to be no merit in 'matching' the Bluetooth side in this case,
> > > > > > if
> > > > > > the
> > > > > > signal was really floating on the BT side we would definitely not
> > > > > > want
> > > > > > this.
> > > > > >
> > > > > > In a reply to v2 you said:
> > > > > >
> > > > > > > Recently on cherokee we worked with BT team and came to an
> > > > > > > agreement
> > > > > > > to
> > > > > > > keep no-pull from our side in order to not conflict with their
> > > > > > > pull in
> > > > > > > any state.
> > > > > >
> > > > > > What are these conflicting pull states?
> > > > > >
> > > > > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in
> > > > > > reset and
> > > > > > boot mode, and no pull in active mode. In reset and boot mode the
> > > > > > host
> > > > > > config with a pull down would match, and no pull in active mode
> > > > > > doesn't
> > > > > > conflict with the pull-down on the host UART. My understanding is
> > > > > > that
> > > > > > the pinconf pulls are weak pulls, so as soon as the BT chip drives
> > > > > > its
> > > > > > RTS the pull on the host side shouldn't matter.
> > > > > >
> > > > >
> > > > > yes, I agree with you, the pinconf pulls are weak. As this is driven
> > > > > by BT
> > > > > SoC (pull on HOST side shouldn't matter), we are not mentioning any
> > > > > bias
> > > > > configuration from our side and simply putting it as no-pull, just
> > > > > to not
> > > > > conflict in any case. It seems that the rationale mentioned is a bit
> > > > > confusing i will change it to clearly specify why we are configuring
> > > > > no-pull.
> > > > >
> > > > > > Is this change actually related with wakeup support? I have the
> > > > > > impression
> > > > > > that multiple things are conflated in this patch. If some of the
> > > > > > changes
> > > > > > are just fixing/improving other things they should be in a separate
> > > > > > patch,
> > > > > > which could be part of this series, otherwise it's really hard to
> > > > > > distinguish between the pieces that are actually relevant for
> > > > > > wakeup and
> > > > > > the rest.
> > > > > >
> > > > > > Independently of whether the changes are done in a single or
> > > > > >
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
Hi Matthias,
On 2020-08-26 22:10, Matthias Kaehlcke wrote:
Hi Satya,
On Wed, Aug 26, 2020 at 09:35:15PM +0530, ska...@codeaurora.org wrote:
Hi Matthias,
On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
> > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > > On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > > > Add sleep pin ctrl for BT uart, and also change the bias
> > > > configuration to match Bluetooth module.
> > > >
> > > > Signed-off-by: satya priya
> > > > Reviewed-by: Akash Asthana
> > > > ---
> > > > 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.
> > > >
> > > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > > +++--
> > > > 1 file changed, 45 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > index d8b5507..806f626 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > @@ -473,20 +473,20 @@
> > > >
> > > > _uart3_default {
> > > > pinconf-cts {
> > > > - /*
> > > > -* Configure a pull-down on 38 (CTS) to match the pull of
> > > > -* the Bluetooth module.
> > > > -*/
> > > > + /* Configure no pull on 38 (CTS) to match Bluetooth module
*/
> > > > pins = "gpio38";
> > > > - bias-pull-down;
> > > > - output-high;
> > > > + bias-disable;
> > >
> > > I think it should be ok in functional terms, but I don't like the
> > > rationale
> > > and also doubt the change is really needed.
> > >
> > > If the pull is removed to match the Bluetooth module, then that sounds
> > > as
> > > if the signal was floating on the the BT side, which I think is not the
> > > case.
> > > Yes, according to the datasheet there is no pull when the BT controller
> > > is
> > > active, but then it drives the signal actively to either high or low.
> > > There
> > > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > > the
> > > signal was really floating on the BT side we would definitely not want
> > > this.
> > >
> > > In a reply to v2 you said:
> > >
> > > > Recently on cherokee we worked with BT team and came to an agreement
> > > > to
> > > > keep no-pull from our side in order to not conflict with their pull in
> > > > any state.
> > >
> > > What are these conflicting pull states?
> > >
> > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> > > boot mode, and no pull in active mode. In reset and boot mode the host
> > > config with a pull down would match, and no pull in active mode doesn't
> > > conflict with the pull-down on the host UART. My understanding is that
> > > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > > RTS the pull on the host side shouldn't matter.
> > >
> >
> > yes, I agree with you, the pinconf pulls are weak. As this is driven
> > by BT
> > SoC (pull on HOST side shouldn't matter), we are not mentioning any
> > bias
> > configuration from our side and simply putting it as no-pull, just
> > to not
> > conflict in any case. It seems that the rationale mentioned is a bit
> > confusing i will change it to clearly specify why we are configuring
> > no-pull.
> >
> > > Is this change actually related with wakeup support? I have the
> > > impression
> > > that multiple things are conflated in this patch. If some of the changes
> > > are just fixing/improving other things they should be in a separate
> > > patch,
> > > which could be part of this series, otherwise it's really hard to
> > > distinguish between the pieces that are actually relevant for wakeup and
> > > the rest.
> > >
> > > Independently of whether the changes are done in a single or multiple
> > > patches, the commit log should include details on why the changes are
> > > necessary, especially when there are not explantatory comments in the
> > > DT/code itself (e.g. the removal of 'output-high', which seems correct
> > > to me, but no reason is given why it is done).
> > >
> >
> > This change is not related to wakeup support, I will make it a
> > separate
> > patch, will also mention the details in commit text.
> >
> > > > };
> > > >
> > > > pinconf-rts {
> > > > - /* We'll drive 39 (RTS), so no pull */
> > > > + /*
> > > > +* Configure pull-down on 39 (RTS). This is needed to avoid
a
> > > > +* floating pin which could mislead Bluetooth controller
> > > > +* with UART RFR state (READY/NOT_READY).
> > > > +*/
> > > > pins = "gpio39";
> > > > drive-strength =
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
Hi Satya,
On Wed, Aug 26, 2020 at 09:35:15PM +0530, ska...@codeaurora.org wrote:
> Hi Matthias,
>
> On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> > On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
> > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > > > On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > > > > Add sleep pin ctrl for BT uart, and also change the bias
> > > > > configuration to match Bluetooth module.
> > > > >
> > > > > Signed-off-by: satya priya
> > > > > Reviewed-by: Akash Asthana
> > > > > ---
> > > > > 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.
> > > > >
> > > > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > > > +++--
> > > > > 1 file changed, 45 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > index d8b5507..806f626 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > @@ -473,20 +473,20 @@
> > > > >
> > > > > _uart3_default {
> > > > > pinconf-cts {
> > > > > - /*
> > > > > - * Configure a pull-down on 38 (CTS) to match the pull
> > > > > of
> > > > > - * the Bluetooth module.
> > > > > - */
> > > > > + /* Configure no pull on 38 (CTS) to match Bluetooth
> > > > > module */
> > > > > pins = "gpio38";
> > > > > - bias-pull-down;
> > > > > - output-high;
> > > > > + bias-disable;
> > > >
> > > > I think it should be ok in functional terms, but I don't like the
> > > > rationale
> > > > and also doubt the change is really needed.
> > > >
> > > > If the pull is removed to match the Bluetooth module, then that sounds
> > > > as
> > > > if the signal was floating on the the BT side, which I think is not the
> > > > case.
> > > > Yes, according to the datasheet there is no pull when the BT controller
> > > > is
> > > > active, but then it drives the signal actively to either high or low.
> > > > There
> > > > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > > > the
> > > > signal was really floating on the BT side we would definitely not want
> > > > this.
> > > >
> > > > In a reply to v2 you said:
> > > >
> > > > > Recently on cherokee we worked with BT team and came to an agreement
> > > > > to
> > > > > keep no-pull from our side in order to not conflict with their pull in
> > > > > any state.
> > > >
> > > > What are these conflicting pull states?
> > > >
> > > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> > > > boot mode, and no pull in active mode. In reset and boot mode the host
> > > > config with a pull down would match, and no pull in active mode doesn't
> > > > conflict with the pull-down on the host UART. My understanding is that
> > > > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > > > RTS the pull on the host side shouldn't matter.
> > > >
> > >
> > > yes, I agree with you, the pinconf pulls are weak. As this is driven
> > > by BT
> > > SoC (pull on HOST side shouldn't matter), we are not mentioning any
> > > bias
> > > configuration from our side and simply putting it as no-pull, just
> > > to not
> > > conflict in any case. It seems that the rationale mentioned is a bit
> > > confusing i will change it to clearly specify why we are configuring
> > > no-pull.
> > >
> > > > Is this change actually related with wakeup support? I have the
> > > > impression
> > > > that multiple things are conflated in this patch. If some of the changes
> > > > are just fixing/improving other things they should be in a separate
> > > > patch,
> > > > which could be part of this series, otherwise it's really hard to
> > > > distinguish between the pieces that are actually relevant for wakeup and
> > > > the rest.
> > > >
> > > > Independently of whether the changes are done in a single or multiple
> > > > patches, the commit log should include details on why the changes are
> > > > necessary, especially when there are not explantatory comments in the
> > > > DT/code itself (e.g. the removal of 'output-high', which seems correct
> > > > to me, but no reason is given why it is done).
> > > >
> > >
> > > This change is not related to wakeup support, I will make it a
> > > separate
> > > patch, will also mention the details in commit text.
> > >
> > > > > };
> > > > >
> > > > > pinconf-rts {
> > > > > - /* We'll drive 39 (RTS), so no pull */
> > > > > + /*
> > > > > + * Configure pull-down on 39 (RTS). This is needed to
> > > > > avoid a
> > > > > +
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
Hi Matthias,
On 2020-08-25 22:08, Matthias Kaehlcke wrote:
On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > Add sleep pin ctrl for BT uart, and also change the bias
> > configuration to match Bluetooth module.
> >
> > Signed-off-by: satya priya
> > Reviewed-by: Akash Asthana
> > ---
> > 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.
> >
> > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > +++--
> > 1 file changed, 45 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > index d8b5507..806f626 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > @@ -473,20 +473,20 @@
> >
> > _uart3_default {
> > pinconf-cts {
> > - /*
> > - * Configure a pull-down on 38 (CTS) to match the pull of
> > - * the Bluetooth module.
> > - */
> > + /* Configure no pull on 38 (CTS) to match Bluetooth module */
> > pins = "gpio38";
> > - bias-pull-down;
> > - output-high;
> > + bias-disable;
>
> I think it should be ok in functional terms, but I don't like the
> rationale
> and also doubt the change is really needed.
>
> If the pull is removed to match the Bluetooth module, then that sounds
> as
> if the signal was floating on the the BT side, which I think is not the
> case.
> Yes, according to the datasheet there is no pull when the BT controller
> is
> active, but then it drives the signal actively to either high or low.
> There
> seems to be no merit in 'matching' the Bluetooth side in this case, if
> the
> signal was really floating on the BT side we would definitely not want
> this.
>
> In a reply to v2 you said:
>
> > Recently on cherokee we worked with BT team and came to an agreement
> > to
> > keep no-pull from our side in order to not conflict with their pull in
> > any state.
>
> What are these conflicting pull states?
>
> The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> boot mode, and no pull in active mode. In reset and boot mode the host
> config with a pull down would match, and no pull in active mode doesn't
> conflict with the pull-down on the host UART. My understanding is that
> the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> RTS the pull on the host side shouldn't matter.
>
yes, I agree with you, the pinconf pulls are weak. As this is driven
by BT
SoC (pull on HOST side shouldn't matter), we are not mentioning any
bias
configuration from our side and simply putting it as no-pull, just to
not
conflict in any case. It seems that the rationale mentioned is a bit
confusing i will change it to clearly specify why we are configuring
no-pull.
> Is this change actually related with wakeup support? I have the
> impression
> that multiple things are conflated in this patch. If some of the changes
> are just fixing/improving other things they should be in a separate
> patch,
> which could be part of this series, otherwise it's really hard to
> distinguish between the pieces that are actually relevant for wakeup and
> the rest.
>
> Independently of whether the changes are done in a single or multiple
> patches, the commit log should include details on why the changes are
> necessary, especially when there are not explantatory comments in the
> DT/code itself (e.g. the removal of 'output-high', which seems correct
> to me, but no reason is given why it is done).
>
This change is not related to wakeup support, I will make it a
separate
patch, will also mention the details in commit text.
> > };
> >
> > pinconf-rts {
> > - /* We'll drive 39 (RTS), so no pull */
> > + /*
> > + * Configure pull-down on 39 (RTS). This is needed to avoid a
> > + * floating pin which could mislead Bluetooth controller
> > + * with UART RFR state (READY/NOT_READY).
> > + */
> > pins = "gpio39";
> > drive-strength = <2>;
> > - bias-disable;
> > + bias-pull-down;
> > };
>
> [copy of my comment on v2]
>
> I'm a bit at a loss here, about two things:
>
> RTS is an output pin controlled by the UART. IIUC if the UART port is
> active
> and hardware flow control is enabled the RTS signal is either driven to
> high
> or low, but not floating.
Yes, RTS is either driven high or low. HW flow control is always
enabled and
only turned off when RX FIFO is full. Whereas SW flow control is
controlled
by upper layers(serial core), also it
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
> On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > > Add sleep pin ctrl for BT uart, and also change the bias
> > > configuration to match Bluetooth module.
> > >
> > > Signed-off-by: satya priya
> > > Reviewed-by: Akash Asthana
> > > ---
> > > 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.
> > >
> > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > +++--
> > > 1 file changed, 45 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > index d8b5507..806f626 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > @@ -473,20 +473,20 @@
> > >
> > > _uart3_default {
> > > pinconf-cts {
> > > - /*
> > > - * Configure a pull-down on 38 (CTS) to match the pull of
> > > - * the Bluetooth module.
> > > - */
> > > + /* Configure no pull on 38 (CTS) to match Bluetooth module */
> > > pins = "gpio38";
> > > - bias-pull-down;
> > > - output-high;
> > > + bias-disable;
> >
> > I think it should be ok in functional terms, but I don't like the
> > rationale
> > and also doubt the change is really needed.
> >
> > If the pull is removed to match the Bluetooth module, then that sounds
> > as
> > if the signal was floating on the the BT side, which I think is not the
> > case.
> > Yes, according to the datasheet there is no pull when the BT controller
> > is
> > active, but then it drives the signal actively to either high or low.
> > There
> > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > the
> > signal was really floating on the BT side we would definitely not want
> > this.
> >
> > In a reply to v2 you said:
> >
> > > Recently on cherokee we worked with BT team and came to an agreement
> > > to
> > > keep no-pull from our side in order to not conflict with their pull in
> > > any state.
> >
> > What are these conflicting pull states?
> >
> > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> > boot mode, and no pull in active mode. In reset and boot mode the host
> > config with a pull down would match, and no pull in active mode doesn't
> > conflict with the pull-down on the host UART. My understanding is that
> > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > RTS the pull on the host side shouldn't matter.
> >
>
> yes, I agree with you, the pinconf pulls are weak. As this is driven by BT
> SoC (pull on HOST side shouldn't matter), we are not mentioning any bias
> configuration from our side and simply putting it as no-pull, just to not
> conflict in any case. It seems that the rationale mentioned is a bit
> confusing i will change it to clearly specify why we are configuring
> no-pull.
>
> > Is this change actually related with wakeup support? I have the
> > impression
> > that multiple things are conflated in this patch. If some of the changes
> > are just fixing/improving other things they should be in a separate
> > patch,
> > which could be part of this series, otherwise it's really hard to
> > distinguish between the pieces that are actually relevant for wakeup and
> > the rest.
> >
> > Independently of whether the changes are done in a single or multiple
> > patches, the commit log should include details on why the changes are
> > necessary, especially when there are not explantatory comments in the
> > DT/code itself (e.g. the removal of 'output-high', which seems correct
> > to me, but no reason is given why it is done).
> >
>
> This change is not related to wakeup support, I will make it a separate
> patch, will also mention the details in commit text.
>
> > > };
> > >
> > > pinconf-rts {
> > > - /* We'll drive 39 (RTS), so no pull */
> > > + /*
> > > + * Configure pull-down on 39 (RTS). This is needed to avoid a
> > > + * floating pin which could mislead Bluetooth controller
> > > + * with UART RFR state (READY/NOT_READY).
> > > + */
> > > pins = "gpio39";
> > > drive-strength = <2>;
> > > - bias-disable;
> > > + bias-pull-down;
> > > };
> >
> > [copy of my comment on v2]
> >
> > I'm a bit at a loss here, about two things:
> >
> > RTS is an output pin controlled by the UART. IIUC if the UART port is
> > active
> > and hardware flow control is enabled the RTS signal is either driven to
> > high
> > or low, but not floating.
>
> Yes, RTS is either driven high or low. HW flow control is always enabled and
> only
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On 2020-08-21 22:52, Matthias Kaehlcke wrote:
On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
Add sleep pin ctrl for BT uart, and also change the bias
configuration to match Bluetooth module.
Signed-off-by: satya priya
Reviewed-by: Akash Asthana
---
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.
arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
+++--
1 file changed, 45 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
index d8b5507..806f626 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
@@ -473,20 +473,20 @@
_uart3_default {
pinconf-cts {
- /*
-* Configure a pull-down on 38 (CTS) to match the pull of
-* the Bluetooth module.
-*/
+ /* Configure no pull on 38 (CTS) to match Bluetooth module */
pins = "gpio38";
- bias-pull-down;
- output-high;
+ bias-disable;
I think it should be ok in functional terms, but I don't like the
rationale
and also doubt the change is really needed.
If the pull is removed to match the Bluetooth module, then that sounds
as
if the signal was floating on the the BT side, which I think is not the
case.
Yes, according to the datasheet there is no pull when the BT controller
is
active, but then it drives the signal actively to either high or low.
There
seems to be no merit in 'matching' the Bluetooth side in this case, if
the
signal was really floating on the BT side we would definitely not want
this.
In a reply to v2 you said:
Recently on cherokee we worked with BT team and came to an agreement
to
keep no-pull from our side in order to not conflict with their pull in
any state.
What are these conflicting pull states?
The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset
and
boot mode, and no pull in active mode. In reset and boot mode the host
config with a pull down would match, and no pull in active mode doesn't
conflict with the pull-down on the host UART. My understanding is that
the pinconf pulls are weak pulls, so as soon as the BT chip drives its
RTS the pull on the host side shouldn't matter.
yes, I agree with you, the pinconf pulls are weak. As this is driven by
BT SoC (pull on HOST side shouldn't matter), we are not mentioning any
bias configuration from our side and simply putting it as no-pull, just
to not conflict in any case. It seems that the rationale mentioned is a
bit confusing i will change it to clearly specify why we are configuring
no-pull.
Is this change actually related with wakeup support? I have the
impression
that multiple things are conflated in this patch. If some of the
changes
are just fixing/improving other things they should be in a separate
patch,
which could be part of this series, otherwise it's really hard to
distinguish between the pieces that are actually relevant for wakeup
and
the rest.
Independently of whether the changes are done in a single or multiple
patches, the commit log should include details on why the changes are
necessary, especially when there are not explantatory comments in the
DT/code itself (e.g. the removal of 'output-high', which seems correct
to me, but no reason is given why it is done).
This change is not related to wakeup support, I will make it a separate
patch, will also mention the details in commit text.
};
pinconf-rts {
- /* We'll drive 39 (RTS), so no pull */
+ /*
+* Configure pull-down on 39 (RTS). This is needed to avoid a
+* floating pin which could mislead Bluetooth controller
+* with UART RFR state (READY/NOT_READY).
+*/
pins = "gpio39";
drive-strength = <2>;
- bias-disable;
+ bias-pull-down;
};
[copy of my comment on v2]
I'm a bit at a loss here, about two things:
RTS is an output pin controlled by the UART. IIUC if the UART port is
active
and hardware flow control is enabled the RTS signal is either driven to
high
or low, but not floating.
Yes, RTS is either driven high or low. HW flow control is always enabled
and only turned off when RX FIFO is full. Whereas SW flow control is
controlled by upper layers(serial core), also it can be enabled/disabled
from host by calling set_mctrl.
Now lets assume I'm wrong with the above and RTS can be floating. We
only want
the BT SoC to send data when the host UART is ready to receive them,
right?
RTS is an active low signal, hence by configuring it as a pull-down the
BT
SoC can send data regardless of whether the host UART
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> Add sleep pin ctrl for BT uart, and also change the bias
> configuration to match Bluetooth module.
>
> Signed-off-by: satya priya
> Reviewed-by: Akash Asthana
> ---
> 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.
>
> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> +++--
> 1 file changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index d8b5507..806f626 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -473,20 +473,20 @@
>
> _uart3_default {
> pinconf-cts {
> - /*
> - * Configure a pull-down on 38 (CTS) to match the pull of
> - * the Bluetooth module.
> - */
> + /* Configure no pull on 38 (CTS) to match Bluetooth module */
> pins = "gpio38";
> - bias-pull-down;
> - output-high;
> + bias-disable;
I think it should be ok in functional terms, but I don't like the rationale
and also doubt the change is really needed.
If the pull is removed to match the Bluetooth module, then that sounds as
if the signal was floating on the the BT side, which I think is not the case.
Yes, according to the datasheet there is no pull when the BT controller is
active, but then it drives the signal actively to either high or low. There
seems to be no merit in 'matching' the Bluetooth side in this case, if the
signal was really floating on the BT side we would definitely not want this.
In a reply to v2 you said:
> Recently on cherokee we worked with BT team and came to an agreement to
> keep no-pull from our side in order to not conflict with their pull in
> any state.
What are these conflicting pull states?
The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
boot mode, and no pull in active mode. In reset and boot mode the host
config with a pull down would match, and no pull in active mode doesn't
conflict with the pull-down on the host UART. My understanding is that
the pinconf pulls are weak pulls, so as soon as the BT chip drives its
RTS the pull on the host side shouldn't matter.
Is this change actually related with wakeup support? I have the impression
that multiple things are conflated in this patch. If some of the changes
are just fixing/improving other things they should be in a separate patch,
which could be part of this series, otherwise it's really hard to
distinguish between the pieces that are actually relevant for wakeup and
the rest.
Independently of whether the changes are done in a single or multiple
patches, the commit log should include details on why the changes are
necessary, especially when there are not explantatory comments in the
DT/code itself (e.g. the removal of 'output-high', which seems correct
to me, but no reason is given why it is done).
> };
>
> pinconf-rts {
> - /* We'll drive 39 (RTS), so no pull */
> + /*
> + * Configure pull-down on 39 (RTS). This is needed to avoid a
> + * floating pin which could mislead Bluetooth controller
> + * with UART RFR state (READY/NOT_READY).
> + */
> pins = "gpio39";
> drive-strength = <2>;
> - bias-disable;
> + bias-pull-down;
> };
[copy of my comment on v2]
I'm a bit at a loss here, about two things:
RTS is an output pin controlled by the UART. IIUC if the UART port is active
and hardware flow control is enabled the RTS signal is either driven to high
or low, but not floating.
Now lets assume I'm wrong with the above and RTS can be floating. We only want
the BT SoC to send data when the host UART is ready to receive them, right?
RTS is an active low signal, hence by configuring it as a pull-down the BT
SoC can send data regardless of whether the host UART actually asserts RTS,
so the host UART may not be ready to receive it. I would argue that if there
is really such a thing as a floating RTS signal then it should have a pull-up,
to prevent the BT SoC from sending data at any time.
I'm not an expert in UART communication and pinconf, so it could be that I
got something wrong, but as of now it seems to me that no pull is the correct
config for RTS.
>
> pinconf-tx {
> @@ -494,7 +494,43 @@
> pins = "gpio40";
> drive-strength = <2>;
> bias-disable;
> - output-high;
> + };
> +
> + pinconf-rx {
> + /*
> + * Configure a pull-up on 41 (RX). This is needed to avoid
> + * garbage data when the TX pin of the Bluetooth module is
> +
