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@xxxxxxxxxxx>
---
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@xxxxxxxxxxx/
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@xxxxxxxxxxxxxxx/
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,
