diff mbox series

[v3,09/14] arm64: dts: qcom: sc7280: Disable pull from pcie1_clkreq

Message ID 20220202132301.v3.9.I5f367dcce8107f2186b2aad4aef0dfcfafa034b9@changeid (mailing list archive)
State Changes Requested
Headers show
Series arm64: dts: qcom: sc7x80: A smattering of misc dts cleanups + herobrine-rev1 | expand

Commit Message

Doug Anderson Feb. 2, 2022, 9:23 p.m. UTC
I believe that the PCIe clkreq pin is an output. That means we
shouldn't have a pull enabled for it. Turn it off.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- ("sc7280-idp: Disable pull from pcie1_clkreq") new for v3.

 arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r0.dts | 2 +-
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi                   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Matthias Kaehlcke Feb. 3, 2022, 5:20 p.m. UTC | #1
On Wed, Feb 02, 2022 at 01:23:43PM -0800, Douglas Anderson wrote:
> I believe that the PCIe clkreq pin is an output. That means we
> shouldn't have a pull enabled for it. Turn it off.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Stephen Boyd Feb. 3, 2022, 9:42 p.m. UTC | #2
Quoting Douglas Anderson (2022-02-02 13:23:43)
> I believe that the PCIe clkreq pin is an output. That means we
> shouldn't have a pull enabled for it. Turn it off.

It sounds like it's a request from the PCI device to the PCI phy that
the clk should be on. I googled pcie clkreq open drain and this pdf[1]
says

"The CLKREQ# signal is an open drain, active low signal that is driven
low by the PCI Express M.2 add-I Card function to request that the PCI
Express reference clock be available (active clock state) in order to
allow the PCI Express interface to send/receive data"

so presumably if there isn't an external pull on the signal the open
drain feature will not work and the PCIe device won't be able to drive
it low.

[1] https://advdownload.advantech.com/productfile/PIS/96FD80-P512-LIS/Product%20-%20Datasheet/96FD80-P512-LIS_datasheet20180110154919.pdf
Doug Anderson Feb. 3, 2022, 9:53 p.m. UTC | #3
Hi,

On Thu, Feb 3, 2022 at 1:42 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Douglas Anderson (2022-02-02 13:23:43)
> > I believe that the PCIe clkreq pin is an output. That means we
> > shouldn't have a pull enabled for it. Turn it off.
>
> It sounds like it's a request from the PCI device to the PCI phy that
> the clk should be on. I googled pcie clkreq open drain and this pdf[1]
> says
>
> "The CLKREQ# signal is an open drain, active low signal that is driven
> low by the PCI Express M.2 add-I Card function to request that the PCI
> Express reference clock be available (active clock state) in order to
> allow the PCI Express interface to send/receive data"
>
> so presumably if there isn't an external pull on the signal the open
> drain feature will not work and the PCIe device won't be able to drive
> it low.
>
> [1] https://advdownload.advantech.com/productfile/PIS/96FD80-P512-LIS/Product%20-%20Datasheet/96FD80-P512-LIS_datasheet20180110154919.pdf

Yeah, I had some trouble figuring this out too, so if someone knows
better than me then I'm more than happy to take advice here. I thought
I had found something claiming that "clkreq" was an output and on the
schematic I have from Qualcomm it shows an arrow going out from the
SoC for this signal indicating that it's an output from the SoC. Of
course, those arrows are notoriously wrong but at least it's one piece
of evidence that someone thought this was an output from the SoC.

Hrm, but I just checked the sc7280 "datasheet" which claims that this
is an input. Sigh.

I guess the options are:
* If we're sure this is an input to the SoC then I think we should
remove the drive-strength, right?
* If we don't know then I guess we can leave both?


In any case, for now we can just drop this patch?

-Doug
Stephen Boyd Feb. 3, 2022, 9:59 p.m. UTC | #4
Quoting Doug Anderson (2022-02-03 13:53:09)
> Hi,
>
> On Thu, Feb 3, 2022 at 1:42 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Douglas Anderson (2022-02-02 13:23:43)
> > > I believe that the PCIe clkreq pin is an output. That means we
> > > shouldn't have a pull enabled for it. Turn it off.
> >
> > It sounds like it's a request from the PCI device to the PCI phy that
> > the clk should be on. I googled pcie clkreq open drain and this pdf[1]
> > says
> >
> > "The CLKREQ# signal is an open drain, active low signal that is driven
> > low by the PCI Express M.2 add-I Card function to request that the PCI
> > Express reference clock be available (active clock state) in order to
> > allow the PCI Express interface to send/receive data"
> >
> > so presumably if there isn't an external pull on the signal the open
> > drain feature will not work and the PCIe device won't be able to drive
> > it low.
> >
> > [1] https://advdownload.advantech.com/productfile/PIS/96FD80-P512-LIS/Product%20-%20Datasheet/96FD80-P512-LIS_datasheet20180110154919.pdf
>
> Yeah, I had some trouble figuring this out too, so if someone knows
> better than me then I'm more than happy to take advice here. I thought
> I had found something claiming that "clkreq" was an output and on the
> schematic I have from Qualcomm it shows an arrow going out from the
> SoC for this signal indicating that it's an output from the SoC. Of
> course, those arrows are notoriously wrong but at least it's one piece
> of evidence that someone thought this was an output from the SoC.
>
> Hrm, but I just checked the sc7280 "datasheet" which claims that this
> is an input. Sigh.
>
> I guess the options are:
> * If we're sure this is an input to the SoC then I think we should
> remove the drive-strength, right?
> * If we don't know then I guess we can leave both?

I'll wait for qcom folks to confirm. Maybe it's bidirectional because it
is an open drain signal. I'm showing my cards that I'm no PCIe expert :)

>
>
> In any case, for now we can just drop this patch?
>

Sounds good to me. It needs to be resolved through for herobrine-r1?
Doug Anderson Feb. 3, 2022, 11:42 p.m. UTC | #5
Hi,

On Thu, Feb 3, 2022 at 1:59 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2022-02-03 13:53:09)
> > Hi,
> >
> > On Thu, Feb 3, 2022 at 1:42 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Douglas Anderson (2022-02-02 13:23:43)
> > > > I believe that the PCIe clkreq pin is an output. That means we
> > > > shouldn't have a pull enabled for it. Turn it off.
> > >
> > > It sounds like it's a request from the PCI device to the PCI phy that
> > > the clk should be on. I googled pcie clkreq open drain and this pdf[1]
> > > says
> > >
> > > "The CLKREQ# signal is an open drain, active low signal that is driven
> > > low by the PCI Express M.2 add-I Card function to request that the PCI
> > > Express reference clock be available (active clock state) in order to
> > > allow the PCI Express interface to send/receive data"
> > >
> > > so presumably if there isn't an external pull on the signal the open
> > > drain feature will not work and the PCIe device won't be able to drive
> > > it low.
> > >
> > > [1] https://advdownload.advantech.com/productfile/PIS/96FD80-P512-LIS/Product%20-%20Datasheet/96FD80-P512-LIS_datasheet20180110154919.pdf
> >
> > Yeah, I had some trouble figuring this out too, so if someone knows
> > better than me then I'm more than happy to take advice here. I thought
> > I had found something claiming that "clkreq" was an output and on the
> > schematic I have from Qualcomm it shows an arrow going out from the
> > SoC for this signal indicating that it's an output from the SoC. Of
> > course, those arrows are notoriously wrong but at least it's one piece
> > of evidence that someone thought this was an output from the SoC.
> >
> > Hrm, but I just checked the sc7280 "datasheet" which claims that this
> > is an input. Sigh.
> >
> > I guess the options are:
> > * If we're sure this is an input to the SoC then I think we should
> > remove the drive-strength, right?
> > * If we don't know then I guess we can leave both?
>
> I'll wait for qcom folks to confirm. Maybe it's bidirectional because it
> is an open drain signal. I'm showing my cards that I'm no PCIe expert :)

Ah ha! I searched some more and found a Qualcomm PCIe user guide on this!

CLKREQ# signal properties – Bi-directional clock request signals
whether the RC or AP requires control

So it sounds as if leaving it as pull-up and having drive-strength as
2 is right.  tl;dr: drop this patch from the series...

> > In any case, for now we can just drop this patch?
> >
>
> Sounds good to me. It needs to be resolved through for herobrine-r1?

Yup. I responded to that patch and for now I'll wait for Bjorn to give
me direction on how to handle it.

-Doug
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r0.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r0.dts
index 82c3c8f0342b..3c5aab225748 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r0.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r0.dts
@@ -827,7 +827,7 @@  &usb_2_hsphy {
 /* PINCTRL - additions to nodes defined in sc7280.dtsi */
 
 &pcie1_clkreq_n {
-	bias-pull-up;
+	bias-disable;
 	drive-strength = <2>;
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 6e20e8c07402..9140dca3b72a 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -367,7 +367,7 @@  key_vol_up_default: key-vol-up-default {
 };
 
 &pcie1_clkreq_n {
-	bias-pull-up;
+	bias-disable;
 	drive-strength = <2>;
 };