phy: ti-pipe3: Add set_mode callback to configure usb3 phy as pcie phy

DRA72 platform has the second instance of PHY shared between USB3
controller and PCIe controller with default as USB3 controller.
Since it is used with USB3 controller by default, it uses the
compatible specific to USB (ti,omap-usb3).

Populate set_mode callback so that the USB3 PHY can be configured
to be used with PCIe controller.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

-- 
2.17.1

Hi Kishon,

On 24/01/19 12:48, Kishon Vijay Abraham I wrote:
> DRA72 platform has the second instance of PHY shared between USB3

> controller and PCIe controller with default as USB3 controller.

> Since it is used with USB3 controller by default, it uses the

> compatible specific to USB (ti,omap-usb3).

> 

> Populate set_mode callback so that the USB3 PHY can be configured

> to be used with PCIe controller.


What is the use case of this? At what point and who decides the
phy_set_mode() to be called with USB vs PCIe?


> 

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> ---

>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>  1 file changed, 54 insertions(+), 12 deletions(-)

> 

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

> index 68ce4a082b9b..8c98f366416d 100644

> --- a/drivers/phy/ti/phy-ti-pipe3.c

> +++ b/drivers/phy/ti/phy-ti-pipe3.c

> @@ -56,6 +56,12 @@

>  

>  #define SATA_PLL_SOFT_RESET	BIT(18)

>  

> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

> +#define MEM_EN_PLLBYP			BIT(7)

> +

> +#define PHY_TX_TEST_CONFIG		0x2C

> +#define MEM_ENTESTCLK			BIT(31)

> +

>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>  

> @@ -110,6 +116,8 @@

>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>  

> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

> +

>  struct pipe3_dpll_params {

>  	u16	m;

>  	u8	n;

> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>  	unsigned int		power_reg; /* power reg. index within syscon */

>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>  	bool			sata_refclk_enabled;

> +	u32			mode;

>  };

>  

>  static struct pipe3_dpll_map dpll_map_usb[] = {

> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>  	rate = rate / 1000000;

>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

> +	val = PIPE3_PHY_TX_RX_POWERON;

> +	if (phy->mode == PHY_MODE_PCIE)

> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>  

>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>  }

>  

> -static int ti_pipe3_init(struct phy *x)

> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>  {

> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

> -	u32 val;

>  	int ret = 0;

> +	u32 val;

>  

> -	ti_pipe3_enable_clocks(phy);

>  	/*

>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>  			return ret;

>  

>  		ti_pipe3_calibrate(phy);

> -

> -		return 0;

> +	} else {

> +		val = ti_pipe3_readl(phy->phy_rx,

> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

> +		val |= MEM_EN_PLLBYP;

> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

> +				val);

> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

> +		val |= MEM_ENTESTCLK;

> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>  	}

>  

> +	return 0;

> +}

> +

> +static int ti_pipe3_init(struct phy *x)

> +{

> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

> +	u32 val;

> +	int ret = 0;

> +

> +	ti_pipe3_enable_clocks(phy);

> +

> +	if (phy->mode == PHY_MODE_PCIE)

> +		return ti_pipe3_pcie_init(phy);

> +

>  	/* Bring it out of IDLE if it is IDLE */

>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>  	if (val & PLL_IDLE) {

> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>  		return 0;

>  

>  	/* PCIe doesn't have internal DPLL */

> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>  		/* Put DPLL in IDLE mode */

>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>  		val |= PLL_IDLE;

> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>  

>  	return 0;

>  }

> +

> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

> +{

> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

> +

> +	if (phy->mode != PHY_MODE_INVALID)

> +		return -EBUSY;

> +

> +	phy->mode = mode;


You are only saving the mode. But not really switching modes here.
How is this intended to work?

> +

> +	return 0;

> +}

> +

>  static const struct phy_ops ops = {

>  	.init		= ti_pipe3_init,

>  	.exit		= ti_pipe3_exit,

>  	.power_on	= ti_pipe3_power_on,

>  	.power_off	= ti_pipe3_power_off,

> +	.set_mode	= ti_pipe3_set_mode,

>  	.owner		= THIS_MODULE,

>  };

>  

> @@ -589,12 +634,8 @@ static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)

>  {

>  	struct resource *res;

>  	struct device *dev = phy->dev;

> -	struct device_node *node = dev->of_node;

>  	struct platform_device *pdev = to_platform_device(dev);

>  

> -	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))

> -		return 0;

> -

>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

>  					   "phy_rx");

>  	phy->phy_rx = devm_ioremap_resource(dev, res);

> @@ -649,6 +690,7 @@ static int ti_pipe3_probe(struct platform_device *pdev)

>  		return -ENOMEM;

>  

>  	phy->dev		= dev;

> +	phy->mode		= PHY_MODE_INVALID;

>  

>  	ret = ti_pipe3_get_pll_base(phy);

>  	if (ret)

> 


cheers,
-roger

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Hi Roger,

On 30/01/19 4:18 PM, Roger Quadros wrote:
> Hi Kishon,

> 

> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>> DRA72 platform has the second instance of PHY shared between USB3

>> controller and PCIe controller with default as USB3 controller.

>> Since it is used with USB3 controller by default, it uses the

>> compatible specific to USB (ti,omap-usb3).

>>

>> Populate set_mode callback so that the USB3 PHY can be configured

>> to be used with PCIe controller.

> 

> What is the use case of this? At what point and who decides the

> phy_set_mode() to be called with USB vs PCIe?


The PHY by default is configured to be used with USB controller (That's why it
has a compatible of ti,omap-usb3). However there is a special case where this
PHY has to be used PCIe controller. So the set_mode() will help the PHY driver
to configure the USB PHY to be used with PCIe controller.

This is not a shared PHY, so the PHY as such can be used only with USB or PCIe
and is determined at designtime.
> 

> 

>>

>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>> ---

>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>

>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>> index 68ce4a082b9b..8c98f366416d 100644

>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>> @@ -56,6 +56,12 @@

>>  

>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>  

>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>> +#define MEM_EN_PLLBYP			BIT(7)

>> +

>> +#define PHY_TX_TEST_CONFIG		0x2C

>> +#define MEM_ENTESTCLK			BIT(31)

>> +

>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>  

>> @@ -110,6 +116,8 @@

>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>  

>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>> +

>>  struct pipe3_dpll_params {

>>  	u16	m;

>>  	u8	n;

>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>  	bool			sata_refclk_enabled;

>> +	u32			mode;

>>  };

>>  

>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>  	rate = rate / 1000000;

>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>> +	val = PIPE3_PHY_TX_RX_POWERON;

>> +	if (phy->mode == PHY_MODE_PCIE)

>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>  

>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>  }

>>  

>> -static int ti_pipe3_init(struct phy *x)

>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>  {

>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>> -	u32 val;

>>  	int ret = 0;

>> +	u32 val;

>>  

>> -	ti_pipe3_enable_clocks(phy);

>>  	/*

>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>  			return ret;

>>  

>>  		ti_pipe3_calibrate(phy);

>> -

>> -		return 0;

>> +	} else {

>> +		val = ti_pipe3_readl(phy->phy_rx,

>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>> +		val |= MEM_EN_PLLBYP;

>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>> +				val);

>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>> +		val |= MEM_ENTESTCLK;

>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>  	}

>>  

>> +	return 0;

>> +}

>> +

>> +static int ti_pipe3_init(struct phy *x)

>> +{

>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>> +	u32 val;

>> +	int ret = 0;

>> +

>> +	ti_pipe3_enable_clocks(phy);

>> +

>> +	if (phy->mode == PHY_MODE_PCIE)

>> +		return ti_pipe3_pcie_init(phy);

>> +

>>  	/* Bring it out of IDLE if it is IDLE */

>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>  	if (val & PLL_IDLE) {

>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>  		return 0;

>>  

>>  	/* PCIe doesn't have internal DPLL */

>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>>  		/* Put DPLL in IDLE mode */

>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>  		val |= PLL_IDLE;

>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>  

>>  	return 0;

>>  }

>> +

>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>> +{

>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>> +

>> +	if (phy->mode != PHY_MODE_INVALID)

>> +		return -EBUSY;

>> +

>> +	phy->mode = mode;

> 

> You are only saving the mode. But not really switching modes here.

> How is this intended to work?


The PHY doesn't have a specific mode. Rather it is a set of configurations
required to operate in a particular mode.

This mode value will be used in other init, power_on callback functions.

Thanks
Kishon

Kishon,

On 30/01/19 12:59, Kishon Vijay Abraham I wrote:
> Hi Roger,

> 

> On 30/01/19 4:18 PM, Roger Quadros wrote:

>> Hi Kishon,

>>

>> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>>> DRA72 platform has the second instance of PHY shared between USB3

>>> controller and PCIe controller with default as USB3 controller.

>>> Since it is used with USB3 controller by default, it uses the

>>> compatible specific to USB (ti,omap-usb3).

>>>

>>> Populate set_mode callback so that the USB3 PHY can be configured

>>> to be used with PCIe controller.

>>

>> What is the use case of this? At what point and who decides the

>> phy_set_mode() to be called with USB vs PCIe?

> 

> The PHY by default is configured to be used with USB controller (That's why it

> has a compatible of ti,omap-usb3). However there is a special case where this

> PHY has to be used PCIe controller. So the set_mode() will help the PHY driver

> to configure the USB PHY to be used with PCIe controller.

> 

> This is not a shared PHY, so the PHY as such can be used only with USB or PCIe

> and is determined at designtime.


If type is defined at design time then the design using the PHY for PCIe shouldn't be
using "ti,omap-usb3" compatible. Right?

Maybe we should fix the DT? Which board/PHY is this about?

cheers,
-roger

>>

>>

>>>

>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>>> ---

>>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>>

>>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>>> index 68ce4a082b9b..8c98f366416d 100644

>>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>>> @@ -56,6 +56,12 @@

>>>  

>>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>>  

>>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>>> +#define MEM_EN_PLLBYP			BIT(7)

>>> +

>>> +#define PHY_TX_TEST_CONFIG		0x2C

>>> +#define MEM_ENTESTCLK			BIT(31)

>>> +

>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>>  

>>> @@ -110,6 +116,8 @@

>>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>>  

>>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>>> +

>>>  struct pipe3_dpll_params {

>>>  	u16	m;

>>>  	u8	n;

>>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>>  	bool			sata_refclk_enabled;

>>> +	u32			mode;

>>>  };

>>>  

>>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>>  	rate = rate / 1000000;

>>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>> +	val = PIPE3_PHY_TX_RX_POWERON;

>>> +	if (phy->mode == PHY_MODE_PCIE)

>>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

>>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>>  

>>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>>  }

>>>  

>>> -static int ti_pipe3_init(struct phy *x)

>>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>>  {

>>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>> -	u32 val;

>>>  	int ret = 0;

>>> +	u32 val;

>>>  

>>> -	ti_pipe3_enable_clocks(phy);

>>>  	/*

>>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>>  			return ret;

>>>  

>>>  		ti_pipe3_calibrate(phy);

>>> -

>>> -		return 0;

>>> +	} else {

>>> +		val = ti_pipe3_readl(phy->phy_rx,

>>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>>> +		val |= MEM_EN_PLLBYP;

>>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>>> +				val);

>>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>>> +		val |= MEM_ENTESTCLK;

>>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>>  	}

>>>  

>>> +	return 0;

>>> +}

>>> +

>>> +static int ti_pipe3_init(struct phy *x)

>>> +{

>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>> +	u32 val;

>>> +	int ret = 0;

>>> +

>>> +	ti_pipe3_enable_clocks(phy);

>>> +

>>> +	if (phy->mode == PHY_MODE_PCIE)

>>> +		return ti_pipe3_pcie_init(phy);

>>> +

>>>  	/* Bring it out of IDLE if it is IDLE */

>>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>  	if (val & PLL_IDLE) {

>>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>>  		return 0;

>>>  

>>>  	/* PCIe doesn't have internal DPLL */

>>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>>>  		/* Put DPLL in IDLE mode */

>>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>  		val |= PLL_IDLE;

>>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>>  

>>>  	return 0;

>>>  }

>>> +

>>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>>> +{

>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>> +

>>> +	if (phy->mode != PHY_MODE_INVALID)

>>> +		return -EBUSY;

>>> +

>>> +	phy->mode = mode;

>>

>> You are only saving the mode. But not really switching modes here.

>> How is this intended to work?

> 

> The PHY doesn't have a specific mode. Rather it is a set of configurations

> required to operate in a particular mode.

> 

> This mode value will be used in other init, power_on callback functions.

> 

> Thanks

> Kishon

> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Hi Roger,

On 30/01/19 6:11 PM, Roger Quadros wrote:
> Kishon,

> 

> On 30/01/19 12:59, Kishon Vijay Abraham I wrote:

>> Hi Roger,

>>

>> On 30/01/19 4:18 PM, Roger Quadros wrote:

>>> Hi Kishon,

>>>

>>> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>>>> DRA72 platform has the second instance of PHY shared between USB3

>>>> controller and PCIe controller with default as USB3 controller.

>>>> Since it is used with USB3 controller by default, it uses the

>>>> compatible specific to USB (ti,omap-usb3).

>>>>

>>>> Populate set_mode callback so that the USB3 PHY can be configured

>>>> to be used with PCIe controller.

>>>

>>> What is the use case of this? At what point and who decides the

>>> phy_set_mode() to be called with USB vs PCIe?

>>

>> The PHY by default is configured to be used with USB controller (That's why it

>> has a compatible of ti,omap-usb3). However there is a special case where this

>> PHY has to be used PCIe controller. So the set_mode() will help the PHY driver

>> to configure the USB PHY to be used with PCIe controller.

>>

>> This is not a shared PHY, so the PHY as such can be used only with USB or PCIe

>> and is determined at designtime

> 

> If type is defined at design time then the design using the PHY for PCIe shouldn't be

> using "ti,omap-usb3" compatible. Right?

The second instance of PIPE3 PHY in dra72x SoC can be used either with USB3 or
as a second lane of the 1st PCIe instance. How it is connected is based on
board design. DRA72x EVM will need board modification for the PHY to be used
with PCIe (by default it is connected to USB). AM571x IDK doesn't have a USB3
port so the PIPE3 PHY is used with PCIe (USB2 also doesn't work with this
configuration but that is a different HW issue).

I'm not sure if we should change the compatible in board dts file as it is
based on board design. Previously I added a dt property to indicate the PHY
should be configured for PCIe but Rob Herring suggested to use set_mode [1].

Thanks
Kishon

[1] -> https://lore.kernel.org/patchwork/patch/865398/
> 

> Maybe we should fix the DT? Which board/PHY is this about?

> 

> cheers,

> -roger

> 

>>>

>>>

>>>>

>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>>>> ---

>>>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>>>

>>>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>>>> index 68ce4a082b9b..8c98f366416d 100644

>>>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>>>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>>>> @@ -56,6 +56,12 @@

>>>>  

>>>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>>>  

>>>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>>>> +#define MEM_EN_PLLBYP			BIT(7)

>>>> +

>>>> +#define PHY_TX_TEST_CONFIG		0x2C

>>>> +#define MEM_ENTESTCLK			BIT(31)

>>>> +

>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>>>  

>>>> @@ -110,6 +116,8 @@

>>>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>>>  

>>>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>>>> +

>>>>  struct pipe3_dpll_params {

>>>>  	u16	m;

>>>>  	u8	n;

>>>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>>>  	bool			sata_refclk_enabled;

>>>> +	u32			mode;

>>>>  };

>>>>  

>>>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>>>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>>>  	rate = rate / 1000000;

>>>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>>>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>> +	val = PIPE3_PHY_TX_RX_POWERON;

>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

>>>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>>>  

>>>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>>>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>>>  }

>>>>  

>>>> -static int ti_pipe3_init(struct phy *x)

>>>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>>>  {

>>>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>> -	u32 val;

>>>>  	int ret = 0;

>>>> +	u32 val;

>>>>  

>>>> -	ti_pipe3_enable_clocks(phy);

>>>>  	/*

>>>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>>>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>>>  			return ret;

>>>>  

>>>>  		ti_pipe3_calibrate(phy);

>>>> -

>>>> -		return 0;

>>>> +	} else {

>>>> +		val = ti_pipe3_readl(phy->phy_rx,

>>>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>>>> +		val |= MEM_EN_PLLBYP;

>>>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>>>> +				val);

>>>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>>>> +		val |= MEM_ENTESTCLK;

>>>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>>>  	}

>>>>  

>>>> +	return 0;

>>>> +}

>>>> +

>>>> +static int ti_pipe3_init(struct phy *x)

>>>> +{

>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>> +	u32 val;

>>>> +	int ret = 0;

>>>> +

>>>> +	ti_pipe3_enable_clocks(phy);

>>>> +

>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>> +		return ti_pipe3_pcie_init(phy);

>>>> +

>>>>  	/* Bring it out of IDLE if it is IDLE */

>>>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>  	if (val & PLL_IDLE) {

>>>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>>>  		return 0;

>>>>  

>>>>  	/* PCIe doesn't have internal DPLL */

>>>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>>>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>>>>  		/* Put DPLL in IDLE mode */

>>>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>  		val |= PLL_IDLE;

>>>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>>>  

>>>>  	return 0;

>>>>  }

>>>> +

>>>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>>>> +{

>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>> +

>>>> +	if (phy->mode != PHY_MODE_INVALID)

>>>> +		return -EBUSY;

>>>> +

>>>> +	phy->mode = mode;

>>>

>>> You are only saving the mode. But not really switching modes here.

>>> How is this intended to work?

>>

>> The PHY doesn't have a specific mode. Rather it is a set of configurations

>> required to operate in a particular mode.

>>

>> This mode value will be used in other init, power_on callback functions.

>>

>> Thanks

>> Kishon

>>

>

Hi,

On 30/01/19 7:27 PM, Roger Quadros wrote:
> +Rob,

> 

> On 30/01/19 15:00, Kishon Vijay Abraham I wrote:

>> Hi Roger,

>>

>> On 30/01/19 6:11 PM, Roger Quadros wrote:

>>> Kishon,

>>>

>>> On 30/01/19 12:59, Kishon Vijay Abraham I wrote:

>>>> Hi Roger,

>>>>

>>>> On 30/01/19 4:18 PM, Roger Quadros wrote:

>>>>> Hi Kishon,

>>>>>

>>>>> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>>>>>> DRA72 platform has the second instance of PHY shared between USB3

>>>>>> controller and PCIe controller with default as USB3 controller.

>>>>>> Since it is used with USB3 controller by default, it uses the

>>>>>> compatible specific to USB (ti,omap-usb3).

>>>>>>

>>>>>> Populate set_mode callback so that the USB3 PHY can be configured

>>>>>> to be used with PCIe controller.

>>>>>

>>>>> What is the use case of this? At what point and who decides the

>>>>> phy_set_mode() to be called with USB vs PCIe?

>>>>

>>>> The PHY by default is configured to be used with USB controller (That's why it

>>>> has a compatible of ti,omap-usb3). However there is a special case where this

>>>> PHY has to be used PCIe controller. So the set_mode() will help the PHY driver

>>>> to configure the USB PHY to be used with PCIe controller.

>>>>

>>>> This is not a shared PHY, so the PHY as such can be used only with USB or PCIe

>>>> and is determined at designtime

>>>

>>> If type is defined at design time then the design using the PHY for PCIe shouldn't be

>>> using "ti,omap-usb3" compatible. Right?

>> The second instance of PIPE3 PHY in dra72x SoC can be used either with USB3 or

>> as a second lane of the 1st PCIe instance. How it is connected is based on

>> board design. DRA72x EVM will need board modification for the PHY to be used

>> with PCIe (by default it is connected to USB). AM571x IDK doesn't have a USB3

>> port so the PIPE3 PHY is used with PCIe (USB2 also doesn't work with this

>> configuration but that is a different HW issue).

>>

>> I'm not sure if we should change the compatible in board dts file as it is

>> based on board design. Previously I added a dt property to indicate the PHY

>> should be configured for PCIe but Rob Herring suggested to use set_mode [1].

> 

> That is because you tried to add a new property "ti,configure-as-pcie" which

> does seem unnecessary.

> 

> Wouldn't it work if you just use "ti,phy-pipe3-pcie" in the relevant device tree?


No, it won't. The configuration required for a PCIe PHY is different from a USB
PHY to be used for PCIe.

Thanks
Kishon

> 

> cheers,

> -roger

> 

>>

>> Thanks

>> Kishon

>>

>> [1] -> https://lore.kernel.org/patchwork/patch/865398/

>>>

>>> Maybe we should fix the DT? Which board/PHY is this about?

>>>

>>> cheers,

>>> -roger

>>>

>>>>>

>>>>>

>>>>>>

>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>>>>>> ---

>>>>>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>>>>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>>>>>

>>>>>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>>>>>> index 68ce4a082b9b..8c98f366416d 100644

>>>>>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>>>>>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>>>>>> @@ -56,6 +56,12 @@

>>>>>>  

>>>>>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>>>>>  

>>>>>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>>>>>> +#define MEM_EN_PLLBYP			BIT(7)

>>>>>> +

>>>>>> +#define PHY_TX_TEST_CONFIG		0x2C

>>>>>> +#define MEM_ENTESTCLK			BIT(31)

>>>>>> +

>>>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>>>>>  

>>>>>> @@ -110,6 +116,8 @@

>>>>>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>>>>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>>>>>  

>>>>>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>>>>>> +

>>>>>>  struct pipe3_dpll_params {

>>>>>>  	u16	m;

>>>>>>  	u8	n;

>>>>>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>>>>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>>>>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>>>>>  	bool			sata_refclk_enabled;

>>>>>> +	u32			mode;

>>>>>>  };

>>>>>>  

>>>>>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>>>>>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>>>>>  	rate = rate / 1000000;

>>>>>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>>>>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>>>>>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>>>> +	val = PIPE3_PHY_TX_RX_POWERON;

>>>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>>>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

>>>>>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>>>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>>>>>  

>>>>>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>>>>>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>>>>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>>>>>  }

>>>>>>  

>>>>>> -static int ti_pipe3_init(struct phy *x)

>>>>>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>>>>>  {

>>>>>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>>> -	u32 val;

>>>>>>  	int ret = 0;

>>>>>> +	u32 val;

>>>>>>  

>>>>>> -	ti_pipe3_enable_clocks(phy);

>>>>>>  	/*

>>>>>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>>>>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>>>>>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>>>>>  			return ret;

>>>>>>  

>>>>>>  		ti_pipe3_calibrate(phy);

>>>>>> -

>>>>>> -		return 0;

>>>>>> +	} else {

>>>>>> +		val = ti_pipe3_readl(phy->phy_rx,

>>>>>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>>>>>> +		val |= MEM_EN_PLLBYP;

>>>>>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>>>>>> +				val);

>>>>>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>>>>>> +		val |= MEM_ENTESTCLK;

>>>>>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>>>>>  	}

>>>>>>  

>>>>>> +	return 0;

>>>>>> +}

>>>>>> +

>>>>>> +static int ti_pipe3_init(struct phy *x)

>>>>>> +{

>>>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>>> +	u32 val;

>>>>>> +	int ret = 0;

>>>>>> +

>>>>>> +	ti_pipe3_enable_clocks(phy);

>>>>>> +

>>>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>>>> +		return ti_pipe3_pcie_init(phy);

>>>>>> +

>>>>>>  	/* Bring it out of IDLE if it is IDLE */

>>>>>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>>>  	if (val & PLL_IDLE) {

>>>>>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>>>>>  		return 0;

>>>>>>  

>>>>>>  	/* PCIe doesn't have internal DPLL */

>>>>>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>>>>>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>>>>>>  		/* Put DPLL in IDLE mode */

>>>>>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>>>  		val |= PLL_IDLE;

>>>>>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>>>>>  

>>>>>>  	return 0;

>>>>>>  }

>>>>>> +

>>>>>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>>>>>> +{

>>>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>>> +

>>>>>> +	if (phy->mode != PHY_MODE_INVALID)

>>>>>> +		return -EBUSY;

>>>>>> +

>>>>>> +	phy->mode = mode;

>>>>>

>>>>> You are only saving the mode. But not really switching modes here.

>>>>> How is this intended to work?

>>>>

>>>> The PHY doesn't have a specific mode. Rather it is a set of configurations

>>>> required to operate in a particular mode.

>>>>

>>>> This mode value will be used in other init, power_on callback functions.

>>>>

>>>> Thanks

>>>> Kishon

>>>>

>>>

>

On 30/01/19 15:59, Kishon Vijay Abraham I wrote:
> Hi,

> 

> On 30/01/19 7:27 PM, Roger Quadros wrote:

>> +Rob,

>>

>> On 30/01/19 15:00, Kishon Vijay Abraham I wrote:

>>> Hi Roger,

>>>

>>> On 30/01/19 6:11 PM, Roger Quadros wrote:

>>>> Kishon,

>>>>

>>>> On 30/01/19 12:59, Kishon Vijay Abraham I wrote:

>>>>> Hi Roger,

>>>>>

>>>>> On 30/01/19 4:18 PM, Roger Quadros wrote:

>>>>>> Hi Kishon,

>>>>>>

>>>>>> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>>>>>>> DRA72 platform has the second instance of PHY shared between USB3

>>>>>>> controller and PCIe controller with default as USB3 controller.

>>>>>>> Since it is used with USB3 controller by default, it uses the

>>>>>>> compatible specific to USB (ti,omap-usb3).

>>>>>>>

>>>>>>> Populate set_mode callback so that the USB3 PHY can be configured

>>>>>>> to be used with PCIe controller.

>>>>>>

>>>>>> What is the use case of this? At what point and who decides the

>>>>>> phy_set_mode() to be called with USB vs PCIe?

>>>>>

>>>>> The PHY by default is configured to be used with USB controller (That's why it

>>>>> has a compatible of ti,omap-usb3). However there is a special case where this

>>>>> PHY has to be used PCIe controller. So the set_mode() will help the PHY driver

>>>>> to configure the USB PHY to be used with PCIe controller.

>>>>>

>>>>> This is not a shared PHY, so the PHY as such can be used only with USB or PCIe

>>>>> and is determined at designtime

>>>>

>>>> If type is defined at design time then the design using the PHY for PCIe shouldn't be

>>>> using "ti,omap-usb3" compatible. Right?

>>> The second instance of PIPE3 PHY in dra72x SoC can be used either with USB3 or

>>> as a second lane of the 1st PCIe instance. How it is connected is based on

>>> board design. DRA72x EVM will need board modification for the PHY to be used

>>> with PCIe (by default it is connected to USB). AM571x IDK doesn't have a USB3

>>> port so the PIPE3 PHY is used with PCIe (USB2 also doesn't work with this

>>> configuration but that is a different HW issue).

>>>

>>> I'm not sure if we should change the compatible in board dts file as it is

>>> based on board design. Previously I added a dt property to indicate the PHY

>>> should be configured for PCIe but Rob Herring suggested to use set_mode [1].

>>

>> That is because you tried to add a new property "ti,configure-as-pcie" which

>> does seem unnecessary.

>>

>> Wouldn't it work if you just use "ti,phy-pipe3-pcie" in the relevant device tree?

> 

> No, it won't. The configuration required for a PCIe PHY is different from a USB

> PHY to be used for PCIe.


I understand now. Compatible shouldn't change. I have a couple of comments still.
I'll respond to the main patch.

cheers,
-roger

> 

> Thanks

> Kishon

> 

>>

>> cheers,

>> -roger

>>

>>>

>>> Thanks

>>> Kishon

>>>

>>> [1] -> https://lore.kernel.org/patchwork/patch/865398/

>>>>

>>>> Maybe we should fix the DT? Which board/PHY is this about?

>>>>

>>>> cheers,

>>>> -roger

>>>>

>>>>>>

>>>>>>

>>>>>>>

>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>>>>>>> ---

>>>>>>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>>>>>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>>>>>>

>>>>>>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>>>>>>> index 68ce4a082b9b..8c98f366416d 100644

>>>>>>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>>>>>>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>>>>>>> @@ -56,6 +56,12 @@

>>>>>>>  

>>>>>>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>>>>>>  

>>>>>>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>>>>>>> +#define MEM_EN_PLLBYP			BIT(7)

>>>>>>> +

>>>>>>> +#define PHY_TX_TEST_CONFIG		0x2C

>>>>>>> +#define MEM_ENTESTCLK			BIT(31)

>>>>>>> +

>>>>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>>>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>>>>>>  

>>>>>>> @@ -110,6 +116,8 @@

>>>>>>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>>>>>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>>>>>>  

>>>>>>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>>>>>>> +

>>>>>>>  struct pipe3_dpll_params {

>>>>>>>  	u16	m;

>>>>>>>  	u8	n;

>>>>>>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>>>>>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>>>>>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>>>>>>  	bool			sata_refclk_enabled;

>>>>>>> +	u32			mode;

>>>>>>>  };

>>>>>>>  

>>>>>>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>>>>>>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>>>>>>  	rate = rate / 1000000;

>>>>>>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>>>>>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>>>>>>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>>>>> +	val = PIPE3_PHY_TX_RX_POWERON;

>>>>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>>>>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

>>>>>>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>>>>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>>>>>>  

>>>>>>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>>>>>>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>>>>>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>>>>>>  }

>>>>>>>  

>>>>>>> -static int ti_pipe3_init(struct phy *x)

>>>>>>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>>>>>>  {

>>>>>>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>>>> -	u32 val;

>>>>>>>  	int ret = 0;

>>>>>>> +	u32 val;

>>>>>>>  

>>>>>>> -	ti_pipe3_enable_clocks(phy);

>>>>>>>  	/*

>>>>>>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>>>>>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>>>>>>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>>>>>>  			return ret;

>>>>>>>  

>>>>>>>  		ti_pipe3_calibrate(phy);

>>>>>>> -

>>>>>>> -		return 0;

>>>>>>> +	} else {

>>>>>>> +		val = ti_pipe3_readl(phy->phy_rx,

>>>>>>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>>>>>>> +		val |= MEM_EN_PLLBYP;

>>>>>>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>>>>>>> +				val);

>>>>>>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>>>>>>> +		val |= MEM_ENTESTCLK;

>>>>>>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>>>>>>  	}

>>>>>>>  

>>>>>>> +	return 0;

>>>>>>> +}

>>>>>>> +

>>>>>>> +static int ti_pipe3_init(struct phy *x)

>>>>>>> +{

>>>>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>>>> +	u32 val;

>>>>>>> +	int ret = 0;

>>>>>>> +

>>>>>>> +	ti_pipe3_enable_clocks(phy);

>>>>>>> +

>>>>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>>>>> +		return ti_pipe3_pcie_init(phy);

>>>>>>> +

>>>>>>>  	/* Bring it out of IDLE if it is IDLE */

>>>>>>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>>>>  	if (val & PLL_IDLE) {

>>>>>>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>>>>>>  		return 0;

>>>>>>>  

>>>>>>>  	/* PCIe doesn't have internal DPLL */

>>>>>>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>>>>>>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>>>>>>>  		/* Put DPLL in IDLE mode */

>>>>>>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>>>>  		val |= PLL_IDLE;

>>>>>>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>>>>>>  

>>>>>>>  	return 0;

>>>>>>>  }

>>>>>>> +

>>>>>>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>>>>>>> +{

>>>>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>>>> +

>>>>>>> +	if (phy->mode != PHY_MODE_INVALID)

>>>>>>> +		return -EBUSY;

>>>>>>> +

>>>>>>> +	phy->mode = mode;

>>>>>>

>>>>>> You are only saving the mode. But not really switching modes here.

>>>>>> How is this intended to work?

>>>>>

>>>>> The PHY doesn't have a specific mode. Rather it is a set of configurations

>>>>> required to operate in a particular mode.

>>>>>

>>>>> This mode value will be used in other init, power_on callback functions.

>>>>>

>>>>> Thanks

>>>>> Kishon

>>>>>

>>>>

>>


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Kishon,

On 24/01/19 12:48, Kishon Vijay Abraham I wrote:
> DRA72 platform has the second instance of PHY shared between USB3

> controller and PCIe controller with default as USB3 controller.

> Since it is used with USB3 controller by default, it uses the

> compatible specific to USB (ti,omap-usb3).

> 

> Populate set_mode callback so that the USB3 PHY can be configured

> to be used with PCIe controller.


How about rewording this to,

"On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed
lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single
lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed.
Provide a way for the PHY user to choose the appropriate mode via the
.set_mode hook"

More comments below.

> 

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> ---

>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>  1 file changed, 54 insertions(+), 12 deletions(-)

> 

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

> index 68ce4a082b9b..8c98f366416d 100644

> --- a/drivers/phy/ti/phy-ti-pipe3.c

> +++ b/drivers/phy/ti/phy-ti-pipe3.c

> @@ -56,6 +56,12 @@

>  

>  #define SATA_PLL_SOFT_RESET	BIT(18)

>  

> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

> +#define MEM_EN_PLLBYP			BIT(7)

> +

> +#define PHY_TX_TEST_CONFIG		0x2C

> +#define MEM_ENTESTCLK			BIT(31)

> +

>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>  

> @@ -110,6 +116,8 @@

>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>  

> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

> +

>  struct pipe3_dpll_params {

>  	u16	m;

>  	u8	n;

> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>  	unsigned int		power_reg; /* power reg. index within syscon */

>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>  	bool			sata_refclk_enabled;

> +	u32			mode;

>  };

>  

>  static struct pipe3_dpll_map dpll_map_usb[] = {

> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>  	rate = rate / 1000000;

>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

> +	val = PIPE3_PHY_TX_RX_POWERON;

> +	if (phy->mode == PHY_MODE_PCIE)

> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;


Is this required only for the USB3 PHY being used as PCIe lane or can it
be done for the PCIe PHY as well?

I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY
and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that
currently.

Maybe this is safer?

	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))

> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>  

>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>  }

>  

> -static int ti_pipe3_init(struct phy *x)

> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>  {

> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

> -	u32 val;

>  	int ret = 0;

> +	u32 val;

>  

> -	ti_pipe3_enable_clocks(phy);

>  	/*

>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>  			return ret;

>  

>  		ti_pipe3_calibrate(phy);

> -

> -		return 0;

> +	} else {


How about

	else if (of_device_is_compatible(node, "ti,phy-usb3")) {
		/* USB3 PHY being used as PCIe Lane */

> +		val = ti_pipe3_readl(phy->phy_rx,

> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

> +		val |= MEM_EN_PLLBYP;

> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

> +				val);

> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

> +		val |= MEM_ENTESTCLK;

> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>  	}

>  

> +	return 0;

> +}

> +

> +static int ti_pipe3_init(struct phy *x)

> +{

> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

> +	u32 val;

> +	int ret = 0;

> +

> +	ti_pipe3_enable_clocks(phy);

> +

> +	if (phy->mode == PHY_MODE_PCIE)


Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE.
Should we set the mode to PCI_MODE_PCIE explicitly it in probe?

> +		return ti_pipe3_pcie_init(phy);

> +

>  	/* Bring it out of IDLE if it is IDLE */

>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>  	if (val & PLL_IDLE) {

> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>  		return 0;

>  

>  	/* PCIe doesn't have internal DPLL */

> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

> +	if (!(phy->mode == PHY_MODE_PCIE)) {


But this might be a USB3 PHY, which has DPLL.

>  		/* Put DPLL in IDLE mode */

>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>  		val |= PLL_IDLE;

> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>  

>  	return 0;

>  }

> +

> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

> +{

> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

> +

> +	if (phy->mode != PHY_MODE_INVALID)

> +		return -EBUSY;

> +

> +	phy->mode = mode;


We need to do a sanity check here and prevent unsupported modes being set.

e.g. we don't want PCIe mode being set on omap-usb3 PHY or SATA PHY.

Which makes me wonder, if all PHYs with compatible "ti,phy-usb3" don't support
PCIe mode then we might need to add a new compatible for USB3 PHY's that do support
PCIe mode?

Or maybe a DT-property of supported PHY modes?

> +

> +	return 0;

> +}

> +

>  static const struct phy_ops ops = {

>  	.init		= ti_pipe3_init,

>  	.exit		= ti_pipe3_exit,

>  	.power_on	= ti_pipe3_power_on,

>  	.power_off	= ti_pipe3_power_off,

> +	.set_mode	= ti_pipe3_set_mode,

>  	.owner		= THIS_MODULE,

>  };

>  

> @@ -589,12 +634,8 @@ static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)

>  {

>  	struct resource *res;

>  	struct device *dev = phy->dev;

> -	struct device_node *node = dev->of_node;

>  	struct platform_device *pdev = to_platform_device(dev);

>  

> -	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))

> -		return 0;

> -


Do all USB3 and SATA PHYs have "phy_rx" and "phy_tx" regions?
If not it will break them.

>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

>  					   "phy_rx");

>  	phy->phy_rx = devm_ioremap_resource(dev, res);

> @@ -649,6 +690,7 @@ static int ti_pipe3_probe(struct platform_device *pdev)

>  		return -ENOMEM;

>  

>  	phy->dev		= dev;

> +	phy->mode		= PHY_MODE_INVALID;

>  


Let's do

	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie"))
		phy->mode = PCI_MODE_PCIE;

>  	ret = ti_pipe3_get_pll_base(phy);

>  	if (ret)

> 


cheers,
-roger

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Roger,

On 30/01/19 8:28 PM, Roger Quadros wrote:
> Kishon,

> 

> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>> DRA72 platform has the second instance of PHY shared between USB3

>> controller and PCIe controller with default as USB3 controller.

>> Since it is used with USB3 controller by default, it uses the

>> compatible specific to USB (ti,omap-usb3).

>>

>> Populate set_mode callback so that the USB3 PHY can be configured

>> to be used with PCIe controller.

> 

> How about rewording this to,

> 

> "On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed

> lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single

> lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed.

> Provide a way for the PHY user to choose the appropriate mode via the

> .set_mode hook"


hmm okay
> 

> More comments below.

> 

>>

>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>> ---

>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>

>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>> index 68ce4a082b9b..8c98f366416d 100644

>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>> @@ -56,6 +56,12 @@

>>  

>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>  

>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>> +#define MEM_EN_PLLBYP			BIT(7)

>> +

>> +#define PHY_TX_TEST_CONFIG		0x2C

>> +#define MEM_ENTESTCLK			BIT(31)

>> +

>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>  

>> @@ -110,6 +116,8 @@

>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>  

>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>> +

>>  struct pipe3_dpll_params {

>>  	u16	m;

>>  	u8	n;

>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>  	bool			sata_refclk_enabled;

>> +	u32			mode;

>>  };

>>  

>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>  	rate = rate / 1000000;

>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>> +	val = PIPE3_PHY_TX_RX_POWERON;

>> +	if (phy->mode == PHY_MODE_PCIE)

>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

> 

> Is this required only for the USB3 PHY being used as PCIe lane or can it

> be done for the PCIe PHY as well?


This setting was given only for USB3 PHY. I did check PCIe PHY (1-lane) to see
if this is causing issues before posting the patch.

Thinking again, it might be safer to just use this setting for USB3 PHY.
> 

> I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY

> and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that

> currently.

> 

> Maybe this is safer?

> 

> 	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))


yeah..
> 

>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>  

>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>  }

>>  

>> -static int ti_pipe3_init(struct phy *x)

>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>  {

>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>> -	u32 val;

>>  	int ret = 0;

>> +	u32 val;

>>  

>> -	ti_pipe3_enable_clocks(phy);

>>  	/*

>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>  			return ret;

>>  

>>  		ti_pipe3_calibrate(phy);

>> -

>> -		return 0;

>> +	} else {

> 

> How about

> 

> 	else if (of_device_is_compatible(node, "ti,phy-usb3")) {


This check is implicit no?
> 		/* USB3 PHY being used as PCIe Lane */

> 

>> +		val = ti_pipe3_readl(phy->phy_rx,

>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>> +		val |= MEM_EN_PLLBYP;

>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>> +				val);

>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>> +		val |= MEM_ENTESTCLK;

>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>  	}

>>  

>> +	return 0;

>> +}

>> +

>> +static int ti_pipe3_init(struct phy *x)

>> +{

>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>> +	u32 val;

>> +	int ret = 0;

>> +

>> +	ti_pipe3_enable_clocks(phy);

>> +

>> +	if (phy->mode == PHY_MODE_PCIE)

> 

> Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE.

> Should we set the mode to PCI_MODE_PCIE explicitly it in probe?


The set_mode callback checks if the phy->mode is INVALID before setting the
mode. The phy_set_mode in the PCIe consumer driver will return error if it is
already set. The phy_set_mode will be called for both the lanes by the PCIe driver.
> 

>> +		return ti_pipe3_pcie_init(phy);

>> +

>>  	/* Bring it out of IDLE if it is IDLE */

>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>  	if (val & PLL_IDLE) {

>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>  		return 0;

>>  

>>  	/* PCIe doesn't have internal DPLL */

>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

> 

> But this might be a USB3 PHY, which has DPLL.


hmm. I have to check this. We should also then reset PLL_IDLE bit in phy_init.
> 

>>  		/* Put DPLL in IDLE mode */

>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>  		val |= PLL_IDLE;

>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>  

>>  	return 0;

>>  }

>> +

>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>> +{

>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>> +

>> +	if (phy->mode != PHY_MODE_INVALID)

>> +		return -EBUSY;

>> +

>> +	phy->mode = mode;

> 

> We need to do a sanity check here and prevent unsupported modes being set.

> 

> e.g. we don't want PCIe mode being set on omap-usb3 PHY or SATA PHY.

> 

> Which makes me wonder, if all PHYs with compatible "ti,phy-usb3" don't support

> PCIe mode then we might need to add a new compatible for USB3 PHY's that do support

> PCIe mode?

> 

> Or maybe a DT-property of supported PHY modes?


hmm yeah, makes sense.
> 

>> +

>> +	return 0;

>> +}

>> +

>>  static const struct phy_ops ops = {

>>  	.init		= ti_pipe3_init,

>>  	.exit		= ti_pipe3_exit,

>>  	.power_on	= ti_pipe3_power_on,

>>  	.power_off	= ti_pipe3_power_off,

>> +	.set_mode	= ti_pipe3_set_mode,

>>  	.owner		= THIS_MODULE,

>>  };

>>  

>> @@ -589,12 +634,8 @@ static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)

>>  {

>>  	struct resource *res;

>>  	struct device *dev = phy->dev;

>> -	struct device_node *node = dev->of_node;

>>  	struct platform_device *pdev = to_platform_device(dev);

>>  

>> -	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))

>> -		return 0;

>> -

> 

> Do all USB3 and SATA PHYs have "phy_rx" and "phy_tx" regions?

> If not it will break them.


yes they have.

Thanks
Kishon

On 31/01/19 08:02, Kishon Vijay Abraham I wrote:
> Roger,

> 

> On 30/01/19 8:28 PM, Roger Quadros wrote:

>> Kishon,

>>

>> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>>> DRA72 platform has the second instance of PHY shared between USB3

>>> controller and PCIe controller with default as USB3 controller.

>>> Since it is used with USB3 controller by default, it uses the

>>> compatible specific to USB (ti,omap-usb3).

>>>

>>> Populate set_mode callback so that the USB3 PHY can be configured

>>> to be used with PCIe controller.

>>

>> How about rewording this to,

>>

>> "On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed

>> lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single

>> lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed.

>> Provide a way for the PHY user to choose the appropriate mode via the

>> .set_mode hook"

> 

> hmm okay

>>

>> More comments below.

>>

>>>

>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>>> ---

>>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>>

>>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>>> index 68ce4a082b9b..8c98f366416d 100644

>>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>>> @@ -56,6 +56,12 @@

>>>  

>>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>>  

>>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>>> +#define MEM_EN_PLLBYP			BIT(7)

>>> +

>>> +#define PHY_TX_TEST_CONFIG		0x2C

>>> +#define MEM_ENTESTCLK			BIT(31)

>>> +

>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>>  

>>> @@ -110,6 +116,8 @@

>>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>>  

>>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>>> +

>>>  struct pipe3_dpll_params {

>>>  	u16	m;

>>>  	u8	n;

>>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>>  	bool			sata_refclk_enabled;

>>> +	u32			mode;

>>>  };

>>>  

>>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>>  	rate = rate / 1000000;

>>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>> +	val = PIPE3_PHY_TX_RX_POWERON;

>>> +	if (phy->mode == PHY_MODE_PCIE)

>>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

>>

>> Is this required only for the USB3 PHY being used as PCIe lane or can it

>> be done for the PCIe PHY as well?

> 

> This setting was given only for USB3 PHY. I did check PCIe PHY (1-lane) to see

> if this is causing issues before posting the patch.

> 

> Thinking again, it might be safer to just use this setting for USB3 PHY.

>>

>> I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY

>> and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that

>> currently.

>>

>> Maybe this is safer?

>>

>> 	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))

> 

> yeah..

>>

>>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>>  

>>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>>  }

>>>  

>>> -static int ti_pipe3_init(struct phy *x)

>>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>>  {

>>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>> -	u32 val;

>>>  	int ret = 0;

>>> +	u32 val;

>>>  

>>> -	ti_pipe3_enable_clocks(phy);

>>>  	/*

>>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>>  			return ret;

>>>  

>>>  		ti_pipe3_calibrate(phy);

>>> -

>>> -		return 0;

>>> +	} else {

>>

>> How about

>>

>> 	else if (of_device_is_compatible(node, "ti,phy-usb3")) {

> 

> This check is implicit no?


For now yes. But phy_set_mode can be called for SATA PHY as well
and that might need different settings than USB3 PHY?

>> 		/* USB3 PHY being used as PCIe Lane */

>>

>>> +		val = ti_pipe3_readl(phy->phy_rx,

>>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>>> +		val |= MEM_EN_PLLBYP;

>>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>>> +				val);

>>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>>> +		val |= MEM_ENTESTCLK;

>>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>>  	}

>>>  

>>> +	return 0;

>>> +}

>>> +

>>> +static int ti_pipe3_init(struct phy *x)

>>> +{

>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>> +	u32 val;

>>> +	int ret = 0;

>>> +

>>> +	ti_pipe3_enable_clocks(phy);

>>> +

>>> +	if (phy->mode == PHY_MODE_PCIE)

>>

>> Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE.

>> Should we set the mode to PCI_MODE_PCIE explicitly it in probe?

> 

> The set_mode callback checks if the phy->mode is INVALID before setting the

> mode. The phy_set_mode in the PCIe consumer driver will return error if it is

> already set. The phy_set_mode will be called for both the lanes by the PCIe driver.


see below.

>>

>>> +		return ti_pipe3_pcie_init(phy);

>>> +

>>>  	/* Bring it out of IDLE if it is IDLE */

>>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>  	if (val & PLL_IDLE) {

>>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>>  		return 0;

>>>  

>>>  	/* PCIe doesn't have internal DPLL */

>>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>>

>> But this might be a USB3 PHY, which has DPLL.

> 

> hmm. I have to check this. We should also then reset PLL_IDLE bit in phy_init.

>>

>>>  		/* Put DPLL in IDLE mode */

>>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>  		val |= PLL_IDLE;

>>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>>  

>>>  	return 0;

>>>  }

>>> +

>>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>>> +{

>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>> +

>>> +	if (phy->mode != PHY_MODE_INVALID)

>>> +		return -EBUSY;


I think you are referring to this. Why is this necessary?
This will restrict the PHY to one mode per boot. We don't want to
impose that limitation.

>>> +

>>> +	phy->mode = mode;

>>

>> We need to do a sanity check here and prevent unsupported modes being set.

>>

>> e.g. we don't want PCIe mode being set on omap-usb3 PHY or SATA PHY.

>>

>> Which makes me wonder, if all PHYs with compatible "ti,phy-usb3" don't support

>> PCIe mode then we might need to add a new compatible for USB3 PHY's that do support

>> PCIe mode?

>>

>> Or maybe a DT-property of supported PHY modes?

> 

> hmm yeah, makes sense.

>>

>>> +

>>> +	return 0;

>>> +}

>>> +

>>>  static const struct phy_ops ops = {

>>>  	.init		= ti_pipe3_init,

>>>  	.exit		= ti_pipe3_exit,

>>>  	.power_on	= ti_pipe3_power_on,

>>>  	.power_off	= ti_pipe3_power_off,

>>> +	.set_mode	= ti_pipe3_set_mode,

>>>  	.owner		= THIS_MODULE,

>>>  };

>>>  

>>> @@ -589,12 +634,8 @@ static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)

>>>  {

>>>  	struct resource *res;

>>>  	struct device *dev = phy->dev;

>>> -	struct device_node *node = dev->of_node;

>>>  	struct platform_device *pdev = to_platform_device(dev);

>>>  

>>> -	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))

>>> -		return 0;

>>> -

>>

>> Do all USB3 and SATA PHYs have "phy_rx" and "phy_tx" regions?

>> If not it will break them.

> 

> yes they have.

> 


cheers,
-roger
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Hi,

On 31/01/19 4:11 PM, Roger Quadros wrote:
> 

> 

> On 31/01/19 08:02, Kishon Vijay Abraham I wrote:

>> Roger,

>>

>> On 30/01/19 8:28 PM, Roger Quadros wrote:

>>> Kishon,

>>>

>>> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>>>> DRA72 platform has the second instance of PHY shared between USB3

>>>> controller and PCIe controller with default as USB3 controller.

>>>> Since it is used with USB3 controller by default, it uses the

>>>> compatible specific to USB (ti,omap-usb3).

>>>>

>>>> Populate set_mode callback so that the USB3 PHY can be configured

>>>> to be used with PCIe controller.

>>>

>>> How about rewording this to,

>>>

>>> "On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed

>>> lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single

>>> lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed.

>>> Provide a way for the PHY user to choose the appropriate mode via the

>>> .set_mode hook"

>>

>> hmm okay

>>>

>>> More comments below.

>>>

>>>>

>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>>>> ---

>>>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>>>

>>>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>>>> index 68ce4a082b9b..8c98f366416d 100644

>>>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>>>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>>>> @@ -56,6 +56,12 @@

>>>>  

>>>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>>>  

>>>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>>>> +#define MEM_EN_PLLBYP			BIT(7)

>>>> +

>>>> +#define PHY_TX_TEST_CONFIG		0x2C

>>>> +#define MEM_ENTESTCLK			BIT(31)

>>>> +

>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>>>  

>>>> @@ -110,6 +116,8 @@

>>>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>>>  

>>>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>>>> +

>>>>  struct pipe3_dpll_params {

>>>>  	u16	m;

>>>>  	u8	n;

>>>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>>>  	bool			sata_refclk_enabled;

>>>> +	u32			mode;

>>>>  };

>>>>  

>>>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>>>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>>>  	rate = rate / 1000000;

>>>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>>>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>> +	val = PIPE3_PHY_TX_RX_POWERON;

>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

>>>

>>> Is this required only for the USB3 PHY being used as PCIe lane or can it

>>> be done for the PCIe PHY as well?

>>

>> This setting was given only for USB3 PHY. I did check PCIe PHY (1-lane) to see

>> if this is causing issues before posting the patch.

>>

>> Thinking again, it might be safer to just use this setting for USB3 PHY.

>>>

>>> I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY

>>> and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that

>>> currently.

>>>

>>> Maybe this is safer?

>>>

>>> 	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))

>>

>> yeah..

>>>

>>>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>>>  

>>>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>>>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>>>  }

>>>>  

>>>> -static int ti_pipe3_init(struct phy *x)

>>>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>>>  {

>>>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>> -	u32 val;

>>>>  	int ret = 0;

>>>> +	u32 val;

>>>>  

>>>> -	ti_pipe3_enable_clocks(phy);

>>>>  	/*

>>>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>>>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>>>  			return ret;

>>>>  

>>>>  		ti_pipe3_calibrate(phy);

>>>> -

>>>> -		return 0;

>>>> +	} else {

>>>

>>> How about

>>>

>>> 	else if (of_device_is_compatible(node, "ti,phy-usb3")) {

>>

>> This check is implicit no?

> 

> For now yes. But phy_set_mode can be called for SATA PHY as well

> and that might need different settings than USB3 PHY?


This function will be invoked only for PCIe.
> 

>>> 		/* USB3 PHY being used as PCIe Lane */

>>>

>>>> +		val = ti_pipe3_readl(phy->phy_rx,

>>>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>>>> +		val |= MEM_EN_PLLBYP;

>>>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>>>> +				val);

>>>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>>>> +		val |= MEM_ENTESTCLK;

>>>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>>>  	}

>>>>  

>>>> +	return 0;

>>>> +}

>>>> +

>>>> +static int ti_pipe3_init(struct phy *x)

>>>> +{

>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>> +	u32 val;

>>>> +	int ret = 0;

>>>> +

>>>> +	ti_pipe3_enable_clocks(phy);

>>>> +

>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>

>>> Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE.

>>> Should we set the mode to PCI_MODE_PCIE explicitly it in probe?

>>

>> The set_mode callback checks if the phy->mode is INVALID before setting the

>> mode. The phy_set_mode in the PCIe consumer driver will return error if it is

>> already set. The phy_set_mode will be called for both the lanes by the PCIe driver.

> 

> see below.

> 

>>>

>>>> +		return ti_pipe3_pcie_init(phy);

>>>> +

>>>>  	/* Bring it out of IDLE if it is IDLE */

>>>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>  	if (val & PLL_IDLE) {

>>>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>>>  		return 0;

>>>>  

>>>>  	/* PCIe doesn't have internal DPLL */

>>>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>>>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>>>

>>> But this might be a USB3 PHY, which has DPLL.

>>

>> hmm. I have to check this. We should also then reset PLL_IDLE bit in phy_init.

>>>

>>>>  		/* Put DPLL in IDLE mode */

>>>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>  		val |= PLL_IDLE;

>>>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>>>  

>>>>  	return 0;

>>>>  }

>>>> +

>>>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>>>> +{

>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>> +

>>>> +	if (phy->mode != PHY_MODE_INVALID)

>>>> +		return -EBUSY;

> 

> I think you are referring to this. Why is this necessary?

> This will restrict the PHY to one mode per boot. We don't want to

> impose that limitation.


Isn't that true? Our PHYs can work only in one mode right?

Maybe we can add a check like below but there too we can't set the PHY mode of
USB PHY to USB in probe, for the PHY that is shared with PCIe.

if (phy->mode != PHY_MODE_INVALID) && (phy->mode != mode)
	return -EBUSY.

Thanks
Kishon

Kishon,

On 31/01/19 13:04, Kishon Vijay Abraham I wrote:
> Hi,

> 

> On 31/01/19 4:11 PM, Roger Quadros wrote:

>>

>>

>> On 31/01/19 08:02, Kishon Vijay Abraham I wrote:

>>> Roger,

>>>

>>> On 30/01/19 8:28 PM, Roger Quadros wrote:

>>>> Kishon,

>>>>

>>>> On 24/01/19 12:48, Kishon Vijay Abraham I wrote:

>>>>> DRA72 platform has the second instance of PHY shared between USB3

>>>>> controller and PCIe controller with default as USB3 controller.

>>>>> Since it is used with USB3 controller by default, it uses the

>>>>> compatible specific to USB (ti,omap-usb3).

>>>>>

>>>>> Populate set_mode callback so that the USB3 PHY can be configured

>>>>> to be used with PCIe controller.

>>>>

>>>> How about rewording this to,

>>>>

>>>> "On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed

>>>> lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single

>>>> lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed.

>>>> Provide a way for the PHY user to choose the appropriate mode via the

>>>> .set_mode hook"

>>>

>>> hmm okay

>>>>

>>>> More comments below.

>>>>

>>>>>

>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>>>>> ---

>>>>>  drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------

>>>>>  1 file changed, 54 insertions(+), 12 deletions(-)

>>>>>

>>>>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c

>>>>> index 68ce4a082b9b..8c98f366416d 100644

>>>>> --- a/drivers/phy/ti/phy-ti-pipe3.c

>>>>> +++ b/drivers/phy/ti/phy-ti-pipe3.c

>>>>> @@ -56,6 +56,12 @@

>>>>>  

>>>>>  #define SATA_PLL_SOFT_RESET	BIT(18)

>>>>>  

>>>>> +#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC

>>>>> +#define MEM_EN_PLLBYP			BIT(7)

>>>>> +

>>>>> +#define PHY_TX_TEST_CONFIG		0x2C

>>>>> +#define MEM_ENTESTCLK			BIT(31)

>>>>> +

>>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000

>>>>>  #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14

>>>>>  

>>>>> @@ -110,6 +116,8 @@

>>>>>  #define PLL_IDLE_TIME	100	/* in milliseconds */

>>>>>  #define PLL_LOCK_TIME	100	/* in milliseconds */

>>>>>  

>>>>> +#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)

>>>>> +

>>>>>  struct pipe3_dpll_params {

>>>>>  	u16	m;

>>>>>  	u8	n;

>>>>> @@ -141,6 +149,7 @@ struct ti_pipe3 {

>>>>>  	unsigned int		power_reg; /* power reg. index within syscon */

>>>>>  	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */

>>>>>  	bool			sata_refclk_enabled;

>>>>> +	u32			mode;

>>>>>  };

>>>>>  

>>>>>  static struct pipe3_dpll_map dpll_map_usb[] = {

>>>>> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)

>>>>>  	rate = rate / 1000000;

>>>>>  	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |

>>>>>  		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;

>>>>> -	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>>> +	val = PIPE3_PHY_TX_RX_POWERON;

>>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>>> +		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

>>>>

>>>> Is this required only for the USB3 PHY being used as PCIe lane or can it

>>>> be done for the PCIe PHY as well?

>>>

>>> This setting was given only for USB3 PHY. I did check PCIe PHY (1-lane) to see

>>> if this is causing issues before posting the patch.

>>>

>>> Thinking again, it might be safer to just use this setting for USB3 PHY.

>>>>

>>>> I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY

>>>> and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that

>>>> currently.

>>>>

>>>> Maybe this is safer?

>>>>

>>>> 	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))

>>>

>>> yeah..

>>>>

>>>>> +	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;

>>>>>  	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;

>>>>>  

>>>>>  	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

>>>>> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)

>>>>>  	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);

>>>>>  }

>>>>>  

>>>>> -static int ti_pipe3_init(struct phy *x)

>>>>> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)

>>>>>  {

>>>>> -	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>> -	u32 val;

>>>>>  	int ret = 0;

>>>>> +	u32 val;

>>>>>  

>>>>> -	ti_pipe3_enable_clocks(phy);

>>>>>  	/*

>>>>>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

>>>>>  	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

>>>>> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)

>>>>>  			return ret;

>>>>>  

>>>>>  		ti_pipe3_calibrate(phy);

>>>>> -

>>>>> -		return 0;

>>>>> +	} else {

>>>>

>>>> How about

>>>>

>>>> 	else if (of_device_is_compatible(node, "ti,phy-usb3")) {

>>>

>>> This check is implicit no?

>>

>> For now yes. But phy_set_mode can be called for SATA PHY as well

>> and that might need different settings than USB3 PHY?

> 

> This function will be invoked only for PCIe.


>>

>>>> 		/* USB3 PHY being used as PCIe Lane */

>>>>

>>>>> +		val = ti_pipe3_readl(phy->phy_rx,

>>>>> +				     PHY_RX_ANA_PRGRAMMABILITY_REG);

>>>>> +		val |= MEM_EN_PLLBYP;

>>>>> +		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,

>>>>> +				val);

>>>>> +		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);

>>>>> +		val |= MEM_ENTESTCLK;

>>>>> +		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);

>>>>>  	}

>>>>>  

>>>>> +	return 0;

>>>>> +}

>>>>> +

>>>>> +static int ti_pipe3_init(struct phy *x)

>>>>> +{

>>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>> +	u32 val;

>>>>> +	int ret = 0;

>>>>> +

>>>>> +	ti_pipe3_enable_clocks(phy);

>>>>> +

>>>>> +	if (phy->mode == PHY_MODE_PCIE)

>>>>

>>>> Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE.

>>>> Should we set the mode to PCI_MODE_PCIE explicitly it in probe?

>>>

>>> The set_mode callback checks if the phy->mode is INVALID before setting the

>>> mode. The phy_set_mode in the PCIe consumer driver will return error if it is

>>> already set. The phy_set_mode will be called for both the lanes by the PCIe driver.

>>

>> see below.

>>

>>>>

>>>>> +		return ti_pipe3_pcie_init(phy);

>>>>> +

>>>>>  	/* Bring it out of IDLE if it is IDLE */

>>>>>  	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>>  	if (val & PLL_IDLE) {

>>>>> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)

>>>>>  		return 0;

>>>>>  

>>>>>  	/* PCIe doesn't have internal DPLL */

>>>>> -	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {

>>>>> +	if (!(phy->mode == PHY_MODE_PCIE)) {

>>>>

>>>> But this might be a USB3 PHY, which has DPLL.

>>>

>>> hmm. I have to check this. We should also then reset PLL_IDLE bit in phy_init.

>>>>

>>>>>  		/* Put DPLL in IDLE mode */

>>>>>  		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);

>>>>>  		val |= PLL_IDLE;

>>>>> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)

>>>>>  

>>>>>  	return 0;

>>>>>  }

>>>>> +

>>>>> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)

>>>>> +{

>>>>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);

>>>>> +

>>>>> +	if (phy->mode != PHY_MODE_INVALID)

>>>>> +		return -EBUSY;

>>

>> I think you are referring to this. Why is this necessary?

>> This will restrict the PHY to one mode per boot. We don't want to

>> impose that limitation.

> 

> Isn't that true? Our PHYs can work only in one mode right?


OK.

> 

> Maybe we can add a check like below but there too we can't set the PHY mode of

> USB PHY to USB in probe, for the PHY that is shared with PCIe.

> 

> if (phy->mode != PHY_MODE_INVALID) && (phy->mode != mode)

> 	return -EBUSY.

> 


USB controller [1] calls phy_set_mode() at every role switch which can happen
multiple times after boot. Although it isn't currently checking the return value
the set_mode behaviour is not consistent for USB.

The first call will return 0 and all future calls will return -EBUSY.

At least for USB, we should return (0?) always.

[1] - drivers/usb/dwc3/drd.c
                                phy_set_mode(dwc->usb2_generic_phy,
                                             PHY_MODE_USB_HOST);
				...
                                phy_set_mode(dwc->usb2_generic_phy,
                                             PHY_MODE_USB_DEVICE);


cheers,
-roger

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki