[2/2] usb: dwc2: gadget reuse ahbcfg assigned from platform

Gadget directly set GAHBCFG_HBSTLEN_INCR4, reuse ahbcfg if assigned from platform

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/usb/dwc2/core.c   | 2 +-
 drivers/usb/dwc2/gadget.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

On 4 February 2015 at 21:51, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 15aa578..20085de 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2314,9 +2314,13 @@ void s3c_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>>                 GINTSTS_USBSUSP | GINTSTS_WKUPINT,
>>                 hsotg->regs + GINTMSK);
>>
>> +       if ((hsotg->core_params) && (hsotg->core_params->ahbcfg != -1))
>
>
>    Inner pares not needed, especially the first ones.

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

Hi Yousaf

On 4 February 2015 at 17:41, Kaukab, Yousaf <yousaf.kaukab@intel.com> wrote:
>> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index
>> d5197d4..8d388cc 100644
>> --- a/drivers/usb/dwc2/core.c
>> +++ b/drivers/usb/dwc2/core.c
>> @@ -2563,7 +2563,7 @@ void dwc2_set_param_reload_ctl(struct dwc2_hsotg
>> *hsotg, int val)
>>
>>  void dwc2_set_param_ahbcfg(struct dwc2_hsotg *hsotg, int val)  {
>> -     if (val != -1)
>> +     if (val)
>>               hsotg->core_params->ahbcfg = val;
>>       else
>>               hsotg->core_params->ahbcfg =
>> GAHBCFG_HBSTLEN_INCR4 << diff --git a/drivers/usb/dwc2/gadget.c
>> b/drivers/usb/dwc2/gadget.c index 15aa578..20085de 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2314,9 +2314,13 @@ void s3c_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>>               GINTSTS_USBSUSP | GINTSTS_WKUPINT,
>>               hsotg->regs + GINTMSK);
>>
>> +     if ((hsotg->core_params) && (hsotg->core_params->ahbcfg != -
>> 1))
>> +             val = hsotg->core_params->ahbcfg &
>> ~GAHBCFG_CTRL_MASK;
>> +     else
>> +             val = GAHBCFG_HBSTLEN_INCR4 <<
>> GAHBCFG_HBSTLEN_SHIFT;
>> +
>>       if (using_dma(hsotg))
>> -             writel(GAHBCFG_GLBL_INTR_EN |
>> GAHBCFG_DMA_EN |
>> -                    (GAHBCFG_HBSTLEN_INCR4 <<
>> GAHBCFG_HBSTLEN_SHIFT),
>> +             writel(GAHBCFG_GLBL_INTR_EN |
>> GAHBCFG_DMA_EN | val,
>>                      hsotg->regs + GAHBCFG);
>>       else
>>               writel(((hsotg->dedicated_fifos) ?
>> (GAHBCFG_NP_TXF_EMP_LVL |
>
> There are other bits in GAHBCFG that can be set from platform. They will be preserved by your patch, as they are not part of GAHBCFG_CTRL_MASK, but only in case dma is enabled. Perhaps preserve them in non-dma case as well.

Here may have issue if also set hsotg->core_params->ahbcfg for non-dma case,
since GAHBCFG[4:1] may be set.

Though from drivers/usb/dwc2/core.h we can not see @ahbcfg is
specifically used for dma case,
most case in drivers/usb/dwc2/platform.c use ahbcfg is set hbstlen,
GAHBCFG[4:1].
For example, our platform set GAHBCFG_HBSTLEN_INCR16.

So I just assume @ahbcfg is used for dma case.
What do you think.

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

On 6 February 2015 at 16:07, Kaukab, Yousaf <yousaf.kaukab@intel.com> wrote:
>> >> GAHBCFG_HBSTLEN_INCR4 << diff --git a/drivers/usb/dwc2/gadget.c
>> >> b/drivers/usb/dwc2/gadget.c index 15aa578..20085de 100644
>> >> --- a/drivers/usb/dwc2/gadget.c
>> >> +++ b/drivers/usb/dwc2/gadget.c
>> >> @@ -2314,9 +2314,13 @@ void s3c_hsotg_core_init_disconnected(struct
>> >> dwc2_hsotg *hsotg,
>> >>               GINTSTS_USBSUSP | GINTSTS_WKUPINT,
>> >>               hsotg->regs + GINTMSK);
>> >>
>> >> +     if ((hsotg->core_params) && (hsotg->core_params->ahbcfg != -
>> >> 1))
>> >> +             val = hsotg->core_params->ahbcfg &
>> >> ~GAHBCFG_CTRL_MASK;
>> >> +     else
>> >> +             val = GAHBCFG_HBSTLEN_INCR4 <<
>> >> GAHBCFG_HBSTLEN_SHIFT;
>> >> +
>> >>       if (using_dma(hsotg))
>> >> -             writel(GAHBCFG_GLBL_INTR_EN |
>> >> GAHBCFG_DMA_EN |
>> >> -                    (GAHBCFG_HBSTLEN_INCR4 <<
>> >> GAHBCFG_HBSTLEN_SHIFT),
>> >> +             writel(GAHBCFG_GLBL_INTR_EN |
>> >> GAHBCFG_DMA_EN | val,
>> >>                      hsotg->regs + GAHBCFG);
>> >>       else
>> >>               writel(((hsotg->dedicated_fifos) ?
>> >> (GAHBCFG_NP_TXF_EMP_LVL |
>> >
>> > There are other bits in GAHBCFG that can be set from platform. They will be
>> preserved by your patch, as they are not part of GAHBCFG_CTRL_MASK, but
>> only in case dma is enabled. Perhaps preserve them in non-dma case as well.
>>
>> Here may have issue if also set hsotg->core_params->ahbcfg for non-dma case,
>> since GAHBCFG[4:1] may be set.
>
> You can mask off HBstLen in that case. However, I don't think setting burst length will be an issue in non DMA case as DWC2 will not act as a bus master. John, can you please confirm if setting burst length will be an issue in non-dma case?

It would be great if John has some input.
I am not sure, just doubt ahbcfg is specifically used for dma mode.

static int dwc2_gahbcfg_init(struct dwc2_hsotg *hsotg)
{
        case GHWCFG2_INT_DMA_ARCH:
                dev_dbg(hsotg->dev, "Internal DMA Mode\n");
                if (hsotg->core_params->ahbcfg != -1) {
                        ahbcfg &= GAHBCFG_CTRL_MASK;
                        ahbcfg |= hsotg->core_params->ahbcfg &
                                  ~GAHBCFG_CTRL_MASK;
                }
                break;
}

Looks like only GHWCFG2_INT_DMA_ARCH case cares the value of ahbcfg.

>
>>
>> Though from drivers/usb/dwc2/core.h we can not see @ahbcfg is specifically
>> used for dma case, most case in drivers/usb/dwc2/platform.c use ahbcfg is set
>> hbstlen, GAHBCFG[4:1].
>> For example, our platform set GAHBCFG_HBSTLEN_INCR16.
>>
>> So I just assume @ahbcfg is used for dma case.
>> What do you think.
>
> While you are fixing it, why not fix it for other bits, for example AHBSingle, InvDescEndianness etc.,  which are part of the same register and will be overwritten at the same place.

Yes, understand.
Not sure other value need to be overwirtten, if only GAHBCFG[4:1],
burst len, maybe we can add another property?

Will update accordingly after John give some info.

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