| Message ID | 20220202212348.1391534-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sc7x80: A smattering of misc dts cleanups + herobrine-rev1 | expand |
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>
Quoting Douglas Anderson (2022-02-02 13:23:39) > Specifying "input-enable" on a MSM GPIO is a no-op for the most > part. The only thing it really does is to explicitly force the output > of a GPIO to be disabled right at the point of a pinctrl > transition. We don't need to do this and we don't typically specify > "input-enable" unless there's a good reason to. Remove it. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Quoting Douglas Anderson (2022-02-02 13:23:41) > Like dp_out, we should have defined edp_out in sc7280.dtsi so we don't > need to do this in the board files. > > Like dp_hot_plug_det, we should define edp_hot_plug_det in > sc7280.dtsi. > > We should set the default pinctrl for edp_hot_plug_det in > sc7280.dtsi. NOTE: this is _unlike_ the dp_hot_plug_det. It is > reasonable that in some boards the dedicated DP Hot Plug Detect will > not be hooked up in favor of Type C mechanisms. This is unlike eDP > where the Hot Plug Detect line (which functions as "panel ready" in > eDP) is highly likely to be used by boards. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
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
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
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?
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
On Wed, 2 Feb 2022 13:23:34 -0800, Douglas Anderson wrote: > This series is "v2" of my "smattering of misc dts cleanups" series > plus v3 of the tail end of the series adding herobrine-rev1. I've set > the version number to the larger of the two to (I hope) help > allevitate confusion. > > For the cleanups, there's not a lot holding this series together > except that it fixes a smattering of random dts stuff that I noticed > recently. There are not a lot of dependencies and some of the patches > could be reordered if desired. > > [...] Applied, thanks! [01/14] arm64: dts: qcom: sc7180-trogdor: Add "-regulator" suffix to pp3300_hub commit: 171bac46700fcdb2310209dffb382533fe54522a [02/14] arm64: dts: qcom: sc7280-herobrine: Consistently add "-regulator" suffix commit: 7a86ac04056569bf5ec663fbb02d79c5e304545a [03/14] arm64: dts: qcom: sc7280: Properly sort sdc pinctrl lines commit: b1969bc522187dc6f436301eb71051b24135b607 [04/14] arm64: dts: qcom: sc7280: Clean up sdc1 / sdc2 pinctrl commit: f9800dde34e678d7ed1de9e95b4b65a257fd0f93 [05/14] arm64: dts: qcom: sc7280-idp: No need for "input-enable" on sw_ctrl commit: 8fdedd6c64643884dc6bbf6d9a7aabda1713354f [06/14] arm64: dts: qcom: sc7280: Fix sort order of dp_hot_plug_det / pcie1_clkreq_n commit: bbef2a9ca08749c89925d2bb49f4ce1c945acc90 [07/14] arm64: dts: qcom: sc7280: Add edp_out port and HPD lines commit: 118cd3b8ec0db02eb7306c30c1551ef9af885689 [08/14] arm64: dts: qcom: sc7280: Move pcie1_clkreq pull / drive str to boards commit: 376e9183c1d1dde6972257a823cf484cc5124b7b [10/14] arm64: dts: qcom: sc7280: Move dp_hot_plug_det pull from SoC dtsi file commit: ad4152d6e2599c62ef012e528acc5e77ca6765c1 [11/14] arm64: dts: qcom: sc7280: Add a blank line in the dp node commit: 96b34a6ea7d03876fb9b82ac8db5648a24fc7b2e Best regards,
This series is "v2" of my "smattering of misc dts cleanups" series plus v3 of the tail end of the series adding herobrine-rev1. I've set the version number to the larger of the two to (I hope) help allevitate confusion. For the cleanups, there's not a lot holding this series together except that it fixes a smattering of random dts stuff that I noticed recently. There are not a lot of dependencies and some of the patches could be reordered if desired. Hopefully these look OK and can be applied quickly to avoid conflicts with other work going on. For herobrine-rev1, it can be noted that it's likely that with the introduction of -rev1 we can drop -rev0 support, but we'll keep it for now (though we won't try to "fit it in" and share code with it). This series is confirmed to boot atop the top of the linux qualcomm tree, commit a5ee6b7720cb ("Merge branches 'arm64-defconfig-for-5.18', 'arm64-for-5.18', 'dts-for-5.18', 'arm64-fixes-for-5.17' and 'dts-fixes-for-5.17' into for-next") Changes in v3: - Removed extra blank lines - ("Fix sort order of dp_hot_plug_det") new for v3. - ("Add edp_out port and HPD lines") new for v3. - ("Move pcie1_clkreq pull / drive str to boards") new for v3. - ("sc7280-idp: Disable pull from pcie1_clkreq") new for v3. - ("Remove dp_hot_plug_det pull from SoC dtsi file") new for v3. - ("Add a blank line in the dp node") new for v3. - Rebased atop dts cleanup patches. - Add regulator suffix as per dts cleanup patches. - Set PCIe bias / pull as per dts cleanup patches. - Add dp_hot_plug_det pull as per dts cleanup patches. - Setup SD card same as dts cleanup patches. - ("sc7280: Add the CPU compatible to the soc@0 node") new for v3. - ("Remove "qcom,sc7280" from top-level") patch new for v3. Changes in v2: - Herobrine compatible on one line, not two - Wording change in comments for components enabled per-board - Always sort "bias" above "drive-strength" in pinctrl. - Properly sort "hub_en" pinctrl. - Two comments moved from multiline to single line. - Space after "/delete-property/" Douglas Anderson (14): arm64: dts: qcom: sc7180-trogdor: Add "-regulator" suffix to pp3300_hub arm64: dts: qcom: sc7280-herobrine: Consistently add "-regulator" suffix arm64: dts: qcom: sc7280: Properly sort sdc pinctrl lines arm64: dts: qcom: sc7280: Clean up sdc1 / sdc2 pinctrl arm64: dts: qcom: sc7280-idp: No need for "input-enable" on sw_ctrl arm64: dts: qcom: sc7280: Fix sort order of dp_hot_plug_det / pcie1_clkreq_n arm64: dts: qcom: sc7280: Add edp_out port and HPD lines arm64: dts: qcom: sc7280: Move pcie1_clkreq pull / drive str to boards arm64: dts: qcom: sc7280: Disable pull from pcie1_clkreq arm64: dts: qcom: sc7280: Move dp_hot_plug_det pull from SoC dtsi file arm64: dts: qcom: sc7280: Add a blank line in the dp node arm64: dts: qcom: sc7280: Add herobrine-r1 arm64: dts: qcom: sc7280: Add the CPU compatible to the soc@0 node arm64: dts: qcom: sc7280: Remove "qcom,sc7280" from top-level of boards arch/arm64/boot/dts/qcom/Makefile | 1 + arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 2 +- arch/arm64/boot/dts/qcom/sc7280-crd.dts | 2 +- .../qcom/sc7280-herobrine-herobrine-r0.dts | 97 +-- .../qcom/sc7280-herobrine-herobrine-r1.dts | 313 +++++++ .../arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 785 ++++++++++++++++++ arch/arm64/boot/dts/qcom/sc7280-idp.dts | 2 +- arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 99 +-- arch/arm64/boot/dts/qcom/sc7280-idp2.dts | 2 +- arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 547 ++++++++++++ arch/arm64/boot/dts/qcom/sc7280.dtsi | 182 ++-- 11 files changed, 1845 insertions(+), 187 deletions(-) create mode 100644 arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts create mode 100644 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi create mode 100644 arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi