Re: [PATCH v3 3/3] usb: dwc: host: add xhci_plat_priv quirk XHCI_SKIP_PHY_INIT

["Sandeep Maheswaram (Temp)" <quic_c_sanm@quicinc.com>]

Hi Mathias,Heikki

On 3/25/2022 9:08 PM, Heikki Krogerus wrote:
> On Fri, Mar 25, 2022 at 04:33:27PM +0200, Mathias Nyman wrote:
>> On 25.3.2022 13.27, Heikki Krogerus wrote:
>>> On Fri, Mar 25, 2022 at 12:36:22AM +0200, Mathias Nyman wrote:
>>>> On 24.3.2022 14.27, Heikki Krogerus wrote:
>>>>> On Thu, Mar 24, 2022 at 12:07:11PM +0530, Sandeep Maheswaram wrote:
>>>>>> Currently the phy init is done from dwc3 and also xhci which makes the
>>>>>> runtime_usage value 2 for the phy which causes issue during runtime
>>>>>> suspend. When we run the below command the runtime_status still shows
>>>>>> active.
>>>>>> echo auto > /sys/bus/platform/devices/88e3000.phy/power/control
>>>>>>
>>>>>> dwc3 manages PHY by own DRD driver, so skip the management by
>>>>>> HCD core by setting this quirk.
>>>>>>
>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
>>>>>> ---
>>>>>>   drivers/usb/dwc3/host.c | 13 +++++++++++++
>>>>>>   1 file changed, 13 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>>>>> index eda8719..d4fcf06 100644
>>>>>> --- a/drivers/usb/dwc3/host.c
>>>>>> +++ b/drivers/usb/dwc3/host.c
>>>>>> @@ -13,6 +13,12 @@
>>>>>>   #include <linux/platform_device.h>
>>>>>>   
>>>>>>   #include "core.h"
>>>>>> +#include <linux/usb/xhci-plat.h>
>>>>>> +#include <linux/usb/xhci-quirks.h>
>>>>>> +
>>>>>> +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
>>>>>> +	.quirks = XHCI_SKIP_PHY_INIT,
>>>>>> +};
>>>>>>   
>>>>>>   static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc,
>>>>>>   					int irq, char *name)
>>>>>> @@ -122,6 +128,13 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>>>>   		}
>>>>>>   	}
>>>>>>   
>>>>>> +	ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
>>>>>> +			sizeof(xhci_plat_dwc3_xhci));
>>>>>> +	if (ret) {
>>>>>> +		dev_err(dwc->dev, "failed to add data to xHCI\n");
>>>>>> +		goto err;
>>>>>> +	}
>>>>>> +
>>>>>>   	ret = platform_device_add(xhci);
>>>>>>   	if (ret) {
>>>>>>   		dev_err(dwc->dev, "failed to register xHCI device\n");
>>>>> I think you should just use device property:
>>>>>
>>>> This was suggested in an earlier series, but was rejected as it also added
>>>> the property as a device tree parameter.
>>>>
>>>> I think adding more device properties can be messy in the long run, especially if we
>>>> need to add them for many of the existing xhci quirks.
>>>> We also end up with a mix where some device properties are listed as device tree
>>>> parameters, and some not.
>>>>
>>>> Defining xhci quirks and platform data structure in headers shared with dwc3 and cdns3
>>>> allow those drivers to easily set any existing xhci quirk, or other possible optional
>>>> callbacks.
>>>>
>>>> cdns3 driver is already doing this, but it includes the full xhci.h header.
>>>> This series cleans up that a bit so cdns3 will only include xhci quirk bits and
>>>> platform data structure.
>>>>
>>>> On the downside we add a couple xhci related header files to include/linux/usb/
>>>> Let me know if you see any other issues I missed with this approach.
>>> The problem here is that these drivers are now coupled together, and
>>> that should not be taken lightly. We have a dependency hell in our
>>> hands with a lot of drivers, and the culprit is always platform data.
>>>
>>> Build-in device properties may be messy, but I would still say they
>>> are less messy than those quirk flags - you got to admit, they are a
>>> mess. The benefit from build-in properties is in any case the fact
>>> that they remove the need to couple these drivers together.
>> Agree, quirk bits are messy. Any suggestion that would work with
>> PCI xHCI devices, devicetree, and "pure" platform devices?
> I think xHCI driver should always be able to rely on being able to
> read this kind of information from the fwnode. If there is no actual
> firmware node (DT or ACPI), or if it's missing some information, the
> glue driver needs to populate software node for the xHCI.
>
> Right now I just want to avoid having to pass the quirks using
> platform data from drivers such as drivers/usb/cdns3/host.c and
> drivers/usb/dwc3/host.c to xHCI.
>
> One way we could do that is by defining compatibility ID for both of
> them that we provide using a single device property (like I guess DT
> does). Then based on that compatibility ID, xhci-plat.c can set the
> actual "static" quirk flags. That we could already do easily. How
> would that sound to you?

This was my previous patch where I was using device tree property. 
Should we go ahead with this approach?

https://patchwork.kernel.org/project/linux-arm-msm/cover/1636353710-25582-1-git-send-email-quic_c_sanm@quicinc.com/

Any further changes to this ?

Regards

Sandeep

>
>>> You can also use something like naming convention if you are worried
>>> about confusion between devicetree properties and build-in only
>>> properties ("build-in:skip-phy-init" or whatever), and of course
>>> require that each of the build-in only property is documented clearly
>>> in drivers/usb/host/xhci-plat.c. But this in any case really can not
>>> be justification for a platform data blob just so you can avoid using
>>> the properties - honestly, it really should to be the other way
>>> around.
>>>
>>> Platform data is in practice always problematic. On top of the driver
>>> coupling, it creates maintenance burden, code duplication, etc. Please
>>> don't just accept it lightly. I'm telling you, for hacks like this, the
>>> build-in device properties is a much much safer bet.
>>>
>> Fair enough.
>>
>> Any idea if there's a long term solution for platform devices?
>> Adding some type of "compatibility" id to platform devices in addition to name?
> We don't need to touch the platform devices if, and when, we have
> always the fwnode. The "compatibility" id should always be possible to
> pick from the fwnode.
>
>> Also just noticed cdns3 driver passes a function pointer via platform_data to
>> xhci-plat. This should be cleared up as well.
> Looks like a PM quirk. For that I would propose this:
> https://lore.kernel.org/linux-acpi/20201029105941.63410-2-heikki.krogerus@linux.intel.com/
>
> So with that you can supply separate PM operations for your software
> fwnode. When ever the device suspends, the suspend quirk is also
> executed. xHCI core does not need to know about it.
>
> Yeah, I guess I need to finish that series...
>
> thanks,
>