On Thursday 24 July 2014 13:22:54 Peter Griffin wrote:
> Thanks for reviewing, see my comments below: -
>
> > Unfortunately, this seems to be done in a rather strange way,
> > I suspect you'll have to start over, but I'll let Alan and Greg
> > weigh in.
> >
> > > +
> > > +struct st_hcd_dev {
> > > + int port_nr;
> > > + struct platform_device *ehci_device;
> > > + struct platform_device *ohci_device;
> > > + struct clk *ic_clk;
> > > + struct clk *ohci_clk;
> > > + struct reset_control *pwr;
> > > + struct reset_control *rst;
> > > + struct phy *phy;
> > > +};
> >
> > The way you do this apparently is to create a device that encapsulates
> > the OHCI and the EHCI and then goes on to create child devices that
> > are bound to the real drivers.
>
> Yes, although this isn't the first driver to take that approach USB_HCD_BCMA
> (bcma-hcd.c) and USB_HCD_SSB (ssb-hcd.c) do much the same thing.
I just had a look at them, and I think the case is different here:
For the two bcma driver, there is a discoverable bus (bcma) rather than
the platform bus, and it only exposes one device, so the bcma-hcd driver
is actually needed so we get two device that can be bound to the regular
drivers.
For the ssb-hcd driver, it's less clear and that one could be
reworked into two separate drivers.
> > The way it should be done however is to put the two host controllers
> > into DT directly and describe their resources (phy, clock, reset, ...)
> > using the DT bindings for those.
>
> I'm of course happy to change it if required. I see looking through that a lot
> of other platforms do it the way you describe with a
>
> ehci-<platname>.c and ohci-<platname>.c
Right. We are trying to gradually move some of them over to use the
generic *hci-platform.c drivers though.
> > Depending on what kind of special requirements the ST version has,
> > this can be done either by using the generic ohci/ehci bindings
> > with extensions where necessary, or by creating a new binding and
> > new driver files that use 'ohci_init_driver'/'ehci_init_driver'
> > to inherit from the common code.
> >
> > The first of the two approaches is preferred.
>
> We don't have anything particularly special, just a couple of reset lines,
> clock, phy, etc.
Ok, good. Please see Documentation/devicetree/bindings/usb/usb-?hci.txt
then. You might actually be able to just use the existing drivers
without new code by just adding the proper DT nodes that follow these
bindings.
> > > + pdev->dev.parent = &parent->dev;
> > > + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> > > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> >
> > This is something we shouldn't ever do these days, the DMA settings
> > should come directly from DT without driver interaction.
>
> Ok, I'll wait to hear the outcome of Greg/Alans views before either fixing
> it or starting over.
Ok.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
