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