diff mbox

[RFC,3/6] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

Message ID 1500004366-241633-2-git-send-email-shawn.lin@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Lin July 14, 2017, 3:52 a.m. UTC
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(-)

Comments

Brian Norris July 14, 2017, 5:10 a.m. UTC | #1
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
> 
>
Shawn Lin July 14, 2017, 6:33 a.m. UTC | #2
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
>>
>>
> 
> 
>
Jeffy Chen July 14, 2017, 7:03 a.m. UTC | #3
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?
Jeffy Chen July 14, 2017, 7:19 a.m. UTC | #4
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.
Shawn Lin July 14, 2017, 9:14 a.m. UTC | #5
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 mbox

Patch

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);
 }