Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: wcheng=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 7E5CBC433C6
Subject: Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following
 programming guide
To:     John Stultz <john.stultz@linaro.org>,
        lkml <linux-kernel@vger.kernel.org>
Cc:     Felipe Balbi <balbi@kernel.org>,
        Tejas Joglekar <tejas.joglekar@synopsys.com>,
        Yang Fei <fei.yang@intel.com>,
        YongQin Liu <yongqin.liu@linaro.org>,
        Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
        Thinh Nguyen <thinhn@synopsys.com>,
        Jun Li <lijun.kernel@gmail.com>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org
References: <20210108015115.27920-1-john.stultz@linaro.org>
 <20210108015115.27920-2-john.stultz@linaro.org>
From:   Wesley Cheng <wcheng@codeaurora.org>
Message-ID: <eb4b8540-a57c-53cc-a371-cf68178bec15@codeaurora.org>
Date:   Sat, 6 Mar 2021 01:00:02 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <20210108015115.27920-2-john.stultz@linaro.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/7/2021 5:51 PM, John Stultz wrote:
> In reviewing the previous patch, Thinh Nguyen pointed out that
> the DRD mode change sequence should be like the following when
> switching from host -> device according to the programming guide
> (for all DRD IPs):
> 1. Reset controller with GCTL.CoreSoftReset
> 2. Set GCTL.PrtCapDir(device)
> 3. Soft reset with DCTL.CSftRst
> 4. Then follow up with the initializing registers sequence
> 
> The current code does:
> a. Soft reset with DCTL.CSftRst on driver probe
> b. Reset controller with GCTL.CoreSoftReset (added in previous
>    patch)
> c. Set GCTL.PrtCapDir(device)
> d. < missing DCTL.CSftRst >
> e. Then follow up with initializing registers sequence
> 
> So this patch adds the DCTL.CSftRst soft reset that was currently
> missing from the dwc3 mode switching.
> 
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
> Cc: Yang Fei <fei.yang@intel.com>
> Cc: YongQin Liu <yongqin.liu@linaro.org>
> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Cc: Thinh Nguyen <thinhn@synopsys.com>
> Cc: Jun Li <lijun.kernel@gmail.com>
> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> Feedback would be appreciated. I'm a little worried I should be
> conditionalizing the DCTL.CSftRst on DRD mode controllers, but
> I'm really not sure what the right thing to do is for non-DRD
> mode controllers.
> ---
>  drivers/usb/dwc3/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index b6a6b90eb2d5..71f8b07ecb99 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -40,6 +40,8 @@
>  
>  #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
>  
> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
> +
>  /**
>   * dwc3_get_dr_mode - Validates and sets dr_mode
>   * @dwc: pointer to our context structure
> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>  
>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>  
> +	dwc3_core_soft_reset(dwc);

Hi John/Thinh/Felipe,

I actually added this change into my local branch, because we were
seeing an issue when switching from host mode --> peripheral mode.  What
was happening was that the RXFIFO register did not update back to the
expected value for peripheral mode by the time
dwc3_gadget_init_out_endpoint() was executed.  With the logic to
calculate the EP max packet limit based on RXFIFO reg, this caused all
EPs to be set with an EP max limit of 0.

With this change, it seemed to help with the above issue.  However, can
we consider moving the core soft reset outside the spinlock?  At least
with our PHY init routines, we have some msleep() calls for waiting for
the PHYs to be ready, which will end up as a sleeping while atomic bug.
(not sure if PHY init is required to be called in atomic context)

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project