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

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

[Felipe Balbi]

Hi,

Ran Wang <ran.wang_1-3arQi8VN3Tc@public.gmane.org> 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?

> Signed-off-by: Changming Huang <jerry.huang-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Rajesh Bhagat <rajesh.bhagat-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Ran Wang <ran.wang_1-3arQi8VN3Tc@public.gmane.org>
> ---
>  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.

> +	}
> +
> +	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?

> @@ -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. 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?

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

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ran Wang]

Hi Balbi,

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Wednesday, November 15, 2017 4:52 PM
> To: Ran Wang <ran.wang_1-3arQi8VN3Tc@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; open
> list:DESIGNWARE USB3 DRD IP DRIVER <linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; open
> list <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; Jerry Huang <jerry.huang-3arQi8VN3Tc@public.gmane.org>;
> Rajesh Bhagat <rajesh.bhagat-3arQi8VN3Tc@public.gmane.org>; Leo Li <leoyang.li-3arQi8VN3Tc@public.gmane.org>; Ran
> Wang <ran.wang_1-3arQi8VN3Tc@public.gmane.org>; Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>;
> devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] usb: dwc3: Enable the USB snooping
> 
> 
> Hi,
> 
> Ran Wang <ran.wang_1-3arQi8VN3Tc@public.gmane.org> 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-3arQi8VN3Tc@public.gmane.org>
> > Signed-off-by: Rajesh Bhagat <rajesh.bhagat-3arQi8VN3Tc@public.gmane.org>
> > Signed-off-by: Ran Wang <ran.wang_1-3arQi8VN3Tc@public.gmane.org>
> > ---
> >  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Ran Wang <ran.wang_1-3arQi8VN3Tc@public.gmane.org> writes:
>> Ran Wang <ran.wang_1-3arQi8VN3Tc@public.gmane.org> 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.

IOW, you have no idea why you need this, right? We're not patching
things for the sake of patching things. We need to understand what these
changes mean to the HW before we send out a patch publicly.

Remember that the moment a patch like this is accepted, it has the
potential of changing behavior for *ALL* users.

>> > +	}
>> > +
>> > +	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. 

they all *have* the register, however, it's sort of expected that RTL
engineer will setup good defaults when instantiating the RTL using SNPS'
coreConsultant tool.

Does your platform work without this patch?

>> > +
>> >  /* 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.

no problem, please figure out the answers to my previous questions,
without which I can't accept your patch.

>> 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

okay, so without these changes, your host doesn't work. What is the
default value on your platform without these changes? (revert patch,
boot platform, let it fail, get register output from our regdump in debugfs)

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ran Wang]

Hi Felipe,

    Sorry for the late reply:

On Wednesday, November 15, 2017 18:23, Felipe Balbi wrote:
> 
> Hi,
> 
> Ran Wang <ran.wang_1@nxp.com> writes:
> >> Ran Wang <ran.wang_1@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.
> 
> IOW, you have no idea why you need this, right? We're not patching things for
> the sake of patching things. We need to understand what these changes mean
> to the HW before we send out a patch publicly.
> 
> Remember that the moment a patch like this is accepted, it has the potential of
> changing behavior for *ALL* users.
> 
> >> > +	}
> >> > +
> >> > +	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.
> 
> they all *have* the register, however, it's sort of expected that RTL engineer will
> setup good defaults when instantiating the RTL using SNPS'
> coreConsultant tool.
> 
> Does your platform work without this patch?

On Layerscape SoC (such as LS1088A, LS1046A, LS1043A) When I add 'dma-coherent'
to USB nodes without this patch, dwc3 will fail on device enumeration as below:
[    3.610620] xhci-hcd xhci-hcd.2.auto: WARNING: Host System Error
[    3.630609] usb usb2-port1: couldn't allocate usb_device

> >> > +
> >> >  /* 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.

According to the DesignWare Cores SuperSpeed USB 3.0 Controller Databook (v2.60a),
it has described Type Bit Assignments for all supported master bus type:
AHB, AXI3, AXI4 and Native. I found the bit definition are different among them.
So, for the example you gave above, feel a little bit confused. 
Did you mean:
    snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read allocate">

> > Got it, learn a lot, need more time to digest and test, thanks for
> > your patiently explanation.
> 
> no problem, please figure out the answers to my previous questions, without
> which I can't accept your patch.
> 
> >> 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
> 
> okay, so without these changes, your host doesn't work. What is the default
> value on your platform without these changes? (revert patch, boot platform, let
> it fail, get register output from our regdump in debugfs)

The register I dumped is as below (partial register):
root@ls1046ardb:/sys/kernel/debug/2f00000.usb# cat regdump     
GSBUSCFG0 = 0x00100080
GSBUSCFG1 = 0x00000700
GTXTHRCFG = 0x00000000
GRXTHRCFG = 0x00000000
GCTL = 0x30c11004
GEVTEN = 0x00000000
GSTS = 0x3e800001
GUCTL1 = 0x0000018a
GSNPSID = 0x5533280a
GGPIO = 0x00000000
GUID = 0x00050100
 GUCTL = 0x0200c010
GBUSERRADDR0 = 0x00000000
GBUSERRADDR1 = 0x00000000
GPRTBIMAP0 = 0x00000000
GPRTBIMAP1 = 0x00000000
...

The value of GSBUSCFG0 is the same to what I dump in bootloader(U-boot)
Which mean that's the default value.

Regards,
Ran

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

[Felipe Balbi]

Hi,

Ran Wang <ran.wang_1@nxp.com> writes:

> Hi Felipe,
>
>     Sorry for the late reply:
>
> On Wednesday, November 15, 2017 18:23, Felipe Balbi wrote:

that's 1.5 year ago. I really don't remember the details of this conversation

>> Ran Wang <ran.wang_1@nxp.com> writes:
>> >> Ran Wang <ran.wang_1@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.
>> 
>> IOW, you have no idea why you need this, right? We're not patching things for
>> the sake of patching things. We need to understand what these changes mean
>> to the HW before we send out a patch publicly.
>> 
>> Remember that the moment a patch like this is accepted, it has the potential of
>> changing behavior for *ALL* users.
>> 
>> >> > +	}
>> >> > +
>> >> > +	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.
>> 
>> they all *have* the register, however, it's sort of expected that RTL engineer will
>> setup good defaults when instantiating the RTL using SNPS'
>> coreConsultant tool.
>> 
>> Does your platform work without this patch?
>
> On Layerscape SoC (such as LS1088A, LS1046A, LS1043A) When I add 'dma-coherent'
> to USB nodes without this patch, dwc3 will fail on device enumeration as below:
> [    3.610620] xhci-hcd xhci-hcd.2.auto: WARNING: Host System Error
> [    3.630609] usb usb2-port1: couldn't allocate usb_device

Right, and same as before: are these devices in the market or are you
dealing with pre-silicon prototypes?

>> >> >  /* 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.
>
> According to the DesignWare Cores SuperSpeed USB 3.0 Controller Databook (v2.60a),
> it has described Type Bit Assignments for all supported master bus type:
> AHB, AXI3, AXI4 and Native. I found the bit definition are different among them.
> So, for the example you gave above, feel a little bit confused. 
> Did you mean:
>     snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read allocate">

yeah, something like that.

>> > Got it, learn a lot, need more time to digest and test, thanks for
>> > your patiently explanation.
>> 
>> no problem, please figure out the answers to my previous questions, without
>> which I can't accept your patch.

^^^

I still don't have all the answers

-- 
balbi

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

[Ran Wang]

HI Felipe,

On Tuesday, May 28, 2019 18:20, Felipe Balbi wrote:
> 
> Hi,
> 
> Ran Wang <ran.wang_1@nxp.com> writes:
> 
> > Hi Felipe,
> >
> >     Sorry for the late reply:
> >
> > On Wednesday, November 15, 2017 18:23, Felipe Balbi wrote:
> 
> that's 1.5 year ago. I really don't remember the details of this conversation
> 
> >> Ran Wang <ran.wang_1@nxp.com> writes:
> >> >> Ran Wang <ran.wang_1@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?

I think I have answered this as blow: to fix issue we found.

> >> >> b) WHY do we need another DT property for this?

I agreed with your suggestion of using ' snps,cache-type = <foobar "cacheable">, ...'

> >> >> c) WHAT does this mean for PCI devices?

According to DWC3 data book, I think this (PCI) mean to the case of 'master bus type = Native'
The data book describes this feature as 'system bus DMA option for the master bus,
which may be configured as AHB, AXI, or Native.' On Table 6-5, it says when MBUS_TYPE
is Native, the definition of 4 transfer types control bits [3-0] is 'Same as AXI'.

However, as to the code implementation to be generic to both PCI and AXI,
I admit I don't have a perfect solution so far, only 2 proposals with concerns:

a. Create another module driver like dwc3-exynos.c (arch/arm/boot/dts/wxynos54xx.dtsi)
    to contain above programming code. However, it will touch the same reg range of DWC3
    I think this is not good.

b. Add #ifdef CONFIG_ARCH_LAYERSCAPE in drivers/usb/dwc3/core.c to constrain hacking code
   can only take effect for Layerscape (AXI case). I know it look ugly.

Do you have any better advice on this (besides changed power on default value from HW perspective)?

> >> >
> >> > So far I cannot have the answer for you, will get you back after
> >> > some discussion with my colleagues.
> >>
> >> IOW, you have no idea why you need this, right? We're not patching
> >> things for the sake of patching things. We need to understand what
> >> these changes mean to the HW before we send out a patch publicly.
> >>
> >> Remember that the moment a patch like this is accepted, it has the
> >> potential of changing behavior for *ALL* users.
> >>
> >> >> > +	}
> >> >> > +
> >> >> > +	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

Yes, noted.

> >> >>
> >> >> Also, I think dma_coherent is likely not the best name for this property.

OK

> >> >>
> >> >> 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?

Several Layerscape platforms like LS1043ARDB, LS1046ARDB, etc. are already on
the market and have this issue. So I have to work out a SW patch to fix them.

> >> >
> >> > Yes, you are right. Actually I thought that all dwc3 IP  will have
> >> > this register, and it can be controlled by DTS property.
> >>
> >> they all *have* the register, however, it's sort of expected that RTL
> >> engineer will setup good defaults when instantiating the RTL using SNPS'
> >> coreConsultant tool.
> >>
> >> Does your platform work without this patch?
> >
> > On Layerscape SoC (such as LS1088A, LS1046A, LS1043A) When I add 'dma-
> coherent'
> > to USB nodes without this patch, dwc3 will fail on device enumeration as
> below:
> > [    3.610620] xhci-hcd xhci-hcd.2.auto: WARNING: Host System Error
> > [    3.630609] usb usb2-port1: couldn't allocate usb_device
> 
> Right, and same as before: are these devices in the market or are you dealing
> with pre-silicon prototypes?

Already in the market, need SW fix.

> >> >> >  /* 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.
> >
> > According to the DesignWare Cores SuperSpeed USB 3.0 Controller
> > Databook (v2.60a), it has described Type Bit Assignments for all supported
> master bus type:
> > AHB, AXI3, AXI4 and Native. I found the bit definition are different among
> them.
> > So, for the example you gave above, feel a little bit confused.
> > Did you mean:
> >     snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD
> > "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read allocate">
> 
> yeah, something like that.
> 
> >> > Got it, learn a lot, need more time to digest and test, thanks for
> >> > your patiently explanation.
> >>
> >> no problem, please figure out the answers to my previous questions,
> >> without which I can't accept your patch.
> 
> ^^^
> 
> I still don't have all the answers

If I still missed any question, please let me know, thanks for your time.

Regards,
Ran

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

[Felipe Balbi]

Hi,

Ran Wang <ran.wang_1@nxp.com> writes:
>> >> >> c) WHAT does this mean for PCI devices?
>
> According to DWC3 data book, I think this (PCI) mean to the case of 'master bus type = Native'
> The data book describes this feature as 'system bus DMA option for the master bus,
> which may be configured as AHB, AXI, or Native.' On Table 6-5, it says when MBUS_TYPE
> is Native, the definition of 4 transfer types control bits [3-0] is 'Same as AXI'.
>
> However, as to the code implementation to be generic to both PCI and AXI,
> I admit I don't have a perfect solution so far, only 2 proposals with concerns:
>
> a. Create another module driver like dwc3-exynos.c (arch/arm/boot/dts/wxynos54xx.dtsi)
>     to contain above programming code. However, it will touch the same reg range of DWC3
>     I think this is not good.

I'd prefer avoiding another glue :-)

> b. Add #ifdef CONFIG_ARCH_LAYERSCAPE in drivers/usb/dwc3/core.c to constrain hacking code
>    can only take effect for Layerscape (AXI case). I know it look ugly.
>
> Do you have any better advice on this (besides changed power on default value from HW perspective)?

Maybe we don't need to care, actually. Since this property will only be
needed for RTL instantiation that didn't configure these defaults
properly during coreConsultant.

>> >> >> 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?
>
> Several Layerscape platforms like LS1043ARDB, LS1046ARDB, etc. are already on
> the market and have this issue. So I have to work out a SW patch to fix them.

Thank you, now I'm certain that this is not some temporary solution :-)

Thanks for going through this again. Please refresh the patch so we can
try to get it merged.

-- 
balbi

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

[Ran Wang]

Hi Felipe,

On Wednesday, May 29, 2019 18:25, Felipe Balbi wrote:
> 
> Hi,
> 
> Ran Wang <ran.wang_1@nxp.com> writes:
> >> >> >> c) WHAT does this mean for PCI devices?
> >
> > According to DWC3 data book, I think this (PCI) mean to the case of 'master
> bus type = Native'
> > The data book describes this feature as 'system bus DMA option for the
> > master bus, which may be configured as AHB, AXI, or Native.' On Table
> > 6-5, it says when MBUS_TYPE is Native, the definition of 4 transfer types
> control bits [3-0] is 'Same as AXI'.
> >
> > However, as to the code implementation to be generic to both PCI and
> > AXI, I admit I don't have a perfect solution so far, only 2 proposals with
> concerns:
> >
> > a. Create another module driver like dwc3-exynos.c
> (arch/arm/boot/dts/wxynos54xx.dtsi)
> >     to contain above programming code. However, it will touch the same reg
> range of DWC3
> >     I think this is not good.
> 
> I'd prefer avoiding another glue :-)

Got it.
 
> > b. Add #ifdef CONFIG_ARCH_LAYERSCAPE in drivers/usb/dwc3/core.c to
> constrain hacking code
> >    can only take effect for Layerscape (AXI case). I know it look ugly.
> >
> > Do you have any better advice on this (besides changed power on default
> value from HW perspective)?
> 
> Maybe we don't need to care, actually. Since this property will only be needed
> for RTL instantiation that didn't configure these defaults properly during
> coreConsultant.

Ok, I think I could add information in bindings to highlight this setting might
be RTL instantiation (SoC) relevant to prevent misusing.

> >> >> >> 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?
> >
> > Several Layerscape platforms like LS1043ARDB, LS1046ARDB, etc. are
> > already on the market and have this issue. So I have to work out a SW patch to
> fix them.
> 
> Thank you, now I'm certain that this is not some temporary solution :-)
> 
> Thanks for going through this again. Please refresh the patch so we can try to
> get it merged.

Sure, as I know all LS1043A and LS1046A relevant platforms have added 'dma-coherent'
to DTS node of 'soc' in mainline, which means without this fix USB function will down
definitely, I will go through all update requirement we've discussed and work out
next version patch. Thank you.

Regards,
Ran 

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

[Ran Wang]

Hi Felipe,

On Tuesday, May 28, 2019 18:20, Felipe Balbi wrote:
> 
<snip>
> >> >> >  /* 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.
> >
> > According to the DesignWare Cores SuperSpeed USB 3.0 Controller
> > Databook (v2.60a), it has described Type Bit Assignments for all supported
> master bus type:
> > AHB, AXI3, AXI4 and Native. I found the bit definition are different among
> them.
> > So, for the example you gave above, feel a little bit confused.
> > Did you mean:
> >     snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD
> > "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read allocate">
> 
> yeah, something like that.

I think DATA_RD  should be a macro, right? So, where I can put its define?
Create a dwc3.h in include/dt-bindings/usb/ ?

Another question about this remain open is: DWC3 data book's Table 6-5
Cache Type Bit Assignments show that bits definition will differ per
MBUS_TYPEs as below:
----------------------------------------------------------------                 
 MBUS_TYPE| bit[3]       |bit[2]       |bit[1]     |bit[0]                        
 ----------------------------------------------------------------                 
 AHB      |Cacheable     |Bufferable   |Privilegge |Data                          
 AXI3     |Write Allocate|Read Allocate|Cacheable  |Bufferable                    
 AXI4     |Allocate Other|Allocate     |Modifiable |Bufferable                    
 AXI4     |Other Allocate|Allocate     |Modifiable |Bufferable                    
 Native   |Same as AXI   |Same as AXI  |Same as AXI|Same as AXI                   
 ----------------------------------------------------------------                 
 Note: The AHB, AXI3, AXI4, and PCIe busses use different names for certain       
 signals, which have the same meaning:                                            
   Bufferable = Posted                                                            
   Cacheable = Modifiable = Snoop (negation of No Snoop)
 
For Layerscape SoCs, MBUS_TYPE is AXI3. So I am not sure how to use
snps,cache-type = <DATA_RD  "write allocate">, to cover all MBUS_TYPE?
(you can notice that AHB and AXI3's cacheable are on different bit)
Or I just need to handle AXI3 case?

Regards,
Ran

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

[Ran Wang]

Hi Felipe,

On Thursday, May 30, 2019 17:09, Ran Wang wrote:
> 
> <snip>
> > >> >> >  /* 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.
> > >
> > > According to the DesignWare Cores SuperSpeed USB 3.0 Controller
> > > Databook (v2.60a), it has described Type Bit Assignments for all
> > > supported
> > master bus type:
> > > AHB, AXI3, AXI4 and Native. I found the bit definition are different
> > > among
> > them.
> > > So, for the example you gave above, feel a little bit confused.
> > > Did you mean:
> > >     snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD
> > > "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read allocate">
> >
> > yeah, something like that.
> 
> I think DATA_RD  should be a macro, right? So, where I can put its define?
> Create a dwc3.h in include/dt-bindings/usb/ ?

Could you please give me some advice here? I'd like to prepare next version patch after
getting this settled.

> Another question about this remain open is: DWC3 data book's Table 6-5 Cache
> Type Bit Assignments show that bits definition will differ per MBUS_TYPEs as
> below:
> ----------------------------------------------------------------
>  MBUS_TYPE| bit[3]       |bit[2]       |bit[1]     |bit[0]
>  ----------------------------------------------------------------
>  AHB      |Cacheable     |Bufferable   |Privilegge |Data
>  AXI3     |Write Allocate|Read Allocate|Cacheable  |Bufferable
>  AXI4     |Allocate Other|Allocate     |Modifiable |Bufferable
>  AXI4     |Other Allocate|Allocate     |Modifiable |Bufferable
>  Native   |Same as AXI   |Same as AXI  |Same as AXI|Same as AXI
>  ----------------------------------------------------------------
>  Note: The AHB, AXI3, AXI4, and PCIe busses use different names for certain
>  signals, which have the same meaning:
>    Bufferable = Posted
>    Cacheable = Modifiable = Snoop (negation of No Snoop)
> 
> For Layerscape SoCs, MBUS_TYPE is AXI3. So I am not sure how to use
> snps,cache-type = <DATA_RD  "write allocate">, to cover all MBUS_TYPE?
> (you can notice that AHB and AXI3's cacheable are on different bit) Or I just need
> to handle AXI3 case?

Also on this open. Thank you in advance.

Regards,
Ran

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

[Felipe Balbi]

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


Hi,

Ran Wang <ran.wang_1@nxp.com> writes:
> Hi Felipe,
>
> On Thursday, May 30, 2019 17:09, Ran Wang wrote:
>> 
>> <snip>
>> > >> >> >  /* 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.
>> > >
>> > > According to the DesignWare Cores SuperSpeed USB 3.0 Controller
>> > > Databook (v2.60a), it has described Type Bit Assignments for all
>> > > supported
>> > master bus type:
>> > > AHB, AXI3, AXI4 and Native. I found the bit definition are different
>> > > among
>> > them.
>> > > So, for the example you gave above, feel a little bit confused.
>> > > Did you mean:
>> > >     snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD
>> > > "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read allocate">
>> >
>> > yeah, something like that.
>> 
>> I think DATA_RD  should be a macro, right? So, where I can put its define?
>> Create a dwc3.h in include/dt-bindings/usb/ ?
>
> Could you please give me some advice here? I'd like to prepare next version patch after
> getting this settled.
>
>> Another question about this remain open is: DWC3 data book's Table 6-5 Cache
>> Type Bit Assignments show that bits definition will differ per MBUS_TYPEs as
>> below:
>> ----------------------------------------------------------------
>>  MBUS_TYPE| bit[3]       |bit[2]       |bit[1]     |bit[0]
>>  ----------------------------------------------------------------
>>  AHB      |Cacheable     |Bufferable   |Privilegge |Data
>>  AXI3     |Write Allocate|Read Allocate|Cacheable  |Bufferable
>>  AXI4     |Allocate Other|Allocate     |Modifiable |Bufferable
>>  AXI4     |Other Allocate|Allocate     |Modifiable |Bufferable
>>  Native   |Same as AXI   |Same as AXI  |Same as AXI|Same as AXI
>>  ----------------------------------------------------------------
>>  Note: The AHB, AXI3, AXI4, and PCIe busses use different names for certain
>>  signals, which have the same meaning:
>>    Bufferable = Posted
>>    Cacheable = Modifiable = Snoop (negation of No Snoop)
>> 
>> For Layerscape SoCs, MBUS_TYPE is AXI3. So I am not sure how to use
>> snps,cache-type = <DATA_RD  "write allocate">, to cover all MBUS_TYPE?
>> (you can notice that AHB and AXI3's cacheable are on different bit) Or I just need
>> to handle AXI3 case?
>
> Also on this open. Thank you in advance.

You could pass two strings and let the driver process them. Something
like:

	snps,cache_type = <"data_wr" "write allocate">, <"desc_rd" "cacheable">...

And so on. The only thing missing is for the mbus_type to be known by
the driver. Is that something we can figure out on any of the HWPARAMS
registers or does it have to be told explicitly?

Another option would be to pass a string followed by one hex digit for
the bits:

	snps,cache_type = <"data_wr" 0x8>, <"desc_rd" 0x2>...;

Then we don't need to describe mbus_type since the bits are what matters.

Rob, any comments?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Ran Wang]

Hi Felipe,

On Monday, June 17, 2019 20:53, Felipe Balbi wrote:
> Hi,
> 
> Ran Wang <ran.wang_1@nxp.com> writes:
> > Hi Felipe,
> >
> > On Thursday, May 30, 2019 17:09, Ran Wang wrote:
> >>
> >> <snip>
> >> > >> >> >  /* 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.
> >> > >
> >> > > According to the DesignWare Cores SuperSpeed USB 3.0 Controller
> >> > > Databook (v2.60a), it has described Type Bit Assignments for all
> >> > > supported
> >> > master bus type:
> >> > > AHB, AXI3, AXI4 and Native. I found the bit definition are
> >> > > different among
> >> > them.
> >> > > So, for the example you gave above, feel a little bit confused.
> >> > > Did you mean:
> >> > >     snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD
> >> > > "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read allocate">
> >> >
> >> > yeah, something like that.
> >>
> >> I think DATA_RD  should be a macro, right? So, where I can put its define?
> >> Create a dwc3.h in include/dt-bindings/usb/ ?
> >
> > Could you please give me some advice here? I'd like to prepare next
> > version patch after getting this settled.
> >
> >> Another question about this remain open is: DWC3 data book's Table
> >> 6-5 Cache Type Bit Assignments show that bits definition will differ
> >> per MBUS_TYPEs as
> >> below:
> >> ----------------------------------------------------------------
> >>  MBUS_TYPE| bit[3]       |bit[2]       |bit[1]     |bit[0]
> >>  ----------------------------------------------------------------
> >>  AHB      |Cacheable     |Bufferable   |Privilegge |Data
> >>  AXI3     |Write Allocate|Read Allocate|Cacheable  |Bufferable
> >>  AXI4     |Allocate Other|Allocate     |Modifiable |Bufferable
> >>  AXI4     |Other Allocate|Allocate     |Modifiable |Bufferable
> >>  Native   |Same as AXI   |Same as AXI  |Same as AXI|Same as AXI
> >>  ----------------------------------------------------------------
> >>  Note: The AHB, AXI3, AXI4, and PCIe busses use different names for
> >> certain  signals, which have the same meaning:
> >>    Bufferable = Posted
> >>    Cacheable = Modifiable = Snoop (negation of No Snoop)
> >>
> >> For Layerscape SoCs, MBUS_TYPE is AXI3. So I am not sure how to use
> >> snps,cache-type = <DATA_RD  "write allocate">, to cover all MBUS_TYPE?
> >> (you can notice that AHB and AXI3's cacheable are on different bit)
> >> Or I just need to handle AXI3 case?
> >
> > Also on this open. Thank you in advance.
> 
> You could pass two strings and let the driver process them. Something
> like:
> 
> 	snps,cache_type = <"data_wr" "write allocate">, <"desc_rd"
> "cacheable">...
> 
> And so on. The only thing missing is for the mbus_type to be known by the driver.
> Is that something we can figure out on any of the HWPARAMS registers or does
> it have to be told explicitly?

I have checked Layerscape Reference manual, HWPARAMS0~8 doesn't contain mbus_type
Info, and I didn't know where have declared it explicitly.

> Another option would be to pass a string followed by one hex digit for the bits:
> 
> 	snps,cache_type = <"data_wr" 0x8>, <"desc_rd" 0x2>...;
> 
> Then we don't need to describe mbus_type since the bits are what matters.

Yes, it's also what we prefer to use, it will be more flexible, I can add above Table
6-5 Cache Type Bit Assignments in binding to help user decide which value they
would use.

I would submit another version of patch for further review, thank you very much.

Regards,
Ran

> Rob, any comments?
> 
> --
> balbi

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

[Felipe Balbi]

Hi,

Ran Wang <ran.wang_1@nxp.com> writes:
>> >> > >> >> >  /* 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.
>> >> > >
>> >> > > According to the DesignWare Cores SuperSpeed USB 3.0 Controller
>> >> > > Databook (v2.60a), it has described Type Bit Assignments for all
>> >> > > supported
>> >> > master bus type:
>> >> > > AHB, AXI3, AXI4 and Native. I found the bit definition are
>> >> > > different among
>> >> > them.
>> >> > > So, for the example you gave above, feel a little bit confused.
>> >> > > Did you mean:
>> >> > >     snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD
>> >> > > "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read allocate">
>> >> >
>> >> > yeah, something like that.
>> >>
>> >> I think DATA_RD  should be a macro, right? So, where I can put its define?
>> >> Create a dwc3.h in include/dt-bindings/usb/ ?
>> >
>> > Could you please give me some advice here? I'd like to prepare next
>> > version patch after getting this settled.
>> >
>> >> Another question about this remain open is: DWC3 data book's Table
>> >> 6-5 Cache Type Bit Assignments show that bits definition will differ
>> >> per MBUS_TYPEs as
>> >> below:
>> >> ----------------------------------------------------------------
>> >>  MBUS_TYPE| bit[3]       |bit[2]       |bit[1]     |bit[0]
>> >>  ----------------------------------------------------------------
>> >>  AHB      |Cacheable     |Bufferable   |Privilegge |Data
>> >>  AXI3     |Write Allocate|Read Allocate|Cacheable  |Bufferable
>> >>  AXI4     |Allocate Other|Allocate     |Modifiable |Bufferable
>> >>  AXI4     |Other Allocate|Allocate     |Modifiable |Bufferable
>> >>  Native   |Same as AXI   |Same as AXI  |Same as AXI|Same as AXI
>> >>  ----------------------------------------------------------------
>> >>  Note: The AHB, AXI3, AXI4, and PCIe busses use different names for
>> >> certain  signals, which have the same meaning:
>> >>    Bufferable = Posted
>> >>    Cacheable = Modifiable = Snoop (negation of No Snoop)
>> >>
>> >> For Layerscape SoCs, MBUS_TYPE is AXI3. So I am not sure how to use
>> >> snps,cache-type = <DATA_RD  "write allocate">, to cover all MBUS_TYPE?
>> >> (you can notice that AHB and AXI3's cacheable are on different bit)
>> >> Or I just need to handle AXI3 case?
>> >
>> > Also on this open. Thank you in advance.
>> 
>> You could pass two strings and let the driver process them. Something
>> like:
>> 
>> 	snps,cache_type = <"data_wr" "write allocate">, <"desc_rd"
>> "cacheable">...
>> 
>> And so on. The only thing missing is for the mbus_type to be known by the driver.
>> Is that something we can figure out on any of the HWPARAMS registers or does
>> it have to be told explicitly?
>
> I have checked Layerscape Reference manual, HWPARAMS0~8 doesn't contain mbus_type
> Info, and I didn't know where have declared it explicitly.
>
>> Another option would be to pass a string followed by one hex digit for the bits:
>> 
>> 	snps,cache_type = <"data_wr" 0x8>, <"desc_rd" 0x2>...;
>> 
>> Then we don't need to describe mbus_type since the bits are what matters.
>
> Yes, it's also what we prefer to use, it will be more flexible, I can add above Table
> 6-5 Cache Type Bit Assignments in binding to help user decide which value they
> would use.
>
> I would submit another version of patch for further review, thank you very much.

cool, thanks

-- 
balbi

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

[Ran Wang]

Hi Felipe,

On Monday, June 24, 2019 13:58, Felipe Balbi wrote:
> 
> Hi,
> 
> Ran Wang <ran.wang_1@nxp.com> writes:
> >> >> > >> >> >  /* 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.
> >> >> > >
> >> >> > > According to the DesignWare Cores SuperSpeed USB 3.0
> >> >> > > Controller Databook (v2.60a), it has described Type Bit
> >> >> > > Assignments for all supported
> >> >> > master bus type:
> >> >> > > AHB, AXI3, AXI4 and Native. I found the bit definition are
> >> >> > > different among
> >> >> > them.
> >> >> > > So, for the example you gave above, feel a little bit confused.
> >> >> > > Did you mean:
> >> >> > >     snps,cache-type = <DATA_RD  "write allocate">, <DESC_RD
> >> >> > > "cacheable">, <DATA_WR  "bufferable">, <DESC_WR  "read
> >> >> > > allocate">
> >> >> >
> >> >> > yeah, something like that.
> >> >>
> >> >> I think DATA_RD  should be a macro, right? So, where I can put its define?
> >> >> Create a dwc3.h in include/dt-bindings/usb/ ?
> >> >
> >> > Could you please give me some advice here? I'd like to prepare next
> >> > version patch after getting this settled.
> >> >
> >> >> Another question about this remain open is: DWC3 data book's Table
> >> >> 6-5 Cache Type Bit Assignments show that bits definition will
> >> >> differ per MBUS_TYPEs as
> >> >> below:
> >> >> ----------------------------------------------------------------
> >> >>  MBUS_TYPE| bit[3]       |bit[2]       |bit[1]     |bit[0]
> >> >>  ----------------------------------------------------------------
> >> >>  AHB      |Cacheable     |Bufferable   |Privilegge |Data
> >> >>  AXI3     |Write Allocate|Read Allocate|Cacheable  |Bufferable
> >> >>  AXI4     |Allocate Other|Allocate     |Modifiable |Bufferable
> >> >>  AXI4     |Other Allocate|Allocate     |Modifiable |Bufferable
> >> >>  Native   |Same as AXI   |Same as AXI  |Same as AXI|Same as AXI
> >> >>  ----------------------------------------------------------------
> >> >>  Note: The AHB, AXI3, AXI4, and PCIe busses use different names
> >> >> for certain  signals, which have the same meaning:
> >> >>    Bufferable = Posted
> >> >>    Cacheable = Modifiable = Snoop (negation of No Snoop)
> >> >>
> >> >> For Layerscape SoCs, MBUS_TYPE is AXI3. So I am not sure how to
> >> >> use snps,cache-type = <DATA_RD  "write allocate">, to cover all
> MBUS_TYPE?
> >> >> (you can notice that AHB and AXI3's cacheable are on different
> >> >> bit) Or I just need to handle AXI3 case?
> >> >
> >> > Also on this open. Thank you in advance.
> >>
> >> You could pass two strings and let the driver process them. Something
> >> like:
> >>
> >> 	snps,cache_type = <"data_wr" "write allocate">, <"desc_rd"
> >> "cacheable">...
> >>
> >> And so on. The only thing missing is for the mbus_type to be known by the
> driver.
> >> Is that something we can figure out on any of the HWPARAMS registers
> >> or does it have to be told explicitly?
> >
> > I have checked Layerscape Reference manual, HWPARAMS0~8 doesn't
> > contain mbus_type Info, and I didn't know where have declared it explicitly.
> >
> >> Another option would be to pass a string followed by one hex digit for the
> bits:
> >>
> >> 	snps,cache_type = <"data_wr" 0x8>, <"desc_rd" 0x2>...;
> >>
> >> Then we don't need to describe mbus_type since the bits are what matters.

For this option, looks like DTC doesn't allow form of <"data_wr" 0x8>, <"desc_rd" 0x2>... 
It will report error when compiling:

DTC     arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dtb
Error: arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi:383.23-24 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:294: recipe for target 'arch/arm64/boot/dts/freescale/fsl-ls1088a-qds.dtb' failed
make[2]: *** [arch/arm64/boot/dts/freescale/fsl-ls1088a-qds.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
Error: arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi:383.23-24 syntax error
FATAL ERROR: Unable to parse input tree

One of the solution I can figure out is to use macro to replace "data_wr", like below:
<DATA_WR 0x8>, <DESC_RD 0x2>...

However, it will require creating file in include/dt-bindings/usb/dwc3.h to
place macro definitions.

Or may I use:  "data_wr", <0x8>,  "desc_rd",  <0x2>... ?

Thanks & Regards,
Ran

> > Yes, it's also what we prefer to use, it will be more flexible, I can
> > add above Table
> > 6-5 Cache Type Bit Assignments in binding to help user decide which
> > value they would use.
> >
> > I would submit another version of patch for further review, thank you very
> much.
> 
> cool, thanks
> 
> --
> balbi