Re: [PATCH v3 1/2] ehci-platform: Add support for controllers with multiple reset lines

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi,

On 24-02-16 12:37, Roger Quadros wrote:
> Hi,
>
> On 24/02/16 13:15, Hans de Goede wrote:
>> From: Reinder de Haan <patchesrdh-I1/eAgTnXDYAvxtiuMwx3w@public.gmane.org>
>>
>> At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple
>> reset lines, the controller will not initialize while the reset for
>> its companion is still asserted, which means we need to de-assert
>> 2 resets for the controller to work.
>>
>> Signed-off-by: Reinder de Haan <patchesrdh-I1/eAgTnXDYAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes in v2:
>> -Use the new reset_control_[de]assert_shared reset-controller functions
>> Changes in v3:
>> -Adjust for changes to shared-reset reset-controller functions
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  2 +-
>>   drivers/usb/host/ehci-platform.c                   | 41 ++++++++++++----------
>>   2 files changed, 24 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index a12d601..0701812 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -18,7 +18,7 @@ Optional properties:
>>    - clocks : a list of phandle + clock specifier pairs
>>    - phys : phandle + phy specifier pair
>>    - phy-names : "usb"
>> - - resets : phandle + reset specifier pair
>> + - resets : a list of phandle + reset specifier pairs
>>
>>   Example (Sequoia 440EPx):
>>       ehci@e0000300 {
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index bd7082f2..3f195ba 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -39,11 +39,12 @@
>>
>>   #define DRIVER_DESC "EHCI generic platform driver"
>>   #define EHCI_MAX_CLKS 3
>> +#define EHCI_MAX_RESETS 2
>>   #define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv)
>>
>>   struct ehci_platform_priv {
>>   	struct clk *clks[EHCI_MAX_CLKS];
>> -	struct reset_control *rst;
>> +	struct reset_control *resets[EHCI_MAX_RESETS];
>>   	struct phy **phys;
>>   	int num_phys;
>>   	bool reset_on_resume;
>> @@ -149,7 +150,7 @@ static int ehci_platform_probe(struct platform_device *dev)
>>   	struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
>>   	struct ehci_platform_priv *priv;
>>   	struct ehci_hcd *ehci;
>> -	int err, irq, phy_num, clk = 0;
>> +	int err, irq, phy_num, clk = 0, rst = 0;
>>
>>   	if (usb_disabled())
>>   		return -ENODEV;
>> @@ -232,18 +233,22 @@ static int ehci_platform_probe(struct platform_device *dev)
>>   				break;
>>   			}
>>   		}
>> -	}
>>
>> -	priv->rst = devm_reset_control_get_optional(&dev->dev, NULL);
>> -	if (IS_ERR(priv->rst)) {
>> -		err = PTR_ERR(priv->rst);
>> -		if (err == -EPROBE_DEFER)
>> -			goto err_put_clks;
>> -		priv->rst = NULL;
>> -	} else {
>> -		err = reset_control_deassert(priv->rst);
>> -		if (err)
>> -			goto err_put_clks;
>> +		for (rst = 0; rst < EHCI_MAX_RESETS; rst++) {
>> +			priv->resets[rst] =
>> +				devm_reset_control_get_shared_by_index(
>> +								&dev->dev, rst);
>> +			if (IS_ERR(priv->resets[rst])) {
>> +				err = PTR_ERR(priv->resets[rst]);
>> +				if (err == -EPROBE_DEFER)
>> +					goto err_reset;
>> +				priv->resets[rst] = NULL;
>> +				break;
>> +			}
>> +			err = reset_control_deassert(priv->resets[rst]);
>> +			if (err)
>> +				goto err_reset;
>> +		}
>
> Why is this code within "if (dev->of.node)" ?
> Previous code looked like it worked for non-DT boots as well.

Because devm_reset_control_get_shared_by_index() only works for DT platforms
(it is safe to call on non DT platforms, it will just always return -EINVAL
there) and the old reset paths were ever only used / only ever worked with
DT too.

Regards,

Hans