| Message ID | 20230425034010.3789376-4-quic_bjorande@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: qcom-qmp-combo: Support orientation switching | expand |
On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote: > In multiple places throughout the driver code has been written in > prepration for handling of orientation switching. > > Introduce a typec_orientation in qmp_combo and fill out the various > "placeholders" with the associated logic. By initializing the > orientation to "normal" this change has no functional impact, but > reduces the size of the upcoming introduction of dynamic orientation > switching. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 54 +++++++++++++---------- > 1 file changed, 30 insertions(+), 24 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > index 7280f7141961..6748f31da7a3 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > @@ -19,6 +19,7 @@ > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > #include <linux/slab.h> > +#include <linux/usb/typec.h> > > #include <dt-bindings/phy/phy-qcom-qmp.h> > > @@ -63,6 +64,10 @@ > /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */ > #define CLAMP_EN BIT(0) /* enables i/o clamp_n */ > > +/* QPHY_V3_DP_COM_TYPEC_CTRL register bits */ > +#define SW_PORTSELECT_VAL BIT(0) > +#define SW_PORTSELECT_MUX BIT(1) > + > #define PHY_INIT_COMPLETE_TIMEOUT 10000 > > struct qmp_phy_init_tbl { > @@ -1323,6 +1328,8 @@ struct qmp_combo { > struct clk_fixed_rate pipe_clk_fixed; > struct clk_hw dp_link_hw; > struct clk_hw dp_pixel_hw; > + > + enum typec_orientation orientation; > }; > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp) > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp) > { > u32 val; > - bool reverse = false; > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; Adding parentheses around the right-hand side should make this a little easier to parse. It also looks like these callbacks end up being called without holding the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a concurrent switch notification and dp phy power-on but it's not that obvious. > + const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts; Also could you add these before u32 val to maintain an approximation of reverse xmas style? And similar below. Johan
On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote: > On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote: > > In multiple places throughout the driver code has been written in > > prepration for handling of orientation switching. > > > > Introduce a typec_orientation in qmp_combo and fill out the various > > "placeholders" with the associated logic. By initializing the > > orientation to "normal" this change has no functional impact, but > > reduces the size of the upcoming introduction of dynamic orientation > > switching. > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > --- > > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 54 +++++++++++++---------- > > 1 file changed, 30 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > > index 7280f7141961..6748f31da7a3 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > > @@ -19,6 +19,7 @@ > > #include <linux/regulator/consumer.h> > > #include <linux/reset.h> > > #include <linux/slab.h> > > +#include <linux/usb/typec.h> > > > > #include <dt-bindings/phy/phy-qcom-qmp.h> > > > > @@ -63,6 +64,10 @@ > > /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */ > > #define CLAMP_EN BIT(0) /* enables i/o clamp_n */ > > > > +/* QPHY_V3_DP_COM_TYPEC_CTRL register bits */ > > +#define SW_PORTSELECT_VAL BIT(0) > > +#define SW_PORTSELECT_MUX BIT(1) > > + > > #define PHY_INIT_COMPLETE_TIMEOUT 10000 > > > > struct qmp_phy_init_tbl { > > @@ -1323,6 +1328,8 @@ struct qmp_combo { > > struct clk_fixed_rate pipe_clk_fixed; > > struct clk_hw dp_link_hw; > > struct clk_hw dp_pixel_hw; > > + > > + enum typec_orientation orientation; > > }; > > > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); > > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp) > > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp) > > { > > u32 val; > > - bool reverse = false; > > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; > > Adding parentheses around the right-hand side should make this a little > easier to parse. > > It also looks like these callbacks end up being called without holding > the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a > concurrent switch notification and dp phy power-on but it's not that > obvious. > It seems we're arriving here from hpd_event_thread(), while phy_power_on() and phy_power_off() will be called in some other context. I've not been able to convince myself if DP driver ensures ordering, or if we have an existing race here... Unless you insist, I would prefer to follow up with an additional patch once we've landed this series. The fix will depend on the phy_mutex shuffling patch anyways... > > + const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts; > > Also could you add these before u32 val to maintain an approximation of > reverse xmas style? > I'd be happy to do so :) Regards, Bjorn > And similar below. > > Johan
On Wed, May 03, 2023 at 08:29:07PM -0700, Bjorn Andersson wrote: > On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote: > > On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote: > > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); > > > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp) > > > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp) > > > { > > > u32 val; > > > - bool reverse = false; > > > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; > > It also looks like these callbacks end up being called without holding > > the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a > > concurrent switch notification and dp phy power-on but it's not that > > obvious. > It seems we're arriving here from hpd_event_thread(), while > phy_power_on() and phy_power_off() will be called in some other context. > I've not been able to convince myself if DP driver ensures ordering, or > if we have an existing race here... > Unless you insist, I would prefer to follow up with an additional patch > once we've landed this series. The fix will depend on the phy_mutex > shuffling patch anyways... Sure. But perhaps you can just move the orientation == qmp->orientation check under the mutex in qmp_combo_typec_switch_set() for now (in case I forgot to point that out earlier). Johan
On Thu, May 04, 2023 at 03:44:53PM +0200, Johan Hovold wrote: > On Wed, May 03, 2023 at 08:29:07PM -0700, Bjorn Andersson wrote: > > On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote: > > > On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote: > > > > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); > > > > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp) > > > > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp) > > > > { > > > > u32 val; > > > > - bool reverse = false; > > > > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; > > > > It also looks like these callbacks end up being called without holding > > > the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a > > > concurrent switch notification and dp phy power-on but it's not that > > > obvious. > > > It seems we're arriving here from hpd_event_thread(), while > > phy_power_on() and phy_power_off() will be called in some other context. > > I've not been able to convince myself if DP driver ensures ordering, or > > if we have an existing race here... > > > Unless you insist, I would prefer to follow up with an additional patch > > once we've landed this series. The fix will depend on the phy_mutex > > shuffling patch anyways... > > Sure. > > But perhaps you can just move the orientation == qmp->orientation check > under the mutex in qmp_combo_typec_switch_set() for now (in case I > forgot to point that out earlier). > qmp_combo_probe() and qmp_combo_typec_switch_set() are the only writers to qmp->orientation, so that check can't race with any updates and hence doesn't need to be protected. Reading the code again, qmp_combo_configure_dp_mode() is invoked from phy_power_on(), not the hpd_event_thread(), as I claimed yesterday. But we shouldn't do qmp_combo_dp_power_on() in parallel with the reinitialization following a switch in orientation, qmp->orientation might change, but we definitely would have two contexts reconfiguring the hardware simultaneously - perhaps this was the cause for the 10% crashes I hit when trying to extend this to handle typec_mux as well... I will grab the phy_mux in qmp_combo_configure_dp_mode() as well, thanks for "insisting" :) Regards, Bjorn
On Thu, May 04, 2023 at 08:16:33AM -0700, Bjorn Andersson wrote: > On Thu, May 04, 2023 at 03:44:53PM +0200, Johan Hovold wrote: > > On Wed, May 03, 2023 at 08:29:07PM -0700, Bjorn Andersson wrote: > > > On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote: > > > > On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote: > > > > > > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); > > > > > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp) > > > > > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp) > > > > > { > > > > > u32 val; > > > > > - bool reverse = false; > > > > > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; > > > > > > It also looks like these callbacks end up being called without holding > > > > the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a > > > > concurrent switch notification and dp phy power-on but it's not that > > > > obvious. > > > > > It seems we're arriving here from hpd_event_thread(), while > > > phy_power_on() and phy_power_off() will be called in some other context. > > > I've not been able to convince myself if DP driver ensures ordering, or > > > if we have an existing race here... > > > > > Unless you insist, I would prefer to follow up with an additional patch > > > once we've landed this series. The fix will depend on the phy_mutex > > > shuffling patch anyways... > > > > Sure. > > > > But perhaps you can just move the orientation == qmp->orientation check > > under the mutex in qmp_combo_typec_switch_set() for now (in case I > > forgot to point that out earlier). > > > > qmp_combo_probe() and qmp_combo_typec_switch_set() are the only writers > to qmp->orientation, so that check can't race with any updates and hence > doesn't need to be protected. Only if you happen to know that the callers of qmp_combo_typec_switch_set() are serialised, right? That happens to be the case for pmic_glink, but it may not be the case generally. > Reading the code again, qmp_combo_configure_dp_mode() is invoked from > phy_power_on(), not the hpd_event_thread(), as I claimed yesterday. Yeah, but phy_power_on() is typically called from that thread. But perhaps not only from there. > But we shouldn't do qmp_combo_dp_power_on() in parallel with the > reinitialization following a switch in orientation, qmp->orientation > might change, but we definitely would have two contexts reconfiguring > the hardware simultaneously - perhaps this was the cause for the 10% > crashes I hit when trying to extend this to handle typec_mux as well... > > I will grab the phy_mux in qmp_combo_configure_dp_mode() as well, thanks > for "insisting" :) :) Johan
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index 7280f7141961..6748f31da7a3 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c @@ -19,6 +19,7 @@ #include <linux/regulator/consumer.h> #include <linux/reset.h> #include <linux/slab.h> +#include <linux/usb/typec.h> #include <dt-bindings/phy/phy-qcom-qmp.h> @@ -63,6 +64,10 @@ /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */ #define CLAMP_EN BIT(0) /* enables i/o clamp_n */ +/* QPHY_V3_DP_COM_TYPEC_CTRL register bits */ +#define SW_PORTSELECT_VAL BIT(0) +#define SW_PORTSELECT_MUX BIT(1) + #define PHY_INIT_COMPLETE_TIMEOUT 10000 struct qmp_phy_init_tbl { @@ -1323,6 +1328,8 @@ struct qmp_combo { struct clk_fixed_rate pipe_clk_fixed; struct clk_hw dp_link_hw; struct clk_hw dp_pixel_hw; + + enum typec_orientation orientation; }; static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp) static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp) { u32 val; - bool reverse = false; + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; + const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts; val = DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN; - /* - * TODO: Assume orientation is CC1 for now and two lanes, need to - * use type-c connector to understand orientation and lanes. - * - * Otherwise val changes to be like below if this code understood - * the orientation of the type-c cable. - * - * if (lane_cnt == 4 || orientation == ORIENTATION_CC2) - * val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN; - * if (lane_cnt == 4 || orientation == ORIENTATION_CC1) - * val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; - * if (orientation == ORIENTATION_CC2) - * writel(0x4c, qmp->dp_dp_phy + QSERDES_V3_DP_PHY_MODE); - */ - val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; + if (dp_opts->lanes == 4 || reverse) + val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN; + if (dp_opts->lanes == 4 || !reverse) + val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; + writel(val, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL); - writel(0x5c, qmp->dp_dp_phy + QSERDES_DP_PHY_MODE); + if (reverse) + writel(0x4c, qmp->pcs + QSERDES_DP_PHY_MODE); + else + writel(0x5c, qmp->pcs + QSERDES_DP_PHY_MODE); return reverse; } @@ -2235,7 +2236,7 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo *qmp) { const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts; u32 bias0_en, drvr0_en, bias1_en, drvr1_en; - bool reverse = false; + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; u32 status; int ret; @@ -2299,7 +2300,7 @@ static int qmp_v5_configure_dp_phy(struct qmp_combo *qmp) { const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts; u32 bias0_en, drvr0_en, bias1_en, drvr1_en; - bool reverse = false; + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; u32 status; int ret; @@ -2358,7 +2359,7 @@ static int qmp_v6_configure_dp_phy(struct qmp_combo *qmp) { const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts; u32 bias0_en, drvr0_en, bias1_en, drvr1_en; - bool reverse = false; + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE; u32 status; int ret; @@ -2462,6 +2463,7 @@ static int qmp_combo_com_init(struct qmp_combo *qmp) const struct qmp_phy_cfg *cfg = qmp->cfg; void __iomem *com = qmp->com; int ret; + u32 val; if (qmp->init_count++) return 0; @@ -2495,10 +2497,12 @@ static int qmp_combo_com_init(struct qmp_combo *qmp) SW_DPPHY_RESET_MUX | SW_DPPHY_RESET | SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); - /* Default type-c orientation, i.e CC1 */ - qphy_setbits(com, QPHY_V3_DP_COM_TYPEC_CTRL, 0x02); - - qphy_setbits(com, QPHY_V3_DP_COM_PHY_MODE_CTRL, USB3_MODE | DP_MODE); + /* Use software based port select and switch on typec orientation */ + val = SW_PORTSELECT_MUX; + if (qmp->orientation == TYPEC_ORIENTATION_REVERSE) + val |= SW_PORTSELECT_VAL; + writel(val, com + QPHY_V3_DP_COM_TYPEC_CTRL); + writel(USB3_MODE | DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL); /* bring both QMP USB and QMP DP PHYs PCS block out of reset */ qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL, @@ -3361,6 +3365,8 @@ static int qmp_combo_probe(struct platform_device *pdev) qmp->dev = dev; + qmp->orientation = TYPEC_ORIENTATION_NORMAL; + qmp->cfg = of_device_get_match_data(dev); if (!qmp->cfg) return -EINVAL;
In multiple places throughout the driver code has been written in prepration for handling of orientation switching. Introduce a typec_orientation in qmp_combo and fill out the various "placeholders" with the associated logic. By initializing the orientation to "normal" this change has no functional impact, but reduces the size of the upcoming introduction of dynamic orientation switching. Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> --- drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 54 +++++++++++++---------- 1 file changed, 30 insertions(+), 24 deletions(-)