>
>On 25/11/2020 02:36, Peter Chen wrote:
>> On 20-11-24 14:22:25, Roger Quadros wrote:
>>> Peter,
>>>
>>> On 24/11/2020 13:47, Peter Chen wrote:
>>>> On 20-11-24 12:33:34, Roger Quadros wrote:
>>>>>>>>
>>>>>>>> I am sorry about that. Do you use role switch /sys entry, if you have
>>>>>>>> used, I prefer using "usb-role-switch" property at dts to judge if SoC
>>>>>>>> OTG signals or external signals for role switch. If you have not used
>>>>>>>> it, I prefer only setting cdns->role_sw for role switch use cases.
>>>>>>>>
>>>>>>>
>>>>>>> We use both hardware role switch and /sys entries for manually forcing a
>>>>>>> certain role.
>>>>>>>
>>>>>>> We do not set any "usb-role-switch" property at DTS.
>>>>>>>
>>>>>>> Currently cdns->role_sw is being always set by driver irrespective of any DT
>>>>>>> property, so this patch is clearly wrong and needs to be reverted.
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>
>>>>>> Could you accept below fix?
>>>>>>
>>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>>> index 2e469139769f..fdd52e87a7b2 100644
>>>>>> --- a/drivers/usb/cdns3/core.c
>>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>>> @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
>>>>>> enum usb_role real_role, current_role;
>>>>>> int ret = 0;
>>>>>>
>>>>>> - /* Depends on role switch class */
>>>>>> - if (cdns->role_sw)
>>>>>> + /* quit if switch role through external signals */
>>>>>> + if (device_property_read_bool(cdns->dev, "usb-role-switch"))
>>>>>> return 0;
>>>>>>
>>>>>> pm_runtime_get_sync(cdns->dev);
>>>>>
>>>>> Although this will fix the issue I don't think this is making the driver to behave
>>>>> as expected with usb-role-switch property.
>>>>>
>>>>> Now, even if usb-role-switch property is not present the driver will still register
>>>>> the role switch driver.
>>>>>
>>>>> I think we need to register the role switch driver only if usb-role-switch property
>>>>> is present. We would also need to set the default role if role-switch-default-mode is present.
>>>>>
>>>>> How about the following? It still doesn't handle role-switch-default-mode property though.
>>>>>
>>>>
>>>> Roger, you said you also use /sys entries (I suppose it means through role
>>>> switch class) to do role switch, with your change, there will be no /sys
>>>> entry for role switch.
>>>
>>> Sorry for the confusion. Although we do need both features (SW role switch + HW role switch)
>>> I don't think it is required to operate simultaneously. If users need SW control they can set the DT flag.
>>>
>>
>> I see. I prefer embracing all things related to role switch under the
>> firmware entry condition. Besides, I find another issue that devm_request_irq
>> for wakeup_irq does not call usb_role_switch_unregister if it has
>> failed. So, probably, two patches are needed. I am OK you send the
>> patches to fix both.
>
>Pawel, can you please confirm that if you are ok with either h/w role switching
>or s/w role switching but not both working at the same time?
>
>It would have been nice by user to to know the current role even for h/w based
>swithcing but it looks like now that won't be possible.
For the moment it's ok for me, but form testing point of view it could be nice
to have possibility to easy choose between these two modes in dynamically way.
For PCI based on platform probably I will have to add some parameter to cdns3-pci.ko
module to allow selecting modes. I don't use DTS.
Don't worry about it now. You can post this fix.
Cheers,
Pawel
>
>cheers,
>-roger
>>
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index 2e469139769f..fc6a8152406c 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -427,7 +427,6 @@ static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
>> */
>> static int cdns3_probe(struct platform_device *pdev)
>> {
>> - struct usb_role_switch_desc sw_desc = { };
>> struct device *dev = &pdev->dev;
>> struct resource *res;
>> struct cdns3 *cdns;
>> @@ -529,18 +528,21 @@ static int cdns3_probe(struct platform_device *pdev)
>> if (ret)
>> goto err2;
>>
>> - sw_desc.set = cdns3_role_set;
>> - sw_desc.get = cdns3_role_get;
>> - sw_desc.allow_userspace_control = true;
>> - sw_desc.driver_data = cdns;
>> - if (device_property_read_bool(dev, "usb-role-switch"))
>> + if (device_property_read_bool(dev, "usb-role-switch")) {
>> + struct usb_role_switch_desc sw_desc = { };
>> +
>> + sw_desc.set = cdns3_role_set;
>> + sw_desc.get = cdns3_role_get;
>> + sw_desc.allow_userspace_control = true;
>> + sw_desc.driver_data = cdns;
>> sw_desc.fwnode = dev->fwnode;
>>
>> - cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
>> - if (IS_ERR(cdns->role_sw)) {
>> - ret = PTR_ERR(cdns->role_sw);
>> - dev_warn(dev, "Unable to register Role Switch\n");
>> - goto err3;
>> + cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
>> + if (IS_ERR(cdns->role_sw)) {
>> + ret = PTR_ERR(cdns->role_sw);
>> + dev_warn(dev, "Unable to register Role Switch\n");
>> + goto err3;
>> + }
>> }
>>
>> if (cdns->wakeup_irq) {
>> @@ -583,7 +585,8 @@ static int cdns3_probe(struct platform_device *pdev)
>> return 0;
>> err4:
>> cdns3_drd_exit(cdns);
>> - usb_role_switch_unregister(cdns->role_sw);
>> + if (cdns->role_sw)
>> + usb_role_switch_unregister(cdns->role_sw);
>> err3:
>> set_phy_power_off(cdns);
>> err2:
>>
>
>--
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
