Re: [PATCH v4 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5704 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]