Changes in v4 as suggested by Bjorn:
* Changed pipe clk mux name as gcc_pcie_1_pipe_clk_src.
* Changed pipe_ext_src as phy_pipe_clk.
* Updated commit message for [PATCH v4 4/4].
Changes in v3:
* Changed pipe clock names in dt bindings as pipe_mux and phy_pipe.
* Moved reset and NVMe GPIO pin configs into board specific file.
* Updated pipe clk mux commit message.
Changes in v2:
* Moved pcie pin control settings into IDP file.
* Replaced pipe_clk_src with pipe_clk_mux in pcie driver
* Included pipe clk mux setting change set in this series
Prasad Malisetty (4):
dt-bindings: pci: qcom: Document PCIe bindings for SC720
arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
PCIe: qcom: Add support to control pipe clk src
.../devicetree/bindings/pci/qcom,pcie.txt | 17 +++
arch/arm64/boot/dts/qcom/sc7280-idp.dts | 38 +++++++
arch/arm64/boot/dts/qcom/sc7280.dtsi | 125 +++++++++++++++++++++
drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++
4 files changed, 202 insertions(+)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
to the one used on SM8250. Add the compatible for SC7280.
Signed-off-by: Prasad Malisetty <redacted>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
@@ -12,6 +12,7 @@ - "qcom,pcie-ipq4019" for ipq4019 - "qcom,pcie-ipq8074" for ipq8074 - "qcom,pcie-qcs404" for qcs404+ - "qcom,pcie-sc7280" for sc7280 - "qcom,pcie-sdm845" for sdm845 - "qcom,pcie-sm8250" for sm8250 - "qcom,pcie-ipq6018" for ipq6018
@@ -144,6 +145,22 @@ - "slave_bus" AXI Slave clock - clock-names:+ Usage: required for sc7280+ Value type: <stringlist>+ Definition: Should contain the following entries+ - "aux" Auxiliary clock+ - "cfg" Configuration clock+ - "bus_master" Master AXI clock+ - "bus_slave" Slave AXI clock+ - "slave_q2a" Slave Q2A clock+ - "tbu" PCIe TBU clock+ - "ddrss_sf_tbu" PCIe SF TBU clock+ - "pipe" PIPE clock+ - "pipe_mux" PIPE MUX+ - "phy_pipe" PIPE output clock+ - "ref" REFERENCE clock++- clock-names: Usage: required for sdm845 Value type: <stringlist> Definition: Should contain the following entries
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Stephen Boyd <hidden> Date: 2021-07-16 19:27:59
Quoting Prasad Malisetty (2021-07-16 06:58:44)
Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
to the one used on SM8250. Add the compatible for SC7280.
Signed-off-by: Prasad Malisetty <redacted>
Acked-by: Rob Herring <robh@kernel.org>
---
Reviewed-by: Stephen Boyd <redacted>
Any chance this file can be converted to YAML?
Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
to the one used on SM8250. Add the compatible for SC7280.
Signed-off-by: Prasad Malisetty <redacted>
Acked-by: Rob Herring <robh@kernel.org>
---
Reviewed-by: Stephen Boyd <redacted>
Any chance this file can be converted to YAML?
Yes, we can convert into YAML. we will incorporate the change in next
version.
Is this the gpucc node? At address 3d90000? Please sort the nodes based
on their address, so this would be 1c08000 which comes before gpucc and
some others in this file.
Is this the gpucc node? At address 3d90000? Please sort the nodes based
on their address, so this would be 1c08000 which comes before gpucc and
some others in this file.
This could be essentially any GPIO, right? Does it really make sense to
have this node in the SoC file? I would say it belongs in the board
file.
Hi Matthias,
Thanks for your review and comments.
Sorry for the delay in acknowledging. I would move this entry in IDP
file in next version.
Thanks
-Prasad
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
Signed-off-by: Prasad Malisetty <redacted>
---
drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Run this:
$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
and make your subject match the style and structure (in particular,
s/PCIe/PCI/). In this case, maybe something like this?
PCI: qcom: Switch sc7280 gcc_pcie_1_pipe_clk_src after PHY init
On Fri, Jul 16, 2021 at 07:28:47PM +0530, Prasad Malisetty wrote:
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
This says what *needs* to happen, but it doesn't actually say what
this patch *does*. I think it's something like:
On the sc7280 SoC, the clock source for pcie_1_pipe must be the TCXO
while gdsc is enabled. But after the PHY is initialized, the clock
source must be switched to gcc_pcie_1_pipe_clk.
On sc7280, switch gcc_pcie_1_pipe_clk_src from TCXO to
gcc_pcie_1_pipe_clk after the PHY has been initialized.
Nits: Rewrap to fill 75 columns or so. Add blank lines between
paragraphs. Start sentences with capital letter.
Using of_device_is_compatible() follows existing style in the driver,
which is good. But I'm not sure that's good style in general because
it's a little repetitious and wasteful.
qcom_pcie_probe() already calls of_device_get_match_data(), which does
basically the same thing as of_device_is_compatible(), so I think we
could take better advantage of that by augmenting struct qcom_pcie_ops
with these device-specific details.
Some drivers that use this strategy:
drivers/pci/controller/cadence/pci-j721e.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-layerscape.c
drivers/pci/controller/dwc/pci-layerscape-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/pci-ftpci100.c
drivers/pci/controller/pcie-brcmstb.c
drivers/pci/controller/pcie-mediatek.c
+ clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);
return clk_prepare_enable(res->pipe_clk);
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri 16 Jul 10:06 CDT 2021, Bjorn Helgaas wrote:
Run this:
$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
and make your subject match the style and structure (in particular,
s/PCIe/PCI/). In this case, maybe something like this?
PCI: qcom: Switch sc7280 gcc_pcie_1_pipe_clk_src after PHY init
On Fri, Jul 16, 2021 at 07:28:47PM +0530, Prasad Malisetty wrote:
quoted
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
This says what *needs* to happen, but it doesn't actually say what
this patch *does*. I think it's something like:
On the sc7280 SoC, the clock source for pcie_1_pipe must be the TCXO
while gdsc is enabled. But after the PHY is initialized, the clock
source must be switched to gcc_pcie_1_pipe_clk.
On sc7280, switch gcc_pcie_1_pipe_clk_src from TCXO to
gcc_pcie_1_pipe_clk after the PHY has been initialized.
Nits: Rewrap to fill 75 columns or so. Add blank lines between
paragraphs. Start sentences with capital letter.
Using of_device_is_compatible() follows existing style in the driver,
which is good. But I'm not sure that's good style in general because
it's a little repetitious and wasteful.
Following the style is good, but up until the recent sm8250 addition it
was just a hack to deal with legacy platforms that we don't know the
exact details about.
But, all platforms I know of has the pipe_clk from the PHY fed into the
pipe_clk_src mux in the gcc block and then ends up in the PCIe
controller. As such, I suspect that the pipe_clk handling should be moved
to the common code path of the driver and there's definitely no harm in
making sure that the pipe_clk_src mux is explicitly configured on
existing platforms (at least all 2.7.0 based ones).
qcom_pcie_probe() already calls of_device_get_match_data(), which does
basically the same thing as of_device_is_compatible(), so I think we
could take better advantage of that by augmenting struct qcom_pcie_ops
with these device-specific details.
I agree.
Regards,
Bjorn
Some drivers that use this strategy:
drivers/pci/controller/cadence/pci-j721e.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-layerscape.c
drivers/pci/controller/dwc/pci-layerscape-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/pci-ftpci100.c
drivers/pci/controller/pcie-brcmstb.c
drivers/pci/controller/pcie-mediatek.c
quoted
+ clk_set_parent(res->gcc_pcie_1_pipe_clk_src, res->phy_pipe_clk);
return clk_prepare_enable(res->pipe_clk);
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri 16 Jul 10:06 CDT 2021, Bjorn Helgaas wrote:
quoted
Run this:
$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
and make your subject match the style and structure (in particular,
s/PCIe/PCI/). In this case, maybe something like this?
PCI: qcom: Switch sc7280 gcc_pcie_1_pipe_clk_src after PHY init
On Fri, Jul 16, 2021 at 07:28:47PM +0530, Prasad Malisetty wrote:
quoted
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
This says what *needs* to happen, but it doesn't actually say what
this patch *does*. I think it's something like:
On the sc7280 SoC, the clock source for pcie_1_pipe must be the TCXO
while gdsc is enabled. But after the PHY is initialized, the clock
source must be switched to gcc_pcie_1_pipe_clk.
On sc7280, switch gcc_pcie_1_pipe_clk_src from TCXO to
gcc_pcie_1_pipe_clk after the PHY has been initialized.
Nits: Rewrap to fill 75 columns or so. Add blank lines between
paragraphs. Start sentences with capital letter.
Agree, looks good. will add more details and update the commit message
in next version.
Using of_device_is_compatible() follows existing style in the driver,
which is good. But I'm not sure that's good style in general because
it's a little repetitious and wasteful.
Following the style is good, but up until the recent sm8250 addition it
was just a hack to deal with legacy platforms that we don't know the
exact details about.
But, all platforms I know of has the pipe_clk from the PHY fed into the
pipe_clk_src mux in the gcc block and then ends up in the PCIe
controller. As such, I suspect that the pipe_clk handling should be
moved
to the common code path of the driver and there's definitely no harm in
making sure that the pipe_clk_src mux is explicitly configured on
existing platforms (at least all 2.7.0 based ones).
quoted
qcom_pcie_probe() already calls of_device_get_match_data(), which does
basically the same thing as of_device_is_compatible(), so I think we
could take better advantage of that by augmenting struct qcom_pcie_ops
with these device-specific details.
I agree.
Regards,
Bjorn
quoted
Some drivers that use this strategy:
drivers/pci/controller/cadence/pci-j721e.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-layerscape.c
drivers/pci/controller/dwc/pci-layerscape-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/pci-ftpci100.c
drivers/pci/controller/pcie-brcmstb.c
drivers/pci/controller/pcie-mediatek.c
Sure, we will make use of struct qcom_pcie_ops and add a new callback to
configure pipe clk src.
In coming platforms, if the platform doesn't need to configure pipe clk
src, it will return as callback not defined.
We will incorporate the changes in next release.
quoted
quoted
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri 16 Jul 10:06 CDT 2021, Bjorn Helgaas wrote:
quoted
Run this:
$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
and make your subject match the style and structure (in particular,
s/PCIe/PCI/). In this case, maybe something like this?
PCI: qcom: Switch sc7280 gcc_pcie_1_pipe_clk_src after PHY init
On Fri, Jul 16, 2021 at 07:28:47PM +0530, Prasad Malisetty wrote:
quoted
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
This says what *needs* to happen, but it doesn't actually say what
this patch *does*. I think it's something like:
On the sc7280 SoC, the clock source for pcie_1_pipe must be the TCXO
while gdsc is enabled. But after the PHY is initialized, the clock
source must be switched to gcc_pcie_1_pipe_clk.
On sc7280, switch gcc_pcie_1_pipe_clk_src from TCXO to
gcc_pcie_1_pipe_clk after the PHY has been initialized.
Nits: Rewrap to fill 75 columns or so. Add blank lines between
paragraphs. Start sentences with capital letter.
Using of_device_is_compatible() follows existing style in the driver,
which is good. But I'm not sure that's good style in general because
it's a little repetitious and wasteful.
Following the style is good, but up until the recent sm8250 addition it
was just a hack to deal with legacy platforms that we don't know the
exact details about.
But, all platforms I know of has the pipe_clk from the PHY fed into the
pipe_clk_src mux in the gcc block and then ends up in the PCIe
controller. As such, I suspect that the pipe_clk handling should be
moved
to the common code path of the driver and there's definitely no harm in
making sure that the pipe_clk_src mux is explicitly configured on
existing platforms (at least all 2.7.0 based ones).
quoted
qcom_pcie_probe() already calls of_device_get_match_data(), which does
basically the same thing as of_device_is_compatible(), so I think we
could take better advantage of that by augmenting struct qcom_pcie_ops
with these device-specific details.
I agree.
Regards,
Bjorn
quoted
Some drivers that use this strategy:
drivers/pci/controller/cadence/pci-j721e.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-layerscape.c
drivers/pci/controller/dwc/pci-layerscape-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/pci-ftpci100.c
drivers/pci/controller/pcie-brcmstb.c
drivers/pci/controller/pcie-mediatek.c
Hi Bjorn,
Thanks for your review and inputs.
I would like to add more details on this requirement. The hardware ver.
of SM8250 & SC7280 platforms is the same but
where as only for SC7280, we need to switch gcc_pcie_1_pipe_clk source
between TXCO and gcc_pcie_1_pipe_clk hence this is SoC level
specific requirement. all existing platforms doesn't need this pipe clk
handling.
I will use boolean flag entry in SoC dtsi such as
"pipe-clk-source-switch" to differentiate between SM8250 and SC7280
instead of compatible
method.
Please provide any other better approach for the above case.
Thanks
-Prasad
quoted
quoted
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Stephen Boyd <hidden> Date: 2021-07-16 19:37:31
Quoting Prasad Malisetty (2021-07-16 06:58:47)
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
Why? Can you add that detail here? Presumably it's something like the
GDSC needs a running clk to send a reset through the flops or something
like that.
quoted hunk
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
Signed-off-by: Prasad Malisetty <redacted>
---
drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Is anything wrong if we call clk_set_parent() here when this driver is
running on previous SoCs where the parent is assigned via DT? Also,
shouldn't we make sure the parent is XO at driver probe time so that
powering on the GDSC works properly?
It all feels like a kludge though given that the GDSC is the one that
requires the clock to be running at XO and we're working around that in
the pcie driver instead of sticking that logic into the GDSC. What do we
do if the GDSC is already enabled out of boot instead of being the power
on reset (POR) configuration?
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
Why? Can you add that detail here? Presumably it's something like the
GDSC needs a running clk to send a reset through the flops or something
like that.
Which presumably means that we need to "park" gcc_pcie_N_pipe_clk_src
whenever the PHY pipe is paused due to a suspend or runtime suspend.
I find this part of the commit message to primarily describing the next
patch (that is yet to be posted).
quoted
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
Signed-off-by: Prasad Malisetty <redacted>
---
drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Is anything wrong if we call clk_set_parent() here when this driver is
running on previous SoCs where the parent is assigned via DT?
We don't assign the parent on previous platforms, we apparently just
rely on the reset value (afaict).
So I think it makes sense for all platforms to explicitly mux
pipe_clk_src to phy::pipe_clk, one the PHY is up and running.
But I was under the impression that we have the BRANCH_HALT_SKIP on the
pipe_clk because there was some sort of feedback loop to the PHY's
calibration... What this patch indicates is that we should park
pipe_clk_src onto XO at boot time, then after the PHY starts ticking we
should enable and reparent the clk_src - at which point I don't see why
we need the HALT_SKIP.
Also, shouldn't we make sure the parent is XO at driver probe time so
that powering on the GDSC works properly?
It all feels like a kludge though given that the GDSC is the one that
requires the clock to be running at XO and we're working around that in
the pcie driver instead of sticking that logic into the GDSC. What do we
do if the GDSC is already enabled out of boot instead of being the power
on reset (POR) configuration?
What happens if we boot the device out of NVME...
PS. Are we certain that it's the PCIe driver and not the PHY that should
do this dance? I really would like to see the continuation of this patch
to see the full picture...
Regards,
Bjorn
From: Stephen Boyd <hidden> Date: 2021-07-16 21:39:16
Quoting Bjorn Andersson (2021-07-16 13:31:55)
On Fri 16 Jul 14:37 CDT 2021, Stephen Boyd wrote:
quoted
Quoting Prasad Malisetty (2021-07-16 06:58:47)
quoted
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
Why? Can you add that detail here? Presumably it's something like the
GDSC needs a running clk to send a reset through the flops or something
like that.
Which presumably means that we need to "park" gcc_pcie_N_pipe_clk_src
whenever the PHY pipe is paused due to a suspend or runtime suspend.
I find this part of the commit message to primarily describing the next
patch (that is yet to be posted).
Ah I see. So there will be another patch to do the park and unpark over
suspend/resume?
quoted
quoted
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
Signed-off-by: Prasad Malisetty <redacted>
---
drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Is anything wrong if we call clk_set_parent() here when this driver is
running on previous SoCs where the parent is assigned via DT?
We don't assign the parent on previous platforms, we apparently just
rely on the reset value (afaict).
Oh sheesh. I thought that was being done already. It looks like at least
on sdm845 that there is only one parent for this clk so we don't need to
call clk_set_parent to set it there.
So I think it makes sense for all platforms to explicitly mux
pipe_clk_src to phy::pipe_clk, one the PHY is up and running.
Sure, except some platforms don't have a mux?
But I was under the impression that we have the BRANCH_HALT_SKIP on the
pipe_clk because there was some sort of feedback loop to the PHY's
calibration... What this patch indicates is that we should park
pipe_clk_src onto XO at boot time, then after the PHY starts ticking we
should enable and reparent the clk_src - at which point I don't see why
we need the HALT_SKIP.
I recall that qcom folks kept saying they needed to enable the
pipe_clk_src clk branch in GCC before enabling the phy. So they required
the halt skip flag so that the clk_prepare_enable() call would
effectively set the enable bit in GCC and move on without caring. Then
they could enable the upstream clk source in the phy without having to
stop halfway through to enable the branch in GCC. The whole design here
is pretty insane.
In fact, I think we discussed this whole topic in late 2019[1] and we
concluded that we could just slam the clk on forever and deal with the
clk_set_parent() when the clk became a mux+gate instead of a pure gate.
quoted
Also, shouldn't we make sure the parent is XO at driver probe time so
that powering on the GDSC works properly?
It all feels like a kludge though given that the GDSC is the one that
requires the clock to be running at XO and we're working around that in
the pcie driver instead of sticking that logic into the GDSC. What do we
do if the GDSC is already enabled out of boot instead of being the power
on reset (POR) configuration?
What happens if we boot the device out of NVME...
I guess it's fine? The GDSC will be on and the parent clk will already
be set so things are a no-op.
PS. Are we certain that it's the PCIe driver and not the PHY that should
do this dance? I really would like to see the continuation of this patch
to see the full picture...
This is a new requirement for sc7280 SoC.
To enable gdsc gcc_pcie_1_pipe_clk_src should be TCXO.
Why? Can you add that detail here? Presumably it's something like the
GDSC needs a running clk to send a reset through the flops or something
like that.
Which presumably means that we need to "park" gcc_pcie_N_pipe_clk_src
whenever the PHY pipe is paused due to a suspend or runtime suspend.
I find this part of the commit message to primarily describing the next
patch (that is yet to be posted).
Ah I see. So there will be another patch to do the park and unpark over
suspend/resume?
That's my understanding.
quoted
quoted
quoted
after PHY initialization gcc_pcie_1_pipe_clk_src needs
to switch from TCXO to gcc_pcie_1_pipe_clk.
Signed-off-by: Prasad Malisetty <redacted>
---
drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Is anything wrong if we call clk_set_parent() here when this driver is
running on previous SoCs where the parent is assigned via DT?
We don't assign the parent on previous platforms, we apparently just
rely on the reset value (afaict).
Oh sheesh. I thought that was being done already. It looks like at least
on sdm845 that there is only one parent for this clk so we don't need to
call clk_set_parent to set it there.
I'll have to check the documentation on that...
quoted
So I think it makes sense for all platforms to explicitly mux
pipe_clk_src to phy::pipe_clk, one the PHY is up and running.
Sure, except some platforms don't have a mux?
quoted
But I was under the impression that we have the BRANCH_HALT_SKIP on the
pipe_clk because there was some sort of feedback loop to the PHY's
calibration... What this patch indicates is that we should park
pipe_clk_src onto XO at boot time, then after the PHY starts ticking we
should enable and reparent the clk_src - at which point I don't see why
we need the HALT_SKIP.
I recall that qcom folks kept saying they needed to enable the
pipe_clk_src clk branch in GCC before enabling the phy. So they required
the halt skip flag so that the clk_prepare_enable() call would
effectively set the enable bit in GCC and move on without caring. Then
they could enable the upstream clk source in the phy without having to
stop halfway through to enable the branch in GCC. The whole design here
is pretty insane.
In fact, I think we discussed this whole topic in late 2019[1] and we
concluded that we could just slam the clk on forever and deal with the
clk_set_parent() when the clk became a mux+gate instead of a pure gate.
That's exactly what I asked Prasad about, because per the description
and content of this patch the parent pipe_clk_src will remain XO until
the PHY is initialized. So either the PHY no longer need gcc in the loop
to calibrate the pipe clock or it used to, but no longer does.
Thanks for the link, we definitely should clean that up, but I think at
this point it might be worth waiting a little bit longer to see what
actually going to happen in the suspend/resume (system and runtime)
paths...
quoted
quoted
Also, shouldn't we make sure the parent is XO at driver probe time so
that powering on the GDSC works properly?
It all feels like a kludge though given that the GDSC is the one that
requires the clock to be running at XO and we're working around that in
the pcie driver instead of sticking that logic into the GDSC. What do we
do if the GDSC is already enabled out of boot instead of being the power
on reset (POR) configuration?
What happens if we boot the device out of NVME...
I guess it's fine? The GDSC will be on and the parent clk will already
be set so things are a no-op.
Yes, if the pipe_clk_src is parked nicely in late initcall, so that when the
pd late init cuts the GDSC things will end up in a clean state.
Regards,
Bjorn
quoted
PS. Are we certain that it's the PCIe driver and not the PHY that should
do this dance? I really would like to see the continuation of this patch
to see the full picture...
[+cc linux-pci]
On Fri, Jul 16, 2021 at 07:28:43PM +0530, Prasad Malisetty wrote:
Changes in v4 as suggested by Bjorn:
* Changed pipe clk mux name as gcc_pcie_1_pipe_clk_src.
* Changed pipe_ext_src as phy_pipe_clk.
* Updated commit message for [PATCH v4 4/4].
Changes in v3:
* Changed pipe clock names in dt bindings as pipe_mux and phy_pipe.
* Moved reset and NVMe GPIO pin configs into board specific file.
* Updated pipe clk mux commit message.
Changes in v2:
* Moved pcie pin control settings into IDP file.
* Replaced pipe_clk_src with pipe_clk_mux in pcie driver
* Included pipe clk mux setting change set in this series
Prasad Malisetty (4):
dt-bindings: pci: qcom: Document PCIe bindings for SC720
arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
PCIe: qcom: Add support to control pipe clk src
.../devicetree/bindings/pci/qcom,pcie.txt | 17 +++
arch/arm64/boot/dts/qcom/sc7280-idp.dts | 38 +++++++
arch/arm64/boot/dts/qcom/sc7280.dtsi | 125 +++++++++++++++++++++
drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++
$ ./scripts/get_maintainer.pl -f drivers/pci/controller/dwc/pcie-qcom.c
tells you that linux-pci should be included. One reason that's
important is because patchwork watches linux-pci for incoming patches,
and I use patchwork as my to-do list.