Hi, On Tue, Sep 02, 2014 at 12:18:12PM +0100, Peter Griffin wrote: > Hi Felipe, > > Sorry for the delay in replying to this mail, I've been trying to get > answers to the suspend/resume questions you had. np > > > +config USB_DWC3_ST > > > + tristate "STMicroelectronics Platforms" > > > + depends on ARCH_STI && OF > > > + default USB_DWC3_HOST > > > > this seems wrong as USB_DWC3_{HOST,GADGET,DUAL_ROLE} are booleans and > > USB_DWC3_ST is a tristate. Better to stick with defaulting to USB_DWC3 > > instead like all the others. > > Ok will fix. tks > > > +static inline void st_dwc3_writel(void __iomem *base, u32 offset, u32 value) > > > +{ > > > + writel_relaxed(value, base + offset); > > > > why relaxed ? > > The writel and readl implementations on ARM are quite expensive. > > The writel, does a memory barrier, and also a outer_sync(), which > involves taking a spinlock, and draining the cache store buffers. > The readl also does a memory barrier. > > These barriers / cache operations are unnecessary here as the > peripheral memory has been ioremap'ed as device memory, and it is only > one device we are writing to, so the writel/readl_relaxed variants are > good enough as the ARM arch guarentees they will arrive in program > order. good point :-) > There is some more info about this here > http://permalink.gmane.org/gmane.linux.ports.arm.kernel/117658 > > Note: It's only possible when we know the driver is not being used on > other architectures which may have different constraints. > However for this driver, we know this IP (st glue logic) has only been > used on ARM based SoC's. alright :-) > > > +} > > > + > > > +/** > > > + * st_dwc3_drd_init: program the port > > > + * @dwc3_data: driver private structure > > > + * Description: this function is to program the port as either host or device > > > + * according to the static configuration passed from devicetree. > > > + * OTG and dual role are not yet supported! > > > + */ > > > +static int st_dwc3_drd_init(struct st_dwc3 *dwc3_data) > > > +{ > > > + u32 val; > > > + int err; > > > + > > > + err = regmap_read(dwc3_data->regmap, dwc3_data->syscfg_reg_off, &val); > > > + if (err) > > > + return err; > > > + > > > + switch (dwc3_data->dr_mode) { > > > + case USB_DR_MODE_PERIPHERAL: > > > + val |= USB_SET_PORT_DEVICE; > > > + dev_dbg(dwc3_data->dev, "Configuring as Device\n"); > > > + break; > > > + > > > + case USB_DR_MODE_HOST: > > > + val &= USB_HOST_DEFAULT_MASK; > > > > are you missing a ~ here ? Also, shouldn't you mask off the bits before > > this switch ? > > Yes your right, good spot! In the next iteration I've defined macros > for the bits in this control register and explitcitly set/clear them > for both cases, also adding a comment regarding the > USB3_DELAY_VBUSVALID bit. ok, cool. > By chance host mode still worked with this bug present as the reset > value of the register on this SoC is OK to have working host mode. heh :-) > > > + dev_dbg(dwc3_data->dev, "Configuring as Host\n"); > > > + break; > > > + > > > + default: > > > + dev_err(dwc3_data->dev, "Unsupported mode of operation %d\n", > > > + dwc3_data->dr_mode); > > > + return -EINVAL; > > > + } > > > + > > > + return regmap_write(dwc3_data->regmap, dwc3_data->syscfg_reg_off, val); > > > +} > > > + > > > +/** > > > + * st_dwc3_init: init the controller via glue logic > > > + * @dwc3_data: driver private structure > > > + */ > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data) > > > +{ > > > + > > > > this blank line isn't necessary. > > Ok, removed in next iteration > > <snip> > > > > + > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-reg"); > > > + if (!res) { > > > + ret = -ENXIO; > > > + goto undo_platform_dev_alloc; > > > + } > > > + > > > + dwc3_data->syscfg_reg_off = res->start; > > > + > > > + dev_dbg(&pdev->dev, "glue-logic addr 0x%p, syscfg-reg offset 0x%x\n", > > > + dwc3_data->glue_base, dwc3_data->syscfg_reg_off); > > > > looks like this message would be more of a dev_vdbg(). > > Ok, changed to dev_vdbg in next iteration > > <snip> > > > > + > > > +#ifdef CONFIG_PM_SLEEP > > > +static int st_dwc3_suspend(struct device *dev) > > > +{ > > > + struct st_dwc3 *dwc3_data = dev_get_drvdata(dev); > > > + > > > + reset_control_assert(dwc3_data->rstc_pwrdn); > > > + reset_control_assert(dwc3_data->rstc_rst); > > > > Two questions: > > > > 1) how would you handle the case when this device is a wakeup source ? > > I've confirmed with ST the usb3 IP can't be a wakeup source on this SoC. > > > 2) when resuming, wouldn't you have to reinitialize the entire core ? > > I asked ST to test this, as a full working suspend / resume setup > involves some firmware for the standby controller which I don't > currently have access to (and it is only with the SBC running that all > power will be removed from this part of the SoC). They have confirmed > that the usb3 port works after a suspend / resume and devices are > correctly enumerated etc after a resume with the code as it was > submitted. > > What I did notice though after re-reading it, is that we are not > re-configuring the ST glue logic registers on a resume. So the > controller could end up with the vbus mux configured differently. So > in the next iteration I've fixed this, and call st_dwc3_drd_init and > st_dwc3_init in the resume path. > > Although ST confirmed that suspend / resume works with or without this > change applied. alright, thanks a lot for confirming. -- balbi
Attachment:
signature.asc
Description: Digital signature