| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ec064186-9b0b-eeb4-5501-6bbc1e90da79@st.com>
Date: Thu, 22 Feb 2018 10:17:52 +0000
From: Amelie DELAUNAY <amelie.delaunay@...com>
To: Alan Stern <stern@...land.harvard.edu>,
Roger Quadros <rogerq@...com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Tony Prisk <linux@...sktech.co.nz>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2] usb: host: ehci-platform: add support for optional
external vbus supply
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@...com>
>>>>>
>>>>> ---
>>>>> 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@...00300 {
>>>>> 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)
>>>>>
>>
>>
>
>
>
Powered by blists - more mailing lists