| Message ID | 1500004366-241633-2-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Jul 14, 2017 at 11:52:43AM +0800, Shawn Lin wrote: > This patch reconstructs the whole driver to support per-lane > PHYs. Note that we could also support the legacy PHY if you > don't provide argument to our of_xlate. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/phy/rockchip/phy-rockchip-pcie.c | 116 +++++++++++++++++++++++++++---- > 1 file changed, 101 insertions(+), 15 deletions(-) > > diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c > index 6904633..da74b47 100644 > --- a/drivers/phy/rockchip/phy-rockchip-pcie.c > +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c > @@ -73,10 +73,35 @@ struct rockchip_pcie_data { > struct rockchip_pcie_phy { > struct rockchip_pcie_data *phy_data; > struct regmap *reg_base; > + struct phy **phys; > struct reset_control *phy_rst; > struct clk *clk_pciephy_ref; > + u32 pwr_cnt; > + bool initialized; > }; > > +static struct phy *rockchip_pcie_phy_of_xlate(struct device *dev, > + struct of_phandle_args *args) > +{ > + struct rockchip_pcie_phy *rk_phy = dev_get_drvdata(dev); > + > + if (!rk_phy) > + return ERR_PTR(-ENODEV); Shouldn't you just check args->args_count to determine legacy vs. new style? > + > + switch (args->args[0]) { > + case 1: > + return rk_phy->phys[1]; > + case 2: > + return rk_phy->phys[2]; > + case 3: > + return rk_phy->phys[3]; > + case 0: > + /* keep backward compatibility to legacy phy */ > + default: This also ends up accepting invalid indeces. You should probably bounds-check args->args[0]. Then this can just be: if (legacy) return rk_phy->phys[0]; else return rk_phy->phys[index]; > + return rk_phy->phys[0]; > + } > +} > + > static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy, > u32 addr, u32 data) > { > @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy, > return val; > } > > -static int rockchip_pcie_phy_power_off(struct phy *phy) > +static int rockchip_pcie_phy_common_power_off(struct phy *phy) > { > struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); > int err = 0; > > + if (WARN_ON(!rk_phy->pwr_cnt)) > + return -EINVAL; > + > + if (rk_phy->pwr_cnt > 0) This should be: if (--rk_phy->pwr_cnt) Also, you technically might need locking, now that multiple phys (which each only have their own independent mutex) are accessing the same refcount. Or maybe just make this an atomic variable. > + return 0; > + > err = reset_control_assert(rk_phy->phy_rst); > if (err) { > dev_err(&phy->dev, "assert phy_rst err %d\n", err); > return err; > } > > + rk_phy->pwr_cnt--; You've got things backwards... how do you expect to ever decrement this, if you return earlier in the function? The effect is that you never power off after you've powered on. (You should try instrumenting and testing this better.) > + > return 0; > } > > +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \ > +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \ What? All this macro magic (and duplicate generated functions) should not be necessary. You just need some per-phy data that keeps the index. > +{ \ > + struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); \ > +\ > + regmap_write(rk_phy->reg_base, \ > + rk_phy->phy_data->pcie_laneoff, \ > + HIWORD_UPDATE(PHY_LANE_IDLE_OFF, \ > + PHY_LANE_IDLE_MASK, \ > + PHY_LANE_IDLE_A_SHIFT + id)); \ > + return rockchip_pcie_phy_common_power_off(phy); \ > +} > + > +DECLARE_PHY_POWER_OFF_PER_LANE(0); > +DECLARE_PHY_POWER_OFF_PER_LANE(1); > +DECLARE_PHY_POWER_OFF_PER_LANE(2); > +DECLARE_PHY_POWER_OFF_PER_LANE(3); > + > +#define PROVIDE_PHY_OPS(id) \ > + { \ > + .init = rockchip_pcie_phy_init, \ > + .exit = rockchip_pcie_phy_exit, \ > + .power_on = rockchip_pcie_phy_power_on, \ > + .power_off = rockchip_pcie_lane##id##_phy_power_off, \ > + .owner = THIS_MODULE, \ > +} > + > static int rockchip_pcie_phy_power_on(struct phy *phy) > { > struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); > @@ -135,6 +195,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy) > u32 status; > unsigned long timeout; > > + if (WARN_ON(rk_phy->pwr_cnt > PHY_MAX_LANE_NUM)) > + return -EINVAL; > + > + if (rk_phy->pwr_cnt) This could just be: if (rk_phy->pwr_cnt++) > + return 0; > + > err = reset_control_deassert(rk_phy->phy_rst); > if (err) { > dev_err(&phy->dev, "deassert phy_rst err %d\n", err); > @@ -214,6 +280,7 @@ static int rockchip_pcie_phy_power_on(struct phy *phy) > goto err_pll_lock; > } > > + rk_phy->pwr_cnt++; Similar problem to what you're doing in power_off(); you're not doing the refcount right. Brian > return 0; > > err_pll_lock: > @@ -226,6 +293,9 @@ static int rockchip_pcie_phy_init(struct phy *phy) > struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); > int err = 0; > > + if (rk_phy->initialized) > + return 0; > + > err = clk_prepare_enable(rk_phy->clk_pciephy_ref); > if (err) { > dev_err(&phy->dev, "Fail to enable pcie ref clock.\n"); > @@ -238,7 +308,9 @@ static int rockchip_pcie_phy_init(struct phy *phy) > goto err_reset; > } > > - return err; > + rk_phy->initialized = true; > + > + return 0; > > err_reset: > clk_disable_unprepare(rk_phy->clk_pciephy_ref); > @@ -250,17 +322,21 @@ static int rockchip_pcie_phy_exit(struct phy *phy) > { > struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); > > + if (!rk_phy->initialized) > + return 0; > + > clk_disable_unprepare(rk_phy->clk_pciephy_ref); > > + rk_phy->initialized = false; > + > return 0; > } > > -static const struct phy_ops ops = { > - .init = rockchip_pcie_phy_init, > - .exit = rockchip_pcie_phy_exit, > - .power_on = rockchip_pcie_phy_power_on, > - .power_off = rockchip_pcie_phy_power_off, > - .owner = THIS_MODULE, > +static const struct phy_ops ops[PHY_MAX_LANE_NUM] = { > + PROVIDE_PHY_OPS(0), > + PROVIDE_PHY_OPS(1), > + PROVIDE_PHY_OPS(2), > + PROVIDE_PHY_OPS(3), > }; > > static const struct rockchip_pcie_data rk3399_pcie_data = { > @@ -283,10 +359,10 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct rockchip_pcie_phy *rk_phy; > - struct phy *generic_phy; > struct phy_provider *phy_provider; > struct regmap *grf; > const struct of_device_id *of_id; > + int i; > > grf = syscon_node_to_regmap(dev->parent->of_node); > if (IS_ERR(grf)) { > @@ -319,14 +395,24 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev) > return PTR_ERR(rk_phy->clk_pciephy_ref); > } > > - generic_phy = devm_phy_create(dev, dev->of_node, &ops); > - if (IS_ERR(generic_phy)) { > - dev_err(dev, "failed to create PHY\n"); > - return PTR_ERR(generic_phy); > + rk_phy->phys = devm_kcalloc(dev, sizeof(struct phy), > + PHY_MAX_LANE_NUM, GFP_KERNEL); > + if (!rk_phy->phys) > + return -ENOMEM; > + > + for (i = 0; i < PHY_MAX_LANE_NUM; i++) { > + rk_phy->phys[i] = devm_phy_create(dev, dev->of_node, &ops[i]); > + if (IS_ERR(rk_phy->phys[i])) { > + dev_err(dev, "failed to create PHY%d\n", i); > + return PTR_ERR(rk_phy->phys[i]); > + } > + > + phy_set_drvdata(rk_phy->phys[i], rk_phy); > } > > - phy_set_drvdata(generic_phy, rk_phy); > - phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > + platform_set_drvdata(pdev, rk_phy); > + phy_provider = devm_of_phy_provider_register(dev, > + rockchip_pcie_phy_of_xlate); > > return PTR_ERR_OR_ZERO(phy_provider); > } > -- > 1.9.1 > >
Hi Brian, On 2017/7/14 13:10, Brian Norris wrote: > On Fri, Jul 14, 2017 at 11:52:43AM +0800, Shawn Lin wrote: >> This patch reconstructs the whole driver to support per-lane >> PHYs. Note that we could also support the legacy PHY if you >> don't provide argument to our of_xlate. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/phy/rockchip/phy-rockchip-pcie.c | 116 +++++++++++++++++++++++++++---- >> 1 file changed, 101 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c >> index 6904633..da74b47 100644 >> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c >> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c >> @@ -73,10 +73,35 @@ struct rockchip_pcie_data { >> struct rockchip_pcie_phy { >> struct rockchip_pcie_data *phy_data; >> struct regmap *reg_base; >> + struct phy **phys; >> struct reset_control *phy_rst; >> struct clk *clk_pciephy_ref; >> + u32 pwr_cnt; >> + bool initialized; >> }; >> >> +static struct phy *rockchip_pcie_phy_of_xlate(struct device *dev, >> + struct of_phandle_args *args) >> +{ >> + struct rockchip_pcie_phy *rk_phy = dev_get_drvdata(dev); >> + >> + if (!rk_phy) >> + return ERR_PTR(-ENODEV); > > Shouldn't you just check args->args_count to determine legacy vs. new > style? > args_count is 1 for legacy mode but could also means you just add one phy with the new per-lane mode? >> + >> + switch (args->args[0]) { >> + case 1: >> + return rk_phy->phys[1]; >> + case 2: >> + return rk_phy->phys[2]; >> + case 3: >> + return rk_phy->phys[3]; >> + case 0: >> + /* keep backward compatibility to legacy phy */ >> + default: > > This also ends up accepting invalid indeces. You should probably > bounds-check args->args[0]. > > Then this can just be: > > if (legacy) > return rk_phy->phys[0]; > else > return rk_phy->phys[index]; However, checking args_count to see if it's legacy seems to simply the code a lot. So I would fix that above. > >> + return rk_phy->phys[0]; >> + } >> +} >> + >> static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy, >> u32 addr, u32 data) >> { >> @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy, >> return val; >> } >> >> -static int rockchip_pcie_phy_power_off(struct phy *phy) >> +static int rockchip_pcie_phy_common_power_off(struct phy *phy) >> { >> struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); >> int err = 0; >> >> + if (WARN_ON(!rk_phy->pwr_cnt)) >> + return -EINVAL; >> + >> + if (rk_phy->pwr_cnt > 0) > > This should be: > > if (--rk_phy->pwr_cnt) > > Also, you technically might need locking, now that multiple phys (which > each only have their own independent mutex) are accessing the same > refcount. Or maybe just make this an atomic variable. Good catch! > >> + return 0; >> + >> err = reset_control_assert(rk_phy->phy_rst); >> if (err) { >> dev_err(&phy->dev, "assert phy_rst err %d\n", err); >> return err; >> } >> >> + rk_phy->pwr_cnt--; > > You've got things backwards... how do you expect to ever decrement this, > if you return earlier in the function? The effect is that you never > power off after you've powered on. (You should try instrumenting and > testing this better.) Right, I should notice this if I checked the power for S3 but unfortunately I didn't.. > >> + >> return 0; >> } >> >> +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \ >> +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \ > > What? All this macro magic (and duplicate generated functions) should > not be necessary. You just need some per-phy data that keeps the index. I can't quite follow yours here. The only argument passing on to the PHY APIs is 'struct phy *phy', and how could you trace the index from it? The caller should save phy instead of 'rockchip_pcie_phy', in which the per-phy data should be. Or could you kindly show me some example here:) > >> +{ \ >> + struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); \ >> +\ >> + regmap_write(rk_phy->reg_base, \ >> + rk_phy->phy_data->pcie_laneoff, \ >> + HIWORD_UPDATE(PHY_LANE_IDLE_OFF, \ >> + PHY_LANE_IDLE_MASK, \ >> + PHY_LANE_IDLE_A_SHIFT + id)); \ >> + return rockchip_pcie_phy_common_power_off(phy); \ >> +} >> + >> +DECLARE_PHY_POWER_OFF_PER_LANE(0); >> +DECLARE_PHY_POWER_OFF_PER_LANE(1); >> +DECLARE_PHY_POWER_OFF_PER_LANE(2); >> +DECLARE_PHY_POWER_OFF_PER_LANE(3); >> + >> +#define PROVIDE_PHY_OPS(id) \ >> + { \ >> + .init = rockchip_pcie_phy_init, \ >> + .exit = rockchip_pcie_phy_exit, \ >> + .power_on = rockchip_pcie_phy_power_on, \ >> + .power_off = rockchip_pcie_lane##id##_phy_power_off, \ >> + .owner = THIS_MODULE, \ >> +} >> + >> static int rockchip_pcie_phy_power_on(struct phy *phy) >> { >> struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); >> @@ -135,6 +195,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy) >> u32 status; >> unsigned long timeout; >> >> + if (WARN_ON(rk_phy->pwr_cnt > PHY_MAX_LANE_NUM)) >> + return -EINVAL; >> + >> + if (rk_phy->pwr_cnt) > > This could just be: > > if (rk_phy->pwr_cnt++) > >> + return 0; >> + >> err = reset_control_deassert(rk_phy->phy_rst); >> if (err) { >> dev_err(&phy->dev, "deassert phy_rst err %d\n", err); >> @@ -214,6 +280,7 @@ static int rockchip_pcie_phy_power_on(struct phy *phy) >> goto err_pll_lock; >> } >> >> + rk_phy->pwr_cnt++; > > Similar problem to what you're doing in power_off(); you're not doing > the refcount right. Will fix as well. > > Brian > >> return 0; >> >> err_pll_lock: >> @@ -226,6 +293,9 @@ static int rockchip_pcie_phy_init(struct phy *phy) >> struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); >> int err = 0; >> >> + if (rk_phy->initialized) >> + return 0; >> + >> err = clk_prepare_enable(rk_phy->clk_pciephy_ref); >> if (err) { >> dev_err(&phy->dev, "Fail to enable pcie ref clock.\n"); >> @@ -238,7 +308,9 @@ static int rockchip_pcie_phy_init(struct phy *phy) >> goto err_reset; >> } >> >> - return err; >> + rk_phy->initialized = true; >> + >> + return 0; >> >> err_reset: >> clk_disable_unprepare(rk_phy->clk_pciephy_ref); >> @@ -250,17 +322,21 @@ static int rockchip_pcie_phy_exit(struct phy *phy) >> { >> struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); >> >> + if (!rk_phy->initialized) >> + return 0; >> + >> clk_disable_unprepare(rk_phy->clk_pciephy_ref); >> >> + rk_phy->initialized = false; >> + >> return 0; >> } >> >> -static const struct phy_ops ops = { >> - .init = rockchip_pcie_phy_init, >> - .exit = rockchip_pcie_phy_exit, >> - .power_on = rockchip_pcie_phy_power_on, >> - .power_off = rockchip_pcie_phy_power_off, >> - .owner = THIS_MODULE, >> +static const struct phy_ops ops[PHY_MAX_LANE_NUM] = { >> + PROVIDE_PHY_OPS(0), >> + PROVIDE_PHY_OPS(1), >> + PROVIDE_PHY_OPS(2), >> + PROVIDE_PHY_OPS(3), >> }; >> >> static const struct rockchip_pcie_data rk3399_pcie_data = { >> @@ -283,10 +359,10 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct rockchip_pcie_phy *rk_phy; >> - struct phy *generic_phy; >> struct phy_provider *phy_provider; >> struct regmap *grf; >> const struct of_device_id *of_id; >> + int i; >> >> grf = syscon_node_to_regmap(dev->parent->of_node); >> if (IS_ERR(grf)) { >> @@ -319,14 +395,24 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev) >> return PTR_ERR(rk_phy->clk_pciephy_ref); >> } >> >> - generic_phy = devm_phy_create(dev, dev->of_node, &ops); >> - if (IS_ERR(generic_phy)) { >> - dev_err(dev, "failed to create PHY\n"); >> - return PTR_ERR(generic_phy); >> + rk_phy->phys = devm_kcalloc(dev, sizeof(struct phy), >> + PHY_MAX_LANE_NUM, GFP_KERNEL); >> + if (!rk_phy->phys) >> + return -ENOMEM; >> + >> + for (i = 0; i < PHY_MAX_LANE_NUM; i++) { >> + rk_phy->phys[i] = devm_phy_create(dev, dev->of_node, &ops[i]); >> + if (IS_ERR(rk_phy->phys[i])) { >> + dev_err(dev, "failed to create PHY%d\n", i); >> + return PTR_ERR(rk_phy->phys[i]); >> + } >> + >> + phy_set_drvdata(rk_phy->phys[i], rk_phy); >> } >> >> - phy_set_drvdata(generic_phy, rk_phy); >> - phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); >> + platform_set_drvdata(pdev, rk_phy); >> + phy_provider = devm_of_phy_provider_register(dev, >> + rockchip_pcie_phy_of_xlate); >> >> return PTR_ERR_OR_ZERO(phy_provider); >> } >> -- >> 1.9.1 >> >> > > >
Hi Shawn, On 07/14/2017 02:33 PM, Shawn Lin wrote: >> >>> + return rk_phy->phys[0]; >>> + } >>> +} >>> + >>> static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy, >>> u32 addr, u32 data) >>> { >>> @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct >>> rockchip_pcie_phy *rk_phy, >>> return val; >>> } >>> -static int rockchip_pcie_phy_power_off(struct phy *phy) >>> +static int rockchip_pcie_phy_common_power_off(struct phy *phy) >>> { >>> struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); >>> int err = 0; >>> + if (WARN_ON(!rk_phy->pwr_cnt)) >>> + return -EINVAL; >>> + >>> + if (rk_phy->pwr_cnt > 0) >> >> This should be: >> >> if (--rk_phy->pwr_cnt) >> >> Also, you technically might need locking, now that multiple phys (which >> each only have their own independent mutex) are accessing the same >> refcount. Or maybe just make this an atomic variable. > > Good catch! Sounds like we need something similar to phy-core.c's power_count and init_count. >>> + >>> return 0; >>> } >>> +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \ >>> +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \ >> >> What? All this macro magic (and duplicate generated functions) should >> not be necessary. You just need some per-phy data that keeps the index. > > I can't quite follow yours here. The only argument passing on to > the PHY APIs is 'struct phy *phy', and how could you trace the index > from it? The caller should save phy instead of 'rockchip_pcie_phy', in > which the per-phy data should be. > > Or could you kindly show me some example here:) > Maybe add a struct rockchip_pcie_phy_data for each phy, contains their index and a pointer to the common struct rockchip_pcie_phy?
Hi Shawn, On 07/14/2017 02:33 PM, Shawn Lin wrote: >>> >>> +static struct phy *rockchip_pcie_phy_of_xlate(struct device *dev, >>> + struct of_phandle_args *args) >>> +{ >>> + struct rockchip_pcie_phy *rk_phy = dev_get_drvdata(dev); >>> + >>> + if (!rk_phy) >>> + return ERR_PTR(-ENODEV); >> >> Shouldn't you just check args->args_count to determine legacy vs. new >> style? >> > > args_count is 1 for legacy mode but could also means you just add one > phy with the new per-lane mode? The new per-lane mode should follow by a lane number, so their args_count should be 1, while in the legacy mode it should be 0.
Hi Jeffy, On 2017/7/14 15:03, jeffy wrote: > Hi Shawn, > > On 07/14/2017 02:33 PM, Shawn Lin wrote: >>> >>>> + return rk_phy->phys[0]; >>>> + } >>>> +} >>>> + >>>> static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy, >>>> u32 addr, u32 data) >>>> { >>>> @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct >>>> rockchip_pcie_phy *rk_phy, >>>> return val; >>>> } >>>> -static int rockchip_pcie_phy_power_off(struct phy *phy) >>>> +static int rockchip_pcie_phy_common_power_off(struct phy *phy) >>>> { >>>> struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); >>>> int err = 0; >>>> + if (WARN_ON(!rk_phy->pwr_cnt)) >>>> + return -EINVAL; >>>> + >>>> + if (rk_phy->pwr_cnt > 0) >>> >>> This should be: >>> >>> if (--rk_phy->pwr_cnt) >>> >>> Also, you technically might need locking, now that multiple phys (which >>> each only have their own independent mutex) are accessing the same >>> refcount. Or maybe just make this an atomic variable. >> >> Good catch! > Sounds like we need something similar to phy-core.c's power_count and > init_count. Probably, and I will look into it later. > >>>> + >>>> return 0; >>>> } >>>> +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \ >>>> +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \ >>> >>> What? All this macro magic (and duplicate generated functions) should >>> not be necessary. You just need some per-phy data that keeps the index. >> >> I can't quite follow yours here. The only argument passing on to >> the PHY APIs is 'struct phy *phy', and how could you trace the index >> from it? The caller should save phy instead of 'rockchip_pcie_phy', in >> which the per-phy data should be. >> >> Or could you kindly show me some example here:) >> > Maybe add a struct rockchip_pcie_phy_data for each phy, contains their > index and a pointer to the common struct rockchip_pcie_phy? yes, I got Brian's point after reading it more times, and I almost finish converting to per-lane data now.. > > > > >
diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c index 6904633..da74b47 100644 --- a/drivers/phy/rockchip/phy-rockchip-pcie.c +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c @@ -73,10 +73,35 @@ struct rockchip_pcie_data { struct rockchip_pcie_phy { struct rockchip_pcie_data *phy_data; struct regmap *reg_base; + struct phy **phys; struct reset_control *phy_rst; struct clk *clk_pciephy_ref; + u32 pwr_cnt; + bool initialized; }; +static struct phy *rockchip_pcie_phy_of_xlate(struct device *dev, + struct of_phandle_args *args) +{ + struct rockchip_pcie_phy *rk_phy = dev_get_drvdata(dev); + + if (!rk_phy) + return ERR_PTR(-ENODEV); + + switch (args->args[0]) { + case 1: + return rk_phy->phys[1]; + case 2: + return rk_phy->phys[2]; + case 3: + return rk_phy->phys[3]; + case 0: + /* keep backward compatibility to legacy phy */ + default: + return rk_phy->phys[0]; + } +} + static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy, u32 addr, u32 data) { @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy, return val; } -static int rockchip_pcie_phy_power_off(struct phy *phy) +static int rockchip_pcie_phy_common_power_off(struct phy *phy) { struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); int err = 0; + if (WARN_ON(!rk_phy->pwr_cnt)) + return -EINVAL; + + if (rk_phy->pwr_cnt > 0) + return 0; + err = reset_control_assert(rk_phy->phy_rst); if (err) { dev_err(&phy->dev, "assert phy_rst err %d\n", err); return err; } + rk_phy->pwr_cnt--; + return 0; } +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \ +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \ +{ \ + struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); \ +\ + regmap_write(rk_phy->reg_base, \ + rk_phy->phy_data->pcie_laneoff, \ + HIWORD_UPDATE(PHY_LANE_IDLE_OFF, \ + PHY_LANE_IDLE_MASK, \ + PHY_LANE_IDLE_A_SHIFT + id)); \ + return rockchip_pcie_phy_common_power_off(phy); \ +} + +DECLARE_PHY_POWER_OFF_PER_LANE(0); +DECLARE_PHY_POWER_OFF_PER_LANE(1); +DECLARE_PHY_POWER_OFF_PER_LANE(2); +DECLARE_PHY_POWER_OFF_PER_LANE(3); + +#define PROVIDE_PHY_OPS(id) \ + { \ + .init = rockchip_pcie_phy_init, \ + .exit = rockchip_pcie_phy_exit, \ + .power_on = rockchip_pcie_phy_power_on, \ + .power_off = rockchip_pcie_lane##id##_phy_power_off, \ + .owner = THIS_MODULE, \ +} + static int rockchip_pcie_phy_power_on(struct phy *phy) { struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); @@ -135,6 +195,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy) u32 status; unsigned long timeout; + if (WARN_ON(rk_phy->pwr_cnt > PHY_MAX_LANE_NUM)) + return -EINVAL; + + if (rk_phy->pwr_cnt) + return 0; + err = reset_control_deassert(rk_phy->phy_rst); if (err) { dev_err(&phy->dev, "deassert phy_rst err %d\n", err); @@ -214,6 +280,7 @@ static int rockchip_pcie_phy_power_on(struct phy *phy) goto err_pll_lock; } + rk_phy->pwr_cnt++; return 0; err_pll_lock: @@ -226,6 +293,9 @@ static int rockchip_pcie_phy_init(struct phy *phy) struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); int err = 0; + if (rk_phy->initialized) + return 0; + err = clk_prepare_enable(rk_phy->clk_pciephy_ref); if (err) { dev_err(&phy->dev, "Fail to enable pcie ref clock.\n"); @@ -238,7 +308,9 @@ static int rockchip_pcie_phy_init(struct phy *phy) goto err_reset; } - return err; + rk_phy->initialized = true; + + return 0; err_reset: clk_disable_unprepare(rk_phy->clk_pciephy_ref); @@ -250,17 +322,21 @@ static int rockchip_pcie_phy_exit(struct phy *phy) { struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); + if (!rk_phy->initialized) + return 0; + clk_disable_unprepare(rk_phy->clk_pciephy_ref); + rk_phy->initialized = false; + return 0; } -static const struct phy_ops ops = { - .init = rockchip_pcie_phy_init, - .exit = rockchip_pcie_phy_exit, - .power_on = rockchip_pcie_phy_power_on, - .power_off = rockchip_pcie_phy_power_off, - .owner = THIS_MODULE, +static const struct phy_ops ops[PHY_MAX_LANE_NUM] = { + PROVIDE_PHY_OPS(0), + PROVIDE_PHY_OPS(1), + PROVIDE_PHY_OPS(2), + PROVIDE_PHY_OPS(3), }; static const struct rockchip_pcie_data rk3399_pcie_data = { @@ -283,10 +359,10 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct rockchip_pcie_phy *rk_phy; - struct phy *generic_phy; struct phy_provider *phy_provider; struct regmap *grf; const struct of_device_id *of_id; + int i; grf = syscon_node_to_regmap(dev->parent->of_node); if (IS_ERR(grf)) { @@ -319,14 +395,24 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev) return PTR_ERR(rk_phy->clk_pciephy_ref); } - generic_phy = devm_phy_create(dev, dev->of_node, &ops); - if (IS_ERR(generic_phy)) { - dev_err(dev, "failed to create PHY\n"); - return PTR_ERR(generic_phy); + rk_phy->phys = devm_kcalloc(dev, sizeof(struct phy), + PHY_MAX_LANE_NUM, GFP_KERNEL); + if (!rk_phy->phys) + return -ENOMEM; + + for (i = 0; i < PHY_MAX_LANE_NUM; i++) { + rk_phy->phys[i] = devm_phy_create(dev, dev->of_node, &ops[i]); + if (IS_ERR(rk_phy->phys[i])) { + dev_err(dev, "failed to create PHY%d\n", i); + return PTR_ERR(rk_phy->phys[i]); + } + + phy_set_drvdata(rk_phy->phys[i], rk_phy); } - phy_set_drvdata(generic_phy, rk_phy); - phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); + platform_set_drvdata(pdev, rk_phy); + phy_provider = devm_of_phy_provider_register(dev, + rockchip_pcie_phy_of_xlate); return PTR_ERR_OR_ZERO(phy_provider); }
This patch reconstructs the whole driver to support per-lane PHYs. Note that we could also support the legacy PHY if you don't provide argument to our of_xlate. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/phy/rockchip/phy-rockchip-pcie.c | 116 +++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 15 deletions(-)