RE: [PATCH] usb: dwc3: Enable the USB snooping

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Balbi,

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@xxxxxxxxxx]
> Sent: Wednesday, November 15, 2017 4:52 PM
> To: Ran Wang <ran.wang_1@xxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; open
> list:DESIGNWARE USB3 DRD IP DRIVER <linux-usb@xxxxxxxxxxxxxxx>; open
> list <linux-kernel@xxxxxxxxxxxxxxx>; Jerry Huang <jerry.huang@xxxxxxx>;
> Rajesh Bhagat <rajesh.bhagat@xxxxxxx>; Leo Li <leoyang.li@xxxxxxx>; Ran
> Wang <ran.wang_1@xxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] usb: dwc3: Enable the USB snooping
> 
> 
> Hi,
> 
> Ran Wang <ran.wang_1@xxxxxxx> writes:
> > Add support for USB3 snooping by asserting bits in register
> > DWC3_GSBUSCFG0 for data and descriptor.
> 
> we know *how* to enable a feature :-) It's always the same, you fiddle with
> some registers and it works. What you failed to tell us is:
> 
> a) WHY do you need this?
> b) WHY do we need another DT property for this?
> c) WHAT does this mean for PCI devices?

So far I cannot have the answer for you, will get you back after some discussion
with my colleagues.

> > Signed-off-by: Changming Huang <jerry.huang@xxxxxxx>
> > Signed-off-by: Rajesh Bhagat <rajesh.bhagat@xxxxxxx>
> > Signed-off-by: Ran Wang <ran.wang_1@xxxxxxx>
> > ---
> >  drivers/usb/dwc3/core.c | 24 ++++++++++++++++++++++++
> > drivers/usb/dwc3/core.h | 10 ++++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > 07832509584f..ffc078ab4a1c 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -236,6 +236,26 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
> >  	return -ETIMEDOUT;
> >  }
> >
> > +/*
> > + * dwc3_enable_snooping - Enable snooping feature
> > + * @dwc3: Pointer to our controller context structure  */ static void
> > +dwc3_enable_snooping(struct dwc3 *dwc) {
> > +	u32 cfg;
> > +
> > +	cfg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> > +	if (dwc->dma_coherent) {
> > +		cfg &= ~DWC3_GSBUSCFG0_SNP_MASK;
> > +		cfg |= (AXI3_CACHE_TYPE_SNP <<
> DWC3_GSBUSCFG0_DATARD_SHIFT) |
> > +			(AXI3_CACHE_TYPE_SNP <<
> DWC3_GSBUSCFG0_DESCRD_SHIFT) |
> > +			(AXI3_CACHE_TYPE_SNP <<
> DWC3_GSBUSCFG0_DATAWR_SHIFT) |
> > +			(AXI3_CACHE_TYPE_SNP <<
> DWC3_GSBUSCFG0_DESCWR_SHIFT);
> 
> This "value << shift" looks super clumsy. I would rather have something akin
> to:
> 
> 	cfg |= DWC3_GSBUSCFG0_DATARD_CACHEABLE |
>         	DWC3_GSBUSCFG0_DESCRD_CACHEABLE ...
> 
> and so on.

Got it. 

> > +	}
> > +
> > +	dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);
> 
> this will *always* read and write GSBUSCFG0 even for those platforms which
> don't need to change anything on this register. You should just bail out early
> if !dwc->dma_coherent
> 
> Also, I think dma_coherent is likely not the best name for this property.
> 
> Another question is: Why wasn't this setup properly during coreConsultant
> instantiation of the RTL? Do you have devices on the market already that
> need this or is this some early FPGA model or test-only ASIC?

Yes, you are right. Actually I thought that all dwc3 IP  will have this register, and
it can be controlled by DTS property. 

> > @@ -776,6 +796,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >  	/* Adjust Frame Length */
> >  	dwc3_frame_length_adjustment(dwc);
> >
> > +	dwc3_enable_snooping(dwc);
> > +
> >  	usb_phy_set_suspend(dwc->usb2_phy, 0);
> >  	usb_phy_set_suspend(dwc->usb3_phy, 0);
> >  	ret = phy_power_on(dwc->usb2_generic_phy);
> > @@ -1021,6 +1043,8 @@ static void dwc3_get_properties(struct dwc3
> *dwc)
> >  				&hird_threshold);
> >  	dwc->usb3_lpm_capable = device_property_read_bool(dev,
> >  				"snps,usb3_lpm_capable");
> > +	dwc->dma_coherent = device_property_read_bool(dev,
> > +				"dma-coherent");
> >
> >  	dwc->disable_scramble_quirk = device_property_read_bool(dev,
> >  				"snps,disable_scramble_quirk");
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
> > 4a4a4c98508c..6e6a66650e53 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -153,6 +153,14 @@
> >
> >  /* Bit fields */
> >
> > +/* Global SoC Bus Configuration Register 0 */
> > +#define AXI3_CACHE_TYPE_SNP		0x2 /* cacheable */
> > +#define DWC3_GSBUSCFG0_DATARD_SHIFT	28
> > +#define DWC3_GSBUSCFG0_DESCRD_SHIFT	24
> > +#define DWC3_GSBUSCFG0_DATAWR_SHIFT	20
> > +#define DWC3_GSBUSCFG0_DESCWR_SHIFT	16
> > +#define DWC3_GSBUSCFG0_SNP_MASK		0xffff0000
> 
> 
> 
> > +
> >  /* Global Debug Queue/FIFO Space Available Register */
> >  #define DWC3_GDBGFIFOSPACE_NUM(n)	((n) & 0x1f)
> >  #define DWC3_GDBGFIFOSPACE_TYPE(n)	(((n) << 5) & 0x1e0)
> > @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array {
> >   * 	3	- Reserved
> >   * @imod_interval: set the interrupt moderation interval in 250ns
> >   *                 increments or 0 to disable.
> > + * @dma_coherent: set if enable dma-coherent.
> 
> you're not enabling dma coherency, you're enabling cache snooping. And
> this property should describe that. Also, keep in mind that different devices
> may want different cache types for each of those fields, so your property
> would have to be a lot more complex. Something like:
> 
> 	snps,cache-type = <foobar "cacheable">, <baz "cacheable">, ...
> 
> Then driver would have to parse this properly to setup GSBUSCFG0.

Got it, learn a lot, need more time to digest and test, thanks for your patiently explanation.

> In any
> case, I still want to know why do you really need this? What's the reason?
> What happens if you don't fix GSBUSCFG0? What's the value you have there
> by default? Why isn't that default good enough?

So far the Layerscape SoC (such as LS1088A) has enabled this feature and I
have tested it. Once we add dma-coherent on DTS without this Patch, dwc3
will fail on device enumeration as below:
[   15.124031] xhci-hcd xhci-hcd.0.auto: Error while assigning device slot ID
[   15.130912] xhci-hcd xhci-hcd.0.auto: Max number of devices this xHCI host supports is 127.
[   15.139268] usb usb1-port1: couldn't allocate usb_device

> 
> ps: since you're fiddling with DT, you should also include devicetree@vger

OK

Best Regards
Ran
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux