Re: [PATCH] phy: ti-pipe3: Add set_mode callback to configure usb3 phy as pcie phy

[Kishon Vijay Abraham I]

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@xxxxxx>
>>>>>> ---
>>>>>>  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
>>>>
>>>
>