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
