by Baolin Wang

[permalink] [raw]

Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Hi

On 8 November 2016 at 04:36, NeilBrown <[email protected]> wrote:
> On Mon, Nov 07 2016, Baolin Wang wrote:
>
>> On 3 November 2016 at 09:25, NeilBrown <[email protected]> wrote:
>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>
>> I agree with your most opinions, but these are optimization.
>
> I see them as bug fixes, not optimizations.
>
>> Firstly I
>> think we should upstream the USB charger driver.
>
> I think you missed the point. The point is that we don't *need* your
> "USB charger driver" because all the infrastructure we need is *already*
> present in the kernel. It is buggy and not used uniformly, and could
> usefully be polished and improved. But the structure is already
> present.
>
> If everyone just added new infrastructure when they didn't like, or
> didn't understand, what was already present, the kernel would become
> like the Mad Hatter's tea party, full of dirty dishes.
>
>> What I want to ask is
>> how can we notify power driver if we don't set the
>> usb_register_notifier(), then I think you give the answer is: power
>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>> should notify the power driver how much current should be drawn, and I
>> still think we should notify the current in usb charger driver which
>> is better, and do not need to notify current for power driver in usb
>> phy driver.
>
> I accept that it isn't clear that the phy *should* be involved in
> communicating the negotiated power availability, but nor is it clear
> that it shouldn't. The power does travel through the physical
> interface, so physically it plays a role.
>
> But more importantly, it already *does* get told (in some cases).
> There is an interface "usb_phy_set_power()" which exists explicitly to
> tell the phy what power has been negotiated. Given that infrastructure
> exists and works, it make sense to use it.
>
> If you think it is a broken design and should be removed, then fine:
> make a case for that. Examine the history. Make sure you know why it
> is there (or make sure that information cannot be found), and then
> present a case as to why it should be removed and replaced with
> something else. But don't just leave it there and pretend it doesn't
> exist and create something similar-but-different and hope people will
> know why yours is better. That way lies madness.

Like Peter said, it is not only PHY can detect the USB charger type,
which means there are other places can detect the charger type.
Second, some controller need to detect the charger type manually which
USB phy did not support. Third, it did not handle what current should
be drawn in USB phy. Fourth, we need integrate all charger plugin/out
event in one framework, not from extcon, maybe type-c in future. In a
word, we need one standard integration of this feature we need, though
like you said we should do some clean up or fix to make it better.

--
Baolin.wang
Best Regards

2016-11-14 04:21:38

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

On Thu, Nov 10 2016, Baolin Wang wrote:

> Hi
>
> On 8 November 2016 at 04:36, NeilBrown <[email protected]> wrote:
>> On Mon, Nov 07 2016, Baolin Wang wrote:
>>
>>> On 3 November 2016 at 09:25, NeilBrown <[email protected]> wrote:
>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>
>>> I agree with your most opinions, but these are optimization.
>>
>> I see them as bug fixes, not optimizations.
>>
>>> Firstly I
>>> think we should upstream the USB charger driver.
>>
>> I think you missed the point. The point is that we don't *need* your
>> "USB charger driver" because all the infrastructure we need is *already*
>> present in the kernel. It is buggy and not used uniformly, and could
>> usefully be polished and improved. But the structure is already
>> present.
>>
>> If everyone just added new infrastructure when they didn't like, or
>> didn't understand, what was already present, the kernel would become
>> like the Mad Hatter's tea party, full of dirty dishes.
>>
>>> What I want to ask is
>>> how can we notify power driver if we don't set the
>>> usb_register_notifier(), then I think you give the answer is: power
>>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>>> should notify the power driver how much current should be drawn, and I
>>> still think we should notify the current in usb charger driver which
>>> is better, and do not need to notify current for power driver in usb
>>> phy driver.
>>
>> I accept that it isn't clear that the phy *should* be involved in
>> communicating the negotiated power availability, but nor is it clear
>> that it shouldn't. The power does travel through the physical
>> interface, so physically it plays a role.
>>
>> But more importantly, it already *does* get told (in some cases).
>> There is an interface "usb_phy_set_power()" which exists explicitly to
>> tell the phy what power has been negotiated. Given that infrastructure
>> exists and works, it make sense to use it.
>>
>> If you think it is a broken design and should be removed, then fine:
>> make a case for that. Examine the history. Make sure you know why it
>> is there (or make sure that information cannot be found), and then
>> present a case as to why it should be removed and replaced with
>> something else. But don't just leave it there and pretend it doesn't
>> exist and create something similar-but-different and hope people will
>> know why yours is better. That way lies madness.
>
> Like Peter said, it is not only PHY can detect the USB charger type,
> which means there are other places can detect the charger type.

If I understand Peter's example correctly, it shows a configuration
where the USB PHysical interface is partly implemented in the SOC and
partly in the PMIC. I appreciate that would make it more challenging to
implement a PHY driver, but there is no reason it should impact anything
outside of the PHY.

> Second, some controller need to detect the charger type manually which
> USB phy did not support.

"manually" is an odd term to use in this context.
I think you mean that to detect the charger type you need to issue some
command to the hardware and wait for it to respond, then assess the
response.
That isn't at all surprising. The charger type is detected by measuring
resistance between ID and GND, which may require setting up a potential
and activating ADCs to measure the voltage. This can all be done
internally to the phy driver.
Sometimes it is easy (I did https://lkml.org/lkml/2015/2/23/746 for
twl4030, though it never got upstream).
The code for the imx7d does look more complex, but not intrinsically
different.

> Third, it did not handle what current should
> be drawn in USB phy.

The standards define that. The extcon just reports the cable type.
Certainly it would be sensible to provide a library function to
translate from cable type to current range. You don't need a new
subsystem to do that, just a library function.

> Fourth, we need integrate all charger plugin/out
> event in one framework, not from extcon, maybe type-c in future.

Why not extcon? Given that a charger is connected by an external
connector, extcon seems like exactly the right thing to use.

Obviously extcon doesn't report the current that was negotiated, but
that is best kept separate. The battery charger can be advised of the
available current either via extcon or separately via the usb
subsystem. Don't conflate the two.

> In a
> word, we need one standard integration of this feature we need, though
> like you said we should do some clean up or fix to make it better.

But really, I'm not the person you need to convince. I'm just a vaguely
interested bystander who is rapidly losing interest. You need to
convince a maintainer, but they have so far shown remarkably little
interest. I don't know why, but I'd guess that reviewing a complex new
subsystem isn't much fun. Reviewing and applying clear bugfixes and
incremental improvements is much easier and more enjoyable. But that is
just a guess.

NeilBrown

Attachments:

signature.asc (800.00 B)

2016-11-14 12:06:12

On Mon, Nov 14 2016, Mark Brown wrote:

> On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
>> On Thu, Nov 10 2016, Baolin Wang wrote:
>
>> > Fourth, we need integrate all charger plugin/out
>> > event in one framework, not from extcon, maybe type-c in future.
>
>> Why not extcon? Given that a charger is connected by an external
>> connector, extcon seems like exactly the right thing to use.
>
>> Obviously extcon doesn't report the current that was negotiated, but
>> that is best kept separate. The battery charger can be advised of the
>> available current either via extcon or separately via the usb
>> subsystem. Don't conflate the two.
>
> Conflating the two seems like the whole point here. We're looking for
> something that sits between the power supply code and the USB code and
> tells the power supply code what it's allowed to do which is the result
> of a combination of physical cable detection and USB protocol. It seems
> reasonable that extcon drivers ought to be part of this but it doesn't
> seem like they are the whole story.

I don't think "between the power supply code and the USB code" is where
this thing sits. I think it sits inside the power-supply driver.
We already have extcon which sits between the phy and the power_supply
code, and the usb_notifier which sits between the USB code and the
power supply code. We don't need another go-between.

If we have extcon able to deliver reliable information about cable type,
and if with have the usb notifier able to deliver reliable information
about negotiated current, and if the power supply manager is able to
register with the correct extcon and the correct usb notifier, then the
power supply manager *could* handle all the notifications and make the
correct determinations and set the current limits itself. All this
could be done entirely internally, without the help of any new
subsystem.
Do you agree?

Clearly doing it that way would result in lots of code duplication and
would mean that each driver probably gets its own private set of bugs,
but it would be possible. Just undesirable.

So yes, it makes perfect to provide common code which handles the
registrations, and captures the events, and translates the different
events into current levels and feeds those back to the driver. This
isn't some new subsystem, this is just a resource, provided by a
library, that power drivers can allocate and initialize if the want to.

To quote myself:

> 5/ Now that all cable connection notifications are sent over extcon and
> all vbus_draw notifications are sent over the usb_phy notifier, write
> some support code that a power supply client can use to be told what
> power is available.
> e.g. a battery charger driver would call:
> register_power_client(.....)
> or similar, providing a phandle (or similar) for the usb phy and a
> function to call back when the available current changes (or maybe a
> work_struct containing the function pointer)
>
> register_power_client() would then register with extcon and separately
> with the usb_phy notifier. When the different events arrive it
> calculates what ranges of currents are expected and calls the
> call-back function with those details.

NeilBrown

Attachments:

signature.asc (800.00 B)

2016-11-15 05:03:27

by Peter Chen

On 25 November 2016 at 21:00, Mark Brown <[email protected]> wrote:
> On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:
>
>> I agree that the question of where the responsibility for information
>> aggregation lies is open for discussion. If fact all details on how
>> things should work are always open for discussion.
>> I don't agree that this is the main different between our positions,
>> though I can see how you might get that impression.
>
>> You could even fix them so they look *exactly* like the notifiers that
>> Baolin is proposing. This is my key point. It is not the end result
>> that I particularly object to (though I do object to some details). It
>
> Ah, OK. This really hadn't been at all clear - both Baolin and I had
> the impression that the it was both that were blockers for you. What
> were the details here?
>
>> is the process of getting to the end result that I don't like. If the
>> current system doesn't work and something different is needed, then the
>> correct thing to do is to transform the existing system into something
>> new that works better. This should be a clear series of steps. Each
>
> Sometimes there's something to be said for working out what we want
> things to look like before setting out to make these gradual
> refactorings and sometimes the refactorings are just more pain than
> they're worth, especially when they go across subsystems. In this case
> I do worry about the cross subsystem aspect causing hassle, it may be
> more practical to do anything that represents an interface change by
> adding the new interface, converting the users to it and then removing
> the old interface.
>
> At the very least the series should grow to incorporate conversion of
> the existing users though. Baolin, I think this does need adding to the
> series but probably best to think about how to do it - some of Neil's
> suggestions for incremental steps do seem like they should be useful
> for organizing things here, probably we can get some things that can be
> done internally within individual drivers merged while everything else
> is under discussion.

OK. I will think about how to incorporate conversion of the existing
users according to Neil's suggestion.

>
>> But I think here my key point got lost too, in part because it was hard
>> to refer to an actual instance.
>> My point was that in the present patch set, the "usb charger" is given
>> a name which is dependant on discovery order, and only supports
>> lookup-by-name. This cannot work.
>
> There's two bits here: one is the way names are assigned and the other
> is the lookup by name. I agree that the lookup by name isn't
> particularly useful as things stand, that could just be dropped until
> some naming mechanism is added. We'd be more likely to use phandles in
> DT systems, I don't know what ACPI systems would look like but I guess
> it'd be something similar.
>
>> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
>> any usb-charger which has power available"), or look up by some other
>> attribute, then discover-order naming could work. But the only
>> lookup-mechanism is by-name, and the names aren't reliably stable. So
>> the name/lookup system proposed cannot possibly do anything useful
>> with more than one usb_charger.
>
> Baolin, I think adding a DT binding and lookup mechanism makes sense
> here - do you agree?

Yes, I agree. But 'usb charger' is one virtual device and we described
nothing in DT about 'usb charger'. But as you and Neil said, we need
one usful method to look up the USB charger. I will think about that.
Thanks.

--
Baolin.wang
Best Regards

2016-12-20 07:05:58

by Baolin Wang

[permalink] [raw]

Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Hi Neil,

On 3 November 2016 at 09:25, NeilBrown <[email protected]> wrote:
> On Tue, Nov 01 2016, Baolin Wang wrote:
>
>
>>> So I won't be responding on this topic any further until I see a genuine
>>> attempt to understand and resolve the inconsistencies with
>>> usb_register_notifier().
>>
>> Any better solution?
>
> I'm not sure exactly what you are asking, so I'll assume you are asking
> the question I want to answer :-)
>
> 1/ Liase with the extcon developers to resolve the inconsistencies
> with USB connector types.
> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
> which both seem to suggest a standard downstream port. There is no
> documentation describing how these relate, and no consistent practice
> to copy.
> I suspect the intention is that
> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
> the cable.
> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
> would normally appear with EXTCON_USB_HOST (I think).
> Some drivers follow this model, particularly extcon-max14577.c
> but it is not consistent.
>
> This policy should be well documented and possibly existing drivers
> should be updated to follow it.
>
> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
> and EXTCON_CHG_USB_FAST. These names don't mean much.
> They were recently removed from drivers/power/axp288_charger.c
> which is good, but are still used in drivers/extcon/extcon-max*
> Possibly they should be changed to names from the standard, or
> possibly they should be renamed to identify the current they are
> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A

Now I am creating the new patchset with fixing and converting exist drivers.

I did some investigation about EXTCON subsystem. From your suggestion:
1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
---- After checking, now all extcon drivers were following this rule.

2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to change.

3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
---- There are no model that shows the slow charger should be 500mA
and fast charger is 1A. (In extcon-max77693.c file, the fast charger
can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
I think.

>From above investigation, I did not find anything need to be changed
now. What do you think? Thanks.

>
> 2/ Change all usb phys to register an extcon and to send appropriate
> notifications. Many already do, but I don't think it is universal.
> It is probable that the extcon should be registered using common code
> instead of each phy driver having its own
> extcon_get_edev_by_phandle()
> or whatever.
> If the usb phy driver needs to look at battery charger registers to
> know what sort of cable was connected (which I believe is the case
> for the chips you are interested in), then it should do that.
>
> 3/ Currently some USB controllers discover that a cable was connected by
> listening on an extcon, and some by registering for a usb_notifier
> (described below) ... though there seem to only be 2 left which do that.
> Now that all USB phys send connection information via extcon (see 2),
> the USB controllers should be changed to all find out about the cable
> using extcon.
>
> 4/ struct usb_phy contains:
> /* for notification of usb_phy_events */
> struct atomic_notifier_head notifier;
>
> This is used inconsistently. Sometimes the argument passed
> is NULL, sometimes it is a pointer to 'vbus_draw' - the current
> limited negotiated via USB, sometimes it is a pointer the the gadget
> though as far as I can tell, that last one is never used.
>
> This should be changed to be consistent. This notifier is no longer
> needed to tell the USB controller that a cable was connected (extcon
> now does that, see 3) so it is only used to communicate the
> 'vbus_draw' information.
> So it should be changed to *only* send a notification when vbus_draw
> is known, and it should carry that information.
> This should probably be done in common code, and removed
> from individual drivers.
>
> 5/ Now that all cable connection notifications are sent over extcon and
> all vbus_draw notifications are sent over the usb_phy notifier, write
> some support code that a power supply client can use to be told what
> power is available.
> e.g. a battery charger driver would call:
> register_power_client(.....)
> or similar, providing a phandle (or similar) for the usb phy and a
> function to call back when the available current changes (or maybe a
> work_struct containing the function pointer)
>
> register_power_client() would then register with extcon and separately
> with the usb_phy notifier. When the different events arrive it
> calculates what ranges of currents are expected and calls the
> call-back function with those details.
>
> 6/ Any battery charger that needs to know the available current can now
> call register_power_client() and get the information delivered.
>
> NeilBrown

--
Baolin.wang
Best Regards

2016-12-20 22:07:56

On Wed, Dec 21 2016, Baolin Wang wrote:

> Hi,
>
> On 21 December 2016 at 06:07, NeilBrown <[email protected]> wrote:
>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>
>>> Hi Neil,
>>>
>>> On 3 November 2016 at 09:25, NeilBrown <[email protected]> wrote:
>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>
>>>>
>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>> usb_register_notifier().
>>>>>
>>>>> Any better solution?
>>>>
>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>> the question I want to answer :-)
>>>>
>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>> with USB connector types.
>>>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>> which both seem to suggest a standard downstream port. There is no
>>>> documentation describing how these relate, and no consistent practice
>>>> to copy.
>>>> I suspect the intention is that
>>>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>> the cable.
>>>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>> would normally appear with EXTCON_USB_HOST (I think).
>>>> Some drivers follow this model, particularly extcon-max14577.c
>>>> but it is not consistent.
>>>>
>>>> This policy should be well documented and possibly existing drivers
>>>> should be updated to follow it.
>>>>
>>>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>>>> They were recently removed from drivers/power/axp288_charger.c
>>>> which is good, but are still used in drivers/extcon/extcon-max*
>>>> Possibly they should be changed to names from the standard, or
>>>> possibly they should be renamed to identify the current they are
>>>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>
>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>
>> Thanks!
>>
>>>
>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>> ---- After checking, now all extcon drivers were following this rule.
>>
>> what about extcon-axp288.c ?
>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>> never sets EXTCON_USB.
>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>
> Ha, sorry, I missed these 2 files, and I will fix them.
>
>>
>>>
>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>> change.
>>
>> Agreed.
>>
>>>
>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>> ---- There are no model that shows the slow charger should be 500mA
>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>> I think.
>>
>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>> anything.
>> The only place where the cable types are registered are in
>> extcon-max{14577,77693,77843,8997}.c
>>
>> In each case, the code strongly suggests that the meaning is that "slow"
>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>
>> With names like "fast" and "slow", any common changer framework cannot
>> make use of these cable types as the name doesn't mean anything.
>> If the names were changed to 500MA/1A, then common code could reasonably
>> assume how much current can safely be drawn from each.
>
> As I know, some fast charger can be drawn 5A, then do we need another
> macro named 5A? then will introduce more macros in future, I am not
> true this is helpful.

It isn't really a question of what the charger can provide. It is a
question of what the cable reports to the phy.

Unfortunately I cannot find any datasheets on line to verify how these
devices work, but the code seems to suggest that they can detect and
report special charger types that provide 500mA and 1A respectively.
If the hardware exists that reports the functionality, it make sense to
use it. Hopefully new hardware will follow the USB BC spec, so further
special cases won't be needed.

Thanks,
NeilBrown

Attachments:

signature.asc (832.00 B)

2016-12-21 09:08:09

by Baolin Wang

[permalink] [raw]

Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

On 21 December 2016 at 11:48, NeilBrown <[email protected]> wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> Hi,
>>
>> On 21 December 2016 at 06:07, NeilBrown <[email protected]> wrote:
>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>
>>>> Hi Neil,
>>>>
>>>> On 3 November 2016 at 09:25, NeilBrown <[email protected]> wrote:
>>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>>
>>>>>
>>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>>> usb_register_notifier().
>>>>>>
>>>>>> Any better solution?
>>>>>
>>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>>> the question I want to answer :-)
>>>>>
>>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>>> with USB connector types.
>>>>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>>> which both seem to suggest a standard downstream port. There is no
>>>>> documentation describing how these relate, and no consistent practice
>>>>> to copy.
>>>>> I suspect the intention is that
>>>>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>>> the cable.
>>>>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>>> would normally appear with EXTCON_USB_HOST (I think).
>>>>> Some drivers follow this model, particularly extcon-max14577.c
>>>>> but it is not consistent.
>>>>>
>>>>> This policy should be well documented and possibly existing drivers
>>>>> should be updated to follow it.
>>>>>
>>>>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>>>>> They were recently removed from drivers/power/axp288_charger.c
>>>>> which is good, but are still used in drivers/extcon/extcon-max*
>>>>> Possibly they should be changed to names from the standard, or
>>>>> possibly they should be renamed to identify the current they are
>>>>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>>
>>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>>
>>> Thanks!
>>>
>>>>
>>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>>> ---- After checking, now all extcon drivers were following this rule.
>>>
>>> what about extcon-axp288.c ?
>>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>>> never sets EXTCON_USB.
>>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>
>> Ha, sorry, I missed these 2 files, and I will fix them.
>>
>>>
>>>>
>>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>>> change.
>>>
>>> Agreed.
>>>
>>>>
>>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>>> ---- There are no model that shows the slow charger should be 500mA
>>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>>> I think.
>>>
>>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>>> anything.
>>> The only place where the cable types are registered are in
>>> extcon-max{14577,77693,77843,8997}.c
>>>
>>> In each case, the code strongly suggests that the meaning is that "slow"
>>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>>
>>> With names like "fast" and "slow", any common changer framework cannot
>>> make use of these cable types as the name doesn't mean anything.
>>> If the names were changed to 500MA/1A, then common code could reasonably
>>> assume how much current can safely be drawn from each.
>>
>> As I know, some fast charger can be drawn 5A, then do we need another
>> macro named 5A? then will introduce more macros in future, I am not
>> true this is helpful.
>
> It isn't really a question of what the charger can provide. It is a
> question of what the cable reports to the phy.

Yes, there is no spec to describe fast/slow charger type and how much
current fast/slow charger can provide. Maybe some fast charger can
provide 1A/2A, others can provide 5A, which depends on users'
platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
charger can provide 1.5A on user's platform, will it report the fast
charger type by EXTCON_CHG_USB_1A on user's platform (but it can
provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
maintainer has applied them).

>
> Unfortunately I cannot find any datasheets on line to verify how these
> devices work, but the code seems to suggest that they can detect and
> report special charger types that provide 500mA and 1A respectively.
> If the hardware exists that reports the functionality, it make sense to
> use it. Hopefully new hardware will follow the USB BC spec, so further
> special cases won't be needed.
>
> Thanks,
> NeilBrown

--
Baolin.wang
Best Regards

2016-12-21 23:48:35

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

On Wed, Dec 21 2016, Baolin Wang wrote:

> On 21 December 2016 at 11:48, NeilBrown <[email protected]> wrote:
>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>
>>> Hi,
>>>
>>> On 21 December 2016 at 06:07, NeilBrown <[email protected]> wrote:
>>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>>
>>>>> Hi Neil,
>>>>>
>>>>> On 3 November 2016 at 09:25, NeilBrown <[email protected]> wrote:
>>>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>>>
>>>>>>
>>>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>>>> usb_register_notifier().
>>>>>>>
>>>>>>> Any better solution?
>>>>>>
>>>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>>>> the question I want to answer :-)
>>>>>>
>>>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>>>> with USB connector types.
>>>>>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>>>> which both seem to suggest a standard downstream port. There is no
>>>>>> documentation describing how these relate, and no consistent practice
>>>>>> to copy.
>>>>>> I suspect the intention is that
>>>>>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>>>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>>>> the cable.
>>>>>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>>>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>>>> would normally appear with EXTCON_USB_HOST (I think).
>>>>>> Some drivers follow this model, particularly extcon-max14577.c
>>>>>> but it is not consistent.
>>>>>>
>>>>>> This policy should be well documented and possibly existing drivers
>>>>>> should be updated to follow it.
>>>>>>
>>>>>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>>>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>>>>>> They were recently removed from drivers/power/axp288_charger.c
>>>>>> which is good, but are still used in drivers/extcon/extcon-max*
>>>>>> Possibly they should be changed to names from the standard, or
>>>>>> possibly they should be renamed to identify the current they are
>>>>>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>>>
>>>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>>>
>>>> Thanks!
>>>>
>>>>>
>>>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>>>> ---- After checking, now all extcon drivers were following this rule.
>>>>
>>>> what about extcon-axp288.c ?
>>>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>>>> never sets EXTCON_USB.
>>>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>>
>>> Ha, sorry, I missed these 2 files, and I will fix them.
>>>
>>>>
>>>>>
>>>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>>>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>>>> change.
>>>>
>>>> Agreed.
>>>>
>>>>>
>>>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>>>> ---- There are no model that shows the slow charger should be 500mA
>>>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>>>> I think.
>>>>
>>>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>>>> anything.
>>>> The only place where the cable types are registered are in
>>>> extcon-max{14577,77693,77843,8997}.c
>>>>
>>>> In each case, the code strongly suggests that the meaning is that "slow"
>>>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>>>
>>>> With names like "fast" and "slow", any common changer framework cannot
>>>> make use of these cable types as the name doesn't mean anything.
>>>> If the names were changed to 500MA/1A, then common code could reasonably
>>>> assume how much current can safely be drawn from each.
>>>
>>> As I know, some fast charger can be drawn 5A, then do we need another
>>> macro named 5A? then will introduce more macros in future, I am not
>>> true this is helpful.
>>
>> It isn't really a question of what the charger can provide. It is a
>> question of what the cable reports to the phy.
>
> Yes, there is no spec to describe fast/slow charger type and how much
> current fast/slow charger can provide. Maybe some fast charger can
> provide 1A/2A, others can provide 5A, which depends on users'
> platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
> charger can provide 1.5A on user's platform, will it report the fast
> charger type by EXTCON_CHG_USB_1A on user's platform (but it can
> provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
> they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
> maintainer has applied them).

I said "It isn't really a question of what the charger can provide."
and you are still taking about "and some fast charger can provide 1.5A".
So it seems like you didn't read, or you didn't understand what I wrote.
I'll try again.

Ignore the chargers. COMPLETELY. This not about chargers AT ALL.
This is about cables and the information that the cable reports.

Some how, and I cannot find the details, these MAXIM devices can report
things like
case MAX14577_CHARGER_TYPE_SPECIAL_500MA:
and
case MAX14577_CHARGER_TYPE_SPECIAL_1A:
and
case MAX77693_CHARGER_TYPE_APPLE_500MA:
and
case MAX77693_CHARGER_TYPE_APPLE_1A_2A:

If this information is to be useful to the USB battery charging
infrastructure, then it must be communicated unambiguously. Reporting
"EXTCON_CHG_USB_SLOW" or "..._FAST" doesn't have an obvious unambiguous
meaning.

If it were documented somewhere that
A cable of type EXTCON_CHG_USB_SLOW can provide at least 500mA of current
at 5V. A cable of type EXTCON_CHG_USB_FAST can provide at least 1A of
current at 5V.
when those cable types could be used by the USB battery chargers.
If we just changed the names, then we wouldn't really need to document
it and the intention would be obvious. Self-documenting names are
better where possible.

So yes, we could leave it as it is a decide not to fix this bug. But
then I would then argue strongly against any attempt for the USB battery
charging infrastructure to do anything with EXTCON_CHG_USB_SLOW and
EXTCON_CHG_USB_FAST. If you are going to fix this thing, you may as
well fix it properly.

NeilBrown

Attachments:

signature.asc (832.00 B)

2016-12-22 07:06:05

by Baolin Wang

[permalink] [raw]

Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

On 22 December 2016 at 07:47, NeilBrown <[email protected]> wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> On 21 December 2016 at 11:48, NeilBrown <[email protected]> wrote:
>>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>>
>>>> Hi,
>>>>
>>>> On 21 December 2016 at 06:07, NeilBrown <[email protected]> wrote:
>>>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>>>
>>>>>> Hi Neil,
>>>>>>
>>>>>> On 3 November 2016 at 09:25, NeilBrown <[email protected]> wrote:
>>>>>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>>>>>> attempt to understand and resolve the inconsistencies with
>>>>>>>>> usb_register_notifier().
>>>>>>>>
>>>>>>>> Any better solution?
>>>>>>>
>>>>>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>>>>>> the question I want to answer :-)
>>>>>>>
>>>>>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>>>>> with USB connector types.
>>>>>>> e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>>>>> which both seem to suggest a standard downstream port. There is no
>>>>>>> documentation describing how these relate, and no consistent practice
>>>>>>> to copy.
>>>>>>> I suspect the intention is that
>>>>>>> EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>>>>> the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>>>>> the cable.
>>>>>>> So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>>>>> while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>>>>> would normally appear with EXTCON_USB_HOST (I think).
>>>>>>> Some drivers follow this model, particularly extcon-max14577.c
>>>>>>> but it is not consistent.
>>>>>>>
>>>>>>> This policy should be well documented and possibly existing drivers
>>>>>>> should be updated to follow it.
>>>>>>>
>>>>>>> At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>>>>> and EXTCON_CHG_USB_FAST. These names don't mean much.
>>>>>>> They were recently removed from drivers/power/axp288_charger.c
>>>>>>> which is good, but are still used in drivers/extcon/extcon-max*
>>>>>>> Possibly they should be changed to names from the standard, or
>>>>>>> possibly they should be renamed to identify the current they are
>>>>>>> expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>>>>>
>>>>>> Now I am creating the new patchset with fixing and converting exist drivers.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>>
>>>>>> I did some investigation about EXTCON subsystem. From your suggestion:
>>>>>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>>>>>> ---- After checking, now all extcon drivers were following this rule.
>>>>>
>>>>> what about extcon-axp288.c ?
>>>>> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
>>>>> never sets EXTCON_USB.
>>>>> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.
>>>>
>>>> Ha, sorry, I missed these 2 files, and I will fix them.
>>>>
>>>>>
>>>>>>
>>>>>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>>>>>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>>>>>> change.
>>>>>
>>>>> Agreed.
>>>>>
>>>>>>
>>>>>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>>>>>> ---- There are no model that shows the slow charger should be 500mA
>>>>>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>>>>>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>>>>>> I think.
>>>>>
>>>>> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
>>>>> anything.
>>>>> The only place where the cable types are registered are in
>>>>> extcon-max{14577,77693,77843,8997}.c
>>>>>
>>>>> In each case, the code strongly suggests that the meaning is that "slow"
>>>>> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>>>>>
>>>>> With names like "fast" and "slow", any common changer framework cannot
>>>>> make use of these cable types as the name doesn't mean anything.
>>>>> If the names were changed to 500MA/1A, then common code could reasonably
>>>>> assume how much current can safely be drawn from each.
>>>>
>>>> As I know, some fast charger can be drawn 5A, then do we need another
>>>> macro named 5A? then will introduce more macros in future, I am not
>>>> true this is helpful.
>>>
>>> It isn't really a question of what the charger can provide. It is a
>>> question of what the cable reports to the phy.
>>
>> Yes, there is no spec to describe fast/slow charger type and how much
>> current fast/slow charger can provide. Maybe some fast charger can
>> provide 1A/2A, others can provide 5A, which depends on users'
>> platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
>> charger can provide 1.5A on user's platform, will it report the fast
>> charger type by EXTCON_CHG_USB_1A on user's platform (but it can
>> provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
>> they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
>> maintainer has applied them).
>
> I said "It isn't really a question of what the charger can provide."
> and you are still taking about "and some fast charger can provide 1.5A".
> So it seems like you didn't read, or you didn't understand what I wrote.
> I'll try again.
>
> Ignore the chargers. COMPLETELY. This not about chargers AT ALL.
> This is about cables and the information that the cable reports.
>
> Some how, and I cannot find the details, these MAXIM devices can report
> things like
> case MAX14577_CHARGER_TYPE_SPECIAL_500MA:
> and
> case MAX14577_CHARGER_TYPE_SPECIAL_1A:
> and
> case MAX77693_CHARGER_TYPE_APPLE_500MA:
> and
> case MAX77693_CHARGER_TYPE_APPLE_1A_2A:
>
> If this information is to be useful to the USB battery charging
> infrastructure, then it must be communicated unambiguously. Reporting
> "EXTCON_CHG_USB_SLOW" or "..._FAST" doesn't have an obvious unambiguous
> meaning.
>
> If it were documented somewhere that
> A cable of type EXTCON_CHG_USB_SLOW can provide at least 500mA of current
> at 5V. A cable of type EXTCON_CHG_USB_FAST can provide at least 1A of
> current at 5V.
> when those cable types could be used by the USB battery chargers.
> If we just changed the names, then we wouldn't really need to document
> it and the intention would be obvious. Self-documenting names are
> better where possible.

Fine, I will submit one patch to document EXTCON_CHG_USB_SLOW/FAST as
you suggested.

>
> So yes, we could leave it as it is a decide not to fix this bug. But
> then I would then argue strongly against any attempt for the USB battery
> charging infrastructure to do anything with EXTCON_CHG_USB_SLOW and
> EXTCON_CHG_USB_FAST. If you are going to fix this thing, you may as
> well fix it properly.
>
> NeilBrown
>

--
Baolin.wang
Best Regards