Re: [PATCH v2 2/4] usb: dwc2: Add binding for AHB burst

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

 




On 11/18/2016 12:18 PM, Christian Lamparter wrote:
> On Friday, November 18, 2016 8:16:08 AM CET Rob Herring wrote:
>> On Thu, Nov 17, 2016 at 04:35:10PM +0100, Stefan Wahren wrote:
>>> Hi John,
>>>
>>> Am 17.11.2016 um 00:47 schrieb John Youn:
>>>> Add the "snps,ahb-burst" binding and read it in.
>>>>
>>>> This property controls which burst type to perform on the AHB bus as a
>>>> master in internal DMA mode. This overrides the legacy param value,
>>>> which we need to keep around for now since several platforms use it.
>>>>
>>>> Some platforms may see better or worse performance based on this
>>>> value. The HAPS platform is one example where all INCRx have worse
>>>> performance than INCR.
>>>>
>>>> Other platforms (such as the Canyonlands board) report that the default
>>>> value causes system hangs.
>>>>
>>>> Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx>
>>>> Cc: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx>
>>>> ---
>>>>  Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
>>>>  drivers/usb/dwc2/core.h                        |  9 +++++
>>>>  drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
>>>>  3 files changed, 67 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
>>>> index 6c7c2bce..9e7b4b4 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
>>>
>>> according to Documentation/devicetree/bindings/submitting-patches.txt
>>> this change should be a separate patch.
>>>
>>>> @@ -26,6 +26,8 @@ Optional properties:
>>>>  Refer to phy/phy-bindings.txt for generic phy consumer properties
>>>>  - dr_mode: shall be one of "host", "peripheral" and "otg"
>>>>    Refer to usb/generic.txt
>>>> +- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
>>>> +  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".
>>>
>>> This doesn't apply in case of the bcm2835. I would prefer this option is
>>> ignored in that case with a dev_warn("snps,ahb-burst is not supported on
>>> this platform").
>>
>> Also, perhaps you should allow that the compatible string can define the 
>> default.
>>
> I hoped you would say that :).
> 
> I've attached a patch (on top of John Youn changes) that does
> just that for the amcc,dwc-otg. I put the GAHBCFG_HBSTLEN_INCR
> value into the .data, if that's a problem, I can certainly 
> respin the patch and put it in a dedicated struct.
> 
> Regards
> 
> Christian
> ---
> From 4c31a029dde714828810b1c3e61a5b1412ac939a Mon Sep 17 00:00:00 2001
> From: Christian Lamparter <chunkeey@xxxxxxxxx>
> Date: Fri, 18 Nov 2016 21:03:19 +0100
> Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg
> 
> This patch adds a of_device_id table which can be used by
> existing devices to supply a ahb-burst value for the platform
> without having to add a "snps,ahb-burst" entry to the dts.
> 
> Note: Adding new devices to this table is discouraged.
>       please consider adding the "snps,ahb-burst" property
>       with the correct configuration to your device tree
>       file instead.
> 
> Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxx>
> ---
>  drivers/usb/dwc2/params.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index e0fc9aa..51be266 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
>  	[GAHBCFG_HBSTLEN_INCR16]	= "INCR16",
>  };
>  
> +/*
> + * This table provides AHB burst configuration for existing
> + * device tree bindings that work poorly with the default setting.
> + *
> + * Note: Adding new devices to this table is discouraged.
> + *	 please consider adding the "snps,ahb-burst" property
> + *	 with the correct configuration to your device tree
> + *	 file instead.
> + */
> +static const struct of_device_id dwc2_compat_ahb_bursts[] = {
> +	{
> +		.compatible = "amcc,dwc-otg",
> +		.data = (void *) GAHBCFG_HBSTLEN_INCR16,
> +	},
> +};
> +
>  static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
>  {
>  	struct device_node *node = hsotg->dev->of_node;
> @@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
>  	ret = device_property_read_string(hsotg->dev,
>  					  "snps,ahb-burst", &str);
>  	if (ret < 0) {
> +		const struct of_device_id *match;
> +
> +		match = of_match_node(dwc2_compat_ahb_bursts, node);
> +		if (match)
> +			ret = (int)match->data;
> +
>  		return ret;
>  	} else if (of_device_is_compatible(node, "brcm,bcm2835-usb")) {
>  		dev_warn(hsotg->dev,
> 

Hi Christian,

I'd prefer if you use the binding which requires no extra code in
dwc2.

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



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