RE: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks

[Anurag Kumar Vulisha]

Hi Mathias,

Thanks for providing your comments, please find my comments inline

>-----Original Message-----
>From: Mathias Nyman [mailto:mathias.nyman@xxxxxxxxx]
>Sent: Friday, July 20, 2018 8:13 PM
>To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Greg Kroah-Hartman
><gregkh@xxxxxxxxxxxxxxxxxxx>
>Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH] usb: host: xhci-plat: Iterate over parent nodes for finding quirks
>
>Hi
>
>Sorry about the delay with this patch,
>I have a couple comments inline.
>

No probs, I  can understand. I was having some internal dependency on this patch,
so pinged again. Sorry for troubling you.

>On 05.06.2018 18:20, Anurag Kumar Vulisha wrote:
>> In xhci_plat_probe() both sysdev and pdev->dev are being used for
>> finding quirks. There are some drivers(like dwc3 host.c) which adds
>> quirks(like usb3-lpm-capable) into pdev and the logic present in
>> xhci_plat_probe() checks for quirks in either sysdev or pdev for
>> finding the quirks. Because of this logic, some of the quirks are
>> getting missed(usb3-lpm-capable quirk added by dwc3 host.c driver is
>> getting missed).This patch fixes this by iterating over all the
>> available parents for finding the quirks. In this way all the quirks
>> which are present in child or parent are correctly updated.
>>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
>> ---
>>   drivers/usb/host/xhci-plat.c | 29 ++++++++++++++++++-----------
>>   1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c
>> b/drivers/usb/host/xhci-plat.c index c1b22fc..0cd0489 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -152,7 +152,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>   {
>>   	const struct xhci_plat_priv *priv_match;
>>   	const struct hc_driver	*driver;
>> -	struct device		*sysdev;
>> +	struct device		*sysdev, *tmpdev;
>>   	struct xhci_hcd		*xhci;
>>   	struct resource         *res;
>>   	struct usb_hcd		*hcd;
>> @@ -272,19 +272,26 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>   		goto disable_clk;
>>   	}
>>
>> -	if (device_property_read_bool(sysdev, "usb2-lpm-disable"))
>> -		xhci->quirks |= XHCI_HW_LPM_DISABLE;
>> +	/* Iterate over all parent nodes for finding quirks */
>> +	for (tmpdev = &pdev->dev; tmpdev; tmpdev = tmpdev->parent) {
>
>Isn't sysdev at this point the topmost device that can have any of those device
>properties set?
>We could loop from &pdev->dev up to and including sysdev.
>
>It doesn't matter much but maybe avoid walking some extra parents.
>

I have seen some drivers ( like dwc3 host.c) which create a child dev, which is
the pdev that is being passed to xhci_plat_probe(). So, sysdev may not be the
topmost parent. There could be some properties which may be present in parent
and may not be populated in the child. So, I made this change to search on all
available parents for finding a valid property

>>
>> -	if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
>> -		xhci->quirks |= XHCI_LPM_SUPPORT;
>> +		if (device_property_read_bool(tmpdev, "usb2-lpm-disable"))
>> +			xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>
>> -	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>> -		xhci->quirks |= XHCI_BROKEN_PORT_PED;
>> +		if (device_property_read_bool(tmpdev, "usb3-lpm-capable"))
>> +			xhci->quirks |= XHCI_LPM_SUPPORT;
>>
>> -	/* imod_interval is the interrupt moderation value in nanoseconds. */
>> -	xhci->imod_interval = 40000;
>
>Setting the default imod_interval could be moved before the for() loop
>

I thought it would be better if everything related to imod_interval is in one place, 
so kept it as is. I can fix this in v2 if you suggest.

>> -	device_property_read_u32(sysdev, "imod-interval-ns",
>> -				 &xhci->imod_interval);
>> +		if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
>> +			xhci->quirks |= XHCI_BROKEN_PORT_PED;
>> +
>> +		/*
>> +		 * imod_interval is the interrupt moderation
>> +		 * value in nanoseconds.
>> +		 */
>> +		xhci->imod_interval = 40000;
>> +		device_property_read_u32(tmpdev, "imod-interval-ns",
>> +					 &xhci->imod_interval);
>> +	}
>>
>>   	hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
>>   	if (IS_ERR(hcd->usb_phy)) {
>>
>
>Otherwise everything looks fine.
>I will unfortunately be away again for another two weeks.

No probs. If you feel there is no harm in applying and satisfied with the changes, it
would be helpful if you can apply this patch. This is because I have some dependency
on this patch waiting  to be applied. Sorry for troubling you again.

Thanks,
Anurag Kumar Vulisha

>
>-Mathias