Re: [PATCH] ARM: DRA7-evm: Enable SATA PHY and USB PHY power supplies

[Tero Kristo <t-kristo@ti.com>]

On 06/26/2014 05:22 PM, Roger Quadros wrote:
> +Tero
>
> On 06/26/2014 12:36 PM, Roger Quadros wrote:
>> On 06/26/2014 10:31 AM, Tony Lindgren wrote:
>>> * Nishanth Menon <nm@ti.com> [140625 15:29]:
>>>> On 06/25/2014 07:56 AM, Roger Quadros wrote:
>>>>> The SATA and USB PHYs need the 1.8V and 3.3V supplies.
>>>>> The PHY drivers/framework don't yet support regulator
>>>>> supply so we have to keep these regulators always-on till
>>>>> then.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> ---
>>>>>   arch/arm/boot/dts/dra7-evm.dts | 2 ++
>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts
>>>>> index 4adc280..99a1f79 100644
>>>>> --- a/arch/arm/boot/dts/dra7-evm.dts
>>>>> +++ b/arch/arm/boot/dts/dra7-evm.dts
>>>>> @@ -241,6 +241,7 @@
>>>>>   					regulator-min-microvolt = <1800000>;
>>>>>   					regulator-max-microvolt = <1800000>;
>>>>>   					regulator-boot-on;
>>>>> +					regulator-always-on;
>>>>>   				};
>>>>>
>>>>>   				ldo9_reg: ldo9 {
>>>>> @@ -266,6 +267,7 @@
>>>>>   					regulator-min-microvolt = <3300000>;
>>>>>   					regulator-max-microvolt = <3300000>;
>>>>>   					regulator-boot-on;
>>>>> +					regulator-always-on;
>>>>>   				};
>>>>>   			};
>>>>>   		};
>>>>>
>>>>
>>>> Why not fix phy driver/framework as needed? the trouble is people
>>>> always forget to remove always-on... who actually audits old logs and
>>>> fixes stuff back up?
>>>
>>> Yes I agree let's not play with the regulator-always-on unless
>>> absolutely necessary.
>>>
>> Agreed. PHY framework must deal with this. Till then USB, SATA and most likely PCIe as well will not work on DRA7-evm.
>>
>
> I tried the below patch to enable the relevant regulators in the PHY drivers but still couldn't manage to get USB to work. The same 1.8V regulator is used to supply 3 pins
> -vdda_usb1: DPLL_USB and HS_USB1 analog supply
> -vdda_usb2: HS_USB2 analog supply
> -vdda_usb3: DPLL_USB_OTG_SS and USB3.0 Rx/Tx analog supply
>
> It seems that the regulator auto disable kicks in before the phy driver enables the regulator thus causing some kind of malfunction. I'm not sure whether this is due to DPLL_USB supply glitch or HS_USB1 analog supply glitch.
>
> In any case, the DPLL_USB (clock driver?) needs to enable the 1.8V regulator in addition to the HS_USB PHY driver to prevent this supply glitch.
>
> Tero, any suggestions about this? If we are not concerned about disabling DPLL_USB anytime then the regulator might just as well be always-on. Alternatively can we place a regulator_get(), regulator_enable() in drivers/clk/ti/dpll.c?

I believe dpll_usb needs to go down for the core to idle. Also, we are 
working on getting suspend-resume working on this platform, so having 
the regulator always on is a no-no.

Also, I am heavily against adding regulator tweaks within the clock 
driver, there needs to be another way.

-Tero

>
> cheers,
> -roger
> ---
>
> diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts
> index 4adc280..23379ab 100644
> --- a/arch/arm/boot/dts/dra7-evm.dts
> +++ b/arch/arm/boot/dts/dra7-evm.dts
> @@ -495,3 +495,11 @@
>   		};
>   	};
>   };
> +
> +&usb2_phy1 {
> +	vdda3v3-supply = <&ldousb_reg>;
> +};
> +
> +&usb3_phy1 {
> +	vdda1v8-supply = <&ldo3_reg>;
> +};
> diff --git a/drivers/phy/phy-omap-usb2.c b/drivers/phy/phy-omap-usb2.c
> index 7007c11..bb1a768 100644
> --- a/drivers/phy/phy-omap-usb2.c
> +++ b/drivers/phy/phy-omap-usb2.c
> @@ -30,6 +30,7 @@
>   #include <linux/phy/omap_control_phy.h>
>   #include <linux/phy/phy.h>
>   #include <linux/of_platform.h>
> +#include <linux/regulator/consumer.h>
>
>   #define USB2PHY_DISCON_BYP_LATCH (1 << 31)
>   #define USB2PHY_ANA_CONFIG1 0x4c
> @@ -107,6 +108,18 @@ static int omap_usb_power_off(struct phy *x)
>
>   	omap_control_phy_power(phy->control_dev, 0);
>
> +	if (phy->pwr) {
> +		int ret;
> +
> +		ret = regulator_disable(phy->pwr);
> +		if (ret) {
> +			dev_err(phy->dev, "failed to disable regulator\n");
> +			return ret;
> +		} else {
> +			dev_info(phy->dev, "disabled regulator\n");
> +		}
> +	}
> +
>   	return 0;
>   }
>
> @@ -114,6 +127,18 @@ static int omap_usb_power_on(struct phy *x)
>   {
>   	struct omap_usb *phy = phy_get_drvdata(x);
>
> +	if (phy->pwr) {
> +		int ret;
> +
> +		ret = regulator_enable(phy->pwr);
> +		if (ret) {
> +			dev_err(phy->dev, "failed to enable regulator\n");
> +			return ret;
> +		} else {
> +			dev_info(phy->dev, "enabled regulator\n");
> +		}
> +	}
> +
>   	omap_control_phy_power(phy->control_dev, 1);
>
>   	return 0;
> @@ -253,6 +278,14 @@ static int omap_usb2_probe(struct platform_device *pdev)
>   	phy->control_dev = &control_pdev->dev;
>   	omap_control_phy_power(phy->control_dev, 0);
>
> +	/* phy-supply */
> +	phy->pwr = devm_regulator_get_optional(phy->dev, "vdda3v3");
> +	if (IS_ERR(phy->pwr)) {
> +		if (PTR_ERR(phy->pwr) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		phy->pwr = NULL;
> +	}
> +
>   	otg->set_host		= omap_usb_set_host;
>   	otg->set_peripheral	= omap_usb_set_peripheral;
>   	if (phy_data->flags & OMAP_USB2_HAS_SET_VBUS)
> diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
> index 5913676..1b46b0b 100644
> --- a/drivers/phy/phy-ti-pipe3.c
> +++ b/drivers/phy/phy-ti-pipe3.c
> @@ -28,6 +28,7 @@
>   #include <linux/delay.h>
>   #include <linux/phy/omap_control_phy.h>
>   #include <linux/of_platform.h>
> +#include <linux/regulator/consumer.h>
>
>   #define	PLL_STATUS		0x00000004
>   #define	PLL_GO			0x00000008
> @@ -81,6 +82,7 @@ struct ti_pipe3 {
>   	struct clk		*sys_clk;
>   	struct clk		*refclk;
>   	struct pipe3_dpll_map	*dpll_map;
> +	struct regulator	*pwr_dpll;
>   };
>
>   static struct pipe3_dpll_map dpll_map_usb[] = {
> @@ -215,6 +217,17 @@ static int ti_pipe3_init(struct phy *x)
>   	u32 val;
>   	int ret = 0;
>
> +	/* Enable DPLL regulator */
> +	if (phy->pwr_dpll) {
> +		ret = regulator_enable(phy->pwr_dpll);
> +		if (ret) {
> +			dev_err(phy->dev, "failed to enable regulator\n");
> +			return ret;
> +		} else {
> +			dev_info(phy->dev, "enabled regulator\n");
> +		}
> +	}
> +
>   	/* Bring it out of IDLE if it is IDLE */
>   	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>   	if (val & PLL_IDLE) {
> @@ -262,6 +275,18 @@ static int ti_pipe3_exit(struct phy *x)
>   		return -EBUSY;
>   	}
>
> +	/* Disable DPLL regulator */
> +	if (phy->pwr_dpll) {
> +		int ret;
> +
> +		ret = regulator_disable(phy->pwr_dpll);
> +		if (ret) {
> +			dev_err(phy->dev, "failed to disable regulator\n");
> +			return ret;
> +		} else {
> +			dev_info(phy->dev, "disabled regulator\n");
> +		}
> +	}
>   	return 0;
>   }
>   static struct phy_ops ops = {
> @@ -308,7 +333,15 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>   	if (IS_ERR(phy->pll_ctrl_base))
>   		return PTR_ERR(phy->pll_ctrl_base);
>
> -	phy->dev		= &pdev->dev;
> +	phy->dev = &pdev->dev;
> +
> +	/* dpll-supply */
> +	phy->pwr_dpll = devm_regulator_get_optional(phy->dev, "vdda1v8");
> +	if (IS_ERR(phy->pwr_dpll)) {
> +		if (PTR_ERR(phy->pwr_dpll) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		phy->pwr_dpll = NULL;
> +	}
>
>   	if (!of_device_is_compatible(node, "ti,phy-pipe3-sata")) {
>
> diff --git a/include/linux/phy/omap_usb.h b/include/linux/phy/omap_usb.h
> index dc2c541..e2c46df 100644
> --- a/include/linux/phy/omap_usb.h
> +++ b/include/linux/phy/omap_usb.h
> @@ -40,6 +40,7 @@ struct omap_usb {
>   	struct clk		*wkupclk;
>   	struct clk		*optclk;
>   	u8			flags;
> +	struct regulator	*pwr;
>   };
>
>   struct usb_phy_data {
>