| Message ID | 20190124104822.22411-1-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | phy: ti-pipe3: Add set_mode callback to configure usb3 phy as pcie phy | expand |
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
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 >
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 >> >
+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? 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 >>> >>
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 >>>>> >>>> >>
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
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
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
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; + + 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)
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(-)