Some QMP PHYs need to remux to their pipe clock input to the pipe clock
output generated by the PHY before powering on the PHY and restore the
default source during power down.
Add support for an optional pipe clock mux which will be reparented to
the generated pipe clock before powering on the PHY and restored to the
default reference source on power off.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
1 file changed, 65 insertions(+), 6 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 7d2d1ab061f7..bc6db9670291 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
* @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
* @pcs_misc: iomapped memory space for lane's pcs_misc
* @pipe_clk: pipe clock
+ * @pipemux_clk: pipe clock source mux
+ * @piperef_clk: pipe clock default reference source
* @index: lane index
* @qmp: QMP phy to which this lane belongs
* @lane_rst: lane's reset controller
@@ -3311,6 +3313,8 @@ struct qmp_phy {
void __iomem *rx2;
void __iomem *pcs_misc;
struct clk *pipe_clk;
+ struct clk *pipemux_clk;
+ struct clk *piperef_clk;
unsigned int index;
struct qcom_qmp *qmp;
struct reset_control *lane_rst;
@@ -3346,6 +3350,7 @@ struct qcom_qmp {
void __iomem *dp_com;
struct clk_bulk_data *clks;
+ struct clk *pipe_clksrc;
struct reset_control **resets;
struct regulator_bulk_data *vregs;
@@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
return 0;
}
+static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
+{
+ struct qcom_qmp *qmp = qphy->qmp;
+ int ret;
+
+ ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
+ if (ret)
+ dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
+
+
+ ret = clk_prepare_enable(qphy->pipe_clk);
+ if (ret) {
+ dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
+ goto err_restore_parent;
+ }
+
+ return 0;
+
+err_restore_parent:
+ clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
+
+ return ret;
+}
+
+static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
+{
+ struct qcom_qmp *qmp = qphy->qmp;
+ int ret;
+
+ clk_disable_unprepare(qphy->pipe_clk);
+
+ ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
+ if (ret)
+ dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
+}
+
static int qcom_qmp_phy_power_on(struct phy *phy)
{
struct qmp_phy *qphy = phy_get_drvdata(phy);
@@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
}
}
- ret = clk_prepare_enable(qphy->pipe_clk);
- if (ret) {
- dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
+ ret = qcom_qmp_phy_pipe_clk_enable(qphy);
+ if (ret)
goto err_reset_lane;
- }
/* Tx, Rx, and PCS configurations */
qcom_qmp_phy_configure_lane(tx, cfg->regs,
@@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
return 0;
err_disable_pipe_clk:
- clk_disable_unprepare(qphy->pipe_clk);
+ qcom_qmp_phy_pipe_clk_disable(qphy);
err_reset_lane:
if (cfg->has_lane_rst)
reset_control_assert(qphy->lane_rst);
@@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
struct qmp_phy *qphy = phy_get_drvdata(phy);
const struct qmp_phy_cfg *cfg = qphy->cfg;
- clk_disable_unprepare(qphy->pipe_clk);
+ qcom_qmp_phy_pipe_clk_disable(qphy);
if (cfg->type == PHY_TYPE_DP) {
/* Assert DP PHY power down */
@@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
if (ret)
return ret;
+ qmp->pipe_clksrc = fixed->hw.clk;
+
ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
if (ret)
return ret;
@@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
qphy->pipe_clk = NULL;
}
+ /* Get optional pipe clock mux and default reference source clock. */
+ qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
+ if (IS_ERR(qphy->pipemux_clk)) {
+ ret = PTR_ERR(qphy->pipemux_clk);
+ if (ret == -EPROBE_DEFER)
+ return ret;
+
+ qphy->pipemux_clk = NULL;
+ } else {
+ qphy->piperef_clk = of_clk_get_by_name(np, "ref");
+ if (IS_ERR(qphy->piperef_clk)) {
+ ret = PTR_ERR(qphy->piperef_clk);
+ return dev_err_probe(dev, ret,
+ "failed to get lane%d piperef_clk\n",
+ id);
+ }
+ }
+
/* Get lane reset, if any */
if (cfg->has_lane_rst) {
snprintf(prop_name, sizeof(prop_name), "lane%d", id);
--
2.35.1
On Thu 21 Apr 03:20 PDT 2022, Johan Hovold wrote:
> Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> output generated by the PHY before powering on the PHY and restore the
> default source during power down.
>
> Add support for an optional pipe clock mux which will be reparented to
> the generated pipe clock before powering on the PHY and restored to the
> default reference source on power off.
>
After considering this for a while, I have two objections to doing this
explicitly:
1) The QMP block is fed &gcc_pcie_N_pipe_clk (and on sc8280xp)
gcc_pcie_N_pipediv2_clk. But neither of these clocks are the mux, so
what this patch (and the existing muxing logic in the controller) does
is to poke into gcc "internals".
2) The actual reason for the mux dance is that toggling the associated
GDSC without a valid parent of this clock would cause the clock to lock
up and GDSC transition to time out. This property is shared with a wide
range of other clocks (so far we have 84 users of clk_rcg2_shared_ops on
sc8280xp).
It would be nice if clk_summary would represent the real state of these
clocks, but unfortunately I don't think the state matches reality with
this approach either.
E.g. we prepare/enable the pipe clock before setting
QPHY_POWER_DOWN_CONTROL, during this time there's shouldn't be any pipe
clock coming out of the PHY...
Regards,
Bjorn
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
> 1 file changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 7d2d1ab061f7..bc6db9670291 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
> * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
> * @pcs_misc: iomapped memory space for lane's pcs_misc
> * @pipe_clk: pipe clock
> + * @pipemux_clk: pipe clock source mux
> + * @piperef_clk: pipe clock default reference source
> * @index: lane index
> * @qmp: QMP phy to which this lane belongs
> * @lane_rst: lane's reset controller
> @@ -3311,6 +3313,8 @@ struct qmp_phy {
> void __iomem *rx2;
> void __iomem *pcs_misc;
> struct clk *pipe_clk;
> + struct clk *pipemux_clk;
> + struct clk *piperef_clk;
> unsigned int index;
> struct qcom_qmp *qmp;
> struct reset_control *lane_rst;
> @@ -3346,6 +3350,7 @@ struct qcom_qmp {
> void __iomem *dp_com;
>
> struct clk_bulk_data *clks;
> + struct clk *pipe_clksrc;
> struct reset_control **resets;
> struct regulator_bulk_data *vregs;
>
> @@ -5355,6 +5360,42 @@ static int qcom_qmp_phy_init(struct phy *phy)
> return 0;
> }
>
> +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy)
> +{
> + struct qcom_qmp *qmp = qphy->qmp;
> + int ret;
> +
> + ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc);
> + if (ret)
> + dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +
> +
> + ret = clk_prepare_enable(qphy->pipe_clk);
> + if (ret) {
> + dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> + goto err_restore_parent;
> + }
> +
> + return 0;
> +
> +err_restore_parent:
> + clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> +
> + return ret;
> +}
> +
> +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy)
> +{
> + struct qcom_qmp *qmp = qphy->qmp;
> + int ret;
> +
> + clk_disable_unprepare(qphy->pipe_clk);
> +
> + ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk);
> + if (ret)
> + dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret);
> +}
> +
> static int qcom_qmp_phy_power_on(struct phy *phy)
> {
> struct qmp_phy *qphy = phy_get_drvdata(phy);
> @@ -5379,11 +5420,9 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> }
> }
>
> - ret = clk_prepare_enable(qphy->pipe_clk);
> - if (ret) {
> - dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> + ret = qcom_qmp_phy_pipe_clk_enable(qphy);
> + if (ret)
> goto err_reset_lane;
> - }
>
> /* Tx, Rx, and PCS configurations */
> qcom_qmp_phy_configure_lane(tx, cfg->regs,
> @@ -5478,7 +5517,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> return 0;
>
> err_disable_pipe_clk:
> - clk_disable_unprepare(qphy->pipe_clk);
> + qcom_qmp_phy_pipe_clk_disable(qphy);
> err_reset_lane:
> if (cfg->has_lane_rst)
> reset_control_assert(qphy->lane_rst);
> @@ -5491,7 +5530,7 @@ static int qcom_qmp_phy_power_off(struct phy *phy)
> struct qmp_phy *qphy = phy_get_drvdata(phy);
> const struct qmp_phy_cfg *cfg = qphy->cfg;
>
> - clk_disable_unprepare(qphy->pipe_clk);
> + qcom_qmp_phy_pipe_clk_disable(qphy);
>
> if (cfg->type == PHY_TYPE_DP) {
> /* Assert DP PHY power down */
> @@ -5777,6 +5816,8 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
> if (ret)
> return ret;
>
> + qmp->pipe_clksrc = fixed->hw.clk;
> +
> ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
> if (ret)
> return ret;
> @@ -6091,6 +6132,24 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id,
> qphy->pipe_clk = NULL;
> }
>
> + /* Get optional pipe clock mux and default reference source clock. */
> + qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> + if (IS_ERR(qphy->pipemux_clk)) {
> + ret = PTR_ERR(qphy->pipemux_clk);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> +
> + qphy->pipemux_clk = NULL;
> + } else {
> + qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> + if (IS_ERR(qphy->piperef_clk)) {
> + ret = PTR_ERR(qphy->piperef_clk);
> + return dev_err_probe(dev, ret,
> + "failed to get lane%d piperef_clk\n",
> + id);
> + }
> + }
> +
> /* Get lane reset, if any */
> if (cfg->has_lane_rst) {
> snprintf(prop_name, sizeof(prop_name), "lane%d", id);
> --
> 2.35.1
>
On Thu, Apr 28, 2022 at 06:11:44AM -0700, Bjorn Andersson wrote:
> On Thu 21 Apr 03:20 PDT 2022, Johan Hovold wrote:
>
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> >
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> >
>
> After considering this for a while, I have two objections to doing this
> explicitly:
>
> 1) The QMP block is fed &gcc_pcie_N_pipe_clk (and on sc8280xp)
> gcc_pcie_N_pipediv2_clk. But neither of these clocks are the mux, so
> what this patch (and the existing muxing logic in the controller) does
> is to poke into gcc "internals".
I agree that this is perhaps the strongest argument against describing
the mux in DT (as is also currently done for sc7280).
> 2) The actual reason for the mux dance is that toggling the associated
> GDSC without a valid parent of this clock would cause the clock to lock
> up and GDSC transition to time out. This property is shared with a wide
> range of other clocks (so far we have 84 users of clk_rcg2_shared_ops on
> sc8280xp).
Right, but the situation with rcg2 is a little different. From what I
gather the problem there is that some downstream clock could have been
enabled by some other part of the system behind the kernel's back and
then things go wrong when the kernel configures the clock.
Here it is the kernel that controls the source of the pipe clock mux
(the PHY PLL) and knows when the source is valid (and the PHY is both
provider and consumer of the pipe clock).
For rcg2, there's no choice but to work around the hardware in the clock
driver, while for QMP the PHY power sequences could be made explicit in
the driver:
clk_prepare_enable(pipe);
clk_set_parent(pipe_src, pll);
start_pll();
stop_pll();
clk_set_parent(pipe_src, xo);
clk_disable_unprepare(pipe);
Note that with the above sequences it would be possible to drop the pipe
clock BRANCH_HALT_SKIP flag, which is only there in the clock driver
because of the how the PHY works (i.e. that the pipe clock must be
ungated before the PLL is started).
(This wouldn't be possible with a pipe-mux implementation in the clock
driver since the parent mux would be enabled before the child pipe
clock.)
But sure, the requirement to restore XO to prevent a later GDSC hang has
little to do with the PHY.
> It would be nice if clk_summary would represent the real state of these
> clocks, but unfortunately I don't think the state matches reality with
> this approach either.
>
> E.g. we prepare/enable the pipe clock before setting
> QPHY_POWER_DOWN_CONTROL, during this time there's shouldn't be any pipe
> clock coming out of the PHY...
Right, there's a small window there where the source is still off (due
to hardware requirements), but at least the topology is always reported
correctly, which is really useful when dealing with boot handover.
I have an sc8280xp here where the boot firmware leaves the pipe clock
muxed in despite the GDSC being disabled. Fortunately, it doesn't seem
to trigger the lockup when toggling the GDSC as it does on an sa8540p.
My concerns are otherwise mostly related to the implementation of the
safe-mux (e.g. ad-hoc, missing locking) and can probably be addressed.
As an example, with the current implementation it is not possible to use
an assigned clock parent in DT to make sure that the XO source is
selected before toggling the GDSC. The muxing doesn't happen until the
pipe clock is enabled, which is much too late in case the boot firmware
left the pipe clock muxed in, and when enabling the pipe clock you
really want the PHY PLL as source and not the cached XO.
Can you still drop the current safe-mux patches from your tree or would
they need to be fixed up incrementally? I think you merged v2, but
there's already a v3 out (addressing the hardcoded mux configuration
values).
Johan