Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <69c0f9c0-5c89-e99e-c807-9963ca377093@theobroma-systems.com>
Date:   Fri, 9 Dec 2022 12:15:34 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation
Content-Language: en-US
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Quentin Schulz <foss+kernel@0leil.net>
Cc:     Minas Harutyunyan <hminas@synopsys.com>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        William Wu <william.wu@rock-chips.com>,
        Bin Yang <yangbin@rock-chips.com>,
        Frank Wang <frank.wang@rock-chips.com>
References: <20221206-dwc2-gadget-dual-role-v1-0-36515e1092cd@theobroma-systems.com>
 <20221206-dwc2-gadget-dual-role-v1-3-36515e1092cd@theobroma-systems.com>
 <Y5IIaeip81DIvEZ6@kroah.com>
From:   Quentin Schulz <quentin.schulz@theobroma-systems.com>
In-Reply-To: <Y5IIaeip81DIvEZ6@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?d3NKZy83dHVKdXQrSTRraTFlVlBUN1d3L25pMkNjWmUyWWxSNXhXMTFRakJp?=
 =?utf-8?B?dWtmcnhkWDdsYy9lUVdCTmxLY0c1eEk4Rnp4TzZLZkFTdndNU3NONDVwUGJ6?=
 =?utf-8?B?WS9XbmdMRkthMzM0dVhqa0lqWDNIUG9ZdnBuam5SdmZTalFxQkZEVjBRSHdi?=
 =?utf-8?B?RFluUWZxNEQwZDhPS09KbTZSV0ozRGdHdWpDOEJBLzZOQzlWQjBKNjE3QWNv?=
 =?utf-8?B?NE93MGRlWlRqNkRtUjFnREdJa21ra1pWQ0tuaUY3NmZCUUFFZXdVWDhPZEds?=
 =?utf-8?B?U1Rid3ZSeGpCU1VUOGNkSmZlV0psaUxDWklFZDdMUGtkZERjcEVKVm9LMW4y?=
 =?utf-8?B?MkM0dVNHc1ZiQmx2Wm9vS3B6YjJ1SEltbzAzZzVmbVdNSkwzKzRLTTJUMXpY?=
 =?utf-8?B?ZHlUQTVlUklnQU5relQ4d2RkMHVQdmpzOS9lNXlaWngrYUZYc2pnRHhzL1pL?=
 =?utf-8?B?cFJQSk5FM3V3TkdVdVdESE0vZkdabGVXTGdXRnJGbmU2ZzMxcTNpYWZUSzhY?=
 =?utf-8?B?d2VBbHhhajJ4b0ZnUXhTenBPKzBFczdFSHJsUkI5NGNxdEFISmJSa1c0dHoy?=
 =?utf-8?B?dXFHdGFpNjVLWTF3MHNDZi9WcmJRN1NpMVAwcUM5WHNqMWdvUkV5V3gyUDJR?=
 =?utf-8?B?NEhWSkplaWZiYXJmOVJ6OUFBelRqa3kxd0crNWEyRzUxSkNha2NlTVh5M1NR?=
 =?utf-8?B?UFFUSnhneHVheks1RFhsMnBveEtpdUhDTXBDOHowbk1veW1mQVliSGEzc0pN?=
 =?utf-8?B?aHIxVC9SZDFQOE9FTjJ2N2dWd0F6cE8rcEVDY1BZL0k0T1JnWkdlV1JJdWJs?=
 =?utf-8?B?L3lXZ3dMSW5rYnQ2a2tGbXI5K2NKTUNKVGtlUk5Uczc5TU9lVkxkSjZVZmcy?=
 =?utf-8?B?M21QbjRxeWpUeklSeWhGVFIzNEZzdHc2YU1rY2tOOVBlSWFHMGdWeWxXdlUr?=
 =?utf-8?B?cmF5Mzc5cVd6dUNjcEFtMm9ydklVdkZiSmdnc2tmL20rRnhQNi80SHpRb3JE?=
 =?utf-8?B?ajh2RFhCcHVHZlNlOEFnRjAxZUt2SzVRdENaYmZlMU5kaHBaZVFYOXhMbEY2?=
 =?utf-8?B?MDdpUmZaNTdFeld6Qy8rWElBcUdHY2VEYi91bDdUYW0rWnRBNUVORFZCZTVO?=
 =?utf-8?B?RVBWK3RFNjZMZnNndmo2UW9RcDJqRjl2RlpiN01nckRlVzRjR3o5TG9EZGhO?=
 =?utf-8?B?RkdYTlBDQUdoZEhFbjdMYWF3Tll2d0xCZzU2N3BqT2txQStqYWtJQVNtNmVH?=
 =?utf-8?B?M2cxMVdGTER3RWphZ09ZYnZKWVgzV2VySjk3d3JvMVhJWGdQWkFaQnlsZFNX?=
 =?utf-8?B?V0FDRkhMN2lpQjE3RFB5eGRMMXpPQk1IYlhvWG5FU1JWeTF4bzBCcmxMTnl2?=
 =?utf-8?B?RTFkdEZFOW5EZVQ1ZWI3VXFOV2d1N2Q2L0JwaHhFK0ZIeFJrcVlEQ1NjQy9t?=
 =?utf-8?B?c2MxQXZPUG5KSDF2bGJyb3FUVWVKOFp1Q1ZOUEh4MTFHdWh6cUF4YjUySGN0?=
 =?utf-8?B?emdCOEphMVRWZm1zRkZZQ3o1UjV0R2F5c2lRSnpmYTdpSUZNNVZGejZONXlq?=
 =?utf-8?B?aGMrTVY5SnppNnd6eVdvQlNlaFU3Zmk3a21obFYydWVQOFhiaGYrRlMxalNW?=
 =?utf-8?B?aGM0WDdibm1ZdTFMZFBBVldIdmZHenJISGVkN2RwM2RWRUxWbDdWQjh5emVY?=
 =?utf-8?B?dkVnWnNvdDZ2cHQ0T2pLNmt4enpUNkRyUjlyTXhDcSt5K2Z3ZzYyYVFVbUp5?=
 =?utf-8?B?OXV4dW14WTRGcHNjVGhhYlFGMFNTazFLM2RPakp1aHZZREtJR0QxVHhIcnR0?=
 =?utf-8?B?ME9UeHBlejFaL0RrZGRRalFET3JaZlhJYXFGM2RBWWp6aXEwZkt3L2V0b3gw?=
 =?utf-8?B?YVVqSGRucFFQM3FGdXBMVVBnRXF1Rk1BUmlVS0w5ZjRhcHB6NmxGSmI2QnNu?=
 =?utf-8?B?STBpY3NNMkx5dEdydG1lOHVMM3dnUUFZUC96dU5xdnNZR2J6a3JLMjBpd2Vx?=
 =?utf-8?B?bVMyV1IvYTJiakRxK0F5cENPWklhS1QxTjNlemdWMW9DbGdudldvRGo4ZGdn?=
 =?utf-8?B?dVVpMEtxS0hXRmQzd1RocC9WV0dYTjN2LzNMOG1qUXljcUV5QU1DZ25vL2ZK?=
 =?utf-8?B?ZHRlUWVFcGEyZkRMSG9RNjBFbUdRTlBMRmtRanFXaGJaMmd4enJSTVFURlRj?=
 =?utf-8?Q?k+bp3TTKkiBDrKf9DgyJ80s=3D?=
X-OriginatorOrg: theobroma-systems.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5776eff5-1e29-404c-9e20-08dad9d6b27d
X-MS-Exchange-CrossTenant-AuthSource: DU2PR04MB8536.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2022 11:15:36.8401
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5e0e1b52-21b5-4e7b-83bb-514ec460677e
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /C26THG4y9XNRMIE0beNu2hjmsVAu0livatoX9VrD/zZnAsUyCRKncavU8HRHb74fBWlbAMd3yj4vYqg0vMeEoYUzO+IH2M1ytz719H0NfsGjMEyI7J38ud4Q1ZVWRha
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR04MB7690
X-Spam-Status: No, score=-1.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,T_SPF_PERMERROR autolearn=no
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Greg,

On 12/8/22 16:53, Greg Kroah-Hartman wrote:
> On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
>> From: Bin Yang <yangbin@rock-chips.com>
>>
>> The usb phys need to be controlled dynamically on some Rockchip SoCs.
>> So set the new HCD flag which prevents USB core from trying to manage
>> our phys.
>>
>> Signed-off-by: Bin Yang <yangbin@rock-chips.com>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>   drivers/usb/dwc2/hcd.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 657f1f659ffaf..757a66fa32fa8 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>>   	if (!IS_ERR_OR_NULL(hsotg->uphy))
>>   		otg_set_host(hsotg->uphy->otg, &hcd->self);
>>   
>> +	/*
>> +	 * do not manage the PHY state in the HCD core, instead let the driver
>> +	 * handle this (for example if the PHY can only be turned on after a
>> +	 * specific event)
>> +	 */
>> +	hcd->skip_phy_initialization = 1;
> 
> Wait, doesn't this mess with the phy logic for all other chips that use
> this IP block?  Have you tested this on other systems?
> 

I have not. I asked this in the cover-letter but I guess I should have 
made the patch series an RFC for this reason?

> I'd like some verification first before taking this change as it seems
> very specific-platform.
> 

There's already some platform-specific callbacks for the driver (see
dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called 
too early, before hcd structure is actually allocated. So we either need 
to use some "proxy"/shadow variable in dwc2_core_params and then update 
it right after hcd gets allocated or have another platform-specific 
callback only for hcd (post-)initialization.

Nothing too fancy so shouldn't take too long to implement. Any 
preference? Something else?

Also on a side note, after further testing, USB peripheral mode in 
dual-role mode does not seem to be entirely fixed with this patch 
series, I still have occasional locks. But considering that it 
absolutely didn't work before, it is some kind of progress.
There are also some issues related to USB host mode in dual-role mode 
but I saw those happening before the patch series too. I'll see what I 
can do, really frustrating to work with IPs for which there's no 
documentation :/

Cheers,
Quentin