| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5C51B5E6.3010603@ti.com>
Date: Wed, 30 Jan 2019 16:34:14 +0200
From: Roger Quadros <rogerq@...com>
To: Kishon Vijay Abraham I <kishon@...com>
CC: <linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH] phy: ti-pipe3: Add set_mode callback to configure usb3
phy as pcie phy
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@...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
Powered by blists - more mailing lists