Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support

[Felipe Balbi]

Hi, 

John Youn  writes:
> On 11/14/2016 3:00 PM, John Youn wrote:
>> On 11/11/2016 3:12 PM, Christian Lamparter wrote:
>>> On Friday, November 11, 2016 2:20:42 PM CET John Youn wrote:
 On 11/11/2016 2:05 PM, Christian Lamparter wrote:
> On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
>> On 11/11/2016 12:59 PM, Christian Lamparter wrote:
>>> This patch adds support for the "amcc,usb-otg" device
>>> which is found in the PowerPC Canyonlands' dts.
>>>
>>> The device definition was added by:
>>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
>>> board")'
>>> but without any driver support as the dwc2 driver wasn't
>>> available at that time.
>>>
>>> Note: The system can't use the generic "snps,dwc2" compatible
>>> because of the special ahbcfg configuration. The default
>>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
>>> when the USB and SATA is used concurrently.
>>
>> I don't want to add any more of these param structures to the driver
>> unless really necessary. We're trying to remove usage of them in favor
>> of using auto-detected defaults and device properties to override
>> them.
> Ok, thanks. I think that would work. I've attached an updated patch.
> Can it be applied/queued now? Or do you want me to resent it later?
>
>> The AHB Burst is actually one of the ones we were going to do next
>> because our platform also doesn't work well with INCR4. In fact I'm
>> thinking of making the default INCR.
> Is that actually possible to change the default still? This would
> require to re-evaluate all existing archs/platforms that use 
> "snps,dwc2" for INCR16 compatibility. 

 INCR, not INCR16, but you're right, so we may not change it even
 though though INCR is usually the right choice over INCR4.
>>> What about making a device-tree property?
>> 
>> Yes, that's what I meant. I'll send a change for this shortly.
>> 
>>>
>>> Recommended properties:
>>>  - g-ahb-bursts : specifies the ahb bursts length, should be one of
>>>"single", "INCRx", "INCR4", "INCR8", or "INCR16". If not specified
>>>the safer but inefficient "INCR4" is used. The optimal setting is
>>>"INCRx".
>>>
>>> Would this work? If so, I can make a patch over the weekend.
 Anyways, with the binding, can't you just set the compatible string to
 snps,dwc2?
>>>
>>> Ah, let me explain. I had a discussion with Mark Rutland and Rob Herring
>>> a while back about device-tree bindings.
>>>
>>> They made it very clear to me, that they don't want any generic "catch all
>>> compatible" strings:
>>>
>>> "Bindings should be for hardware (either specific device models, or for
>>> classes), and not for Linux drivers. The latter is subject to arbitrary
>>> changes while the former is not, as old hardware continues to exist and
>>> does not change while drivers get completely reworked." [0]
>>>
>>> Furthermore, this is an existing binding in kernel's canyonlands.dts [1]
>>> and this binding can't be easily changed. Rob Herring explained this in
>>> the context of the "basic-mmio-gpio" patch [2] when I was editing the dts
>>> to make them work with the changes I made:
>>>
>>> "You can't remove the old drivers as they are needed to work with 
>>> old dtbs, so there is no gain.
>>>
>>> You would need to match on existing compatibles such as
>>> moxa,moxart-gpio and provide a match data struct that has all the info
>>> you are adding here (e.g. data register offset). Then additionally you
>>> could add "basic-mmio-gpio" (I would drop "basic" part) and the
>>> additional data associated with it. But it has to be new properties,
>>> not changing properties. Changing the reg values doesn't work."
>>>
>>> So, for this to work with the existing canyonlands.dts, I need to have
>>> the "amcc,dwc-otg" compatible string.
>> 
>> Ok, if that's the case. But still a bit confused as to what driver was
>> working with it before since the binding was not defined for dwc2.
>> 
>>>
>>> Of course, it would be great to hear from Rob Herring and/or Mark Rutland
>>> about this case.
>>>
>>> Regards,
>>> Christian
>>>
>>> [0] 
>>> [1] 
>>> 
>>> [2] 
>>>
>>>  
>
> From what I can tell based would be:
> bcm11351, bcm21664, bcm23550, exynos3250, stm32f429, rk3xxx,
> stratix10, meson-gxbb, rt3050 and some Altera FPGAs.
>
>> If that's all you need then a devicetree binding should be enough
>> right?
> Yes. The device is working fine so far.
>
> Regards,
> Christian
>
> ---
> From 70dd4be016b89655a56bc8260f04683b50f07644 Mon Sep 17 00:00:00 2001
> From: Christian Lamparter 
> Date: Sun, 6 Nov 2016 00:39:24 

Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support

[John Youn]

On 11/14/2016 3:00 PM, John Youn wrote:
> On 11/11/2016 3:12 PM, Christian Lamparter wrote:
>> On Friday, November 11, 2016 2:20:42 PM CET John Youn wrote:
>>> On 11/11/2016 2:05 PM, Christian Lamparter wrote:
 On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
> On 11/11/2016 12:59 PM, Christian Lamparter wrote:
>> This patch adds support for the "amcc,usb-otg" device
>> which is found in the PowerPC Canyonlands' dts.
>>
>> The device definition was added by:
>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
>> board")'
>> but without any driver support as the dwc2 driver wasn't
>> available at that time.
>>
>> Note: The system can't use the generic "snps,dwc2" compatible
>> because of the special ahbcfg configuration. The default
>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
>> when the USB and SATA is used concurrently.
>
> I don't want to add any more of these param structures to the driver
> unless really necessary. We're trying to remove usage of them in favor
> of using auto-detected defaults and device properties to override
> them.
 Ok, thanks. I think that would work. I've attached an updated patch.
 Can it be applied/queued now? Or do you want me to resent it later?

> The AHB Burst is actually one of the ones we were going to do next
> because our platform also doesn't work well with INCR4. In fact I'm
> thinking of making the default INCR.
 Is that actually possible to change the default still? This would
 require to re-evaluate all existing archs/platforms that use 
 "snps,dwc2" for INCR16 compatibility. 
>>>
>>> INCR, not INCR16, but you're right, so we may not change it even
>>> though though INCR is usually the right choice over INCR4.
>> What about making a device-tree property?
> 
> Yes, that's what I meant. I'll send a change for this shortly.
> 
>>
>> Recommended properties:
>>  - g-ahb-bursts : specifies the ahb bursts length, should be one of
>>"single", "INCRx", "INCR4", "INCR8", or "INCR16". If not specified
>>the safer but inefficient "INCR4" is used. The optimal setting is
>>"INCRx".
>>
>> Would this work? If so, I can make a patch over the weekend.
>>> Anyways, with the binding, can't you just set the compatible string to
>>> snps,dwc2?
>>
>> Ah, let me explain. I had a discussion with Mark Rutland and Rob Herring
>> a while back about device-tree bindings.
>>
>> They made it very clear to me, that they don't want any generic "catch all
>> compatible" strings:
>>
>> "Bindings should be for hardware (either specific device models, or for
>> classes), and not for Linux drivers. The latter is subject to arbitrary
>> changes while the former is not, as old hardware continues to exist and
>> does not change while drivers get completely reworked." [0]
>>
>> Furthermore, this is an existing binding in kernel's canyonlands.dts [1]
>> and this binding can't be easily changed. Rob Herring explained this in
>> the context of the "basic-mmio-gpio" patch [2] when I was editing the dts
>> to make them work with the changes I made:
>>
>> "You can't remove the old drivers as they are needed to work with 
>> old dtbs, so there is no gain.
>>
>> You would need to match on existing compatibles such as
>> moxa,moxart-gpio and provide a match data struct that has all the info
>> you are adding here (e.g. data register offset). Then additionally you
>> could add "basic-mmio-gpio" (I would drop "basic" part) and the
>> additional data associated with it. But it has to be new properties,
>> not changing properties. Changing the reg values doesn't work."
>>
>> So, for this to work with the existing canyonlands.dts, I need to have
>> the "amcc,dwc-otg" compatible string.
> 
> Ok, if that's the case. But still a bit confused as to what driver was
> working with it before since the binding was not defined for dwc2.
> 
>>
>> Of course, it would be great to hear from Rob Herring and/or Mark Rutland
>> about this case.
>>
>> Regards,
>> Christian
>>
>> [0] 
>> [1] 
>> 
>> [2] 
>>
>>  

 From what I can tell based would be:
 bcm11351, bcm21664, bcm23550, exynos3250, stm32f429, rk3xxx,
 stratix10, meson-gxbb, rt3050 and some Altera FPGAs.

> If that's all you need then a devicetree binding should be enough
> right?
 Yes. The device is working fine so far.

 Regards,
 Christian

 ---
 From 70dd4be016b89655a56bc8260f04683b50f07644 Mon Sep 17 00:00:00 2001
 From: Christian Lamparter 
 Date: Sun, 6 Nov 2016 00:39:24 +0100
 Subject: [PATCH] usb: dwc2: add amcc,dwc-otg support

 This patch adds support for the "amcc,usb-otg" device
 which is found in the PowerPC 

Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support

[John Youn]

On 11/11/2016 3:12 PM, Christian Lamparter wrote:
> On Friday, November 11, 2016 2:20:42 PM CET John Youn wrote:
>> On 11/11/2016 2:05 PM, Christian Lamparter wrote:
>>> On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
 On 11/11/2016 12:59 PM, Christian Lamparter wrote:
> This patch adds support for the "amcc,usb-otg" device
> which is found in the PowerPC Canyonlands' dts.
>
> The device definition was added by:
> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
> board")'
> but without any driver support as the dwc2 driver wasn't
> available at that time.
>
> Note: The system can't use the generic "snps,dwc2" compatible
> because of the special ahbcfg configuration. The default
> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
> when the USB and SATA is used concurrently.

 I don't want to add any more of these param structures to the driver
 unless really necessary. We're trying to remove usage of them in favor
 of using auto-detected defaults and device properties to override
 them.
>>> Ok, thanks. I think that would work. I've attached an updated patch.
>>> Can it be applied/queued now? Or do you want me to resent it later?
>>>
 The AHB Burst is actually one of the ones we were going to do next
 because our platform also doesn't work well with INCR4. In fact I'm
 thinking of making the default INCR.
>>> Is that actually possible to change the default still? This would
>>> require to re-evaluate all existing archs/platforms that use 
>>> "snps,dwc2" for INCR16 compatibility. 
>>
>> INCR, not INCR16, but you're right, so we may not change it even
>> though though INCR is usually the right choice over INCR4.
> What about making a device-tree property?

Yes, that's what I meant. I'll send a change for this shortly.

> 
> Recommended properties:
>  - g-ahb-bursts : specifies the ahb bursts length, should be one of
>"single", "INCRx", "INCR4", "INCR8", or "INCR16". If not specified
>the safer but inefficient "INCR4" is used. The optimal setting is
>"INCRx".
> 
> Would this work? If so, I can make a patch over the weekend.
>> Anyways, with the binding, can't you just set the compatible string to
>> snps,dwc2?
> 
> Ah, let me explain. I had a discussion with Mark Rutland and Rob Herring
> a while back about device-tree bindings.
> 
> They made it very clear to me, that they don't want any generic "catch all
> compatible" strings:
> 
> "Bindings should be for hardware (either specific device models, or for
> classes), and not for Linux drivers. The latter is subject to arbitrary
> changes while the former is not, as old hardware continues to exist and
> does not change while drivers get completely reworked." [0]
> 
> Furthermore, this is an existing binding in kernel's canyonlands.dts [1]
> and this binding can't be easily changed. Rob Herring explained this in
> the context of the "basic-mmio-gpio" patch [2] when I was editing the dts
> to make them work with the changes I made:
> 
> "You can't remove the old drivers as they are needed to work with 
> old dtbs, so there is no gain.
> 
> You would need to match on existing compatibles such as
> moxa,moxart-gpio and provide a match data struct that has all the info
> you are adding here (e.g. data register offset). Then additionally you
> could add "basic-mmio-gpio" (I would drop "basic" part) and the
> additional data associated with it. But it has to be new properties,
> not changing properties. Changing the reg values doesn't work."
> 
> So, for this to work with the existing canyonlands.dts, I need to have
> the "amcc,dwc-otg" compatible string.

Ok, if that's the case. But still a bit confused as to what driver was
working with it before since the binding was not defined for dwc2.

> 
> Of course, it would be great to hear from Rob Herring and/or Mark Rutland
> about this case.
> 
> Regards,
> Christian
> 
> [0] 
> [1] 
> 
> [2] 
> 
>  
>>>
>>> From what I can tell based would be:
>>> bcm11351, bcm21664, bcm23550, exynos3250, stm32f429, rk3xxx,
>>> stratix10, meson-gxbb, rt3050 and some Altera FPGAs.
>>>
 If that's all you need then a devicetree binding should be enough
 right?
>>> Yes. The device is working fine so far.
>>>
>>> Regards,
>>> Christian
>>>
>>> ---
>>> From 70dd4be016b89655a56bc8260f04683b50f07644 Mon Sep 17 00:00:00 2001
>>> From: Christian Lamparter 
>>> Date: Sun, 6 Nov 2016 00:39:24 +0100
>>> Subject: [PATCH] usb: dwc2: add amcc,dwc-otg support
>>>
>>> This patch adds support for the "amcc,usb-otg" device
>>> which is found in the PowerPC Canyonlands' dts.
>>>
>>> The device definition was added by:
>>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
>>> 

Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support

[Christian Lamparter]

On Friday, November 11, 2016 2:20:42 PM CET John Youn wrote:
> On 11/11/2016 2:05 PM, Christian Lamparter wrote:
> > On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
> >> On 11/11/2016 12:59 PM, Christian Lamparter wrote:
> >>> This patch adds support for the "amcc,usb-otg" device
> >>> which is found in the PowerPC Canyonlands' dts.
> >>>
> >>> The device definition was added by:
> >>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
> >>> board")'
> >>> but without any driver support as the dwc2 driver wasn't
> >>> available at that time.
> >>>
> >>> Note: The system can't use the generic "snps,dwc2" compatible
> >>> because of the special ahbcfg configuration. The default
> >>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
> >>> when the USB and SATA is used concurrently.
> >>
> >> I don't want to add any more of these param structures to the driver
> >> unless really necessary. We're trying to remove usage of them in favor
> >> of using auto-detected defaults and device properties to override
> >> them.
> > Ok, thanks. I think that would work. I've attached an updated patch.
> > Can it be applied/queued now? Or do you want me to resent it later?
> > 
> >> The AHB Burst is actually one of the ones we were going to do next
> >> because our platform also doesn't work well with INCR4. In fact I'm
> >> thinking of making the default INCR.
> > Is that actually possible to change the default still? This would
> > require to re-evaluate all existing archs/platforms that use 
> > "snps,dwc2" for INCR16 compatibility. 
> 
> INCR, not INCR16, but you're right, so we may not change it even
> though though INCR is usually the right choice over INCR4.
What about making a device-tree property?

Recommended properties:
 - g-ahb-bursts : specifies the ahb bursts length, should be one of
   "single", "INCRx", "INCR4", "INCR8", or "INCR16". If not specified
   the safer but inefficient "INCR4" is used. The optimal setting is
   "INCRx".

Would this work? If so, I can make a patch over the weekend.
> Anyways, with the binding, can't you just set the compatible string to
> snps,dwc2?

Ah, let me explain. I had a discussion with Mark Rutland and Rob Herring
a while back about device-tree bindings.

They made it very clear to me, that they don't want any generic "catch all
compatible" strings:

"Bindings should be for hardware (either specific device models, or for
classes), and not for Linux drivers. The latter is subject to arbitrary
changes while the former is not, as old hardware continues to exist and
does not change while drivers get completely reworked." [0]

Furthermore, this is an existing binding in kernel's canyonlands.dts [1]
and this binding can't be easily changed. Rob Herring explained this in
the context of the "basic-mmio-gpio" patch [2] when I was editing the dts
to make them work with the changes I made:

"You can't remove the old drivers as they are needed to work with 
old dtbs, so there is no gain.

You would need to match on existing compatibles such as
moxa,moxart-gpio and provide a match data struct that has all the info
you are adding here (e.g. data register offset). Then additionally you
could add "basic-mmio-gpio" (I would drop "basic" part) and the
additional data associated with it. But it has to be new properties,
not changing properties. Changing the reg values doesn't work."

So, for this to work with the existing canyonlands.dts, I need to have
the "amcc,dwc-otg" compatible string.

Of course, it would be great to hear from Rob Herring and/or Mark Rutland
about this case.

Regards,
Christian

[0] 
[1] 

[2] 

 
> > 
> > From what I can tell based would be:
> > bcm11351, bcm21664, bcm23550, exynos3250, stm32f429, rk3xxx,
> > stratix10, meson-gxbb, rt3050 and some Altera FPGAs.
> > 
> >> If that's all you need then a devicetree binding should be enough
> >> right?
> > Yes. The device is working fine so far.
> > 
> > Regards,
> > Christian
> > 
> > ---
> > From 70dd4be016b89655a56bc8260f04683b50f07644 Mon Sep 17 00:00:00 2001
> > From: Christian Lamparter 
> > Date: Sun, 6 Nov 2016 00:39:24 +0100
> > Subject: [PATCH] usb: dwc2: add amcc,dwc-otg support
> > 
> > This patch adds support for the "amcc,usb-otg" device
> > which is found in the PowerPC Canyonlands' dts.
> > 
> > The device definition was added by:
> > commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
> > board")'
> > but without any driver support as the dwc2 driver wasn't
> > available at that time.
> > 
> > Note: The system can't use the generic "snps,dwc2" compatible
> > because of the special ahbcfg configuration. The default
> > GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
> > when the USB and SATA is used concurrently.
> > 
> > Cc: Felipe 

Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support

[John Youn]

On 11/11/2016 2:05 PM, Christian Lamparter wrote:
> Hello,
> 
> On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
>> On 11/11/2016 12:59 PM, Christian Lamparter wrote:
>>> This patch adds support for the "amcc,usb-otg" device
>>> which is found in the PowerPC Canyonlands' dts.
>>>
>>> The device definition was added by:
>>> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
>>> board")'
>>> but without any driver support as the dwc2 driver wasn't
>>> available at that time.
>>>
>>> Note: The system can't use the generic "snps,dwc2" compatible
>>> because of the special ahbcfg configuration. The default
>>> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
>>> when the USB and SATA is used concurrently.
>>
>> I don't want to add any more of these param structures to the driver
>> unless really necessary. We're trying to remove usage of them in favor
>> of using auto-detected defaults and device properties to override
>> them.
> Ok, thanks. I think that would work. I've attached an updated patch.
> Can it be applied/queued now? Or do you want me to resent it later?
> 
>> The AHB Burst is actually one of the ones we were going to do next
>> because our platform also doesn't work well with INCR4. In fact I'm
>> thinking of making the default INCR.
> Is that actually possible to change the default still? This would
> require to re-evaluate all existing archs/platforms that use 
> "snps,dwc2" for INCR16 compatibility. 

INCR, not INCR16, but you're right, so we may not change it even
though though INCR is usually the right choice over INCR4.

Anyways, with the binding, can't you just set the compatible string to
snps,dwc2?

Regards,
John


> 
> From what I can tell based would be:
> bcm11351, bcm21664, bcm23550, exynos3250, stm32f429, rk3xxx,
> stratix10, meson-gxbb, rt3050 and some Altera FPGAs.
> 
>> If that's all you need then a devicetree binding should be enough
>> right?
> Yes. The device is working fine so far.
> 
> Regards,
> Christian
> 
> ---
> From 70dd4be016b89655a56bc8260f04683b50f07644 Mon Sep 17 00:00:00 2001
> From: Christian Lamparter 
> Date: Sun, 6 Nov 2016 00:39:24 +0100
> Subject: [PATCH] usb: dwc2: add amcc,dwc-otg support
> 
> This patch adds support for the "amcc,usb-otg" device
> which is found in the PowerPC Canyonlands' dts.
> 
> The device definition was added by:
> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
> board")'
> but without any driver support as the dwc2 driver wasn't
> available at that time.
> 
> Note: The system can't use the generic "snps,dwc2" compatible
> because of the special ahbcfg configuration. The default
> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
> when the USB and SATA is used concurrently.
> 
> Cc: Felipe Balbi 
> Cc: John Youn 
> Signed-off-by: Christian Lamparter 
> ---
> v1->v2:
>   - moved definitons to params.c
>   - removed dma_enable / host_dma parameter
>   - added dma_desc_fs_enable parameter
> v2->v3:
>   - removed parameters
> 
> Please queue this patch until GAHBCFG_HBSTLEN_INCR16 is the default
> for ahbcfg.
> ---
>  Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
>  drivers/usb/dwc2/params.c  | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt 
> b/Documentation/devicetree/bindings/usb/dwc2.txt
> index 10a2a4b..6ccfe85 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> @@ -12,6 +12,7 @@ Required properties:
>- "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
>- "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlogic 
> Meson8b SoCs;
>- "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic 
> S905 SoCs;
> +  - "amcc,dwc-otg": The DWC2 USB controller instance in AMCC Canyonlands 
> 460EX SoCs;
>- snps,dwc2: A generic DWC2 USB controller with default parameters.
>  - reg : Should contain 1 register range (address and length)
>  - interrupts : Should contain 1 interrupt
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 64d5c66..9506ab0 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -239,6 +239,7 @@ const struct of_device_id dwc2_of_match_table[] = {
>   { .compatible = "samsung,s3c6400-hsotg", .data = NULL},
>   { .compatible = "amlogic,meson8b-usb", .data = _amlogic },
>   { .compatible = "amlogic,meson-gxbb-usb", .data = _amlogic },
> + { .compatible = "amcc,dwc-otg", .data = NULL },
>   {},
>  };
>  MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support

[Christian Lamparter]

Hello,

On Friday, November 11, 2016 1:22:16 PM CET John Youn wrote:
> On 11/11/2016 12:59 PM, Christian Lamparter wrote:
> > This patch adds support for the "amcc,usb-otg" device
> > which is found in the PowerPC Canyonlands' dts.
> > 
> > The device definition was added by:
> > commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
> > board")'
> > but without any driver support as the dwc2 driver wasn't
> > available at that time.
> > 
> > Note: The system can't use the generic "snps,dwc2" compatible
> > because of the special ahbcfg configuration. The default
> > GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
> > when the USB and SATA is used concurrently.
> 
> I don't want to add any more of these param structures to the driver
> unless really necessary. We're trying to remove usage of them in favor
> of using auto-detected defaults and device properties to override
> them.
Ok, thanks. I think that would work. I've attached an updated patch.
Can it be applied/queued now? Or do you want me to resent it later?

> The AHB Burst is actually one of the ones we were going to do next
> because our platform also doesn't work well with INCR4. In fact I'm
> thinking of making the default INCR.
Is that actually possible to change the default still? This would
require to re-evaluate all existing archs/platforms that use 
"snps,dwc2" for INCR16 compatibility. 

>From what I can tell based would be:
bcm11351, bcm21664, bcm23550, exynos3250, stm32f429, rk3xxx,
stratix10, meson-gxbb, rt3050 and some Altera FPGAs.

> If that's all you need then a devicetree binding should be enough
> right?
Yes. The device is working fine so far.

Regards,
Christian

---
>From 70dd4be016b89655a56bc8260f04683b50f07644 Mon Sep 17 00:00:00 2001
From: Christian Lamparter 
Date: Sun, 6 Nov 2016 00:39:24 +0100
Subject: [PATCH] usb: dwc2: add amcc,dwc-otg support

This patch adds support for the "amcc,usb-otg" device
which is found in the PowerPC Canyonlands' dts.

The device definition was added by:
commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands board")'
but without any driver support as the dwc2 driver wasn't
available at that time.

Note: The system can't use the generic "snps,dwc2" compatible
because of the special ahbcfg configuration. The default
GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
when the USB and SATA is used concurrently.

Cc: Felipe Balbi 
Cc: John Youn 
Signed-off-by: Christian Lamparter 
---
v1->v2:
- moved definitons to params.c
- removed dma_enable / host_dma parameter
- added dma_desc_fs_enable parameter
v2->v3:
- removed parameters

Please queue this patch until GAHBCFG_HBSTLEN_INCR16 is the default
for ahbcfg.
---
 Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
 drivers/usb/dwc2/params.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt 
b/Documentation/devicetree/bindings/usb/dwc2.txt
index 10a2a4b..6ccfe85 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -12,6 +12,7 @@ Required properties:
   - "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
   - "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlogic Meson8b 
SoCs;
   - "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic S905 
SoCs;
+  - "amcc,dwc-otg": The DWC2 USB controller instance in AMCC Canyonlands 460EX 
SoCs;
   - snps,dwc2: A generic DWC2 USB controller with default parameters.
 - reg : Should contain 1 register range (address and length)
 - interrupts : Should contain 1 interrupt
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 64d5c66..9506ab0 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -239,6 +239,7 @@ const struct of_device_id dwc2_of_match_table[] = {
{ .compatible = "samsung,s3c6400-hsotg", .data = NULL},
{ .compatible = "amlogic,meson8b-usb", .data = _amlogic },
{ .compatible = "amlogic,meson-gxbb-usb", .data = _amlogic },
+   { .compatible = "amcc,dwc-otg", .data = NULL },
{},
 };
 MODULE_DEVICE_TABLE(of, dwc2_of_match_table);
-- 
2.10.2


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: dwc2: add amcc,dwc-otg support

[John Youn]

On 11/11/2016 12:59 PM, Christian Lamparter wrote:
> This patch adds support for the "amcc,usb-otg" device
> which is found in the PowerPC Canyonlands' dts.
> 
> The device definition was added by:
> commit c89b3458d8cc ("powerpc/44x: Add USB DWC DTS entry to Canyonlands 
> board")'
> but without any driver support as the dwc2 driver wasn't
> available at that time.
> 
> Note: The system can't use the generic "snps,dwc2" compatible
> because of the special ahbcfg configuration. The default
> GAHBCFG_HBSTLEN_INCR4 of snps,dwc2 can cause a system hang
> when the USB and SATA is used concurrently.

Hi,

I don't want to add any more of these param structures to the driver
unless really necessary. We're trying to remove usage of them in favor
of using auto-detected defaults and device properties to override
them.

The AHB Burst is actually one of the ones we were going to do next
because our platform also doesn't work well with INCR4. In fact I'm
thinking of making the default INCR.

If that's all you need then a devicetree binding should be enough
right?

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html