| Message ID | 20191219150433.2785427-4-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: qcom-qmp: Fixes and updates for sm8150 | expand |
On Thu, Dec 19, 2019 at 8:05 AM Vinod Koul <vkoul@kernel.org> wrote: > > For V4 QMP UFS Phy, we need to assert reset bits, configure the phy and > then deassert it, so add optional has_sw_reset flag and use that to > configure the QPHY_SW_RESET register. > > Signed-off-by: Vinod Koul <vkoul@kernel.org> Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > --- > drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 06f971ca518e..80304b7cd895 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg { > > /* true, if PCS block has no separate SW_RESET register */ > bool no_pcs_sw_reset; > + > + /* true if sw reset needs to be invoked */ > + bool has_sw_reset; > }; > > /** > @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = { > > .is_dual_lane_phy = true, > .no_pcs_sw_reset = true, > + .has_sw_reset = true, > }; > > static void qcom_qmp_phy_configure(void __iomem *base, > @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy) > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); > } > > + if (cfg->has_sw_reset) > + qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET); > + > if (cfg->has_phy_com_ctrl) > qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], > SW_PWRDN); > @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy) > if (cfg->has_phy_dp_com_ctrl) > qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); > > + if (cfg->has_sw_reset) > + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > + > /* start SerDes and Phy-Coding-Sublayer */ > qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl); > > -- > 2.23.0 >
On 2019-12-19 23:04, Vinod Koul wrote: > For V4 QMP UFS Phy, we need to assert reset bits, configure the phy and > then deassert it, so add optional has_sw_reset flag and use that to > configure the QPHY_SW_RESET register. > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c > b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 06f971ca518e..80304b7cd895 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg { > > /* true, if PCS block has no separate SW_RESET register */ > bool no_pcs_sw_reset; > + > + /* true if sw reset needs to be invoked */ > + bool has_sw_reset; > }; > > /** > @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg > = { > > .is_dual_lane_phy = true, > .no_pcs_sw_reset = true, > + .has_sw_reset = true, > }; > > static void qcom_qmp_phy_configure(void __iomem *base, > @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy > *qphy) > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); > } > > + if (cfg->has_sw_reset) > + qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET); > + Are you sure you want to set this in the serdes register? QPHY_SW_RESET is in its pcs register. > if (cfg->has_phy_com_ctrl) > qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], > SW_PWRDN); > @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy) > if (cfg->has_phy_dp_com_ctrl) > qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); > > + if (cfg->has_sw_reset) > + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > + Yet you are clearing it from pcs register. Regards, Can Guo > /* start SerDes and Phy-Coding-Sublayer */ > qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
On 2019-12-20 08:22, cang@codeaurora.org wrote: > On 2019-12-19 23:04, Vinod Koul wrote: >> For V4 QMP UFS Phy, we need to assert reset bits, configure the phy >> and >> then deassert it, so add optional has_sw_reset flag and use that to >> configure the QPHY_SW_RESET register. >> >> Signed-off-by: Vinod Koul <vkoul@kernel.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c >> b/drivers/phy/qualcomm/phy-qcom-qmp.c >> index 06f971ca518e..80304b7cd895 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c >> @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg { >> >> /* true, if PCS block has no separate SW_RESET register */ >> bool no_pcs_sw_reset; >> + >> + /* true if sw reset needs to be invoked */ >> + bool has_sw_reset; >> }; >> >> /** >> @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg >> sm8150_ufsphy_cfg = { >> >> .is_dual_lane_phy = true, >> .no_pcs_sw_reset = true, >> + .has_sw_reset = true, >> }; >> >> static void qcom_qmp_phy_configure(void __iomem *base, >> @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy >> *qphy) >> SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); >> } >> >> + if (cfg->has_sw_reset) >> + qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET); >> + > > Are you sure you want to set this in the serdes register? QPHY_SW_RESET > is in its pcs register. > >> if (cfg->has_phy_com_ctrl) >> qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], >> SW_PWRDN); >> @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy) >> if (cfg->has_phy_dp_com_ctrl) >> qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); >> >> + if (cfg->has_sw_reset) >> + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> + > > Yet you are clearing it from pcs register. > > Regards, > Can Guo > >> /* start SerDes and Phy-Coding-Sublayer */ >> qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl); I thought your change would be like this diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c index 8e642a6..a4ab4b7 100755 --- a/drivers/phy/qualcomm/phy-qcom-qmp.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c @@ -166,6 +166,7 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = { }; static const unsigned int sm8150_ufsphy_regs_layout[] = { + [QPHY_SW_RESET] = 0x08, [QPHY_START_CTRL] = 0x00, [QPHY_PCS_READY_STATUS] = 0x180, }; @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = { .pwrdn_ctrl = SW_PWRDN, .is_dual_lane_phy = true, - .no_pcs_sw_reset = true, }; static void qcom_qmp_phy_configure(void __iomem *base, @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy) SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); } + if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset)) + qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); + if (cfg->has_phy_com_ctrl) qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], SW_PWRDN); Regards, Can Guo.
On 20-12-19, 08:49, cang@codeaurora.org wrote: > On 2019-12-20 08:22, cang@codeaurora.org wrote: > > On 2019-12-19 23:04, Vinod Koul wrote: > > > For V4 QMP UFS Phy, we need to assert reset bits, configure the phy > > > and > > > then deassert it, so add optional has_sw_reset flag and use that to > > > configure the QPHY_SW_RESET register. > > > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > > --- > > > drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c > > > b/drivers/phy/qualcomm/phy-qcom-qmp.c > > > index 06f971ca518e..80304b7cd895 100644 > > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > > > @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg { > > > > > > /* true, if PCS block has no separate SW_RESET register */ > > > bool no_pcs_sw_reset; > > > + > > > + /* true if sw reset needs to be invoked */ > > > + bool has_sw_reset; > > > }; > > > > > > /** > > > @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg > > > sm8150_ufsphy_cfg = { > > > > > > .is_dual_lane_phy = true, > > > .no_pcs_sw_reset = true, > > > + .has_sw_reset = true, > > > }; > > > > > > static void qcom_qmp_phy_configure(void __iomem *base, > > > @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct > > > qmp_phy *qphy) > > > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); > > > } > > > > > > + if (cfg->has_sw_reset) > > > + qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET); > > > + > > > > Are you sure you want to set this in the serdes register? QPHY_SW_RESET > > is in its pcs register. > > > > > if (cfg->has_phy_com_ctrl) > > > qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], > > > SW_PWRDN); > > > @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy) > > > if (cfg->has_phy_dp_com_ctrl) > > > qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); > > > > > > + if (cfg->has_sw_reset) > > > + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > > > + > > > > Yet you are clearing it from pcs register. updated now, will send v2 > > > > Regards, > > Can Guo > > > > > /* start SerDes and Phy-Coding-Sublayer */ > > > qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl); > > I thought your change would be like this > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c > b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 8e642a6..a4ab4b7 100755 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -166,6 +166,7 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = > { > }; > > static const unsigned int sm8150_ufsphy_regs_layout[] = { > + [QPHY_SW_RESET] = 0x08, > [QPHY_START_CTRL] = 0x00, > [QPHY_PCS_READY_STATUS] = 0x180, > }; > @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = { > .pwrdn_ctrl = SW_PWRDN, > > .is_dual_lane_phy = true, > - .no_pcs_sw_reset = true, > }; > > static void qcom_qmp_phy_configure(void __iomem *base, > @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy) > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); > } > > + if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset)) > + qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); Well am not sure if no_pcs_sw_reset would do this and side effect on other phys (somehow older ones dont seem to need this). That was the reason for a new flag and to be used for specific instances Thanks
On 2019-12-20 12:24, Vinod Koul wrote: > On 20-12-19, 08:49, cang@codeaurora.org wrote: >> On 2019-12-20 08:22, cang@codeaurora.org wrote: >> > On 2019-12-19 23:04, Vinod Koul wrote: >> > > For V4 QMP UFS Phy, we need to assert reset bits, configure the phy >> > > and >> > > then deassert it, so add optional has_sw_reset flag and use that to >> > > configure the QPHY_SW_RESET register. >> > > >> > > Signed-off-by: Vinod Koul <vkoul@kernel.org> >> > > --- >> > > drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++ >> > > 1 file changed, 10 insertions(+) >> > > >> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c >> > > b/drivers/phy/qualcomm/phy-qcom-qmp.c >> > > index 06f971ca518e..80304b7cd895 100644 >> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c >> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c >> > > @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg { >> > > >> > > /* true, if PCS block has no separate SW_RESET register */ >> > > bool no_pcs_sw_reset; >> > > + >> > > + /* true if sw reset needs to be invoked */ >> > > + bool has_sw_reset; >> > > }; >> > > >> > > /** >> > > @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg >> > > sm8150_ufsphy_cfg = { >> > > >> > > .is_dual_lane_phy = true, >> > > .no_pcs_sw_reset = true, >> > > + .has_sw_reset = true, >> > > }; >> > > >> > > static void qcom_qmp_phy_configure(void __iomem *base, >> > > @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct >> > > qmp_phy *qphy) >> > > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); >> > > } >> > > >> > > + if (cfg->has_sw_reset) >> > > + qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET); >> > > + >> > >> > Are you sure you want to set this in the serdes register? QPHY_SW_RESET >> > is in its pcs register. >> > >> > > if (cfg->has_phy_com_ctrl) >> > > qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], >> > > SW_PWRDN); >> > > @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy) >> > > if (cfg->has_phy_dp_com_ctrl) >> > > qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); >> > > >> > > + if (cfg->has_sw_reset) >> > > + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> > > + >> > >> > Yet you are clearing it from pcs register. > > updated now, will send v2 > >> > >> > Regards, >> > Can Guo >> > >> > > /* start SerDes and Phy-Coding-Sublayer */ >> > > qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl); >> >> I thought your change would be like this >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c >> b/drivers/phy/qualcomm/phy-qcom-qmp.c >> index 8e642a6..a4ab4b7 100755 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c >> @@ -166,6 +166,7 @@ static const unsigned int >> sdm845_ufsphy_regs_layout[] = >> { >> }; >> >> static const unsigned int sm8150_ufsphy_regs_layout[] = { >> + [QPHY_SW_RESET] = 0x08, >> [QPHY_START_CTRL] = 0x00, >> [QPHY_PCS_READY_STATUS] = 0x180, >> }; >> @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg >> sm8150_ufsphy_cfg = { >> .pwrdn_ctrl = SW_PWRDN, >> >> .is_dual_lane_phy = true, >> - .no_pcs_sw_reset = true, >> }; >> >> static void qcom_qmp_phy_configure(void __iomem *base, >> @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy >> *qphy) >> SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); >> } >> >> + if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset)) >> + qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > > Well am not sure if no_pcs_sw_reset would do this and side effect on > other phys (somehow older ones dont seem to need this). That was the > reason for a new flag and to be used for specific instances > > Thanks Hi Vinod, That is why I added the check as cfg->type == PHY_TYPE_UFS, meaning this change will only apply to UFS. FYI, start from 8150(include 8150), QPHY_SW_RESET is present in PHY's PCS register. no_pcs_sw_reset = TRUE should only be given to 845 and older targets, like 8998, because they don't have this QPHY_SW_RESET in PHY's register per their design, that's why they leverage the reset control provided by UFS controller. Thanks, Can Guo.
On 20-12-19, 14:00, Can Guo wrote: > On 2019-12-20 12:24, Vinod Koul wrote: > > On 20-12-19, 08:49, cang@codeaurora.org wrote: > > > On 2019-12-20 08:22, cang@codeaurora.org wrote: > > > > On 2019-12-19 23:04, Vinod Koul wrote: > > > > > > > > > /* start SerDes and Phy-Coding-Sublayer */ > > > > > qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl); > > > > > > I thought your change would be like this > > > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c > > > b/drivers/phy/qualcomm/phy-qcom-qmp.c > > > index 8e642a6..a4ab4b7 100755 > > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > > > @@ -166,6 +166,7 @@ static const unsigned int > > > sdm845_ufsphy_regs_layout[] = > > > { > > > }; > > > > > > static const unsigned int sm8150_ufsphy_regs_layout[] = { > > > + [QPHY_SW_RESET] = 0x08, > > > [QPHY_START_CTRL] = 0x00, > > > [QPHY_PCS_READY_STATUS] = 0x180, > > > }; > > > @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg > > > sm8150_ufsphy_cfg = { > > > .pwrdn_ctrl = SW_PWRDN, > > > > > > .is_dual_lane_phy = true, > > > - .no_pcs_sw_reset = true, > > > }; > > > > > > static void qcom_qmp_phy_configure(void __iomem *base, > > > @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct > > > qmp_phy *qphy) > > > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); > > > } > > > > > > + if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset)) > > > + qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > > > > Well am not sure if no_pcs_sw_reset would do this and side effect on > > other phys (somehow older ones dont seem to need this). That was the > > reason for a new flag and to be used for specific instances > > > > Thanks > > Hi Vinod, > > That is why I added the check as cfg->type == PHY_TYPE_UFS, meaning this > change will only apply to UFS. > FYI, start from 8150(include 8150), QPHY_SW_RESET is present in PHY's > PCS register. no_pcs_sw_reset = TRUE should only be given to 845 and older > targets, like 8998, because they don't have this QPHY_SW_RESET in PHY's > register per their design, that's why they leverage the reset control > provided by UFS controller. I have removed no_pcs_sw_reset and tested. Well as you said even with UFS we have variations between various chips, so I thought leaving it separate might be better than creating a chance of regression on older platforms! Moreover, are we sure that the reset wont be there for other qmp phy's in future other than UFS... Thanks
On 2019-12-20 15:10, Vinod Koul wrote: > On 20-12-19, 14:00, Can Guo wrote: >> On 2019-12-20 12:24, Vinod Koul wrote: >> > On 20-12-19, 08:49, cang@codeaurora.org wrote: >> > > On 2019-12-20 08:22, cang@codeaurora.org wrote: >> > > > On 2019-12-19 23:04, Vinod Koul wrote: >> > > > >> > > > > /* start SerDes and Phy-Coding-Sublayer */ >> > > > > qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl); >> > > >> > > I thought your change would be like this >> > > >> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c >> > > b/drivers/phy/qualcomm/phy-qcom-qmp.c >> > > index 8e642a6..a4ab4b7 100755 >> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c >> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c >> > > @@ -166,6 +166,7 @@ static const unsigned int >> > > sdm845_ufsphy_regs_layout[] = >> > > { >> > > }; >> > > >> > > static const unsigned int sm8150_ufsphy_regs_layout[] = { >> > > + [QPHY_SW_RESET] = 0x08, >> > > [QPHY_START_CTRL] = 0x00, >> > > [QPHY_PCS_READY_STATUS] = 0x180, >> > > }; >> > > @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg >> > > sm8150_ufsphy_cfg = { >> > > .pwrdn_ctrl = SW_PWRDN, >> > > >> > > .is_dual_lane_phy = true, >> > > - .no_pcs_sw_reset = true, >> > > }; >> > > >> > > static void qcom_qmp_phy_configure(void __iomem *base, >> > > @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct >> > > qmp_phy *qphy) >> > > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); >> > > } >> > > >> > > + if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset)) >> > > + qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> > >> > Well am not sure if no_pcs_sw_reset would do this and side effect on >> > other phys (somehow older ones dont seem to need this). That was the >> > reason for a new flag and to be used for specific instances >> > >> > Thanks >> >> Hi Vinod, >> >> That is why I added the check as cfg->type == PHY_TYPE_UFS, meaning >> this >> change will only apply to UFS. >> FYI, start from 8150(include 8150), QPHY_SW_RESET is present in PHY's >> PCS register. no_pcs_sw_reset = TRUE should only be given to 845 and >> older >> targets, like 8998, because they don't have this QPHY_SW_RESET in >> PHY's >> register per their design, that's why they leverage the reset control >> provided by UFS controller. > > I have removed no_pcs_sw_reset and tested. > > Well as you said even with UFS we have variations between various > chips, > so I thought leaving it separate might be better than creating a chance > of regression on older platforms! > > Moreover, are we sure that the reset wont be there for other qmp phy's > in future other than UFS... > > Thanks Hi Vinod We are just removing the no_pcs_sw_reset for 8150, right? Why is it possibly impacting 845 or older paltforms? In future, we will no longer need no_pcs_sw_reset for any newer QCOM UFS PHY designs, as it is only for 845 and older platforms. I am sure QPHY_SW_RESET will be within PHY's address space since 8150. Otherwise, it will be a regression in UFS PHY design. We had a lot of discussion about this on 845 years ago, then design team decided to add it on later platforms, so I don't see a reason to remove it again. :) I am not sure about the other qmp phys, but so long as UFS PHY needs the reset, we need to keep it, as phy-qcom-qmp.c is a common driver. I am not sure if I get your point here. Please correct me I am wrong. Thanks, Can Guo.
On 20-12-19, 15:41, Can Guo wrote: > On 2019-12-20 15:10, Vinod Koul wrote: > > On 20-12-19, 14:00, Can Guo wrote: > Hi Vinod > > We are just removing the no_pcs_sw_reset for 8150, right? Why is it > possibly impacting 845 or older paltforms? > > In future, we will no longer need no_pcs_sw_reset for any newer QCOM UFS > PHY designs, as it is only for 845 and older platforms. > > I am sure QPHY_SW_RESET will be within PHY's address space since 8150. > Otherwise, it will be a regression in UFS PHY design. We had a lot of > discussion about this on 845 years ago, then design team decided to add > it on later platforms, so I don't see a reason to remove it again. :) > > I am not sure about the other qmp phys, but so long as UFS PHY needs the > reset, we need to keep it, as phy-qcom-qmp.c is a common driver. I am > not sure if I get your point here. Please correct me I am wrong. The argument here is that we are making this UFS specific and we do not know if this will be true in future as QMP is a common phy, so adding a separate flag helps to keep it independent and to be used in other situations. Thanks
On 12/20/2019 1:23 PM, Vinod Koul wrote: > On 20-12-19, 15:41, Can Guo wrote: >> On 2019-12-20 15:10, Vinod Koul wrote: >>> On 20-12-19, 14:00, Can Guo wrote: >> Hi Vinod >> >> We are just removing the no_pcs_sw_reset for 8150, right? Why is it >> possibly impacting 845 or older paltforms? >> >> In future, we will no longer need no_pcs_sw_reset for any newer QCOM UFS >> PHY designs, as it is only for 845 and older platforms. >> >> I am sure QPHY_SW_RESET will be within PHY's address space since 8150. >> Otherwise, it will be a regression in UFS PHY design. We had a lot of >> discussion about this on 845 years ago, then design team decided to add >> it on later platforms, so I don't see a reason to remove it again. :) >> >> I am not sure about the other qmp phys, but so long as UFS PHY needs the >> reset, we need to keep it, as phy-qcom-qmp.c is a common driver. I am >> not sure if I get your point here. Please correct me I am wrong. > The argument here is that we are making this UFS specific and we do not > know if this will be true in future as QMP is a common phy, so adding a > separate flag helps to keep it independent and to be used in other > situations. > > Thanks We should just remove no_pcs_sw_reset and let existing code take care of PHY reset for UFS. QMP PHY reset for UFS was differently handled earlier compared to USB/PCie and relied on core for PHY reset. That is not the case with addition of PCS based sw_reset and this won't change in future. There is no need to have UFS specific flag in this driver.
On 12/20/2019 6:19 AM, cang@codeaurora.org wrote: > On 2019-12-20 08:22, cang@codeaurora.org wrote: >> On 2019-12-19 23:04, Vinod Koul wrote: >>> For V4 QMP UFS Phy, we need to assert reset bits, configure the phy and >>> then deassert it, so add optional has_sw_reset flag and use that to >>> configure the QPHY_SW_RESET register. >>> >>> Signed-off-by: Vinod Koul <vkoul@kernel.org> >>> --- >>> drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c >>> b/drivers/phy/qualcomm/phy-qcom-qmp.c >>> index 06f971ca518e..80304b7cd895 100644 >>> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c >>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c >>> @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg { >>> >>> /* true, if PCS block has no separate SW_RESET register */ >>> bool no_pcs_sw_reset; >>> + >>> + /* true if sw reset needs to be invoked */ >>> + bool has_sw_reset; >>> }; >>> >>> /** >>> @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = { >>> >>> .is_dual_lane_phy = true, >>> .no_pcs_sw_reset = true, >>> + .has_sw_reset = true, >>> }; >>> >>> static void qcom_qmp_phy_configure(void __iomem *base, >>> @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy) >>> SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); >>> } >>> >>> + if (cfg->has_sw_reset) >>> + qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET); >>> + >> >> Are you sure you want to set this in the serdes register? QPHY_SW_RESET >> is in its pcs register. >> >>> if (cfg->has_phy_com_ctrl) >>> qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], >>> SW_PWRDN); >>> @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy) >>> if (cfg->has_phy_dp_com_ctrl) >>> qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); >>> >>> + if (cfg->has_sw_reset) >>> + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >>> + >> >> Yet you are clearing it from pcs register. >> >> Regards, >> Can Guo >> >>> /* start SerDes and Phy-Coding-Sublayer */ >>> qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl); > > I thought your change would be like this > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 8e642a6..a4ab4b7 100755 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -166,6 +166,7 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = { > }; > > static const unsigned int sm8150_ufsphy_regs_layout[] = { > + [QPHY_SW_RESET] = 0x08, > [QPHY_START_CTRL] = 0x00, > [QPHY_PCS_READY_STATUS] = 0x180, > }; > @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = { > .pwrdn_ctrl = SW_PWRDN, > > .is_dual_lane_phy = true, > - .no_pcs_sw_reset = true, This makes sense to me. > }; > > static void qcom_qmp_phy_configure(void __iomem *base, > @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy) > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); > } > > + if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset)) > + qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > + This change is not needed as POR value of SW_RESET bit is '1' which will be set as part of GCC or clk_reset. We just need to clear this bit which code already takes care of. > if (cfg->has_phy_com_ctrl) > qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], > SW_PWRDN); > > Regards, > Can Guo.
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c index 06f971ca518e..80304b7cd895 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg { /* true, if PCS block has no separate SW_RESET register */ bool no_pcs_sw_reset; + + /* true if sw reset needs to be invoked */ + bool has_sw_reset; }; /** @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = { .is_dual_lane_phy = true, .no_pcs_sw_reset = true, + .has_sw_reset = true, }; static void qcom_qmp_phy_configure(void __iomem *base, @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy) SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET); } + if (cfg->has_sw_reset) + qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET); + if (cfg->has_phy_com_ctrl) qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL], SW_PWRDN); @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy) if (cfg->has_phy_dp_com_ctrl) qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); + if (cfg->has_sw_reset) + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); + /* start SerDes and Phy-Coding-Sublayer */ qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
For V4 QMP UFS Phy, we need to assert reset bits, configure the phy and then deassert it, so add optional has_sw_reset flag and use that to configure the QPHY_SW_RESET register. Signed-off-by: Vinod Koul <vkoul@kernel.org> --- drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++ 1 file changed, 10 insertions(+)