USBD add core-power and Vbus handle #50

alexrayne · 2017-02-04T10:09:08Z

here provided enhances on usbd-driver for:

Vbus sensing - <usbd_is_vbus> method and callback. This helps to detect cable plug in/out, or it can be USBpowering detection
USBcore power/clock domain control - <usbd_enable> method and <power_control> backend implementation for STM32F4xxx. This intends to power-saving by usb-core and PHY stops.

kuldeepdhaka · 2017-02-04T10:34:41Z

lib/usbd/backend/usbd_stm32_otg_fs.c

@@ -75,10 +79,10 @@ static usbd_device *init(const usbd_backend_config *config)
 	_usbd_dev.config = config;

 	if (config->feature & USBD_VBUS_SENSE) {
-		if (OTG_FS_CID >= 0x00002000) { /* 2.0 */
+		if (OTG_FS_CID >= 0x00002000) { /* 2.0 HS core*/


According to this comment, CID = 2.0+ and FS core should not exists right?

dow understand you. CID indicates an core version. according to RM0090 FS CID=0x0000 1100 and HS CID=0x0200 0600
what you ask about?

Please see RM0385.
OTG_CID = 0x00002000 for OTG_FS and OTG_HS

heh. my version of RM0385 says:
Reset value: 0x0000 3000 for USB OTG FS
Reset value: 0x0000 3100 for USB OTG HS
looks like this code need target MCU specification - it depends on MCU family. And even more - look like this code not aplicatable on stm32f7 family 8/ But for my stm32f4 - works ok 8)

…definition to DWC_OTG_GOTGINT, DWC_OTG_PCGCCTL

…. it intends to handle cable connetion, Vbus powering + usbd_register_session_callback + :usbd_backend:is_vbus, power_control - add this methods to provide VBus status, and USBcore powering control + usbd_is_vbus, usbd_enable - this methods invokes backend power controls + usbd_stm32_otg_fs:otgfs_is_powered, otgfs_power_control - implements above controls of stm32f4xx FS core

kuldeepdhaka · 2017-02-04T10:59:15Z

include/unicore-mx/usbd/usbd.h

+ *                     false disable PHY and stops usb-core
+ */
+void usbd_enable(usbd_device *dev, bool onoff);
+


There is already a function, see "void usbd_disconnect(usbd_device *dev, bool disconnected);".

usbd_disconnect stops session, it is not control power mode of core or phy.

kuldeepdhaka · 2017-02-04T11:02:42Z

include/unicore-mx/usbd/usbd.h

+ * @param[in] dev USB Device
+ * @param[in] callback callback to be invoked
+ */
+void usbd_register_session_callback(usbd_device *dev,


session callback facility look like a good idea.

kuldeepdhaka · 2017-02-04T11:03:12Z

include/unicore-mx/usbd/usbd.h

+ * @param[in] dev USB Device
+ * @return true - cable connected, and Vbus active
+ */
+bool usbd_is_vbus(usbd_device *dev);


ability to know VBUS state also look like a good idea too.

kuldeepdhaka · 2017-02-08T12:39:20Z

VBUS value reading (your current code look ok)
various session callback {connected, suspended, resumed, disconnected}.
on suspend, disconnected, the USB periph can go into a state that can reduce power consuption of periop and allow detecting SOF. (it should be atleast able to detect activity from host)
two way of implementation:
- [breaking] usbd_register_suspend_callback() and usbd_register_resume_callback() already exists. we can remove them and pass an enum as state to session callback.
- [non-breaking] make two new function usbd_register_connected_callback() and usbd_register_disconnected_callback() for the missing functionality.
USB enable/disable. enable or disable the usb periph communication with host. we can drop usbd_disconnect(). [ on _init() the periph is by default enabled for communcation ]
not strongly related: we already have a SOF callback (maybe pass frame number value as 2nd argument)

kuldeepdhaka · 2017-02-08T12:46:52Z

lib/usbd/usbd_private.h

@@ -174,6 +179,11 @@ struct usbd_device {
 #endif
 };

+typedef enum {
+	  usbd_paShutdown


Enum labels are uppercase. (see coding style for full information)

Without compelling reason, please dont use typedef. (see coding style for full information)
If you really want, you can later do typedef enum usbd_power_action usbd_power_action.

alexrayne · 2017-02-08T15:27:59Z

various session callback ...

in my version of unicoremx there no callback concected/disconected - what you talk about?
imho control over usbcore power mode should be delegated to user, or must be noted by usbd_conf features.

... we can drop usbd_disconnect(). ...

imho this function made an different thing, vs power control - it initiates session disconnect. but your proposal well - this function can be followed by disable call, that powerdown usbcore after session disconnects.
imho this functionality should be denoted by a feature set in usbd_conf_xx structure, to provide backport compatibility with existing code.

kuldeepdhaka · 2017-02-09T12:06:41Z

Yes, there are no connected/disconnected callback. (dont exists), they need to be implemented.

I think, we should do things transparently for user till they make sense (ie don't get in their way).
for example: if user has requested to disable communication with host, it is a good thing that the stack should put the periph in power down mode [saves power]. i see no point in providing a function to put periph in powerdown mode manually (instead of automatically doing it for user).

alexrayne · 2017-02-09T12:56:05Z

there s possible problem with powerdown - no event generates,no callbacks invokes. Therefore if i powerdown usbcore, suspend callback not affected!

in y board when i plugoff USBcable, session callback invokes and powerdown core. That is all. IMHO here colud be suspend action notified too.

alexrayne · 2017-02-09T12:56:10Z

i see no point in providing a function to put periph in powerdown mode manually (instead of automatically doing it for user).

IMHO, user should no bruteforces against library for control over periferia. library should provide API as smart as uer requests. If somebody need manual control - publish this functions not a bad.
Maybe automatic power downing is good for default, and it can be disabled by a feature . but i`m aware about code that alredy writen with old library. maybe should be defined some macro, like USBD_FEATURE_POWERDOWN_DEFAULT, that choose behaviour. can you imagine some good name for this macro?

kuldeepdhaka · 2017-02-09T13:28:32Z

What are the possible usecases of manually controlling power down of usb periph?

The stack can provide an abstraction over power down mode via session management {connected, disconnected, suspended, resumed}.
For example, after suspend, application code can either either save power (or not save power) but the stack can do [power down] (mundane task) transparently.

alexrayne · 2017-02-09T15:50:21Z

Ta present i misunderstand differense between suspend/resume and connect/disconnect operations. can you explain? what happen on invoking suspend and disconnect?

kuldeepdhaka · 2017-02-09T16:17:39Z

@alexrayne USB 2.0 specs (usb_20.pdf) page 240

alexrayne · 2017-02-09T19:12:53Z

Thank you for this picture. There almost see that suspended is surely not an powerdown mode.
but where threr is an mode whre Vbus down - maybe it is "attached" ?

As i remember we discuss about enable vs [dis]connect semantic of usbd api?
i agree about that functionality of this methods are close. but here is aspects. disconnect in current implementation makes an logical bus disconnection, after that we can reconfigure usb OTG (or not) for say - host mode, and reconnect again. But if we make power-down in disconnection, usbcore comes not accesible, until powerup. So for reconfigure, we need power enable without connection?

kuldeepdhaka · 2017-02-09T21:57:08Z

afaiu, Taking power from VBUS dont not mean that a device is connected [ ie self power device]. (yea, Inrush current could be a hint for "something attached"). the actual detection occur via D+ and D- pull-{up/down}.

USB Host control (master) the suspend and resume process. device only have to make sure that it implement the protocol correctly (reduce power consumption and wait for a SOF to detect resume signal).

usbd_disconnect() -> dont communicate with Host.
usbd_enable() - disable clocking of periph etc.. (ultimately lead to loss of communicate with host)
i would vote for usbd_disconnect() because of (my) intution with function name and its already in api.

I haven't though on reconfiguring api (host mode -> device mode and vice versa - OTG Mode) yet :)

alexrayne · 2017-02-09T22:41:28Z

because of (my) intution with function name

my intuition - disconnect say nothing about device operational mode, its about connection status. enable(false) says that device should turnoff, and therefore disconnects all connections.

should we fight about names? why this semantics both cant be present?

kuldeepdhaka · 2017-02-09T23:00:15Z

should we fight about names? why this semantics both cant be present?

But oh boy, bike shedding is fun!

Requirement for the API

stop communication with host
OTG mode support (switch between Host and device mode)
Reduce power consumption.
anything else?

alexrayne · 2017-02-19T14:17:12Z

Kuldeep:

about backend api - one contain method <disconnect (bool )> - but this name confusing me. is it same as <connect(!bool)>?
about handling power on\off on session disconect - in wich sequence should be called on_session callback and backend.power_control(usbd_paActivate)? have you any mention on it?

…ture.USBD_USE_POWERDOWN * warns - fixed some compiler warnings about mixing enum as integer

alexrayne · 2017-02-20T22:32:29Z

kuldeep, enums was rewrites with your prefered style

danielinux · 2017-05-20T08:42:38Z

@kuldeepdhaka any input on this?

kuldeepdhaka · 2017-05-21T06:52:38Z

lib/usbd/usbd.c

@@ -224,6 +234,8 @@ bool usbd_is_vbus(usbd_device *dev){
 }

 void usbd_enable(usbd_device *dev, bool onoff){
+	if (!onoff)
+		usbd_disconnect(dev, false);
 	if (dev->backend->power_control)
 		dev->backend->power_control(dev, (onoff)? usbd_paActivate : usbd_paShutdown );
 }


Application code = usbd_disconnect(dev, true) == dev->backend->disconnect(dev, true); == usbd_enable(dev, false); === usbd_disconnect(dev, false); ==== usbd_enable(dev, true); ===== dev->backend->power_control(dev, usbd_paActivate); ==== dev->backend->disconnect(dev, false); === dev->backend->power_control(dev, usbd_paShutdown );

Look at the flow of control.
At the end, NOT disconnected with SHUTDOWN power control.

don`t understand you - enable(false) MUST not shutdown?

oh, i see, thank you

for your new code.

=usbd_disconnect(dev, true) <----------------\ ==dev->backend->disconnect(dev, true); | RECURSIVE CALL! STACK OVERFLOW! ==usbd_enable(dev, false); | ===usbd_disconnect(dev, true) ---------------/ ===dev->backend->power_control(dev, usbd_paShutdown);

yeah, see alredy. more trouble now bother me - cantt understand how to wake up FS from powerdown, cause it want normaly detect online and reset after ungating core clock

well, resolve it. but not preferable way - not like what i write

When usbd_enable(dev, false) is called, it disconnect and shutdown. but when usbd_enable(dev, true) is called, it only power-up (it remove the disconnect condition).

yes, this was confused me too. disconnection MUST be provided before powerdown. but fro stm DWC core it is mean that USB enter to non-connectable state. imho, name 'disconnect' - not very suitable for this operation.
i`ll change behaviour to restore disconnection state on powerdown complete.
i not sure, is it good to place disconnection before powerdown into backend->power_control, instead of botherig it on frontend?

kuldeepdhaka · 2017-05-21T07:00:35Z

include/unicore-mx/usbd/usbd.h

+		USBD_PHY_EXT = (1 << 0),
+		USBD_VBUS_SENSE = (1 << 1),
+		USBD_VBUS_EXT = (1 << 2)
+		//* provide usb-core auto power-up/down on connect/disconnect


"//*" (IMO) Please C style comment only (code consistancy).
@danielinux @brabo what do you think?

kuldeepdhaka · 2017-05-21T07:03:47Z

include/unicore-mx/usbd/usbd.h

+		USBD_VBUS_SENSE = (1 << 1),
+		USBD_VBUS_EXT = (1 << 2)
+		//* provide usb-core auto power-up/down on connect/disconnect
+		, USBD_USE_POWERDOWN		= (1<<3)


(IHMO) ", USBD_USE_POWERDOWN" code consistancy. "," not placed there.
(IHMO) "(1<<3)" , "(1 << 2)", "(1 << 0)," space missing in "1" "<<" "3" code consistancy.

@brabo @danielinux Can you please give me a hand at code style / consistancy work.

kuldeepdhaka · 2017-05-21T07:03:57Z

include/unicore-mx/usbd/usbd.h

@@ -279,7 +284,7 @@ enum usbd_transfer_flags {
 	USBD_FLAG_PACKET_PER_FRAME_MASK = (0x3 << 5)
 };

-typedef enum usbd_transfer_flags usbd_transfer_flags;


possibly ive got a warning here? enum and typedef - with same name. its confusing me, i newer see such trick.
i`ve saw overriding enum item name with macro definition, but override enum by typedef - is it vlalid practice for C?

@alexrayne https://stackoverflow.com/questions/17918380/typedef-and-enum-with-same-name

yep, i know about namespaces, and that code was compiled succesfully.
when i ask about "valid practice for C" i mean - is it good? is it helpful? can it lead to some errors?
the name "usbd_transfer_flags" imho nott very suitable for enum. such name also can be imagine for struct of a binary set of flags.
anyway, if you insist - let`s revert this typedef back to code.

you forgot to denote
+typedef unsigned char usbd_transfer_flags;
ARMCC compiler generates warning when we try to assign to enum variable integer value (with set of flags).
therefore i denote for usbd_transfer_flags as numeric type

! :otg init:HS detection - now valid detection of HS core stm32F4 and F7 ! - fixed code style

…tatus check *dwc_otg:poll - poll skips most work ша core powered off

kuldeepdhaka · 2017-05-24T14:42:30Z

lib/usbd/backend/usbd_stm32_otg_fs.c

 	if (config->feature & USBD_VBUS_SENSE) {
-		if (OTG_FS_CID >= 0x00002000) { /* 2.0 HS core*/
+		if (OTG_FS_CID >= otg_hs_cid_boundary) { /* 2.0 HS core*/


OTG_FS_CID >= 0x00003000 leads to running 1.0 HS core init code on 2.0 HS core init

…n. therefore disconnection moved to background. background disconnects before PHY disabling, and restore disconnection status after.

kuldeepdhaka reviewed Feb 4, 2017

View reviewed changes

alexrayne added 2 commits February 4, 2017 12:52

+common:regs:dwc - append some USB FS power and session control bits …

21d87c3

…definition to DWC_OTG_GOTGINT, DWC_OTG_PCGCCTL

kuldeepdhaka reviewed Feb 4, 2017

View reviewed changes

kuldeepdhaka reviewed Feb 8, 2017

View reviewed changes

kuldeepdhaka mentioned this pull request Feb 8, 2017

USBD thing to be done (TODO) #42

Open

kuldeepdhaka closed this Feb 9, 2017

kuldeepdhaka reopened this Feb 9, 2017

*usbd:session - fixed code style, linked disconnect and enablevia fea…

65231b0

…ture.USBD_USE_POWERDOWN * warns - fixed some compiler warnings about mixing enum as integer

kuldeepdhaka reviewed May 21, 2017

View reviewed changes

alexrayne added 3 commits May 24, 2017 12:46

!usbd:session:usbd_enable - fixed disconnection on disabling

5624172

! :otg init:HS detection - now valid detection of HS core stm32F4 and F7 ! - fixed code style

+usbd:is_enabled - enhanse usbd_backend.power_control to allow core s…

c1cda4f

…tatus check *dwc_otg:poll - poll skips most work ша core powered off

!usbd:otg init:HS detection - valid CID for stm32f7

114158f

kuldeepdhaka reviewed May 24, 2017

View reviewed changes

alexrayne added 2 commits May 25, 2017 00:48

!usbd:powerdown:enable, disconnect - recursion release

d9d1cf9

!usbd:powedrown:disconnect -keep disconnection status during powerdow…

aff4105

…n. therefore disconnection moved to background. background disconnects before PHY disabling, and restore disconnection status after.

USBD add core-power and Vbus handle #50

Are you sure you want to change the base?

USBD add core-power and Vbus handle #50

Conversation

alexrayne commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrayne Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrayne Feb 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuldeepdhaka commented Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

alexrayne commented Feb 8, 2017 • edited Loading

kuldeepdhaka commented Feb 9, 2017

alexrayne commented Feb 9, 2017

alexrayne commented Feb 9, 2017

kuldeepdhaka commented Feb 9, 2017 • edited Loading

alexrayne commented Feb 9, 2017

kuldeepdhaka commented Feb 9, 2017

alexrayne commented Feb 9, 2017

kuldeepdhaka commented Feb 9, 2017

alexrayne commented Feb 9, 2017 • edited Loading

kuldeepdhaka commented Feb 9, 2017 • edited Loading

alexrayne commented Feb 19, 2017 • edited Loading

alexrayne commented Feb 20, 2017

danielinux commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrayne May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrayne May 24, 2017 • edited Loading

Choose a reason for hiding this comment

alexrayne May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrayne Feb 5, 2017 •

edited

Loading

alexrayne Feb 4, 2017 •

edited

Loading

kuldeepdhaka commented Feb 8, 2017 •

edited

Loading

alexrayne commented Feb 8, 2017 •

edited

Loading

kuldeepdhaka commented Feb 9, 2017 •

edited

Loading

alexrayne commented Feb 9, 2017 •

edited

Loading

kuldeepdhaka commented Feb 9, 2017 •

edited

Loading

alexrayne commented Feb 19, 2017 •

edited

Loading

alexrayne May 26, 2017 •

edited

Loading

alexrayne May 24, 2017 •

edited

Loading

alexrayne May 24, 2017 •

edited

Loading