Hi Balbi,
> -----Original Message-----
> From: Felipe Balbi [mailto:ba...@kernel.org]
> Sent: Wednesday, November 15, 2017 4:52 PM
> To: Ran Wang <ran.wan...@nxp.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>; open
> list:DESIGNWARE USB3 DRD IP DRIVER <linux-...@vger.kernel.org>; open
> list <linux-kernel@vger.kernel.org>; Jerry Huang <jerry.hu...@nxp.com>;
> Rajesh Bhagat <rajesh.bha...@nxp.com>; Leo Li <leoyang...@nxp.com>; Ran
> Wang <ran.wan...@nxp.com>; Rob Herring <robh...@kernel.org>;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH] usb: dwc3: Enable the USB snooping
>
>
> Hi,
>
> Ran Wang <ran.wan...@nxp.com> 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.hu...@nxp.com>
> > Signed-off-by: Rajesh Bhagat <rajesh.bha...@nxp.com>
> > Signed-off-by: Ran Wang <ran.wan...@nxp.com>
> > ---
> > 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
