| Message ID | 1566900127-11148-2-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: host: xhci-{plat,rcar}: clean up and add a workaround | expand |
Hi Shimoda-san, On Tue, Aug 27, 2019 at 12:05 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > To simplify adding xhci->quirks instead of the .init_quirk() > function, this patch adds a new parameter "quirks" into > the struct xhci_plat_priv. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/usb/host/xhci-plat.h > +++ b/drivers/usb/host/xhci-plat.h > @@ -12,10 +12,12 @@ > > struct xhci_plat_priv { > const char *firmware_name; > + unsigned long long quirks; > void (*plat_start)(struct usb_hcd *); > int (*init_quirk)(struct usb_hcd *); > int (*resume_quirk)(struct usb_hcd *); > }; > > #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv) > +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv) Just wondering: is x->priv guaranteed to be of type struct xhci_plat_priv *, also in the future? struct xhci_hcd { ... unsigned long priv[0] __aligned(sizeof(s64)); Gr{oetje,eeting}s, Geert
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Tuesday, August 27, 2019 9:36 PM > > Hi Shimoda-san, > > On Tue, Aug 27, 2019 at 12:05 PM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > To simplify adding xhci->quirks instead of the .init_quirk() > > function, this patch adds a new parameter "quirks" into > > the struct xhci_plat_priv. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thank you for your review! > > --- a/drivers/usb/host/xhci-plat.h > > +++ b/drivers/usb/host/xhci-plat.h > > @@ -12,10 +12,12 @@ > > > > struct xhci_plat_priv { > > const char *firmware_name; > > + unsigned long long quirks; > > void (*plat_start)(struct usb_hcd *); > > int (*init_quirk)(struct usb_hcd *); > > int (*resume_quirk)(struct usb_hcd *); > > }; > > > > #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv) > > +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv) > > Just wondering: is x->priv guaranteed to be of type struct xhci_plat_priv *, > also in the future? > > struct xhci_hcd { > ... > unsigned long priv[0] __aligned(sizeof(s64)); It seems so. But, I'm not sure that we can change type of the priv[0] to struct xhci_plat_priv *, because this implementation is related to all usb_hcd drivers: < Details if you are interested in :) > --- include/linux/usb/hcd.h --- struct usb_hcd { ... /* The HC driver's private data is stored at the end of * this structure. */ unsigned long hcd_priv[0] __attribute__ ((aligned(sizeof(s64)))); }; --- drivers/usb/host/xhci.c --- static const struct hc_driver xhci_hc_driver = { ... .hcd_priv_size = sizeof(struct xhci_hcd), and void xhci_init_driver(struct hc_driver *drv, const struct xhci_driver_overrides *over) { BUG_ON(!over); /* Copy the generic table to drv then apply the overrides */ *drv = xhci_hc_driver; if (over) { drv->hcd_priv_size += over->extra_priv_size; --- drivers/usb/host/xhci-plat.c --- static const struct xhci_driver_overrides xhci_plat_overrides __initconst = { .extra_priv_size = sizeof(struct xhci_plat_priv), void xhci_init_driver(struct hc_driver *drv, const struct xhci_driver_overrides *over) { BUG_ON(!over); /* Copy the generic table to drv then apply the overrides */ *drv = xhci_hc_driver; if (over) { drv->hcd_priv_size += over->extra_priv_size; --- drivers/usb/core/hcd.c --- struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver, struct device *sysdev, struct device *dev, const char *bus_name, struct usb_hcd *primary_hcd) { struct usb_hcd *hcd; hcd = kzalloc(sizeof(*hcd) + driver->hcd_priv_size, GFP_KERNEL); ------------------------------ Best regards, Yoshihiro Shimoda > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Shimoda-san, On Wed, Aug 28, 2019 at 6:05 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Geert Uytterhoeven, Sent: Tuesday, August 27, 2019 9:36 PM > > On Tue, Aug 27, 2019 at 12:05 PM Yoshihiro Shimoda > > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > To simplify adding xhci->quirks instead of the .init_quirk() > > > function, this patch adds a new parameter "quirks" into > > > the struct xhci_plat_priv. > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Thank you for your review! > > > > --- a/drivers/usb/host/xhci-plat.h > > > +++ b/drivers/usb/host/xhci-plat.h > > > @@ -12,10 +12,12 @@ > > > > > > struct xhci_plat_priv { > > > const char *firmware_name; > > > + unsigned long long quirks; > > > void (*plat_start)(struct usb_hcd *); > > > int (*init_quirk)(struct usb_hcd *); > > > int (*resume_quirk)(struct usb_hcd *); > > > }; > > > > > > #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv) > > > +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv) > > > > Just wondering: is x->priv guaranteed to be of type struct xhci_plat_priv *, > > also in the future? > > > > struct xhci_hcd { > > ... > > unsigned long priv[0] __aligned(sizeof(s64)); > > It seems so. But, I'm not sure that we can change type of the priv[0] to > struct xhci_plat_priv *, because this implementation is related to all usb_hcd drivers: > > < Details if you are interested in :) > Thank you! Not being familiar with the usb and xhci subsystem, I couldn't find where it was used/allocated, hence my question. This confirms it's always of type struct xhci_plat_priv * iff when dealing with the xhci-plat driver. Gr{oetje,eeting}s, Geert
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index a1e5ce4..1843b69 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -66,12 +66,14 @@ static int xhci_priv_resume_quirk(struct usb_hcd *hcd) static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci) { + struct xhci_plat_priv *priv = xhci_to_priv(xhci); + /* * As of now platform drivers don't provide MSI support so we ensure * here that the generic code does not try to make a pci_dev from our * dev struct in order to setup MSI */ - xhci->quirks |= XHCI_PLAT; + xhci->quirks |= XHCI_PLAT | priv->quirks; } /* called during probe() after chip reset completes */ diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h index ae29f22..5681723 100644 --- a/drivers/usb/host/xhci-plat.h +++ b/drivers/usb/host/xhci-plat.h @@ -12,10 +12,12 @@ struct xhci_plat_priv { const char *firmware_name; + unsigned long long quirks; void (*plat_start)(struct usb_hcd *); int (*init_quirk)(struct usb_hcd *); int (*resume_quirk)(struct usb_hcd *); }; #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv) +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv) #endif /* _XHCI_PLAT_H */
To simplify adding xhci->quirks instead of the .init_quirk() function, this patch adds a new parameter "quirks" into the struct xhci_plat_priv. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/usb/host/xhci-plat.c | 4 +++- drivers/usb/host/xhci-plat.h | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-)