Hi,
On Tue, Feb 28, 2012 at 06:06:38PM -0800, Paul Zimmerman wrote:
> > > > You could split the PCI address space into DWC USB3 IP specific and the
> > > > rest (which is HAPS specific), unless you're mapping your HAPS-specific
> > > > part on an unused area of the DWC USB3 IP address space, then, well, we
> > > > need to think how to handle it.
> > >
> > > I played around with that, but it was really ugly, and there were still
> > > problems because 'struct dwc3' was not accessible from dwc3-pci.
> > >
> > > Let me cook up an RFC patch for the modifications I have in mind. Code
> > > talks, after all :) It might take me a few days, I have some higher-
> > > priority stuff going on right now.
> >
> > Sure, go ahead ;-) I'll go over the other patches you sent meanwhile.
>
> Hi Felipe,
>
> I think I found a less invasive way to do what I need, by defining a
> platform_data struct for the dwc3 device, so that it can share function
that won't fly. We're all moving to devicetree so that will not work
properly :-(
> pointers and the dwc3 context with the bus glue. Below is a POC patch
> to show roughly what I plan to do. Is this an acceptable use of platform
> data?
The way you use to pass function pointers is ok (if we weren't concerned
about devicetree), but using it to pass dwc3 pointer back to glue layer
isn't very wise and we have proven that with MUSB.
> If so, maybe we could also remove the dwc3_{get|put}_device_id functions
> and just have dwc3_probe write the devid into the platform_data? Then we
> could get rid of those two exported functions.
We need the ID before dwc3 platform_device is added to driver core,
otherwise dwc3's probe() won't be called. Another issue with the ID is
that as soon as you add the platform_device to the driver model, sysfs
files will be created and if we don't have a unique name at that time
($name.$id) we could try to create directories which already exist.
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 6c7945b..3f5d0dc 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -529,6 +529,18 @@ struct dwc3_request {
> unsigned queued:1;
> };
>
> +/*
> + * Platform data (used for hibernation control)
> + */
> +struct dwc3_pdata {
> + /* set by dwc3 module */
> + struct dwc3 *dwc;
> +
> + /* set by bus glue module */
> + void (*power_control)(struct dwc3 *, int);
> + int (*interrupt_hook)(struct dwc3 *);
> +};
> +
> /**
> * struct dwc3 - representation of our controller
> * @ctrl_req: usb control request which is used for ep0
> @@ -584,6 +596,7 @@ struct dwc3 {
>
> struct platform_device *xhci;
> struct resource *res;
> + struct dwc3_pdata *pdata;
>
> struct dwc3_event_buffer **ev_buffs;
> struct dwc3_ep *eps[DWC3_ENDPOINTS_NUM];
>
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index a81b30e..e647570 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -51,8 +51,38 @@
> struct dwc3_pci {
> struct device *dev;
> struct platform_device *dwc3;
> + struct dwc3_pdata pdata;
> };
>
> +/**
> + * dwc3_haps_power_ctl - platform-specific power control routine, for HAPS board
> + *
> + * @dwc: pointer to our controller context structure
> + * @on: 1 to turn the power on, 0 to turn it off
> + */
> +static void dwc3_haps_power_ctl(struct dwc3 *dwc, int on)
> +{
> + /* ... */
> +}
> +
> +/**
> + * dwc3_haps_intr_hook - platform-specific interrupt handling, for HAPS board
> + *
> + * If this routine returns false, the caller should handle the interrupt
> + * as usual.
> + *
> + * If this routine returns true, the caller should exit the interrupt handler
> + * immediately, without touching the dwc3 registers.
> + *
> + * @dwc: pointer to our controller context structure
> + */
> +static int dwc3_haps_intr_hook(struct dwc3 *dwc)
> +{
> + /* ... */
> +
> + return 0;
> +}
> +
> static int __devinit dwc3_pci_probe(struct pci_dev *pci,
> const struct pci_device_id *id)
> {
> @@ -109,12 +139,16 @@ static int __devinit dwc3_pci_probe(struct pci_dev *pci,
> goto err2;
> }
>
> + glue->pdata.power_control = dwc3_haps_power_ctl;
> + glue->pdata.interrupt_hook = dwc3_haps_intr_hook;
One thing that I still didn't understand and you didn't answer when I
asked, if why you don't do the haps_power_control from the dwc3-pci.c
file ?
On that file you have access to the PCI device ID and can do runtime
detection if you need to call those functions or not.
Next week I'm off on a business trip, but after I'm back, I'll go over
your patches again and try to propose how I wanted things to look.
Unfortunately, I won't have how to test it because of lack of a recent
model with this feature.
cheers
--
balbi
Attachment:
signature.asc
Description: Digital signature
