Hi Paul,
On Tue, Jul 16, 2013 at 12:22:12PM -0700, Paul Zimmerman wrote:
> The dwc2 driver sets the value of the DWC2 GAHBCFG register to 0x6,
> which is GAHBCFG_HBSTLEN_INCR4. But different platforms may require
> different values. In particular, the Broadcom 2835 SOC used in the
> Raspberry Pi needs a value of 0x10, otherwise the DWC2 controller
> stops working after a short period of heavy USB traffic.
>
> So this patch adds another driver parameter named 'ahbcfg'. The
> default value is 0x6. Any platform needing a different value should
> add a DT attribute to set it.
>
> This patch also removes the 'ahb_single' driver parameter, since
> that bit can now be set using 'ahbcfg'.
>
> This patch does not add DT support to platform.c, I will leave that
> to whoever owns the first platform that needs a non-default value.
> (Stephen?)
I previously submitted a patch to load everything from DT, which might
serve as inspiration: http://article.gmane.org/gmane.linux.usb.general/85086
(but note this reply: http://article.gmane.org/gmane.linux.usb.general/85104)
> Signed-off-by: Paul Zimmerman <pa...@synopsys.com>
Reviewed-by: Matthijs Kooijman <matth...@stdin.nl>
However, I do wonder if it makes sense to let (almost) the entire
GAHBCFG be set from one variable. IMHO it would be more elegant to split
out the separate fields that can be set and have separate config
variables for them (I especially have the feeling that a literal
register value does not belong in a DT attribute, though I'm not quite
sure what's the policy there). Not sure if it's worth the extra effort,
though.
> -int dwc2_set_param_ahb_single(struct dwc2_hsotg *hsotg, int val)
> +int dwc2_set_param_ahbcfg(struct dwc2_hsotg *hsotg, int val)
> {
> - int valid = 1;
> - int retval = 0;
> -
> - if (DWC2_PARAM_TEST(val, 0, 1)) {
> - if (val >= 0) {
> - dev_err(hsotg->dev,
> - "'%d' invalid for parameter ahb_single\n", val);
> - dev_err(hsotg->dev, "ahb_single must be 0 or 1\n");
> - }
> - valid = 0;
> - }
> -
> - if (val > 0 && hsotg->snpsid < DWC2_CORE_REV_2_94a)
> - valid = 0;
> -
> - if (!valid) {
> - if (val >= 0)
> - dev_err(hsotg->dev,
> - "%d invalid for parameter ahb_single. Check HW
> configuration.\n",
> - val);
> - val = 0;
> - dev_dbg(hsotg->dev, "Setting ahb_single to %d\n", val);
> - retval = -EINVAL;
> - }
> -
> - hsotg->core_params->ahb_single = val;
> - return retval;
> + if (val != -1)
> + hsotg->core_params->ahbcfg = val;
> + else
> + hsotg->core_params->ahbcfg = GAHBCFG_HBSTLEN_INCR4;
> + return 0;
> }
Does it make sense to remove all the validity checking here? I could
imagine adding a check like:
if (val & GAHBCFG_CTRL_MASK) {
// Bits in GAHBCFG_CTRL_MASK cannot be set this way
...
valid = 0
}
to prevent silently dropping bits from an invalid value?
Also, it seems that there used to be a requirement of having at least
core revision 2.94a for types other than single. Is this no longer
relevant?
> diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
> index fc075a7..e771e40 100644
> --- a/drivers/staging/dwc2/core.h
> +++ b/drivers/staging/dwc2/core.h
> @@ -150,10 +150,11 @@ enum dwc2_lx_state {
> * are enabled
> * @reload_ctl: True to allow dynamic reloading of HFIR register
> during
> * runtime
> - * @ahb_single: This bit enables SINGLE transfers for remainder data
> in
> - * a transfer for DMA mode of operation.
> - * 0 - remainder data will be sent using INCR burst
> size
> - * 1 - remainder data will be sent using SINGLE burst
> size
> + * @ahbcfg: This field allows the default value of the GAHBCFG
> + * register to be overridden
> + * -1 - GAHBCFG value will not be overridden
> + * all others - GAHBCFG value will be overridden with
> + * this value
> * @otg_ver: OTG version supported
> * 0 - 1.3
> * 1 - 2.0
Shouldn't this mention that a few of the bits in the register cannot be
set like this?
Gr.
Matthijs
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
