Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen <hzpeterchen@gmail.com>]

On Thu, Sep 08, 2016 at 03:59:19PM +0300, Grygorii Strashko wrote:
> On 09/08/2016 03:28 PM, Peter Chen wrote:
> > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> >> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >>> Arnd Bergmann <arnd@arndb.de> writes:
> >>>> On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> >>>>>> If we do that, we have to put child devices of the dwc3 devices into
> >>>>>> the platform glue, and it also breaks those dwc3 devices that don't
> >>>>>> have a parent driver.
> >>>>>
> >>>>> Well, this is easy to fix:
> >>>>>
> >>>>>         if (dwc->dev->parent) {
> >>>>>                 dwc->sysdev = dwc->dev->parent;
> >>>>>         } else {
> >>>>>                 dev_info(dwc->dev, "Please provide a glue layer!\n");
> >>>>>                 dwc->sysdev = dwc->dev;
> >>>>>         }
> >>>>
> >>>> I don't understand. Do you mean we should have an extra level of
> >>>> stacking and splitting "static struct platform_driver dwc3_driver"
> >>>> in two so instead of
> >>>>
> >>>>       "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> >>>>
> >>>> we do this?
> >>>>
> >>>>       "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" (usb_bus.dev)
> >>>
> >>> no 
> >>>
> >>> If we have a parent device, use that as sysdev, otherwise use self as
> >>> sysdev.
> >>
> >> But there is often a parent device in DT, as the xhci device is
> >> attached to some internal bus that gets turned into a platform_device
> >> as well, so checking whether there is a parent will get the wrong
> >> device node.
> > 
> > From my point, all platform and firmware information at dwc3 are
> > correct, so we don't need to change dwc3/core.c, only changing for
> > xhci-plat.c is ok.
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index ed56bf9..fd57c0d 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	struct clk              *clk;
> >  	int			ret;
> >  	int			irq;
> > +	struct device *dev = &pdev->dev, *sysdev;
> >  
> >  	if (usb_disabled())
> >  		return -ENODEV;
> > @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	if (irq < 0)
> >  		return -ENODEV;
> >  
> > +	if (dev->parent) {
> > +		sysdev = dev->parent;
> > +	} else {
> > +        	sysdev = dev;
> > +	}
> > +
> 
> Shouldn't we be more careful with that?
> 

Above code does not consider pci device case, Arnd's patch covers
all cases.

> armada-375.dtsi
> 
> 	soc {
> 		compatible = "marvell,armada375-mbus", "simple-bus";
> 
> 		internal-regs {
> 			compatible = "simple-bus";
> 
> 			usb3@58000 {
> 				compatible = "marvell,armada-375-xhci";
> 				reg = <0x58000 0x20000>,<0x5b880 0x80>;
> 				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> 				clocks = <&gateclk 16>;
> 				phys = <&usbcluster PHY_TYPE_USB3>;
> 				phy-names = "usb";
> 				status = "disabled";
> 			};
> 
> 
> What will be the parent dev in above case?
> 

In this case, no parent dev for above case, it will use itself as sysdev
since it has of_node at dts.

-- 

Best Regards,
Peter Chen