[PATCH 1/2] phy-rockchip-pcie: add set_mode callback

[PATCH 1/2] phy-rockchip-pcie: add set_mode callback

[Shawn Lin]

phy_mode was added for switching USB mode purposely as
well as phy_set_mode API. However other types of PHY could
also have some miscellaneous setting/modes need to be
handled. This patch is gonna support this callback for
phy-rockchip-pcie and do some power-saving work there.
Note that we just stuff in some other values other that the
existing phy_mode and convert it in the coressponding driver
instead, otherwise we should extend the phy_mode again which
it doesn't make sense to add in new driver's specificed value.
Overall it looks fine to me as the controller's driver and the
phy's driver are paired so that the caller and the consumer should
be able to keep the value(mode) in consistent.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/phy/rockchip/phy-rockchip-pcie.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
index 6904633..9ffad15 100644
--- a/drivers/phy/rockchip/phy-rockchip-pcie.c
+++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
@@ -255,11 +255,33 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
 	return 0;
 }
 
+static int rockchip_pcie_phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	u8 map = (u8)mode;
+	int i;
+
+	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
+		if (map & BIT(i))
+			continue;
+
+		dev_dbg(&phy->dev, "idling lane %d\n", i);
+		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 + i));
+	}
+
+	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,
+	.set_mode	= rockchip_pcie_phy_set_mode,
 	.owner		= THIS_MODULE,
 };
 
-- 
1.9.1

[PATCH 2/2] PCI: rockchip: do some post-cleanup work for phy

[Shawn Lin]

This patch invents rockchip_pcie_lane_map for looking
up all lanes and find active one(s). Then we pass
on the active lane map for phy_set_mode so that the
we could do some basic cleanup work for power-saving by
idle the failed lane(s).

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/pci/host/pcie-rockchip.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 0e020b6..c1916fd 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -15,6 +15,7 @@
  * (at your option) any later version.
  */
 
+#include <linux/bitrev.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
@@ -111,6 +112,9 @@
 #define   PCIE_CORE_TXCREDIT_CFG1_MUI_SHIFT	16
 #define   PCIE_CORE_TXCREDIT_CFG1_MUI_ENCODE(x) \
 		(((x) >> 3) << PCIE_CORE_TXCREDIT_CFG1_MUI_SHIFT)
+#define PCIE_CORE_LANE_MAP		(PCIE_CORE_CTRL_MGMT_BASE + 0x200)
+#define   PCIE_CORE_LANE_MAP_MASK		0x0000000f
+#define   PCIE_CORE_LANE_MAP_REVERSE		BIT(16)
 #define PCIE_CORE_INT_STATUS		(PCIE_CORE_CTRL_MGMT_BASE + 0x20c)
 #define   PCIE_CORE_INT_PRFPE			BIT(0)
 #define   PCIE_CORE_INT_CRFPE			BIT(1)
@@ -292,6 +296,19 @@ static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip,
 	return 1;
 }
 
+
+static u8 rockchip_pcie_lane_map(struct rockchip_pcie *rockchip)
+{
+	u32 val = rockchip_pcie_read(rockchip, PCIE_CORE_LANE_MAP);
+	u8 map = val & PCIE_CORE_LANE_MAP_MASK;
+
+	/* The link may be using a reverse-indexed mapping. */
+	if (val & PCIE_CORE_LANE_MAP_REVERSE)
+		map = bitrev8(map) >> 4;
+
+	return map;
+}
+
 static int rockchip_pcie_rd_own_conf(struct rockchip_pcie *rockchip,
 				     int where, int size, u32 *val)
 {
@@ -646,6 +663,9 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 			  PCIE_CORE_PL_CONF_LANE_SHIFT);
 	dev_dbg(dev, "current link width is x%d\n", status);
 
+	/* Pass on the lane map for phy to do some power-saving work */
+	phy_set_mode(rockchip->phy, rockchip_pcie_lane_map(rockchip));
+
 	rockchip_pcie_write(rockchip, ROCKCHIP_VENDOR_ID,
 			    PCIE_CORE_CONFIG_VENDOR);
 	rockchip_pcie_write(rockchip,
-- 
1.9.1

Re: [PATCH 1/2] phy-rockchip-pcie: add set_mode callback

[Shawn Lin]

Hi Kishon,

On 2017/6/16 16:17, Shawn Lin wrote:
> phy_mode was added for switching USB mode purposely as
> well as phy_set_mode API. However other types of PHY could
> also have some miscellaneous setting/modes need to be
> handled. This patch is gonna support this callback for
> phy-rockchip-pcie and do some power-saving work there.
> Note that we just stuff in some other values other that the
> existing phy_mode and convert it in the coressponding driver
> instead, otherwise we should extend the phy_mode again which
> it doesn't make sense to add in new driver's specificed value.
> Overall it looks fine to me as the controller's driver and the
> phy's driver are paired so that the caller and the consumer should
> be able to keep the value(mode) in consistent.
> 

Any chance for you to have a look at this patch? :)

> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>   drivers/phy/rockchip/phy-rockchip-pcie.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index 6904633..9ffad15 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -255,11 +255,33 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
>   	return 0;
>   }
>   
> +static int rockchip_pcie_phy_set_mode(struct phy *phy, enum phy_mode mode)
> +{
> +	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> +	u8 map = (u8)mode;
> +	int i;
> +
> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> +		if (map & BIT(i))
> +			continue;
> +
> +		dev_dbg(&phy->dev, "idling lane %d\n", i);
> +		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 + i));
> +	}
> +
> +	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,
> +	.set_mode	= rockchip_pcie_phy_set_mode,
>   	.owner		= THIS_MODULE,
>   };
>   
> 

Re: [PATCH 1/2] phy-rockchip-pcie: add set_mode callback

[Bjorn Helgaas]

On Fri, Jun 16, 2017 at 04:17:47PM +0800, Shawn Lin wrote:
> phy_mode was added for switching USB mode purposely as
> well as phy_set_mode API. However other types of PHY could
> also have some miscellaneous setting/modes need to be
> handled. This patch is gonna support this callback for
> phy-rockchip-pcie and do some power-saving work there.

> Note that we just stuff in some other values other that the
> existing phy_mode and convert it in the coressponding driver
> instead, otherwise we should extend the phy_mode again which
> it doesn't make sense to add in new driver's specificed value.
> Overall it looks fine to me as the controller's driver and the
> phy's driver are paired so that the caller and the consumer should
> be able to keep the value(mode) in consistent.

s/coressponding/corresponding/
s/specificed/specified/
s/in consistent/consistent/

> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/phy/rockchip/phy-rockchip-pcie.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index 6904633..9ffad15 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -255,11 +255,33 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
>  	return 0;
>  }
>  
> +static int rockchip_pcie_phy_set_mode(struct phy *phy, enum phy_mode mode)
> +{
> +	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> +	u8 map = (u8)mode;
> +	int i;
> +
> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> +		if (map & BIT(i))
> +			continue;

phy_mode is defined as:

  enum phy_mode {
	  PHY_MODE_INVALID,
	  PHY_MODE_USB_HOST,
	  PHY_MODE_USB_DEVICE,
	  PHY_MODE_USB_OTG,
  };

You're using phy_mode as something entirely different -- a bitmap of
which lanes you want to keep active.  I understand you're trying to
avoid extending phy_mode again, but I just want to point out that
there are no other places that subvert phy_mode as you're doing.

It's probably worth a comment here about why we're not using the
standard enum values.  I see you already have one at the caller.

I'm not the PHY guy, so I'd like Kishon's ack before going down this
road.

> +
> +		dev_dbg(&phy->dev, "idling lane %d\n", i);
> +		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 + i));
> +	}
> +
> +	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,
> +	.set_mode	= rockchip_pcie_phy_set_mode,
>  	.owner		= THIS_MODULE,
>  };
>  
> -- 
> 1.9.1
> 
> 

Re: [PATCH 1/2] phy-rockchip-pcie: add set_mode callback

[Kishon Vijay Abraham I]

Hi,

On Friday 16 June 2017 01:47 PM, Shawn Lin wrote:
> phy_mode was added for switching USB mode purposely as
> well as phy_set_mode API. However other types of PHY could
> also have some miscellaneous setting/modes need to be
> handled. This patch is gonna support this callback for
> phy-rockchip-pcie and do some power-saving work there.
> Note that we just stuff in some other values other that the
> existing phy_mode and convert it in the coressponding driver
> instead, otherwise we should extend the phy_mode again which
> it doesn't make sense to add in new driver's specificed value.
> Overall it looks fine to me as the controller's driver and the
> phy's driver are paired so that the caller and the consumer should
> be able to keep the value(mode) in consistent.

I really don't prefer using an API other that what it is intended for. We have
to come up with a better way for handling this.

IIUC this patch sets unused lanes in idle mode? If yes, then can't we model
each lane as a separate phy, so that we can power-on/power-off each lane
independently.

Thanks
Kishon
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/phy/rockchip/phy-rockchip-pcie.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index 6904633..9ffad15 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -255,11 +255,33 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
>  	return 0;
>  }
>  
> +static int rockchip_pcie_phy_set_mode(struct phy *phy, enum phy_mode mode)
> +{
> +	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> +	u8 map = (u8)mode;
> +	int i;
> +
> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> +		if (map & BIT(i))
> +			continue;
> +
> +		dev_dbg(&phy->dev, "idling lane %d\n", i);
> +		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 + i));
> +	}
> +
> +	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,
> +	.set_mode	= rockchip_pcie_phy_set_mode,
>  	.owner		= THIS_MODULE,
>  };
>  
> 

Re: [PATCH 1/2] phy-rockchip-pcie: add set_mode callback

[Brian Norris]

Hi Kishon,

On Mon, Jul 10, 2017 at 9:45 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> On Friday 16 June 2017 01:47 PM, Shawn Lin wrote:
> > phy_mode was added for switching USB mode purposely as
> > well as phy_set_mode API. However other types of PHY could
> > also have some miscellaneous setting/modes need to be
> > handled. This patch is gonna support this callback for
> > phy-rockchip-pcie and do some power-saving work there.
> > Note that we just stuff in some other values other that the
> > existing phy_mode and convert it in the coressponding driver
> > instead, otherwise we should extend the phy_mode again which
> > it doesn't make sense to add in new driver's specificed value.
> > Overall it looks fine to me as the controller's driver and the
> > phy's driver are paired so that the caller and the consumer should
> > be able to keep the value(mode) in consistent.
>
> I really don't prefer using an API other that what it is intended for. We have
> to come up with a better way for handling this.
>
> IIUC this patch sets unused lanes in idle mode? If yes, then can't we model
> each lane as a separate phy, so that we can power-on/power-off each lane
> independently.

I suggested the same to Shawn previously. I'm interested in seeing his response.

But personally I'm still not sure if that's the most accurate way to
describe this PHY, except for the fact that inevitably, most device
tree bindings get molded to match the related kernel APIs, rather than
the other way around. What if instead, we added an enum mode entry
that represents "finished link training", at which point the PHY would
then know it can power down the unused lanes (that's assuming the PHY
can figure that part out)?

Or if we did re-model this PHY, how are we supposed to handle backward
compatibility? Can the same PHY driver support 2 different of_xlate
methods? I guess we'd have to provide an implementation that sets the
driver up to behave completely differently based on the number of
#phy-cells?

Brian

Re: [PATCH 1/2] phy-rockchip-pcie: add set_mode callback

[Brian Norris]

On Mon, Jul 10, 2017 at 10:20:32PM -0700, Brian Norris wrote:
> Hi Kishon,

Hi again Kishon,

I didn't get a response from you, but Shawn is continuing to send
several versions of a patchset implementing one of these suggestions.
I'd like to ping back here, in the hopes of getting an answer.

> On Mon, Jul 10, 2017 at 9:45 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> > On Friday 16 June 2017 01:47 PM, Shawn Lin wrote:
> > > phy_mode was added for switching USB mode purposely as
> > > well as phy_set_mode API. However other types of PHY could
> > > also have some miscellaneous setting/modes need to be
> > > handled. This patch is gonna support this callback for
> > > phy-rockchip-pcie and do some power-saving work there.
> > > Note that we just stuff in some other values other that the
> > > existing phy_mode and convert it in the coressponding driver
> > > instead, otherwise we should extend the phy_mode again which
> > > it doesn't make sense to add in new driver's specificed value.
> > > Overall it looks fine to me as the controller's driver and the
> > > phy's driver are paired so that the caller and the consumer should
> > > be able to keep the value(mode) in consistent.
> >
> > I really don't prefer using an API other that what it is intended for. We have
> > to come up with a better way for handling this.
> >
> > IIUC this patch sets unused lanes in idle mode? If yes, then can't we model
> > each lane as a separate phy, so that we can power-on/power-off each lane
> > independently.
> 
> I suggested the same to Shawn previously. I'm interested in seeing his response.

Judging by Shawn's subsequent patches, I guess he saw no problem with
this. I'm still not so sure.

> But personally I'm still not sure if that's the most accurate way to
> describe this PHY, except for the fact that inevitably, most device
> tree bindings get molded to match the related kernel APIs, rather than
> the other way around. What if instead, we added an enum mode entry
> that represents "finished link training", at which point the PHY would
> then know it can power down the unused lanes (that's assuming the PHY
> can figure that part out)?

Can I get an answer here? Is it possible that we extend the PHY API with
either a new 'phy_mode', or with some other PCIe-specific extension? As
I said above, it doesn't actually seem like a natural description of the
hardware to say that there are 4 PHYs here, instead of just 1. And then
it puts more burden on the provider/consumer drivers to do a lot of
weird accounting, just so that this single piece of hardware can fit the
current limited set of APIs.

This is not the first time that some concept isn't so easy to fit into
the existing framework. I think it would be prudent to consider ways to
better extend the API to support modes specific to device classes.
You've already done that for some USB pieces, and I think it would make
sense to extend this to PCIe. For instance, we could have the PHY export
things like "number of PCIe lanes", and support callbacks for "disable
unused lanes".

> Or if we did re-model this PHY, how are we supposed to handle backward
> compatibility? Can the same PHY driver support 2 different of_xlate
> methods? I guess we'd have to provide an implementation that sets the
> driver up to behave completely differently based on the number of
> #phy-cells?

I guess Shawn was able to work out some of this. But all the extra
state-tracking and reference counting appears to be hard to get right...

Brian

Re: [PATCH 1/2] phy-rockchip-pcie: add set_mode callback

[Shawn Lin]

Hi Kishon,

On 2017/7/11 12:45, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Friday 16 June 2017 01:47 PM, Shawn Lin wrote:
>> phy_mode was added for switching USB mode purposely as
>> well as phy_set_mode API. However other types of PHY could
>> also have some miscellaneous setting/modes need to be
>> handled. This patch is gonna support this callback for
>> phy-rockchip-pcie and do some power-saving work there.
>> Note that we just stuff in some other values other that the
>> existing phy_mode and convert it in the coressponding driver
>> instead, otherwise we should extend the phy_mode again which
>> it doesn't make sense to add in new driver's specificed value.
>> Overall it looks fine to me as the controller's driver and the
>> phy's driver are paired so that the caller and the consumer should
>> be able to keep the value(mode) in consistent.
> 
> I really don't prefer using an API other that what it is intended for. We have
> to come up with a better way for handling this.

okay.

> 
> IIUC this patch sets unused lanes in idle mode? If yes, then can't we model
> each lane as a separate phy, so that we can power-on/power-off each lane
> independently.

I will have a try and see how far I will go.

Thanks.

> 
> Thanks
> Kishon
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>   drivers/phy/rockchip/phy-rockchip-pcie.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
>> index 6904633..9ffad15 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
>> @@ -255,11 +255,33 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
>>   	return 0;
>>   }
>>   
>> +static int rockchip_pcie_phy_set_mode(struct phy *phy, enum phy_mode mode)
>> +{
>> +	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>> +	u8 map = (u8)mode;
>> +	int i;
>> +
>> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
>> +		if (map & BIT(i))
>> +			continue;
>> +
>> +		dev_dbg(&phy->dev, "idling lane %d\n", i);
>> +		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 + i));
>> +	}
>> +
>> +	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,
>> +	.set_mode	= rockchip_pcie_phy_set_mode,
>>   	.owner		= THIS_MODULE,
>>   };
>>   
>>
> 
> 
>