Re: [PATCH v2 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Arnd Bergmann]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/