extcon-intel-cht-wc: Don't reset USB data connection at probe

extcon-intel-cht-wc: Don't reset USB data connection at probe

[Yauhen Kharuzhy]

Patch to prevent resetting USB connection if OTG device is connected
during of driver initialization.

v2:
Change (id == INTEL_USB_ID_FLOAT) check to (id != INTEL_USB_ID_GND) as
Hans de Goede advised.

[PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe

[Yauhen Kharuzhy]

Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
PMIC at driver probing for further charger detection. This causes reset of
USB data sessions and removing all devices from bus. If system was
booted from Live CD or USB dongle, this makes system unusable.

Check if USB ID pin is floating and re-route data lines in this case
only, don't touch otherwise.

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 9d32150e68db..da1886a92f75 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
 	struct cht_wc_extcon_data *ext;
 	unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
+	int pwrsrc_sts, id;
 	int irq, ret;
 
 	irq = platform_get_irq(pdev, 0);
@@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 		goto disable_sw_control;
 	}
 
-	/* Route D+ and D- to PMIC for initial charger detection */
-	cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
+	ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
+	if (ret) {
+		dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
+		goto disable_sw_control;
+	}
+
+	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
+
+	/* If no USB host or device connected, route D+ and D- to PMIC for
+	 * initial charger detection
+	 */
+	if (id != INTEL_USB_ID_GND)
+		cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
 
 	/* Get initial state */
 	cht_wc_extcon_pwrsrc_event(ext);
-- 
2.23.0.rc1

Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe

[Hans de Goede]

Hi,

On 16-09-2019 23:15, Yauhen Kharuzhy wrote:
> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> PMIC at driver probing for further charger detection. This causes reset of
> USB data sessions and removing all devices from bus. If system was
> booted from Live CD or USB dongle, this makes system unusable.
> 
> Check if USB ID pin is floating and re-route data lines in this case
> only, don't touch otherwise.
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



>   drivers/extcon/extcon-intel-cht-wc.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 9d32150e68db..da1886a92f75 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>   	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
>   	struct cht_wc_extcon_data *ext;
>   	unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
> +	int pwrsrc_sts, id;
>   	int irq, ret;
>   
>   	irq = platform_get_irq(pdev, 0);
> @@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>   		goto disable_sw_control;
>   	}
>   
> -	/* Route D+ and D- to PMIC for initial charger detection */
> -	cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
> +	ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> +		goto disable_sw_control;
> +	}
> +
> +	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> +
> +	/* If no USB host or device connected, route D+ and D- to PMIC for
> +	 * initial charger detection
> +	 */
> +	if (id != INTEL_USB_ID_GND)
> +		cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
>   
>   	/* Get initial state */
>   	cht_wc_extcon_pwrsrc_event(ext);
> 

Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe

[Andy Shevchenko]

On Tue, Sep 17, 2019 at 12:15:36AM +0300, Yauhen Kharuzhy wrote:
> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> PMIC at driver probing for further charger detection. This causes reset of
> USB data sessions and removing all devices from bus. If system was
> booted from Live CD or USB dongle, this makes system unusable.
> 
> Check if USB ID pin is floating and re-route data lines in this case
> only, don't touch otherwise.

> +	ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> +		goto disable_sw_control;
> +	}
> +
> +	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);

We have second implementation of this. Would it make sense to split to some
helper?

> +	/* If no USB host or device connected, route D+ and D- to PMIC for
> +	 * initial charger detection
> +	 */

I'm not sure it's acceptable style of multi-line comment, but it's up to
maintainer.

> +	if (id != INTEL_USB_ID_GND)
> +		cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
>  
>  	/* Get initial state */
>  	cht_wc_extcon_pwrsrc_event(ext);
> -- 
> 2.23.0.rc1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe

[Yauhen Kharuzhy]

On Tue, Sep 17, 2019 at 02:13:22PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 17, 2019 at 12:15:36AM +0300, Yauhen Kharuzhy wrote:
> > Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> > PMIC at driver probing for further charger detection. This causes reset of
> > USB data sessions and removing all devices from bus. If system was
> > booted from Live CD or USB dongle, this makes system unusable.
> > 
> > Check if USB ID pin is floating and re-route data lines in this case
> > only, don't touch otherwise.
> 
> > +	ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> > +	if (ret) {
> > +		dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> > +		goto disable_sw_control;
> > +	}
> > +
> > +	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> 
> We have second implementation of this. Would it make sense to split to some
> helper?

Do you mean the combination of regmap_read(...CHT_WC_PWRSRC_STS,
&pwrsrc_sts) with cht_wc_extcon_get_id()?

In the cht_wc_extcon_pwrsrc_event() function the pwrsrc_sts is checked
for other bits also, so separation of PWRSRC_STS read and id calculation
to one routine will cause non-clear function calls like as
get_powersrc_and_check_id(..., &powersrc_sts, &id) which is not looks
better than current code duplication. Or we need to spend some time for
refactoring and testing of cht_wc_extcon_pwrsrc_event() code.

> 
> > +	/* If no USB host or device connected, route D+ and D- to PMIC for
> > +	 * initial charger detection
> > +	 */
> 
> I'm not sure it's acceptable style of multi-line comment, but it's up to
> maintainer.
Argh, you are right, I fixed this in local copy but this is not
significant typo.

> 
> > +	if (id != INTEL_USB_ID_GND)
> > +		cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
> >  
> >  	/* Get initial state */
> >  	cht_wc_extcon_pwrsrc_event(ext);
> > -- 
> > 2.23.0.rc1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
Yauhen Kharuzhy

Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe

[Andy Shevchenko]

On Wed, Sep 18, 2019 at 2:04 AM Yauhen Kharuzhy <jekhor@gmail.com> wrote:
>
> On Tue, Sep 17, 2019 at 02:13:22PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 17, 2019 at 12:15:36AM +0300, Yauhen Kharuzhy wrote:
> > > Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> > > PMIC at driver probing for further charger detection. This causes reset of
> > > USB data sessions and removing all devices from bus. If system was
> > > booted from Live CD or USB dongle, this makes system unusable.
> > >
> > > Check if USB ID pin is floating and re-route data lines in this case
> > > only, don't touch otherwise.
> >
> > > +   ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> > > +   if (ret) {
> > > +           dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> > > +           goto disable_sw_control;
> > > +   }
> > > +
> > > +   id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> >
> > We have second implementation of this. Would it make sense to split to some
> > helper?
>
> Do you mean the combination of regmap_read(...CHT_WC_PWRSRC_STS,
> &pwrsrc_sts) with cht_wc_extcon_get_id()?

Yes.

> In the cht_wc_extcon_pwrsrc_event() function the pwrsrc_sts is checked
> for other bits also, so separation of PWRSRC_STS read and id calculation
> to one routine will cause non-clear function calls like as
> get_powersrc_and_check_id(..., &powersrc_sts, &id) which is not looks
> better than current code duplication.

I see. Thanks for answer.

> Or we need to spend some time for
> refactoring and testing of cht_wc_extcon_pwrsrc_event() code.

Perhaps, In any case I'm not objecting of the current approach.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe

[Chanwoo Choi]

Hi Andy,

On 19. 9. 18. 오후 4:17, Andy Shevchenko wrote:
> On Wed, Sep 18, 2019 at 2:04 AM Yauhen Kharuzhy <jekhor@gmail.com> wrote:
>>
>> On Tue, Sep 17, 2019 at 02:13:22PM +0300, Andy Shevchenko wrote:
>>> On Tue, Sep 17, 2019 at 12:15:36AM +0300, Yauhen Kharuzhy wrote:
>>>> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
>>>> PMIC at driver probing for further charger detection. This causes reset of
>>>> USB data sessions and removing all devices from bus. If system was
>>>> booted from Live CD or USB dongle, this makes system unusable.
>>>>
>>>> Check if USB ID pin is floating and re-route data lines in this case
>>>> only, don't touch otherwise.
>>>
>>>> +   ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
>>>> +   if (ret) {
>>>> +           dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
>>>> +           goto disable_sw_control;
>>>> +   }
>>>> +
>>>> +   id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
>>>
>>> We have second implementation of this. Would it make sense to split to some
>>> helper?
>>
>> Do you mean the combination of regmap_read(...CHT_WC_PWRSRC_STS,
>> &pwrsrc_sts) with cht_wc_extcon_get_id()?
> 
> Yes.
> 
>> In the cht_wc_extcon_pwrsrc_event() function the pwrsrc_sts is checked
>> for other bits also, so separation of PWRSRC_STS read and id calculation
>> to one routine will cause non-clear function calls like as
>> get_powersrc_and_check_id(..., &powersrc_sts, &id) which is not looks
>> better than current code duplication.
> 
> I see. Thanks for answer.
> 
>> Or we need to spend some time for
>> refactoring and testing of cht_wc_extcon_pwrsrc_event() code.
> 
> Perhaps, In any case I'm not objecting of the current approach.
> 

If you think it is OK, could you reply with your tag?
and I'll fix the multi-line comment by myself.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe

[Andy Shevchenko]

On Tue, Sep 17, 2019 at 10:29 AM Yauhen Kharuzhy <jekhor@gmail.com> wrote:
>
> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> PMIC at driver probing for further charger detection. This causes reset of
> USB data sessions and removing all devices from bus. If system was
> booted from Live CD or USB dongle, this makes system unusable.
>
> Check if USB ID pin is floating and re-route data lines in this case
> only, don't touch otherwise.
>

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>  drivers/extcon/extcon-intel-cht-wc.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 9d32150e68db..da1886a92f75 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>         struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
>         struct cht_wc_extcon_data *ext;
>         unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
> +       int pwrsrc_sts, id;
>         int irq, ret;
>
>         irq = platform_get_irq(pdev, 0);
> @@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>                 goto disable_sw_control;
>         }
>
> -       /* Route D+ and D- to PMIC for initial charger detection */
> -       cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
> +       ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> +       if (ret) {
> +               dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> +               goto disable_sw_control;
> +       }
> +
> +       id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> +
> +       /* If no USB host or device connected, route D+ and D- to PMIC for
> +        * initial charger detection
> +        */
> +       if (id != INTEL_USB_ID_GND)
> +               cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
>
>         /* Get initial state */
>         cht_wc_extcon_pwrsrc_event(ext);
> --
> 2.23.0.rc1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] extcon-intel-cht-wc: Don't reset USB data connection at probe

[Chanwoo Choi]

Hi,

On 19. 9. 17. 오전 6:15, Yauhen Kharuzhy wrote:
> Intel Cherry Trail Whiskey Cove extcon driver connect USB data lines to
> PMIC at driver probing for further charger detection. This causes reset of
> USB data sessions and removing all devices from bus. If system was
> booted from Live CD or USB dongle, this makes system unusable.
> 
> Check if USB ID pin is floating and re-route data lines in this case
> only, don't touch otherwise.
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>  drivers/extcon/extcon-intel-cht-wc.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 9d32150e68db..da1886a92f75 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>  	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
>  	struct cht_wc_extcon_data *ext;
>  	unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
> +	int pwrsrc_sts, id;
>  	int irq, ret;
>  
>  	irq = platform_get_irq(pdev, 0);
> @@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>  		goto disable_sw_control;
>  	}
>  
> -	/* Route D+ and D- to PMIC for initial charger detection */
> -	cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
> +	ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
> +		goto disable_sw_control;
> +	}
> +
> +	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> +
> +	/* If no USB host or device connected, route D+ and D- to PMIC for
> +	 * initial charger detection
> +	 */
> +	if (id != INTEL_USB_ID_GND)
> +		cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
>  
>  	/* Get initial state */
>  	cht_wc_extcon_pwrsrc_event(ext);
> 

I edit this patch as following by myself and then applied it.

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 9d32150e68db..771f6f4cf92e 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -338,6 +338,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
        struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
        struct cht_wc_extcon_data *ext;
        unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
+       int pwrsrc_sts, id;
        int irq, ret;

        irq = platform_get_irq(pdev, 0);
@@ -387,8 +388,19 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
                goto disable_sw_control;
        }

-       /* Route D+ and D- to PMIC for initial charger detection */
-       cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
+       ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
+       if (ret) {
+               dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
+               goto disable_sw_control;
+       }
+
+       /*
+        * If no USB host or device connected, route D+ and D- to PMIC for
+        * initial charger detection
+        */
+       id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
+       if (id != INTEL_USB_ID_GND)
+               cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics