Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 07:10 PM, Alan Stern wrote:
> On Tue, 20 Feb 2018, Roger Quadros wrote:
> 
>> On 20/02/18 16:46, Amelie DELAUNAY wrote:
>>> Hi,
>>>
>>> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>>>> On some boards, especially when vbus supply requires large current,
>>>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>>>> may be used.
>>>>> Add support for this optional external vbus supply in ehci-platform.
>>>>>
>>>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@xxxxxx>
>>>>>
>>>>> ---
>>>>> Changes in v2:
>>>>>    * Address Roger Quadros comments: move regulator_enable/disable from
>>>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>>>> ---
>>>>>    Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>>>    drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>>>    2 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> index 3efde12..fc480cd 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>     - phys : phandle + phy specifier pair
>>>>>     - phy-names : "usb"
>>>>>     - resets : phandle + reset specifier pair
>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>    
>>>>
>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>
>>>
>>> I imagine that yes, platforms could have one regulator per port.
>>> Regulator consumers bindings impose a <name>-supply property per
>>> regulator, so, what do you think about :
>>> vbus0-supply for port#0
>>> vbus1-supply for port#1
>>> ...
>>> vbusN-supply for port#N
>>>
>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>> regulator *)'.
>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>> And then enable/disable the corresponding regulator in
>>> ehci_platform_port_power thanks to portnum.
>>
>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>> If there isn't a single platform needing it we could probably do without it
>> but the DT binding must be scalable to add this feature in the future.
> 
> I agree that for now there don't seem to be any platforms requiring
> more than one regulator, but this should be implemented in a way that
> could be expanded if necessary.
> 
> Anyway, the basic idea is reasonable.  I don't know to what extent
> people want to power-off their EHCI ports, but if they do then we ought
> to turn off external regulators at the same time.
> 
> Is there a real-life use case for this?
> 
> Alan Stern
> 

On my setup I have the following:

                  regulator_____vbus
  _________________             \
| EHCI controller |-port0-----[USB connector]
|_________________|-port1-----X

So, I have one regulator only for port0. But I could I have one more if 
port1 was connected. My current regulator could also supplies port1.

To allocate a vbus_supplies array depending on N_PORTS, I have to move 
this initialization from probe to ehci_platform_reset, after ehci_setup 
is done.
Then, I have to define each regulator id depending on the port number.
This imposes a binding like
- portN_vbus-supply : phandle of regulator supplying vbus for port N
But I don't know if we can describe it like this in dt-bindings ?

&ehci {
	...
	port0_vbus-supply = <&vbus_reg>;
	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
specified if port is not connected.
	...
};

Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Regards,
Amelie

>> And what if it is ganged power? i.e. one regulator for more than one port.
>> Probably they all can point to the same regulator instance and it should work.
>>
>>>
>>>>>    Example (Sequoia 440EPx):
>>>>>        ehci@e0000300 {
>>>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>>>> index b065a96..05be100 100644
>>>>> --- a/drivers/usb/host/ehci-platform.c
>>>>> +++ b/drivers/usb/host/ehci-platform.c
>>>>> @@ -29,6 +29,7 @@
>>>>>    #include <linux/of.h>
>>>>>    #include <linux/phy/phy.h>
>>>>>    #include <linux/platform_device.h>
>>>>> +#include <linux/regulator/consumer.h>
>>>>>    #include <linux/reset.h>
>>>>>    #include <linux/usb.h>
>>>>>    #include <linux/usb/hcd.h>
>>>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>>>    	struct reset_control *rsts;
>>>>>    	struct phy **phys;
>>>>>    	int num_phys;
>>>>> +	struct regulator *vbus_supply;
>>>>>    	bool reset_on_resume;
>>>>>    };
>>>>>    
>>>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>>>    	return 0;
>>>>>    }
>>>>>    
>>>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>>>> +				    bool enable)
>>>>> +{
>>>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	if (priv->vbus_supply) {
>>>>> +		if (enable)
>>>>> +			ret = regulator_enable(priv->vbus_supply);
>>>>> +		else
>>>>> +			ret = regulator_disable(priv->vbus_supply);
>>>>> +		if (ret)
>>>>> +			dev_err(hcd->self.controller,
>>>>> +				"failed to %s vbus supply: %d\n",
>>>>> +				enable ? "enable" : "disable", ret);
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>    static int ehci_platform_power_on(struct platform_device *dev)
>>>>>    {
>>>>>    	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>>>    static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>>>    	.reset =		ehci_platform_reset,
>>>>>    	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>>>> +	.port_power =		ehci_platform_port_power,
>>>>>    };
>>>>>    
>>>>>    static struct usb_ehci_pdata ehci_platform_defaults = {
>>>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>>>    	if (err)
>>>>>    		goto err_put_clks;
>>>>>    
>>>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>>>> +	if (IS_ERR(priv->vbus_supply)) {
>>>>> +		err = PTR_ERR(priv->vbus_supply);
>>>>> +		if (err == -ENODEV)
>>>>> +			priv->vbus_supply = NULL;
>>>>> +		else
>>>>> +			goto err_reset;
>>>>> +	}
>>>>> +
>>>>>    	if (pdata->big_endian_desc)
>>>>>    		ehci->big_endian_desc = 1;
>>>>>    	if (pdata->big_endian_mmio)
>>>>>
>>
>>
> 
> 
>