> From: Matthijs Kooijman [mailto:matthijs@xxxxxxxx] > Sent: Wednesday, July 17, 2013 4:15 AM > > 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 <paulz@xxxxxxxxxxxx> > Reviewed-by: Matthijs Kooijman <matthijs@xxxxxxxx> > > 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. Hi Matthijs, I don't think it's worth the extra code, frankly. The HSOTG core is highly configurable, we can't have a driver parameter for every field in every configuration register. > > -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? This value should only be set by PWKWTAD (People Who Know What They Are Doing). I just made that up :) I don't want to add a bunch of code checking for all possible valid bits for all versions of the core. That way lies madness. > > 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? I could have added a comment along the lines of "see the HSOTG databook for valid values for this register". But e.g. the value used by the Broadcom SOC is not documented in the Synopsys databook. I'm guessing they made some enhancement to our core. So I don't even know what bits cannot be set in this register. -- Paul -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
