From: Peter Chen <hidden> Date: 2017-06-13 02:00:07
This is a follow-up for my last power sequence framework patch set [1].
According to Rob Herring and Ulf Hansson's comments[2]. The kinds of
power sequence instances will be added at postcore_initcall, the match
criteria is compatible string first, if the compatible string is not
matched between dts and library, it will try to use generic power sequence.
The host driver just needs to call of_pwrseq_on/of_pwrseq_off
if only one power sequence instance is needed, for more power sequences
are used, using of_pwrseq_on_list/of_pwrseq_off_list instead (eg, USB hub driver).
In future, if there are special power sequence requirements, the special
power sequence library can be created.
This patch set is tested on i.mx6 sabresx evk using a dts change, I use
two hot-plug devices to simulate this use case, the related binding
change is updated at patch [1/6], The udoo board changes were tested
using my last power sequence patch set.[3]
Except for hard-wired MMC and USB devices, I find the USB ULPI PHY also
need to power on itself before it can be found by ULPI bus.
@Rafael, USB Maintainer has already acked the USB related changes, would you
consider queueing it at v4.13-rc1, sereval guys are waiting for this series.
Thanks.
Changes for v15:
- Change pwrseq list name at USB hub structure (pwrseq_on_list->pwrseq_list). [Patch 4/7]
- Add USB Maintainer's Ack [Patch 4/7]
Changes for v14:
- Rebase for v4.12-rc1
- Delete some USB sysdev patches which has already merged
Changes for v13:
- Add more design descriptions at design doc and fix one build error
introduced by v12 wrongly [Patch 2/12]
- Add the last three dts patches which were forgotten at last series
- Move the comment for usb_create_shared_hcd to correct place [Patch 3/12]
- Add sysdev for shared hcd too for xhci-plat.c [Patch 6/12]
Rafael, if the first two power sequence patches are ok for you, would you consider
accept these first, the other USB patches can go through USB tree at v4.12-rc1?
Changes for v12:
- Add design doc and more comments at generic power sequence source file [Patch 2/9]
- Introduce four Arnd Bergmann patches and one my ehci related patches, these patches
are used to get property DT/firmware information at USB code, and these information
are needed for power sequence operation at USB. With these five patches, my chipidea
hack patch in previous patch set can be removed. [Patch 3-7/9]
- Add -ENOENT judgement to avoid USB error if no power sequence library is chosen [9/9]
Changes for v11:
- Fix warning: (USB) selects POWER_SEQUENCE which has unmet direct dependencies (OF)
- Delete redundant copyright statement.
- Change pr_warn to pr_debug at wrseq_find_available_instance
- Refine kerneldoc
- %s/ENONET/ENOENT
- Allocate pwrseq list node before than carry out power sequence on
- Add mutex_lock/mutex_lock for pwrseq node browse at pwrseq_find_available_instance
- Add pwrseq_suspend/resume for API both single instance and list
- Add .pwrseq_suspend/resume for pwrseq_generic.c
- Add pwrseq_suspend_list and pwrseq_resume_list for USB hub suspend
and resume routine
Changes for v10:
- Improve the kernel-doc for power sequence core, including exported APIs and
main structure. [Patch 2/8]
- Change Kconfig, and let the user choose power sequence. [Patch 2/8]
- Delete EXPORT_SYMBOL and change related APIs as local, these APIs do not
be intended to export currently. [Patch 2/8]
- Selete POWER_SEQUENCE at USB core's Kconfig. [Patch 4/8]
Changes for v9:
- Add Vaibhav Hiremath's reviewed-by [Patch 4/8]
- Rebase to v4.9-rc1
Changes for v8:
- Allocate one extra pwrseq instance if pwrseq_get has succeed, it can avoid
preallocate instances problem which the number of instance is decided at
compile time, thanks for Heiko Stuebner's suggestion [Patch 2/8]
- Delete pwrseq_compatible_sample.c which is the demo purpose to show compatible
match method. [Patch 2/8]
- Add Maciej S. Szmigiero's tested-by. [Patch 7/8]
Changes for v7:
- Create kinds of power sequence instance at postcore_initcall, and match
the instance with node using compatible string, the beneit of this is
the host driver doesn't need to consider which pwrseq instance needs
to be used, and pwrseq core will match it, however, it eats some memories
if less power sequence instances are used. [Patch 2/8]
- Add pwrseq_compatible_sample.c to test match pwrseq using device_id. [Patch 2/8]
- Fix the comments Vaibhav Hiremath adds for error path for clock and do not
use device_node for parameters at pwrseq_on. [Patch 2/8]
- Simplify the caller to use power sequence, follows Alan's commnets [Patch 4/8]
- Tested three pwrseq instances together using both specific compatible string and
generic libraries.
Changes for v6:
- Add Matthias Kaehlcke's Reviewed-by and Tested-by. (patch [2/6])
- Change chipidea core of_node assignment for coming user. (patch [5/6])
- Applies Joshua Clayton's three dts changes for two boards,
the USB device's reg has only #address-cells, but without #size-cells.
Changes for v5:
- Delete pwrseq_register/pwrseq_unregister, which is useless currently
- Fix the linker error when the pwrseq user is compiled as module
Changes for v4:
- Create the patch on next-20160722
- Fix the of_node is not NULL after chipidea driver is unbinded [Patch 5/6]
- Using more friendly wait method for reset gpio [Patch 2/6]
- Support multiple input clocks [Patch 2/6]
- Add Rob Herring's ack for DT changes
- Add Joshua Clayton's Tested-by
Changes for v3:
- Delete "power-sequence" property at binding-doc, and change related code
at both library and user code.
- Change binding-doc example node name with Rob's comments
- of_get_named_gpio_flags only gets the gpio, but without setting gpio flags,
add additional code request gpio with proper gpio flags
- Add Philipp Zabel's Ack and MAINTAINER's entry
Changes for v2:
- Delete "pwrseq" prefix and clock-names for properties at dt binding
- Should use structure not but its pointer for kzalloc
- Since chipidea core has no of_node, let core's of_node equals glue
layer's at core's probe
[1] http://www.spinics.net/lists/linux-usb/msg142755.html
[2] http://www.spinics.net/lists/linux-usb/msg143106.html
[3] http://www.spinics.net/lists/linux-usb/msg142815.html
[4] http://www.spinics.net/lists/linux-usb/msg152375.html
Joshua Clayton (2):
ARM: dts: imx6qdl: Enable usb node children with <reg>
ARM: dts: imx6q-evi: Fix onboard hub reset line
Peter Chen (5):
binding-doc: power: pwrseq-generic: add binding doc for generic power
sequence library
power: add power sequence library
binding-doc: usb: usb-device: add optional properties for power
sequence
usb: core: add power sequence handling for USB devices
ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property
.../bindings/power/pwrseq/pwrseq-generic.txt | 48 +++
.../devicetree/bindings/usb/usb-device.txt | 10 +-
Documentation/power/power-sequence/design.rst | 54 ++++
MAINTAINERS | 9 +
arch/arm/boot/dts/imx6q-evi.dts | 25 +-
arch/arm/boot/dts/imx6qdl-udoo.dtsi | 26 +-
arch/arm/boot/dts/imx6qdl.dtsi | 6 +
drivers/power/Kconfig | 1 +
drivers/power/Makefile | 1 +
drivers/power/pwrseq/Kconfig | 20 ++
drivers/power/pwrseq/Makefile | 2 +
drivers/power/pwrseq/core.c | 335 +++++++++++++++++++++
drivers/power/pwrseq/pwrseq_generic.c | 234 ++++++++++++++
drivers/usb/Kconfig | 1 +
drivers/usb/core/hub.c | 49 ++-
drivers/usb/core/hub.h | 1 +
include/linux/power/pwrseq.h | 81 +++++
17 files changed, 866 insertions(+), 37 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
create mode 100644 Documentation/power/power-sequence/design.rst
create mode 100644 drivers/power/pwrseq/Kconfig
create mode 100644 drivers/power/pwrseq/Makefile
create mode 100644 drivers/power/pwrseq/core.c
create mode 100644 drivers/power/pwrseq/pwrseq_generic.c
create mode 100644 include/linux/power/pwrseq.h
--
2.7.4
@@ -0,0 +1,48 @@+The generic power sequence library++Some hard-wired devices (eg USB/MMC) need to do power sequence before+the device can be enumerated on the bus, the typical power sequence+like: enable USB PHY clock, toggle reset pin, etc. But current+Linux device driver lacks of such code to do it, it may cause some+hard-wired devices works abnormal or can't be recognized by+controller at all. The power sequence will be done before this device+can be found at the bus.++The power sequence properties is under the device node.++Optional properties:+- clocks: the input clocks for device.+- reset-gpios: Should specify the GPIO for reset.+- reset-duration-us: the duration in microsecond for assert reset signal.++Below is the example of USB power sequence properties on USB device+nodes which have two level USB hubs.++&usbotg1 {+ vbus-supply = <®_usb_otg1_vbus>;+ pinctrl-names = "default";+ pinctrl-0 = <&pinctrl_usb_otg1_id>;+ status = "okay";++ #address-cells = <1>;+ #size-cells = <0>;+ genesys: hub at 1 {+ compatible = "usb5e3,608";+ reg = <1>;++ clocks = <&clks IMX6SX_CLK_CKO>;+ reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */+ reset-duration-us = <10>;++ #address-cells = <1>;+ #size-cells = <0>;+ asix: ethernet at 1 {+ compatible = "usbb95,1708";+ reg = <1>;++ clocks = <&clks IMX6SX_CLK_IPG>;+ reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */+ reset-duration-us = <15>;+ };+ };+};
@@ -13,6 +13,10 @@ Required properties: - reg: the port number which this device is connecting to, the range is 1-31.+Optional properties:+power sequence properties, see+Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt for detail+ Example: &usb1 {
From: Peter Chen <hidden> Date: 2017-06-13 02:00:53
From: Joshua Clayton <redacted>
Give usb nodes #address and #size attributes, so that a child node
representing a permanently connected device such as an onboard hub may
be addressed with a <reg> attribute
Signed-off-by: Joshua Clayton <redacted>
Signed-off-by: Peter Chen <redacted>
---
arch/arm/boot/dts/imx6qdl.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
From: Peter Chen <hidden> Date: 2017-06-13 02:01:03
The current dts describes USB HUB's property at USB controller's
entry, it is improper. The USB HUB should be the child node
under USB controller, and power sequence properties are under
it. Besides, using gpio pinctrl setting for USB2415's reset pin.
Signed-off-by: Peter Chen <redacted>
Signed-off-by: Joshua Clayton <redacted>
Tested-by: Maciej S. Szmigiero <redacted>
---
arch/arm/boot/dts/imx6qdl-udoo.dtsi | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
@@ -58,17 +60,6 @@#address-cells=<1>;#size-cells=<0>;-reg_usb_h1_vbus:regulatorat0{-compatible="regulator-fixed";-reg=<0>;-regulator-name="usb_h1_vbus";-regulator-min-microvolt=<5000000>;-regulator-max-microvolt=<5000000>;-enable-active-high;-startup-delay-us=<2>;/* USB2415 requires a POR of 1 us minimum */-gpio=<&gpio7120>;-};-reg_panel:regulatorat1{compatible="regulator-fixed";reg=<1>;
From: Peter Chen <hidden> Date: 2017-06-13 02:01:15
From: Joshua Clayton <redacted>
Previously the onboard hub was made to work by treating its
reset gpio as a regulator enable.
Get rid of that kludge now that pwseq has added reset gpio support
Move pin muxing the hub reset pin into the usbh1 group
Signed-off-by: Joshua Clayton <redacted>
Signed-off-by: Peter Chen <redacted>
---
arch/arm/boot/dts/imx6q-evi.dts | 25 +++++++------------------
1 file changed, 7 insertions(+), 18 deletions(-)
From: Peter Chen <hidden> Date: 2017-06-13 02:01:49
Some hard-wired USB devices need to do power sequence to let the
device work normally, the typical power sequence like: enable USB
PHY clock, toggle reset pin, etc. But current Linux USB driver
lacks of such code to do it, it may cause some hard-wired USB devices
works abnormal or can't be recognized by controller at all.
In this patch, it calls power sequence library APIs to finish
the power sequence events. It will do power on sequence at hub's
probe for all devices under this hub (includes root hub).
At hub_disconnect, it will do power off sequence which is at powered
on list.
Signed-off-by: Peter Chen <redacted>
Tested-by Joshua Clayton [off-list ref]
Tested-by: Maciej S. Szmigiero <redacted>
Reviewed-by: Vaibhav Hiremath <hvaibhav.linux@gmail.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
drivers/usb/Kconfig | 1 +
drivers/usb/core/hub.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
drivers/usb/core/hub.h | 1 +
3 files changed, 47 insertions(+), 4 deletions(-)
From: Peter Chen <hidden> Date: 2017-06-13 02:02:40
We have an well-known problem that the device needs to do some power
sequence before it can be recognized by related host, the typical
example like hard-wired mmc devices and usb devices.
This power sequence is hard to be described at device tree and handled by
related host driver, so we have created a common power sequence
library to cover this requirement. The core code has supplied
some common helpers for host driver, and individual power sequence
libraries handle kinds of power sequence for devices. The pwrseq
librares always need to allocate extra instance for compatible
string match.
pwrseq_generic is intended for general purpose of power sequence, which
handles gpios and clocks currently, and can cover other controls in
future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
if only one power sequence is needed, else call of_pwrseq_on_list
/of_pwrseq_off_list instead (eg, USB hub driver).
For new power sequence library, it can add its compatible string
to pwrseq_of_match_table, then the pwrseq core will match it with
DT's, and choose this library at runtime.
Signed-off-by: Peter Chen <redacted>
Tested-by: Maciej S. Szmigiero <redacted>
Tested-by Joshua Clayton [off-list ref]
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
---
Documentation/power/power-sequence/design.rst | 54 +++++
MAINTAINERS | 9 +
drivers/power/Kconfig | 1 +
drivers/power/Makefile | 1 +
drivers/power/pwrseq/Kconfig | 20 ++
drivers/power/pwrseq/Makefile | 2 +
drivers/power/pwrseq/core.c | 335 ++++++++++++++++++++++++++
drivers/power/pwrseq/pwrseq_generic.c | 234 ++++++++++++++++++
include/linux/power/pwrseq.h | 81 +++++++
9 files changed, 737 insertions(+)
create mode 100644 Documentation/power/power-sequence/design.rst
create mode 100644 drivers/power/pwrseq/Kconfig
create mode 100644 drivers/power/pwrseq/Makefile
create mode 100644 drivers/power/pwrseq/core.c
create mode 100644 drivers/power/pwrseq/pwrseq_generic.c
create mode 100644 include/linux/power/pwrseq.h
@@ -0,0 +1,54 @@+====================================+Power Sequence Library+====================================++:Date: Feb, 2017+:Author: Peter Chen <peter.chen@nxp.com>+++Introduction+============++We have an well-known problem that the device needs to do a power+sequence before it can be recognized by related host, the typical+examples are hard-wired mmc devices and usb devices. The host controller+can't know what kinds of this device is in its bus if the power+sequence has not done, since the related devices driver's probe calling+is determined by runtime according to eunumeration results. Besides,+the devices may have custom power sequence, so the power sequence library+which is independent with the devices is needed.++Design+============++The power sequence library includes the core file and customer power+sequence library. The core file exports interfaces are called by+host controller driver for power sequence and customer power sequence+library files to register its power sequence instance to global+power sequence list. The custom power sequence library creates power+sequence instance and implement custom power sequence.++Since the power sequence describes hardware design, the description is+located at board description file, eg, device tree dts file. And+a specific power sequence belongs to device, so its description+is under the device node, please refer to:+Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt++Custom power sequence library allocates one power sequence instance at+bootup periods using postcore_initcall, this static allocated instance is+used to compare with device-tree (DT) node to see if this library can be+used for the node or not. When the result is matched, the core API will+try to get resourses (->get, implemented at each library) for power+sequence, if all resources are got, it will try to allocate another+instance for next possible request from host driver.++Then, the host controller driver can carry out power sequence on for this+DT node, the library will do corresponding operations, like open clocks,+toggle gpio, etc. The power sequence off routine will close and free the+resources, and is called when the parent is removed. And the power+sequence suspend and resume routine can be called at host driver's+suspend and resume routine if needed.++The exported interfaces+..kernel-doc:: drivers/power/pwrseq/core.c+:export:
@@ -0,0 +1,335 @@+/*+*core.cpowersequencecorefile+*+*Copyright(C)2016FreescaleSemiconductor,Inc.+*Author:PeterChen<peter.chen@nxp.com>+*+*Thisprogramisfreesoftware:youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseversion2of+*theLicenseaspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*alongwiththisprogram.+*/++#include<linux/list.h>+#include<linux/mutex.h>+#include<linux/of.h>+#include<linux/slab.h>+#include<linux/power/pwrseq.h>++staticDEFINE_MUTEX(pwrseq_list_mutex);+staticLIST_HEAD(pwrseq_list);++staticintpwrseq_get(structdevice_node*np,structpwrseq*p)+{+if(p&&p->get)+returnp->get(np,p);++return-ENOTSUPP;+}++staticintpwrseq_on(structpwrseq*p)+{+if(p&&p->on)+returnp->on(p);++return-ENOTSUPP;+}++staticvoidpwrseq_off(structpwrseq*p)+{+if(p&&p->off)+p->off(p);+}++staticvoidpwrseq_put(structpwrseq*p)+{+if(p&&p->put)+p->put(p);+}++/**+*pwrseq_register-Addpwrseqinstancetoglobalpwrseqlist+*+*@pwrseq:thepwrseqinstance+*/+voidpwrseq_register(structpwrseq*pwrseq)+{+mutex_lock(&pwrseq_list_mutex);+list_add(&pwrseq->node,&pwrseq_list);+mutex_unlock(&pwrseq_list_mutex);+}+EXPORT_SYMBOL_GPL(pwrseq_register);++/**+*pwrseq_unregister-Removepwrseqinstancefromglobalpwrseqlist+*+*@pwrseq:thepwrseqinstance+*/+voidpwrseq_unregister(structpwrseq*pwrseq)+{+mutex_lock(&pwrseq_list_mutex);+list_del(&pwrseq->node);+mutex_unlock(&pwrseq_list_mutex);+}+EXPORT_SYMBOL_GPL(pwrseq_unregister);++staticstructpwrseq*pwrseq_find_available_instance(structdevice_node*np)+{+structpwrseq*pwrseq;++mutex_lock(&pwrseq_list_mutex);+list_for_each_entry(pwrseq,&pwrseq_list,node){+if(pwrseq->used)+continue;++/* compare compatible string for pwrseq node */+if(of_match_node(pwrseq->pwrseq_of_match_table,np)){+pwrseq->used=true;+mutex_unlock(&pwrseq_list_mutex);+returnpwrseq;+}++/* return generic pwrseq instance */+if(!strcmp(pwrseq->pwrseq_of_match_table->compatible,+"generic")){+pr_debug("using generic pwrseq instance for %s\n",+np->full_name);+pwrseq->used=true;+mutex_unlock(&pwrseq_list_mutex);+returnpwrseq;+}+}+mutex_unlock(&pwrseq_list_mutex);+pr_debug("Can't find any pwrseq instances for %s\n",np->full_name);++returnNULL;+}++/**+*of_pwrseq_on-Carryoutpowersequenceonfordevicenode+*+*@np:thedevicenodewouldliketopoweron+*+*Carryoutasingledevicepoweron.Ifmultipledevices+*needtobehandled,useof_pwrseq_on_list()instead.+*+*Returnapointertothepowersequenceinstanceonsuccess,+*oranerrorcodeotherwise.+*/+structpwrseq*of_pwrseq_on(structdevice_node*np)+{+structpwrseq*pwrseq;+intret;++pwrseq=pwrseq_find_available_instance(np);+if(!pwrseq)+returnERR_PTR(-ENOENT);++ret=pwrseq_get(np,pwrseq);+if(ret){+/* Mark current pwrseq as unused */+pwrseq->used=false;+returnERR_PTR(ret);+}++ret=pwrseq_on(pwrseq);+if(ret)+gotopwr_put;++returnpwrseq;++pwr_put:+pwrseq_put(pwrseq);+returnERR_PTR(ret);+}+EXPORT_SYMBOL_GPL(of_pwrseq_on);++/**+*of_pwrseq_off-Carryoutpowersequenceoffforthispwrseqinstance+*+*@pwrseq:thepwrseqinstancewhichrelateddevicewouldliketobeoff+*+*ThisAPIisusedtopoweroffsingledevice,itistheopposite+*operationforof_pwrseq_on.+*/+voidof_pwrseq_off(structpwrseq*pwrseq)+{+pwrseq_off(pwrseq);+pwrseq_put(pwrseq);+}+EXPORT_SYMBOL_GPL(of_pwrseq_off);++/**+*of_pwrseq_on_list-Carryoutpowersequenceonforlist+*+*@np:thedevicenodewouldliketopoweron+*@head:thelistheadforpwrseqlistonthisbus+*+*ThisAPIisusedtopoweronmultipledevicesatsinglebus.+*Ifthereareseveraldevicesonbus(eg,USBbus),usesthis+*thisAPI.Otherwise,useof_pwrseq_oninstead.Afterthedevice+*ispoweredonsuccessfully,itwillbeaddedtopwrseqlistfor+*thisbus.Thecallerneedstousemutex_lockforconcurrent.+*+*Return0onsuccess,oranerrorvalueotherwise.+*/+intof_pwrseq_on_list(structdevice_node*np,structlist_head*head)+{+structpwrseq*pwrseq;+structpwrseq_list_per_dev*pwrseq_list_node;++pwrseq_list_node=kzalloc(sizeof(*pwrseq_list_node),GFP_KERNEL);+if(!pwrseq_list_node)+return-ENOMEM;++pwrseq=of_pwrseq_on(np);+if(IS_ERR(pwrseq)){+kfree(pwrseq_list_node);+returnPTR_ERR(pwrseq);+}++pwrseq_list_node->pwrseq=pwrseq;+list_add(&pwrseq_list_node->list,head);++return0;+}+EXPORT_SYMBOL_GPL(of_pwrseq_on_list);++/**+*of_pwrseq_off_list-Carryoutpowersequenceoffforthelist+*+*@head:thelistheadforpwrseqinstancelistonthisbus+*+*ThisAPIisusedtopoweroffalldevicesonthisbus,itis+*theoppositeoperationforof_pwrseq_on_list.+*Thecallerneedstousemutex_lockforconcurrent.+*/+voidof_pwrseq_off_list(structlist_head*head)+{+structpwrseq*pwrseq;+structpwrseq_list_per_dev*pwrseq_list_node,*tmp_node;++list_for_each_entry_safe(pwrseq_list_node,tmp_node,head,list){+pwrseq=pwrseq_list_node->pwrseq;+of_pwrseq_off(pwrseq);+list_del(&pwrseq_list_node->list);+kfree(pwrseq_list_node);+}+}+EXPORT_SYMBOL_GPL(of_pwrseq_off_list);++/**+*pwrseq_suspend-Carryoutpowersequencesuspendforthispwrseqinstance+*+*@pwrseq:thepwrseqinstance+*+*ThisAPIisusedtodosuspendoperationonpwrseqinstance.+*+*Return0onsuccess,oranerrorvalueotherwise.+*/+intpwrseq_suspend(structpwrseq*p)+{+intret=0;++if(p&&p->suspend)+ret=p->suspend(p);+else+returnret;++if(!ret)+p->suspended=true;+else+pr_err("%s failed\n",__func__);++returnret;+}+EXPORT_SYMBOL_GPL(pwrseq_suspend);++/**+*pwrseq_resume-Carryoutpowersequenceresumeforthispwrseqinstance+*+*@pwrseq:thepwrseqinstance+*+*ThisAPIisusedtodoresumeoperationonpwrseqinstance.+*+*Return0onsuccess,oranerrorvalueotherwise.+*/+intpwrseq_resume(structpwrseq*p)+{+intret=0;++if(p&&p->resume)+ret=p->resume(p);+else+returnret;++if(!ret)+p->suspended=false;+else+pr_err("%s failed\n",__func__);++returnret;+}+EXPORT_SYMBOL_GPL(pwrseq_resume);++/**+*pwrseq_suspend_list-Carryoutpowersequencesuspendforlist+*+*@head:thelistheadforpwrseqinstancelistonthisbus+*+*ThisAPIisusedtodosuspendonallpowersequenceinstancesonthisbus.+*Thecallerneedstousemutex_lockforconcurrent.+*/+intpwrseq_suspend_list(structlist_head*head)+{+structpwrseq*pwrseq;+structpwrseq_list_per_dev*pwrseq_list_node;+intret=0;++list_for_each_entry(pwrseq_list_node,head,list){+ret=pwrseq_suspend(pwrseq_list_node->pwrseq);+if(ret)+break;+}++if(ret){+list_for_each_entry(pwrseq_list_node,head,list){+pwrseq=pwrseq_list_node->pwrseq;+if(pwrseq->suspended)+pwrseq_resume(pwrseq);+}+}++returnret;+}+EXPORT_SYMBOL_GPL(pwrseq_suspend_list);++/**+*pwrseq_resume_list-Carryoutpowersequenceresumeforthelist+*+*@head:thelistheadforpwrseqinstancelistonthisbus+*+*ThisAPIisusedtodoresumeonallpowersequenceinstancesonthisbus.+*Thecallerneedstousemutex_lockforconcurrent.+*/+intpwrseq_resume_list(structlist_head*head)+{+structpwrseq_list_per_dev*pwrseq_list_node;+intret=0;++list_for_each_entry(pwrseq_list_node,head,list){+ret=pwrseq_resume(pwrseq_list_node->pwrseq);+if(ret)+break;+}++returnret;+}+EXPORT_SYMBOL_GPL(pwrseq_resume_list);
@@ -0,0 +1,234 @@+/*+*pwrseq_generic.cGenericpowersequencehandling+*+*Copyright(C)2016FreescaleSemiconductor,Inc.+*Author:PeterChen<peter.chen@nxp.com>+*+*Thisprogramisfreesoftware:youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseversion2of+*theLicenseaspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*alongwiththisprogram.+*/++#include<linux/clk.h>+#include<linux/delay.h>+#include<linux/gpio.h>+#include<linux/gpio/consumer.h>+#include<linux/of.h>+#include<linux/of_gpio.h>+#include<linux/slab.h>++#include<linux/power/pwrseq.h>++structpwrseq_generic{+structpwrseqpwrseq;+structgpio_desc*gpiod_reset;+structclk*clks[PWRSEQ_MAX_CLKS];+u32duration_us;+boolsuspended;+};++#define to_generic_pwrseq(p) container_of(p, struct pwrseq_generic, pwrseq)++staticintpwrseq_generic_alloc_instance(void);+staticconststructof_device_idgeneric_id_table[]={+{.compatible="generic",},+{/* sentinel */}+};++staticintpwrseq_generic_suspend(structpwrseq*pwrseq)+{+structpwrseq_generic*pwrseq_gen=to_generic_pwrseq(pwrseq);+intclk;++for(clk=PWRSEQ_MAX_CLKS-1;clk>=0;clk--)+clk_disable_unprepare(pwrseq_gen->clks[clk]);++pwrseq_gen->suspended=true;+return0;+}++staticintpwrseq_generic_resume(structpwrseq*pwrseq)+{+structpwrseq_generic*pwrseq_gen=to_generic_pwrseq(pwrseq);+intclk,ret=0;++for(clk=0;clk<PWRSEQ_MAX_CLKS&&pwrseq_gen->clks[clk];clk++){+ret=clk_prepare_enable(pwrseq_gen->clks[clk]);+if(ret){+pr_err("Can't enable clock, ret=%d\n",ret);+gotoerr_disable_clks;+}+}++pwrseq_gen->suspended=false;+returnret;++err_disable_clks:+while(--clk>=0)+clk_disable_unprepare(pwrseq_gen->clks[clk]);++returnret;+}++staticvoidpwrseq_generic_put(structpwrseq*pwrseq)+{+structpwrseq_generic*pwrseq_gen=to_generic_pwrseq(pwrseq);+intclk;++if(pwrseq_gen->gpiod_reset)+gpiod_put(pwrseq_gen->gpiod_reset);++for(clk=0;clk<PWRSEQ_MAX_CLKS;clk++)+clk_put(pwrseq_gen->clks[clk]);++pwrseq_unregister(&pwrseq_gen->pwrseq);+kfree(pwrseq_gen);+}++staticvoidpwrseq_generic_off(structpwrseq*pwrseq)+{+structpwrseq_generic*pwrseq_gen=to_generic_pwrseq(pwrseq);+intclk;++if(pwrseq_gen->suspended)+return;++for(clk=PWRSEQ_MAX_CLKS-1;clk>=0;clk--)+clk_disable_unprepare(pwrseq_gen->clks[clk]);+}++staticintpwrseq_generic_on(structpwrseq*pwrseq)+{+structpwrseq_generic*pwrseq_gen=to_generic_pwrseq(pwrseq);+intclk,ret=0;+structgpio_desc*gpiod_reset=pwrseq_gen->gpiod_reset;++for(clk=0;clk<PWRSEQ_MAX_CLKS&&pwrseq_gen->clks[clk];clk++){+ret=clk_prepare_enable(pwrseq_gen->clks[clk]);+if(ret){+pr_err("Can't enable clock, ret=%d\n",ret);+gotoerr_disable_clks;+}+}++if(gpiod_reset){+u32duration_us=pwrseq_gen->duration_us;++if(duration_us<=10)+udelay(10);+else+usleep_range(duration_us,duration_us+100);+gpiod_set_value(gpiod_reset,0);+}++returnret;++err_disable_clks:+while(--clk>=0)+clk_disable_unprepare(pwrseq_gen->clks[clk]);++returnret;+}++staticintpwrseq_generic_get(structdevice_node*np,structpwrseq*pwrseq)+{+structpwrseq_generic*pwrseq_gen=to_generic_pwrseq(pwrseq);+enumof_gpio_flagsflags;+intreset_gpio,clk,ret=0;++for(clk=0;clk<PWRSEQ_MAX_CLKS;clk++){+pwrseq_gen->clks[clk]=of_clk_get(np,clk);+if(IS_ERR(pwrseq_gen->clks[clk])){+ret=PTR_ERR(pwrseq_gen->clks[clk]);+if(ret!=-ENOENT)+gotoerr_put_clks;+pwrseq_gen->clks[clk]=NULL;+break;+}+}++reset_gpio=of_get_named_gpio_flags(np,"reset-gpios",0,&flags);+if(gpio_is_valid(reset_gpio)){+unsignedlonggpio_flags;++if(flags&OF_GPIO_ACTIVE_LOW)+gpio_flags=GPIOF_ACTIVE_LOW|GPIOF_OUT_INIT_LOW;+else+gpio_flags=GPIOF_OUT_INIT_HIGH;++ret=gpio_request_one(reset_gpio,gpio_flags,+"pwrseq-reset-gpios");+if(ret)+gotoerr_put_clks;++pwrseq_gen->gpiod_reset=gpio_to_desc(reset_gpio);+of_property_read_u32(np,"reset-duration-us",+&pwrseq_gen->duration_us);+}elseif(reset_gpio==-ENOENT){+;/* no such gpio */+}else{+ret=reset_gpio;+pr_err("Failed to get reset gpio on %s, err = %d\n",+np->full_name,reset_gpio);+gotoerr_put_clks;+}++/* allocate new one for later pwrseq instance request */+ret=pwrseq_generic_alloc_instance();+if(ret)+gotoerr_put_gpio;++return0;++err_put_gpio:+if(pwrseq_gen->gpiod_reset)+gpiod_put(pwrseq_gen->gpiod_reset);+err_put_clks:+while(--clk>=0)+clk_put(pwrseq_gen->clks[clk]);+returnret;+}++/**+*pwrseq_generic_alloc_instance-powersequenceinstanceallocation+*+*Thisfunctionisusedtoallocateonegenericpowersequenceinstance,+*itiscalledwhenthesystembootsupandafteronepowersequence+*instanceisgotsuccessfully.+*+*Returnzeroonsuccessoranerrorcodeotherwise.+*/+staticintpwrseq_generic_alloc_instance(void)+{+structpwrseq_generic*pwrseq_gen;++pwrseq_gen=kzalloc(sizeof(*pwrseq_gen),GFP_KERNEL);+if(!pwrseq_gen)+return-ENOMEM;++pwrseq_gen->pwrseq.pwrseq_of_match_table=generic_id_table;+pwrseq_gen->pwrseq.get=pwrseq_generic_get;+pwrseq_gen->pwrseq.on=pwrseq_generic_on;+pwrseq_gen->pwrseq.off=pwrseq_generic_off;+pwrseq_gen->pwrseq.put=pwrseq_generic_put;+pwrseq_gen->pwrseq.suspend=pwrseq_generic_suspend;+pwrseq_gen->pwrseq.resume=pwrseq_generic_resume;++pwrseq_register(&pwrseq_gen->pwrseq);+return0;+}++/* Allocate one pwrseq instance during boots up */+staticint__initpwrseq_generic_register(void)+{+returnpwrseq_generic_alloc_instance();+}+postcore_initcall(pwrseq_generic_register)
@@ -0,0 +1,81 @@+#ifndef __LINUX_PWRSEQ_H+#define __LINUX_PWRSEQ_H++#include<linux/of.h>++#define PWRSEQ_MAX_CLKS 3++/**+*structpwrseq-thepowersequencestructure+*@pwrseq_of_match_table:theOFdeviceidtablethispwrseqlibrarysupports+*@node:thelistpointertobeaddedtopwrseqlist+*@get:theAPIisusedtogetpwrseqinstancefromthedevicenode+*@on:dopoweronforthispwrseqinstance+*@off:dopoweroffforthispwrseqinstance+*@put:releasetheresourcesonthispwrseqinstance+*@suspend:dosuspendoperationonthispwrseqinstance+*@resume:doresumeoperationonthispwrseqinstance+*@used:thispwrseqinstanceisusedbydevice+*/+structpwrseq{+conststructof_device_id*pwrseq_of_match_table;+structlist_headnode;+int(*get)(structdevice_node*np,structpwrseq*p);+int(*on)(structpwrseq*p);+void(*off)(structpwrseq*p);+void(*put)(structpwrseq*p);+int(*suspend)(structpwrseq*p);+int(*resume)(structpwrseq*p);+boolused;+boolsuspended;+};++/* used for power sequence instance list in one driver */+structpwrseq_list_per_dev{+structpwrseq*pwrseq;+structlist_headlist;+};++#if IS_ENABLED(CONFIG_POWER_SEQUENCE)+voidpwrseq_register(structpwrseq*pwrseq);+voidpwrseq_unregister(structpwrseq*pwrseq);+structpwrseq*of_pwrseq_on(structdevice_node*np);+voidof_pwrseq_off(structpwrseq*pwrseq);+intof_pwrseq_on_list(structdevice_node*np,structlist_head*head);+voidof_pwrseq_off_list(structlist_head*head);+intpwrseq_suspend(structpwrseq*p);+intpwrseq_resume(structpwrseq*p);+intpwrseq_suspend_list(structlist_head*head);+intpwrseq_resume_list(structlist_head*head);+#else+staticinlinevoidpwrseq_register(structpwrseq*pwrseq){}+staticinlinevoidpwrseq_unregister(structpwrseq*pwrseq){}+staticinlinestructpwrseq*of_pwrseq_on(structdevice_node*np)+{+returnNULL;+}+staticvoidof_pwrseq_off(structpwrseq*pwrseq){}+staticintof_pwrseq_on_list(structdevice_node*np,structlist_head*head)+{+return0;+}+staticvoidof_pwrseq_off_list(structlist_head*head){}+staticintpwrseq_suspend(structpwrseq*p)+{+return0;+}+staticintpwrseq_resume(structpwrseq*p)+{+return0;+}+staticintpwrseq_suspend_list(structlist_head*head)+{+return0;+}+staticintpwrseq_resume_list(structlist_head*head)+{+return0;+}+#endif /* CONFIG_POWER_SEQUENCE */++#endif /* __LINUX_PWRSEQ_H */
+
+/**
+ * of_pwrseq_on - Carry out power sequence on for device node
+ *
+ * @np: the device node would like to power on
+ *
+ * Carry out a single device power on. If multiple devices
+ * need to be handled, use of_pwrseq_on_list() instead.
+ *
+ * Return a pointer to the power sequence instance on success,
+ * or an error code otherwise.
+ */
+struct pwrseq *of_pwrseq_on(struct device_node *np)
+{
+ struct pwrseq *pwrseq;
+ int ret;
+
+ pwrseq = pwrseq_find_available_instance(np);
+ if (!pwrseq)
+ return ERR_PTR(-ENOENT);
In case the pwrseq instance hasn't been registered yet, then there is
no way to deal with -EPROBE_DEFER properly here.
I haven't been following the discussions in-depth during all
iterations, so perhaps you have already discussed why doing it like
this.
Anyway, that means all pwrseq instances needs to be registered an
early boot level, to be safe. To me, that seems like poor design
choice.
[...]
Otherwise I think this looks okay to me.
Kind regards
Uffe
From: Peter Chen <hidden> Date: 2017-06-14 01:54:37
On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
[...]
quoted
+
+/**
+ * of_pwrseq_on - Carry out power sequence on for device node
+ *
+ * @np: the device node would like to power on
+ *
+ * Carry out a single device power on. If multiple devices
+ * need to be handled, use of_pwrseq_on_list() instead.
+ *
+ * Return a pointer to the power sequence instance on success,
+ * or an error code otherwise.
+ */
+struct pwrseq *of_pwrseq_on(struct device_node *np)
+{
+ struct pwrseq *pwrseq;
+ int ret;
+
+ pwrseq = pwrseq_find_available_instance(np);
+ if (!pwrseq)
+ return ERR_PTR(-ENOENT);
In case the pwrseq instance hasn't been registered yet, then there is
no way to deal with -EPROBE_DEFER properly here.
I haven't been following the discussions in-depth during all
iterations, so perhaps you have already discussed why doing it like
this.
Yes, it has been discussed. In order to compare with compatible string
at dts, we need to have one registered pwrseq instance for each
pwrseq library, this pre-registered one is allocated using
postcore_initcall, and the new (eg, second) instance is registered
after pwrseq_get has succeeded.
Peter
Anyway, that means all pwrseq instances needs to be registered an
early boot level, to be safe. To me, that seems like poor design
choice.
Otherwise I think this looks okay to me.
On 14 June 2017 at 03:53, Peter Chen [off-list ref] wrote:
On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
quoted
[...]
quoted
+
+/**
+ * of_pwrseq_on - Carry out power sequence on for device node
+ *
+ * @np: the device node would like to power on
+ *
+ * Carry out a single device power on. If multiple devices
+ * need to be handled, use of_pwrseq_on_list() instead.
+ *
+ * Return a pointer to the power sequence instance on success,
+ * or an error code otherwise.
+ */
+struct pwrseq *of_pwrseq_on(struct device_node *np)
+{
+ struct pwrseq *pwrseq;
+ int ret;
+
+ pwrseq = pwrseq_find_available_instance(np);
+ if (!pwrseq)
+ return ERR_PTR(-ENOENT);
In case the pwrseq instance hasn't been registered yet, then there is
no way to deal with -EPROBE_DEFER properly here.
I haven't been following the discussions in-depth during all
iterations, so perhaps you have already discussed why doing it like
this.
Yes, it has been discussed. In order to compare with compatible string
at dts, we need to have one registered pwrseq instance for each
pwrseq library, this pre-registered one is allocated using
postcore_initcall, and the new (eg, second) instance is registered
after pwrseq_get has succeeded.
I understand you need one compatible per pwrseq library, but how does
that have anything to do with -EPROBE_DEFER?
My point is that, if a driver calls of_pwrseq_on() (which calls
pwrseq_find_available_instance()), but the corresponding pwrseq
library and instance has not yet been registered for that device. Then
how will you handle -EPROBE_DEFER? I guess you simply can't, which is
why *all* pwrseq libraries needs to be registered in early boot phase,
like at postcore_initcall(). Right?
If that is the case, I really don't like it.
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Sorry for jumping in late, however to me it seems like there is still
some pieces missing to make this work.
[...]
Kind regards
Uffe
From: Peter Chen <hidden> Date: 2017-06-15 06:59:38
On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote:
On 14 June 2017 at 03:53, Peter Chen [off-list ref] wrote:
quoted
On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
quoted
[...]
quoted
+
+/**
+ * of_pwrseq_on - Carry out power sequence on for device node
+ *
+ * @np: the device node would like to power on
+ *
+ * Carry out a single device power on. If multiple devices
+ * need to be handled, use of_pwrseq_on_list() instead.
+ *
+ * Return a pointer to the power sequence instance on success,
+ * or an error code otherwise.
+ */
+struct pwrseq *of_pwrseq_on(struct device_node *np)
+{
+ struct pwrseq *pwrseq;
+ int ret;
+
+ pwrseq = pwrseq_find_available_instance(np);
+ if (!pwrseq)
+ return ERR_PTR(-ENOENT);
In case the pwrseq instance hasn't been registered yet, then there is
no way to deal with -EPROBE_DEFER properly here.
I haven't been following the discussions in-depth during all
iterations, so perhaps you have already discussed why doing it like
this.
Yes, it has been discussed. In order to compare with compatible string
at dts, we need to have one registered pwrseq instance for each
pwrseq library, this pre-registered one is allocated using
postcore_initcall, and the new (eg, second) instance is registered
after pwrseq_get has succeeded.
I understand you need one compatible per pwrseq library, but how does
that have anything to do with -EPROBE_DEFER?
My point is that, if a driver calls of_pwrseq_on() (which calls
pwrseq_find_available_instance()), but the corresponding pwrseq
library and instance has not yet been registered for that device. Then
how will you handle -EPROBE_DEFER? I guess you simply can't, which is
why *all* pwrseq libraries needs to be registered in early boot phase,
like at postcore_initcall(). Right?
If that is the case, I really don't like it.
Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.
Sorry for jumping in late, however to me it seems like there is still
some pieces missing to make this work.
[...]
Kind regards
Uffe
On 15 June 2017 at 08:58, Peter Chen [off-list ref] wrote:
On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote:
quoted
On 14 June 2017 at 03:53, Peter Chen [off-list ref] wrote:
quoted
On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
quoted
[...]
quoted
+
+/**
+ * of_pwrseq_on - Carry out power sequence on for device node
+ *
+ * @np: the device node would like to power on
+ *
+ * Carry out a single device power on. If multiple devices
+ * need to be handled, use of_pwrseq_on_list() instead.
+ *
+ * Return a pointer to the power sequence instance on success,
+ * or an error code otherwise.
+ */
+struct pwrseq *of_pwrseq_on(struct device_node *np)
+{
+ struct pwrseq *pwrseq;
+ int ret;
+
+ pwrseq = pwrseq_find_available_instance(np);
+ if (!pwrseq)
+ return ERR_PTR(-ENOENT);
In case the pwrseq instance hasn't been registered yet, then there is
no way to deal with -EPROBE_DEFER properly here.
I haven't been following the discussions in-depth during all
iterations, so perhaps you have already discussed why doing it like
this.
Yes, it has been discussed. In order to compare with compatible string
at dts, we need to have one registered pwrseq instance for each
pwrseq library, this pre-registered one is allocated using
postcore_initcall, and the new (eg, second) instance is registered
after pwrseq_get has succeeded.
I understand you need one compatible per pwrseq library, but how does
that have anything to do with -EPROBE_DEFER?
My point is that, if a driver calls of_pwrseq_on() (which calls
pwrseq_find_available_instance()), but the corresponding pwrseq
library and instance has not yet been registered for that device. Then
how will you handle -EPROBE_DEFER? I guess you simply can't, which is
why *all* pwrseq libraries needs to be registered in early boot phase,
like at postcore_initcall(). Right?
If that is the case, I really don't like it.
Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.
In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.
In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.
quoted
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.
Another option is to entirely skip to two step approach.
In other words, make the library to cope with multiple users via the
same registered library instance.
[...]
Kind regards
Uffe
From: Peter Chen <hidden> Date: 2017-06-15 09:12:11
On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
quoted
Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.
In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.
In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.
The most difficulty for this is we can't know whether the requested
pwrseq instance will be registered or not, the kernel configuration
for this pwrseq library may not be chosen at all.
quoted
quoted
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.
Another option is to entirely skip to two step approach.
In other words, make the library to cope with multiple users via the
same registered library instance.
No, the pwrseq instance stores dtb information (clock, gpio, etc), it
needs to be per device.
--
Best Regards,
Peter Chen
On 15 June 2017 at 11:11, Peter Chen [off-list ref] wrote:
On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
quoted
quoted
Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.
In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.
In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.
The most difficulty for this is we can't know whether the requested
pwrseq instance will be registered or not, the kernel configuration
for this pwrseq library may not be chosen at all.
In such case it is still correct to return -EPROBE_DEFER, because the
driver that tries to probe its device will fail unless it can run the
needed pwrseq. Right?
quoted
quoted
quoted
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.
Another option is to entirely skip to two step approach.
In other words, make the library to cope with multiple users via the
same registered library instance.
No, the pwrseq instance stores dtb information (clock, gpio, etc), it
needs to be per device.
I think you misunderstood my suggestion here. Of course you need to
allocate one pwrseq data per device.
However, my point is that you shouldn't need more than one instance of
the library functions to be registered in the list of available pwrseq
libraries.
Kind regards
Uffe
From: Peter Chen <hidden> Date: 2017-06-15 10:06:44
On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
On 15 June 2017 at 11:11, Peter Chen [off-list ref] wrote:
quoted
On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
quoted
quoted
Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.
In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.
In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.
The most difficulty for this is we can't know whether the requested
pwrseq instance will be registered or not, the kernel configuration
for this pwrseq library may not be chosen at all.
In such case it is still correct to return -EPROBE_DEFER, because the
driver that tries to probe its device will fail unless it can run the
needed pwrseq. Right?
Unlike the MMC design, there is no dts entry to indicate whether this
device needs pwrseq or not at this design, it will only carry out power
on sequence after matching. So, return -EPROBE_DEFER may not work since
this device may never need pwrseq.
quoted
quoted
quoted
quoted
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.
Another option is to entirely skip to two step approach.
In other words, make the library to cope with multiple users via the
same registered library instance.
No, the pwrseq instance stores dtb information (clock, gpio, etc), it
needs to be per device.
I think you misunderstood my suggestion here. Of course you need to
allocate one pwrseq data per device.
However, my point is that you shouldn't need more than one instance of
the library functions to be registered in the list of available pwrseq
libraries.
This additional instance is used to store compatible information for
this pwrseq library, it is used for the next matching between device
and pwrseq library, it just likes we need the first pwrseq instance
registered at boot stage.
--
Best Regards,
Peter Chen
From: Peter Chen <hidden> Date: 2017-06-19 00:59:43
On Thu, Jun 15, 2017 at 06:06:04PM +0800, Peter Chen wrote:
On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
quoted
On 15 June 2017 at 11:11, Peter Chen [off-list ref] wrote:
quoted
On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
quoted
quoted
Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.
In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.
In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.
The most difficulty for this is we can't know whether the requested
pwrseq instance will be registered or not, the kernel configuration
for this pwrseq library may not be chosen at all.
In such case it is still correct to return -EPROBE_DEFER, because the
driver that tries to probe its device will fail unless it can run the
needed pwrseq. Right?
Unlike the MMC design, there is no dts entry to indicate whether this
device needs pwrseq or not at this design, it will only carry out power
on sequence after matching. So, return -EPROBE_DEFER may not work since
this device may never need pwrseq.
Ulf, since it is the use case limitation, it can't work like device
driver. Do you have more comments for it, thanks.
Peter
quoted
quoted
quoted
quoted
quoted
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.
Another option is to entirely skip to two step approach.
In other words, make the library to cope with multiple users via the
same registered library instance.
No, the pwrseq instance stores dtb information (clock, gpio, etc), it
needs to be per device.
I think you misunderstood my suggestion here. Of course you need to
allocate one pwrseq data per device.
However, my point is that you shouldn't need more than one instance of
the library functions to be registered in the list of available pwrseq
libraries.
This additional instance is used to store compatible information for
this pwrseq library, it is used for the next matching between device
and pwrseq library, it just likes we need the first pwrseq instance
registered at boot stage.
--
Best Regards,
Peter Chen
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 15 June 2017 at 12:06, Peter Chen [off-list ref] wrote:
On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
quoted
On 15 June 2017 at 11:11, Peter Chen [off-list ref] wrote:
quoted
On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
quoted
quoted
Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.
In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.
In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.
The most difficulty for this is we can't know whether the requested
pwrseq instance will be registered or not, the kernel configuration
for this pwrseq library may not be chosen at all.
In such case it is still correct to return -EPROBE_DEFER, because the
driver that tries to probe its device will fail unless it can run the
needed pwrseq. Right?
Unlike the MMC design, there is no dts entry to indicate whether this
device needs pwrseq or not at this design, it will only carry out power
on sequence after matching. So, return -EPROBE_DEFER may not work since
this device may never need pwrseq.
Then, how will you really be able to fetch the correct pwrseq library
instance for the device node?
Suppose their is a *list* of pwrseq library instances available. In
pwrseq_find_available_instance() you call of_match_node(table, np).
The "table" there corresponds to the compatible for the pwrseq library
and the np is the device node provided by the caller of
of_pwrseq_on().
Why is this match done?
Why can't the match be done before trying to fetch a library instance
and then in a second step, really try to fetch the instance? If only
the second step fails, returning -EPROBE_DEFER can be done, no?
BTW, I didn't compatible for the generic pwrseq library being
documented in this series.
quoted
quoted
quoted
quoted
quoted
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.
Another option is to entirely skip to two step approach.
In other words, make the library to cope with multiple users via the
same registered library instance.
No, the pwrseq instance stores dtb information (clock, gpio, etc), it
needs to be per device.
I think you misunderstood my suggestion here. Of course you need to
allocate one pwrseq data per device.
However, my point is that you shouldn't need more than one instance of
the library functions to be registered in the list of available pwrseq
libraries.
This additional instance is used to store compatible information for
this pwrseq library, it is used for the next matching between device
and pwrseq library, it just likes we need the first pwrseq instance
registered at boot stage.
Why can't the compatible information be a static table, known by the
pwrseq core library?
Then when of_pwrseq_on() is called, that static table is parsed and
matched, then a corresponding pwrseq library instance tries to be
fetched.
Kind regards
Uffe
From: Peter Chen <hidden> Date: 2017-06-19 09:03:42
On Mon, Jun 19, 2017 at 10:09:58AM +0200, Ulf Hansson wrote:
On 15 June 2017 at 12:06, Peter Chen [off-list ref] wrote:
quoted
On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
quoted
On 15 June 2017 at 11:11, Peter Chen [off-list ref] wrote:
quoted
On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
quoted
quoted
Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.
In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.
In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.
The most difficulty for this is we can't know whether the requested
pwrseq instance will be registered or not, the kernel configuration
for this pwrseq library may not be chosen at all.
In such case it is still correct to return -EPROBE_DEFER, because the
driver that tries to probe its device will fail unless it can run the
needed pwrseq. Right?
Unlike the MMC design, there is no dts entry to indicate whether this
device needs pwrseq or not at this design, it will only carry out power
on sequence after matching. So, return -EPROBE_DEFER may not work since
this device may never need pwrseq.
Then, how will you really be able to fetch the correct pwrseq library
instance for the device node?
Suppose their is a *list* of pwrseq library instances available. In
pwrseq_find_available_instance() you call of_match_node(table, np).
The "table" there corresponds to the compatible for the pwrseq library
and the np is the device node provided by the caller of
of_pwrseq_on().
Why is this match done?
The compatible in table is from the source code, and the compatible in
np is from the dts. This is the current match way, I comment your
suggestion below.
Why can't the match be done before trying to fetch a library instance
How? If there is no pwrseq instance, how can we do match?
and then in a second step, really try to fetch the instance? If only
the second step fails, returning -EPROBE_DEFER can be done, no?
BTW, I didn't compatible for the generic pwrseq library being
documented in this series.
quoted
quoted
quoted
quoted
quoted
quoted
Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.
The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?
Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.
Another option is to entirely skip to two step approach.
In other words, make the library to cope with multiple users via the
same registered library instance.
No, the pwrseq instance stores dtb information (clock, gpio, etc), it
needs to be per device.
I think you misunderstood my suggestion here. Of course you need to
allocate one pwrseq data per device.
However, my point is that you shouldn't need more than one instance of
the library functions to be registered in the list of available pwrseq
libraries.
This additional instance is used to store compatible information for
this pwrseq library, it is used for the next matching between device
and pwrseq library, it just likes we need the first pwrseq instance
registered at boot stage.
Why can't the compatible information be a static table, known by the
pwrseq core library?
Then when of_pwrseq_on() is called, that static table is parsed and
matched, then a corresponding pwrseq library instance tries to be
fetched.
So, you suggest allocating and registering pwrseq instance on the
demand? Eg, we maintain a power sequence static table, including
compatible and allocate function.
static const struct pwrseq_match_table pwrseq_match_table_list[] = {
{ PWRSEQ_DEV(0x0204, 0x6025), .alloc_instance = pwrseq_AA_alloc_instance },
{ PWRSEQ_DEV(0x0204, 0x6026), .alloc_instance = pwrseq_BB_alloc_instance },
{ PWRSEQ_DEV(0xffff, 0xffff), .alloc_instance = pwrseq_generic_alloc_instance },
};
And pwrseq_AA{BB}_alloc_instance are defined at each pwrseq library, and
are exported.
Since the pwrseq_match_table_list is static, we can always do match, and
will not return -EPROBE_DEFER anymore, one problem for this is we need
always compile all pwrseq libraries. Any good suggestions?
--
Best Regards,
Peter Chen
Unlike the MMC design, there is no dts entry to indicate whether this
device needs pwrseq or not at this design, it will only carry out power
on sequence after matching. So, return -EPROBE_DEFER may not work since
this device may never need pwrseq.
Then, how will you really be able to fetch the correct pwrseq library
instance for the device node?
Suppose their is a *list* of pwrseq library instances available. In
pwrseq_find_available_instance() you call of_match_node(table, np).
The "table" there corresponds to the compatible for the pwrseq library
and the np is the device node provided by the caller of
of_pwrseq_on().
Why is this match done?
The compatible in table is from the source code, and the compatible in
np is from the dts. This is the current match way, I comment your
suggestion below.
quoted
Why can't the match be done before trying to fetch a library instance
How? If there is no pwrseq instance, how can we do match?
quoted
and then in a second step, really try to fetch the instance? If only
the second step fails, returning -EPROBE_DEFER can be done, no?
BTW, I didn't compatible for the generic pwrseq library being
documented in this series.
Seems like you need to update the DT documentation for the below
compatible, which is used for the generic pwrseq library. Perhaps this
is what puzzles me a bit on *why* the match is done.
+static const struct of_device_id generic_id_table[] = {
+ { .compatible = "generic",},
+ { /* sentinel */ }
+};
[...]
quoted
quoted
This additional instance is used to store compatible information for
this pwrseq library, it is used for the next matching between device
and pwrseq library, it just likes we need the first pwrseq instance
registered at boot stage.
Why can't the compatible information be a static table, known by the
pwrseq core library?
Then when of_pwrseq_on() is called, that static table is parsed and
matched, then a corresponding pwrseq library instance tries to be
fetched.
So, you suggest allocating and registering pwrseq instance on the
demand? Eg, we maintain a power sequence static table, including
compatible and allocate function.
};
And pwrseq_AA{BB}_alloc_instance are defined at each pwrseq library, and
are exported.
With "exported", I guess you mean shared via a common pwrseq header?
Since the pwrseq_match_table_list is static, we can always do match, and
will not return -EPROBE_DEFER anymore, one problem for this is we need
always compile all pwrseq libraries. Any good suggestions?
You never returned -EPROBE_DEFER in the first case. That's why I complained. :-)
So, in case the OF match doesn't succeed, there are no reason to
propagate an error, but instead just bail out and returning 0 to the
caller.
If the OF match succeeds, it means the device requires a pwrseq
library to be used. Then, pwrseq_XX_alloc_instance() will be called,
on demand and which tries to fetch the resources (clocks, gpios etc).
If any of those attempts fetching a resource fails, its corresponding
error code should be propagated to the caller - including
-EPROBE_DEFER.
Regarding the "always compile all pwrseq libraries"; no we don't need
to do that. Instead we only need a to have a stub function for
pwrseq_XX_alloc_instance, in case its corresponding Kconfig option is
unset. That stub, should of course return an error code.
Kind regards
Uffe
From: Peter Chen <hidden> Date: 2017-06-20 02:38:28
On Mon, Jun 19, 2017 at 11:48:05AM +0200, Ulf Hansson wrote:
[...]
quoted
quoted
quoted
Unlike the MMC design, there is no dts entry to indicate whether this
device needs pwrseq or not at this design, it will only carry out power
on sequence after matching. So, return -EPROBE_DEFER may not work since
this device may never need pwrseq.
Then, how will you really be able to fetch the correct pwrseq library
instance for the device node?
Suppose their is a *list* of pwrseq library instances available. In
pwrseq_find_available_instance() you call of_match_node(table, np).
The "table" there corresponds to the compatible for the pwrseq library
and the np is the device node provided by the caller of
of_pwrseq_on().
Why is this match done?
The compatible in table is from the source code, and the compatible in
np is from the dts. This is the current match way, I comment your
suggestion below.
quoted
Why can't the match be done before trying to fetch a library instance
How? If there is no pwrseq instance, how can we do match?
quoted
and then in a second step, really try to fetch the instance? If only
the second step fails, returning -EPROBE_DEFER can be done, no?
BTW, I didn't compatible for the generic pwrseq library being
documented in this series.
Seems like you need to update the DT documentation for the below
compatible, which is used for the generic pwrseq library. Perhaps this
is what puzzles me a bit on *why* the match is done.
+static const struct of_device_id generic_id_table[] = {
+ { .compatible = "generic",},
+ { /* sentinel */ }
+};
Sorry, I should update this "generic" compatible at 1st binding-doc
patch.
[...]
quoted
quoted
quoted
This additional instance is used to store compatible information for
this pwrseq library, it is used for the next matching between device
and pwrseq library, it just likes we need the first pwrseq instance
registered at boot stage.
Why can't the compatible information be a static table, known by the
pwrseq core library?
Then when of_pwrseq_on() is called, that static table is parsed and
matched, then a corresponding pwrseq library instance tries to be
fetched.
So, you suggest allocating and registering pwrseq instance on the
demand? Eg, we maintain a power sequence static table, including
compatible and allocate function.
In fact, this should be compatible string, I exampled it as USB vid,pid
wrongly.
quoted
Since the pwrseq_match_table_list is static, we can always do match, and
will not return -EPROBE_DEFER anymore, one problem for this is we need
always compile all pwrseq libraries. Any good suggestions?
You never returned -EPROBE_DEFER in the first case. That's why I complained. :-)
So, in case the OF match doesn't succeed, there are no reason to
propagate an error, but instead just bail out and returning 0 to the
caller.
If the OF match succeeds, it means the device requires a pwrseq
library to be used. Then, pwrseq_XX_alloc_instance() will be called,
on demand and which tries to fetch the resources (clocks, gpios etc).
If any of those attempts fetching a resource fails, its corresponding
error code should be propagated to the caller - including
-EPROBE_DEFER.
Regarding the "always compile all pwrseq libraries"; no we don't need
to do that. Instead we only need a to have a stub function for
pwrseq_XX_alloc_instance, in case its corresponding Kconfig option is
unset. That stub, should of course return an error code.
I will have a updated version for your suggestion, thanks.
--
Best Regards,
Peter Chen