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@xxxxxx>
> ---
> drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
> 1 file changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index 68ce4a082b9b..8c98f366416d 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -56,6 +56,12 @@
>
> #define SATA_PLL_SOFT_RESET BIT(18)
>
> +#define PHY_RX_ANA_PRGRAMMABILITY_REG 0xC
> +#define MEM_EN_PLLBYP BIT(7)
> +
> +#define PHY_TX_TEST_CONFIG 0x2C
> +#define MEM_ENTESTCLK BIT(31)
> +
> #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK 0x003FC000
> #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT 14
>
> @@ -110,6 +116,8 @@
> #define PLL_IDLE_TIME 100 /* in milliseconds */
> #define PLL_LOCK_TIME 100 /* in milliseconds */
>
> +#define PIPE3_PHY_DISABLE_SYNC_POWER BIT(4)
> +
> struct pipe3_dpll_params {
> u16 m;
> u8 n;
> @@ -141,6 +149,7 @@ struct ti_pipe3 {
> unsigned int power_reg; /* power reg. index within syscon */
> unsigned int pcie_pcs_reg; /* pcs reg. index in syscon */
> bool sata_refclk_enabled;
> + u32 mode;
> };
>
> static struct pipe3_dpll_map dpll_map_usb[] = {
> @@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
> rate = rate / 1000000;
> mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
> OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
> - val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
> + val = PIPE3_PHY_TX_RX_POWERON;
> + if (phy->mode == PHY_MODE_PCIE)
> + val |= PIPE3_PHY_DISABLE_SYNC_POWER;
Is this required only for the USB3 PHY being used as PCIe lane or can it
be done for the PCIe PHY as well?
I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY
and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that
currently.
Maybe this is safer?
if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))
> + val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
> val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
>
> ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,
> @@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
> ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
> }
>
> -static int ti_pipe3_init(struct phy *x)
> +static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
> {
> - struct ti_pipe3 *phy = phy_get_drvdata(x);
> - u32 val;
> int ret = 0;
> + u32 val;
>
> - ti_pipe3_enable_clocks(phy);
> /*
> * Set pcie_pcs register to 0x96 for proper functioning of phy
> * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table
> @@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
> return ret;
>
> ti_pipe3_calibrate(phy);
> -
> - return 0;
> + } else {
How about
else if (of_device_is_compatible(node, "ti,phy-usb3")) {
/* USB3 PHY being used as PCIe Lane */
> + val = ti_pipe3_readl(phy->phy_rx,
> + PHY_RX_ANA_PRGRAMMABILITY_REG);
> + val |= MEM_EN_PLLBYP;
> + ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
> + val);
> + val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
> + val |= MEM_ENTESTCLK;
> + ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
> }
>
> + return 0;
> +}
> +
> +static int ti_pipe3_init(struct phy *x)
> +{
> + struct ti_pipe3 *phy = phy_get_drvdata(x);
> + u32 val;
> + int ret = 0;
> +
> + ti_pipe3_enable_clocks(phy);
> +
> + if (phy->mode == PHY_MODE_PCIE)
Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE.
Should we set the mode to PCI_MODE_PCIE explicitly it in probe?
> + return ti_pipe3_pcie_init(phy);
> +
> /* Bring it out of IDLE if it is IDLE */
> val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
> if (val & PLL_IDLE) {
> @@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
> return 0;
>
> /* PCIe doesn't have internal DPLL */
> - if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
> + if (!(phy->mode == PHY_MODE_PCIE)) {
But this might be a USB3 PHY, which has DPLL.
> /* Put DPLL in IDLE mode */
> val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
> val |= PLL_IDLE;
> @@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
>
> return 0;
> }
> +
> +static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
> +{
> + struct ti_pipe3 *phy = phy_get_drvdata(x);
> +
> + if (phy->mode != PHY_MODE_INVALID)
> + return -EBUSY;
> +
> + phy->mode = mode;
We need to do a sanity check here and prevent unsupported modes being set.
e.g. we don't want PCIe mode being set on omap-usb3 PHY or SATA PHY.
Which makes me wonder, if all PHYs with compatible "ti,phy-usb3" don't support
PCIe mode then we might need to add a new compatible for USB3 PHY's that do support
PCIe mode?
Or maybe a DT-property of supported PHY modes?
> +
> + return 0;
> +}
> +
> static const struct phy_ops ops = {
> .init = ti_pipe3_init,
> .exit = ti_pipe3_exit,
> .power_on = ti_pipe3_power_on,
> .power_off = ti_pipe3_power_off,
> + .set_mode = ti_pipe3_set_mode,
> .owner = THIS_MODULE,
> };
>
> @@ -589,12 +634,8 @@ static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)
> {
> struct resource *res;
> struct device *dev = phy->dev;
> - struct device_node *node = dev->of_node;
> struct platform_device *pdev = to_platform_device(dev);
>
> - if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))
> - return 0;
> -
Do all USB3 and SATA PHYs have "phy_rx" and "phy_tx" regions?
If not it will break them.
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "phy_rx");
> phy->phy_rx = devm_ioremap_resource(dev, res);
> @@ -649,6 +690,7 @@ static int ti_pipe3_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> phy->dev = dev;
> + phy->mode = PHY_MODE_INVALID;
>
Let's do
if (of_device_is_compatible(node, "ti,phy-pipe3-pcie"))
phy->mode = PCI_MODE_PCIE;
> ret = ti_pipe3_get_pll_base(phy);
> if (ret)
>
cheers,
-roger
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
