* [PATCH 0/3] firmware: scpi: add device power domain support @ 2016-06-06 15:53 Sudeep Holla 2016-06-06 15:53 ` [PATCH 1/3] firmware: arm_scpi: add support for device power state management Sudeep Holla ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Sudeep Holla @ 2016-06-06 15:53 UTC (permalink / raw) To: linux-kernel Cc: Sudeep Holla, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose This series add support for SCPI based device device power state management using genpd. Regards, Sudeep Sudeep Holla (3): firmware: arm_scpi: add support for device power state management Documentation: add DT bindings for ARM SCPI power domains firmware: scpi: add device power domain support using genpd Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 +++++ drivers/firmware/Kconfig | 8 ++ drivers/firmware/Makefile | 1 + drivers/firmware/arm_scpi.c | 29 ++++ drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++ include/linux/scpi_protocol.h | 2 + 6 files changed, 226 insertions(+) create mode 100644 drivers/firmware/scpi_pd.c -- 2.7.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] firmware: arm_scpi: add support for device power state management 2016-06-06 15:53 [PATCH 0/3] firmware: scpi: add device power domain support Sudeep Holla @ 2016-06-06 15:53 ` Sudeep Holla 2016-06-07 12:58 ` Jon Medhurst (Tixy) 2016-06-06 15:53 ` [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains Sudeep Holla 2016-06-06 15:53 ` [PATCH 3/3] firmware: scpi: add device power domain support using genpd Sudeep Holla 2 siblings, 1 reply; 16+ messages in thread From: Sudeep Holla @ 2016-06-06 15:53 UTC (permalink / raw) To: linux-kernel Cc: Sudeep Holla, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, linux-arm-kernel SCPI protocol supports device power state management. This deals with power states of various peripheral devices in the system other than the core compute subsystem. This patch adds support for the power state management of those peripheral devices. Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scpi.c | 29 +++++++++++++++++++++++++++++ include/linux/scpi_protocol.h | 2 ++ 2 files changed, 31 insertions(+) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index 51c6db0774cc..ca6afd2217a8 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -231,6 +231,11 @@ struct sensor_value { __le32 hi_val; } __packed; +struct dev_pstate_set { + u16 dev_id; + u8 pstate; +} __packed; + static struct scpi_drvinfo *scpi_info; static int scpi_linux_errmap[SCPI_ERR_MAX] = { @@ -537,6 +542,28 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val) return ret; } +static int scpi_device_get_power_state(u16 dev_id) +{ + int ret; + u8 pstate; + + ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &dev_id, + sizeof(dev_id), &pstate, sizeof(pstate)); + return ret ? ret : pstate; +} + +static int scpi_device_set_power_state(u16 dev_id, u8 pstate) +{ + int stat; + struct dev_pstate_set dev_set = { + .dev_id = cpu_to_le16(dev_id), + .pstate = pstate, + }; + + return scpi_send_message(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set, + sizeof(dev_set), &stat, sizeof(stat)); +} + static struct scpi_ops scpi_ops = { .get_version = scpi_get_version, .clk_get_range = scpi_clk_get_range, @@ -548,6 +575,8 @@ static struct scpi_ops scpi_ops = { .sensor_get_capability = scpi_sensor_get_capability, .sensor_get_info = scpi_sensor_get_info, .sensor_get_value = scpi_sensor_get_value, + .device_get_power_state = scpi_device_get_power_state, + .device_set_power_state = scpi_device_set_power_state, }; struct scpi_ops *get_scpi_ops(void) diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h index 35de50a65665..dc5f989be226 100644 --- a/include/linux/scpi_protocol.h +++ b/include/linux/scpi_protocol.h @@ -70,6 +70,8 @@ struct scpi_ops { int (*sensor_get_capability)(u16 *sensors); int (*sensor_get_info)(u16 sensor_id, struct scpi_sensor_info *); int (*sensor_get_value)(u16, u64 *); + int (*device_get_power_state)(u16); + int (*device_set_power_state)(u16, u8); }; #if IS_REACHABLE(CONFIG_ARM_SCPI_PROTOCOL) -- 2.7.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] firmware: arm_scpi: add support for device power state management 2016-06-06 15:53 ` [PATCH 1/3] firmware: arm_scpi: add support for device power state management Sudeep Holla @ 2016-06-07 12:58 ` Jon Medhurst (Tixy) 2016-06-07 13:00 ` Sudeep Holla 0 siblings, 1 reply; 16+ messages in thread From: Jon Medhurst (Tixy) @ 2016-06-07 12:58 UTC (permalink / raw) To: Sudeep Holla Cc: linux-kernel, Mathieu Poirier, Suzuki K Poulose, linux-arm-kernel On Mon, 2016-06-06 at 16:53 +0100, Sudeep Holla wrote: > SCPI protocol supports device power state management. This deals with > power states of various peripheral devices in the system other than the > core compute subsystem. > > This patch adds support for the power state management of those > peripheral devices. > > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scpi.c | 29 +++++++++++++++++++++++++++++ > include/linux/scpi_protocol.h | 2 ++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index 51c6db0774cc..ca6afd2217a8 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/drivers/firmware/arm_scpi.c > @@ -231,6 +231,11 @@ struct sensor_value { > __le32 hi_val; > } __packed; > > +struct dev_pstate_set { > + u16 dev_id; > + u8 pstate; > +} __packed; > + > static struct scpi_drvinfo *scpi_info; > > static int scpi_linux_errmap[SCPI_ERR_MAX] = { > @@ -537,6 +542,28 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val) > return ret; > } > > +static int scpi_device_get_power_state(u16 dev_id) > +{ > + int ret; > + u8 pstate; > + > + ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &dev_id, > + sizeof(dev_id), &pstate, sizeof(pstate)); Don't you need to run translate dev_id to little-endian before sending it? You remembered to do that next function... > + return ret ? ret : pstate; > +} > + > +static int scpi_device_set_power_state(u16 dev_id, u8 pstate) > +{ > + int stat; > + struct dev_pstate_set dev_set = { > + .dev_id = cpu_to_le16(dev_id), > + .pstate = pstate, > + }; > + > + return scpi_send_message(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set, > + sizeof(dev_set), &stat, sizeof(stat)); > +} > + [...] -- Tixy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] firmware: arm_scpi: add support for device power state management 2016-06-07 12:58 ` Jon Medhurst (Tixy) @ 2016-06-07 13:00 ` Sudeep Holla 0 siblings, 0 replies; 16+ messages in thread From: Sudeep Holla @ 2016-06-07 13:00 UTC (permalink / raw) To: Jon Medhurst (Tixy) Cc: Sudeep Holla, linux-kernel, Mathieu Poirier, Suzuki K Poulose, linux-arm-kernel On 07/06/16 13:58, Jon Medhurst (Tixy) wrote: > On Mon, 2016-06-06 at 16:53 +0100, Sudeep Holla wrote: >> SCPI protocol supports device power state management. This deals with >> power states of various peripheral devices in the system other than the >> core compute subsystem. >> >> This patch adds support for the power state management of those >> peripheral devices. >> >> Cc: linux-arm-kernel@lists.infradead.org >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/firmware/arm_scpi.c | 29 +++++++++++++++++++++++++++++ >> include/linux/scpi_protocol.h | 2 ++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c >> index 51c6db0774cc..ca6afd2217a8 100644 >> --- a/drivers/firmware/arm_scpi.c >> +++ b/drivers/firmware/arm_scpi.c >> @@ -231,6 +231,11 @@ struct sensor_value { >> __le32 hi_val; >> } __packed; >> >> +struct dev_pstate_set { >> + u16 dev_id; >> + u8 pstate; >> +} __packed; >> + >> static struct scpi_drvinfo *scpi_info; >> >> static int scpi_linux_errmap[SCPI_ERR_MAX] = { >> @@ -537,6 +542,28 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val) >> return ret; >> } >> >> +static int scpi_device_get_power_state(u16 dev_id) >> +{ >> + int ret; >> + u8 pstate; >> + >> + ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &dev_id, >> + sizeof(dev_id), &pstate, sizeof(pstate)); > > Don't you need to run translate dev_id to little-endian before sending > it? You remembered to do that next function... > Good catch, that's the reason I added the get_power_state call in scpi_genpd code to cover testing but still I failed. Thanks for catching this. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains 2016-06-06 15:53 [PATCH 0/3] firmware: scpi: add device power domain support Sudeep Holla 2016-06-06 15:53 ` [PATCH 1/3] firmware: arm_scpi: add support for device power state management Sudeep Holla @ 2016-06-06 15:53 ` Sudeep Holla 2016-06-07 13:22 ` Mark Rutland 2016-06-06 15:53 ` [PATCH 3/3] firmware: scpi: add device power domain support using genpd Sudeep Holla 2 siblings, 1 reply; 16+ messages in thread From: Sudeep Holla @ 2016-06-06 15:53 UTC (permalink / raw) To: linux-kernel Cc: Sudeep Holla, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rob Herring, Mark Rutland, linux-arm-kernel, devicetree The System Control Processor (SCP) provides peripheral devices with power domains that can be enabled and disabled viathe System Control and Power Interface (SCPI) Message Protocol. Add bindings to allow probing of these device power domians. Cc: Rob Herring <robh+dt@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: devicetree@vger.kernel.org Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt index 313dabdc14f9..7141670d649b 100644 --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt @@ -87,10 +87,33 @@ SCPI provides an API to access the various sensors on the SoC. implementation for the IDs to use. For Juno R0 and Juno R1 refer to [3]. +Power domain bindings for the power domains based on SCPI Message Protocol +------------------------------------------------------------ + +This binding uses the generic power domain binding[4]. + +PM domain providers +=================== + +Required properties: + - #power-domain-cells : Should be 1. Contains the device or the power + domain ID value used by SCPI commands. + - num-domains: Total number of power domains provided by SCPI. This is + needed as the SCPI message protocol lacks a mechanism to + query this information runtime. + +PM domain consumers +=================== + +Required properties: + - power-domains : A phandle and PM domain specifier as defined by bindings of + the power controller specified by phandle. + [0] http://infocenter.arm.com/help/topic/com.arm.doc.dui0922b/index.html [1] Documentation/devicetree/bindings/clock/clock-bindings.txt [2] Documentation/devicetree/bindings/thermal/thermal.txt [3] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922b/apas03s22.html +[4] Documentation/devicetree/bindings/power/power_domain.txt Example: @@ -144,6 +167,12 @@ scpi_protocol: scpi@2e000000 { compatible = "arm,scpi-sensors"; #thermal-sensor-cells = <1>; }; + + scpi_devpd: scpi-power-domains { + compatible = "arm,scpi-power-domains"; + num-domains = <2>; + #power-domain-cells = <1>; + }; }; cpu@0 { @@ -156,6 +185,7 @@ hdlcd@7ff60000 { ... reg = <0 0x7ff60000 0 0x1000>; clocks = <&scpi_clk 4>; + power-domains = <&scpi_devpd 1>; }; thermal-zones { @@ -186,3 +216,7 @@ The thermal-sensors property in the soc_thermal node uses the temperature sensor provided by SCP firmware to setup a thermal zone. The ID "3" is the sensor identifier for the temperature sensor as used by the firmware. + +The num-domains property in scpi-power-domains domain specifies that +SCPI provides 2 power domains. The hdlcd node uses the power domain with +domain ID 1. -- 2.7.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains 2016-06-06 15:53 ` [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains Sudeep Holla @ 2016-06-07 13:22 ` Mark Rutland 2016-06-07 13:39 ` Sudeep Holla 0 siblings, 1 reply; 16+ messages in thread From: Mark Rutland @ 2016-06-07 13:22 UTC (permalink / raw) To: Sudeep Holla Cc: linux-kernel, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rob Herring, linux-arm-kernel, devicetree On Mon, Jun 06, 2016 at 04:53:58PM +0100, Sudeep Holla wrote: > The System Control Processor (SCP) provides peripheral devices with > power domains that can be enabled and disabled viathe System Control > and Power Interface (SCPI) Message Protocol. Add bindings to allow > probing of these device power domians. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: devicetree@vger.kernel.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 ++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt > index 313dabdc14f9..7141670d649b 100644 > --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt > +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt > @@ -87,10 +87,33 @@ SCPI provides an API to access the various sensors on the SoC. > implementation for the IDs to use. For Juno > R0 and Juno R1 refer to [3]. > > +Power domain bindings for the power domains based on SCPI Message Protocol > +------------------------------------------------------------ > + > +This binding uses the generic power domain binding[4]. > + > +PM domain providers > +=================== > + > +Required properties: > + - #power-domain-cells : Should be 1. Contains the device or the power > + domain ID value used by SCPI commands. > + - num-domains: Total number of power domains provided by SCPI. This is > + needed as the SCPI message protocol lacks a mechanism to > + query this information runtime. ^ I guess there should be an 'at' here. Are domain IDs zero-based and definitely non-sparse? What exactly does this matter for? Just for validation at parsing time, or is this strictly required for correctness? If we send a command with an invalid domain ID, would the FW reliably report an error that we can recover from? Otherwise, this looks ok. I'd just like to make sure I've understood correctly. Thanks, Mark. > + > +PM domain consumers > +=================== > + > +Required properties: > + - power-domains : A phandle and PM domain specifier as defined by bindings of > + the power controller specified by phandle. > + > [0] http://infocenter.arm.com/help/topic/com.arm.doc.dui0922b/index.html > [1] Documentation/devicetree/bindings/clock/clock-bindings.txt > [2] Documentation/devicetree/bindings/thermal/thermal.txt > [3] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922b/apas03s22.html > +[4] Documentation/devicetree/bindings/power/power_domain.txt > > Example: > > @@ -144,6 +167,12 @@ scpi_protocol: scpi@2e000000 { > compatible = "arm,scpi-sensors"; > #thermal-sensor-cells = <1>; > }; > + > + scpi_devpd: scpi-power-domains { > + compatible = "arm,scpi-power-domains"; > + num-domains = <2>; > + #power-domain-cells = <1>; > + }; > }; > > cpu@0 { > @@ -156,6 +185,7 @@ hdlcd@7ff60000 { > ... > reg = <0 0x7ff60000 0 0x1000>; > clocks = <&scpi_clk 4>; > + power-domains = <&scpi_devpd 1>; > }; > > thermal-zones { > @@ -186,3 +216,7 @@ The thermal-sensors property in the soc_thermal node uses the > temperature sensor provided by SCP firmware to setup a thermal > zone. The ID "3" is the sensor identifier for the temperature sensor > as used by the firmware. > + > +The num-domains property in scpi-power-domains domain specifies that > +SCPI provides 2 power domains. The hdlcd node uses the power domain with > +domain ID 1. > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains 2016-06-07 13:22 ` Mark Rutland @ 2016-06-07 13:39 ` Sudeep Holla 2016-06-07 14:45 ` Mark Rutland 0 siblings, 1 reply; 16+ messages in thread From: Sudeep Holla @ 2016-06-07 13:39 UTC (permalink / raw) To: Mark Rutland Cc: Sudeep Holla, linux-kernel, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rob Herring, linux-arm-kernel, devicetree On 07/06/16 14:22, Mark Rutland wrote: > On Mon, Jun 06, 2016 at 04:53:58PM +0100, Sudeep Holla wrote: >> The System Control Processor (SCP) provides peripheral devices with >> power domains that can be enabled and disabled viathe System Control >> and Power Interface (SCPI) Message Protocol. Add bindings to allow >> probing of these device power domians. >> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: devicetree@vger.kernel.org >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 ++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt >> index 313dabdc14f9..7141670d649b 100644 >> --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt >> +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt >> @@ -87,10 +87,33 @@ SCPI provides an API to access the various sensors on the SoC. >> implementation for the IDs to use. For Juno >> R0 and Juno R1 refer to [3]. >> >> +Power domain bindings for the power domains based on SCPI Message Protocol >> +------------------------------------------------------------ >> + >> +This binding uses the generic power domain binding[4]. >> + >> +PM domain providers >> +=================== >> + >> +Required properties: >> + - #power-domain-cells : Should be 1. Contains the device or the power >> + domain ID value used by SCPI commands. >> + - num-domains: Total number of power domains provided by SCPI. This is >> + needed as the SCPI message protocol lacks a mechanism to >> + query this information runtime. > ^ > I guess there should be an 'at' here. > Will fix. > Are domain IDs zero-based and definitely non-sparse? > Yes > What exactly does this matter for? Just for validation at parsing time, > or is this strictly required for correctness? > This is mainly to know the maximum number of power domains that firmware supports. This will help the software handling the provider part to setup the information in advance before any consumer request for the service. > If we send a command with an invalid domain ID, would the FW reliably > report an error that we can recover from? > Yes for anything above this value, firmware returns invalid parameter error. It's would be good to have that as a separate command instead of getting it via DT. We already have that for OPPs and clocks. Just this lacks that feature. > Otherwise, this looks ok. I'd just like to make sure I've understood > correctly. > Sure, thanks for the review. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains 2016-06-07 13:39 ` Sudeep Holla @ 2016-06-07 14:45 ` Mark Rutland 0 siblings, 0 replies; 16+ messages in thread From: Mark Rutland @ 2016-06-07 14:45 UTC (permalink / raw) To: Sudeep Holla Cc: linux-kernel, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rob Herring, linux-arm-kernel, devicetree On Tue, Jun 07, 2016 at 02:39:25PM +0100, Sudeep Holla wrote: > > > On 07/06/16 14:22, Mark Rutland wrote: > >On Mon, Jun 06, 2016 at 04:53:58PM +0100, Sudeep Holla wrote: > >>The System Control Processor (SCP) provides peripheral devices with > >>power domains that can be enabled and disabled viathe System Control > >>and Power Interface (SCPI) Message Protocol. Add bindings to allow > >>probing of these device power domians. > >> > >>Cc: Rob Herring <robh+dt@kernel.org> > >>Cc: Mark Rutland <mark.rutland@arm.com> > >>Cc: linux-arm-kernel@lists.infradead.org > >>Cc: devicetree@vger.kernel.org > >>Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > >>--- > >> Documentation/devicetree/bindings/arm/arm,scpi.txt | 34 ++++++++++++++++++++++ > >> 1 file changed, 34 insertions(+) > >> > >>diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt > >>index 313dabdc14f9..7141670d649b 100644 > >>--- a/Documentation/devicetree/bindings/arm/arm,scpi.txt > >>+++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt > >>@@ -87,10 +87,33 @@ SCPI provides an API to access the various sensors on the SoC. > >> implementation for the IDs to use. For Juno > >> R0 and Juno R1 refer to [3]. > >> > >>+Power domain bindings for the power domains based on SCPI Message Protocol > >>+------------------------------------------------------------ > >>+ > >>+This binding uses the generic power domain binding[4]. > >>+ > >>+PM domain providers > >>+=================== > >>+ > >>+Required properties: > >>+ - #power-domain-cells : Should be 1. Contains the device or the power > >>+ domain ID value used by SCPI commands. > >>+ - num-domains: Total number of power domains provided by SCPI. This is > >>+ needed as the SCPI message protocol lacks a mechanism to > >>+ query this information runtime. > > ^ > >I guess there should be an 'at' here. > > > > Will fix. > > >Are domain IDs zero-based and definitely non-sparse? > > > > Yes Ok. So FWIW, with the fix above: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] firmware: scpi: add device power domain support using genpd 2016-06-06 15:53 [PATCH 0/3] firmware: scpi: add device power domain support Sudeep Holla 2016-06-06 15:53 ` [PATCH 1/3] firmware: arm_scpi: add support for device power state management Sudeep Holla 2016-06-06 15:53 ` [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains Sudeep Holla @ 2016-06-06 15:53 ` Sudeep Holla 2016-06-07 13:18 ` Jon Medhurst (Tixy) ` (2 more replies) 2 siblings, 3 replies; 16+ messages in thread From: Sudeep Holla @ 2016-06-06 15:53 UTC (permalink / raw) To: linux-kernel Cc: Sudeep Holla, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, linux-pm This patch hooks up the support for device power domain provided by SCPI using the Linux generic power domain infrastructure. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Kevin Hilman <khilman@kernel.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: linux-pm@vger.kernel.org Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/Kconfig | 8 +++ drivers/firmware/Makefile | 1 + drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+) create mode 100644 drivers/firmware/scpi_pd.c Hi, Since most of the power controller drivers are place in drivers/soc/<soc_name>, I am not sure where to put this SCPI power domain code as it can be used on multiple SoC. I have placed it in drivers/firmware temporarily for review. Please suggest the most apt place to put this driver. Regards, Sudeep diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 41abdc54815e..80c963c60f13 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL This protocol library provides interface for all the client drivers making use of the features offered by the SCP. +config ARM_SCPI_POWER_DOMAIN + tristate "SCPI power domain driver" + depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST + select PM_GENERIC_DOMAINS_OF + help + This enables support for the SCPI power domains which can be + enabled or disabled via the SCP firmware + config EDD tristate "BIOS Enhanced Disk Drive calls determine boot disk" depends on X86 diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index 474bada56fcd..24f7fe8e3fc3 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -3,6 +3,7 @@ # obj-$(CONFIG_ARM_PSCI_FW) += psci.o obj-$(CONFIG_ARM_SCPI_PROTOCOL) += arm_scpi.o +obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pd.o obj-$(CONFIG_DMI) += dmi_scan.o obj-$(CONFIG_DMI_SYSFS) += dmi-sysfs.o obj-$(CONFIG_EDD) += edd.o diff --git a/drivers/firmware/scpi_pd.c b/drivers/firmware/scpi_pd.c new file mode 100644 index 000000000000..fb24631d2b2e --- /dev/null +++ b/drivers/firmware/scpi_pd.c @@ -0,0 +1,152 @@ +/* + * SCPI Generic power domain support. + * + * Copyright (C) 2016 ARM Ltd. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/pm_domain.h> +#include <linux/scpi_protocol.h> + +struct scpi_pm_domain { + struct generic_pm_domain genpd; + struct scpi_ops *ops; + u32 domain; + char name[20]; +}; + +enum scpi_power_domain_state { + SCPI_PD_STATE_ON = 0, + SCPI_PD_STATE_OFF = 3, +}; + +#define to_scpi_pd(gpd) container_of(gpd, struct scpi_pm_domain, genpd) + +static int scpi_pd_power(struct scpi_pm_domain *pd, bool power_on) +{ + int ret; + enum scpi_power_domain_state state; + + if (power_on) + state = SCPI_PD_STATE_ON; + else + state = SCPI_PD_STATE_OFF; + + ret = pd->ops->device_set_power_state(pd->domain, state); + if (ret) + return ret; + + return (state == pd->ops->device_get_power_state(pd->domain)); +} + +static int scpi_pd_power_on(struct generic_pm_domain *domain) +{ + struct scpi_pm_domain *pd = to_scpi_pd(domain); + + return scpi_pd_power(pd, true); +} + +static int scpi_pd_power_off(struct generic_pm_domain *domain) +{ + struct scpi_pm_domain *pd = to_scpi_pd(domain); + + return scpi_pd_power(pd, false); +} + +static int scpi_pm_domain_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct scpi_pm_domain *scpi_pd; + struct genpd_onecell_data *scpi_pd_data; + struct generic_pm_domain **domains; + struct scpi_ops *scpi_ops; + int ret, num_domains, i; + + scpi_ops = get_scpi_ops(); + if (!scpi_ops) + return -EPROBE_DEFER; + + if (!np) { + dev_err(dev, "device tree node not found\n"); + return -ENODEV; + } + + ret = of_property_read_u32(np, "num-domains", &num_domains); + if (ret) { + dev_err(dev, "number of domains not found\n"); + return -EINVAL; + } + + scpi_pd = devm_kcalloc(dev, num_domains, sizeof(*scpi_pd), GFP_KERNEL); + if (!scpi_pd) + return -ENOMEM; + + scpi_pd_data = devm_kzalloc(dev, sizeof(*scpi_pd_data), GFP_KERNEL); + if (!scpi_pd_data) + return -ENOMEM; + + domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL); + if (!domains) + return -ENOMEM; + + for (i = 0; i < num_domains; i++, scpi_pd++) { + domains[i] = &scpi_pd->genpd; + + scpi_pd->domain = i; + scpi_pd->ops = scpi_ops; + sprintf(scpi_pd->name, "%s.%d", np->name, i); + scpi_pd->genpd.name = scpi_pd->name; + scpi_pd->genpd.power_off = scpi_pd_power_off; + scpi_pd->genpd.power_on = scpi_pd_power_on; + + /* + * Treat all power domains as off at boot. + * + * The SCP firmware itself may have switched on some domains, + * but for reference counting purpose, keep it this way. + */ + pm_genpd_init(&scpi_pd->genpd, NULL, true); + } + + scpi_pd_data->domains = domains; + scpi_pd_data->num_domains = num_domains; + + of_genpd_add_provider_onecell(np, scpi_pd_data); + + return 0; +} + +static const struct of_device_id scpi_power_domain_ids[] = { + { .compatible = "arm,scpi-power-domains", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, scpi_power_domain_ids); + +static struct platform_driver scpi_power_domain_driver = { + .driver = { + .name = "scpi_power_domain", + .of_match_table = scpi_power_domain_ids, + }, + .probe = scpi_pm_domain_probe, +}; +module_platform_driver(scpi_power_domain_driver); + +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>"); +MODULE_DESCRIPTION("ARM SCPI power domain driver"); +MODULE_LICENSE("GPL v2"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd 2016-06-06 15:53 ` [PATCH 3/3] firmware: scpi: add device power domain support using genpd Sudeep Holla @ 2016-06-07 13:18 ` Jon Medhurst (Tixy) 2016-06-07 13:29 ` Sudeep Holla 2016-06-10 16:19 ` Sudeep Holla 2016-06-15 13:05 ` Ulf Hansson 2 siblings, 1 reply; 16+ messages in thread From: Jon Medhurst (Tixy) @ 2016-06-07 13:18 UTC (permalink / raw) To: Sudeep Holla Cc: linux-kernel, Mathieu Poirier, Suzuki K Poulose, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, linux-pm On Mon, 2016-06-06 at 16:53 +0100, Sudeep Holla wrote: > This patch hooks up the support for device power domain provided by > SCPI using the Linux generic power domain infrastructure. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/Kconfig | 8 +++ > drivers/firmware/Makefile | 1 + > drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 161 insertions(+) > create mode 100644 drivers/firmware/scpi_pd.c > > Hi, > > Since most of the power controller drivers are place in drivers/soc/<soc_name>, > I am not sure where to put this SCPI power domain code as it can be used > on multiple SoC. I have placed it in drivers/firmware temporarily for > review. Please suggest the most apt place to put this driver. > > Regards, > Sudeep > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > index 41abdc54815e..80c963c60f13 100644 > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL > This protocol library provides interface for all the client drivers > making use of the features offered by the SCP. > > +config ARM_SCPI_POWER_DOMAIN > + tristate "SCPI power domain driver" > + depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST > + select PM_GENERIC_DOMAINS_OF That select doesn't work for me and gives: warning: (ARM_SCPI_POWER_DOMAIN) selects PM_GENERIC_DOMAINS_OF which has unmet direct dependencies (PM_GENERIC_DOMAINS && OF) Followed by link errors due to missing symbols. I think you need to select PM_GENERIC_DOMAINS as well. Or perhaps just instead of, as PM_GENERIC_DOMAINS_OF defaults 'y' and isn't user selectable. From kernel/power/Kconfig ... config PM_GENERIC_DOMAINS_OF def_bool y depends on PM_GENERIC_DOMAINS && OF [...] -- Tixy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd 2016-06-07 13:18 ` Jon Medhurst (Tixy) @ 2016-06-07 13:29 ` Sudeep Holla 0 siblings, 0 replies; 16+ messages in thread From: Sudeep Holla @ 2016-06-07 13:29 UTC (permalink / raw) To: Jon Medhurst (Tixy) Cc: Sudeep Holla, linux-kernel, Mathieu Poirier, Suzuki K Poulose, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, linux-pm On 07/06/16 14:18, Jon Medhurst (Tixy) wrote: > On Mon, 2016-06-06 at 16:53 +0100, Sudeep Holla wrote: >> This patch hooks up the support for device power domain provided by >> SCPI using the Linux generic power domain infrastructure. >> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Cc: Kevin Hilman <khilman@kernel.org> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> Cc: linux-pm@vger.kernel.org >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/firmware/Kconfig | 8 +++ >> drivers/firmware/Makefile | 1 + >> drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 161 insertions(+) >> create mode 100644 drivers/firmware/scpi_pd.c >> >> Hi, >> >> Since most of the power controller drivers are place in drivers/soc/<soc_name>, >> I am not sure where to put this SCPI power domain code as it can be used >> on multiple SoC. I have placed it in drivers/firmware temporarily for >> review. Please suggest the most apt place to put this driver. >> >> Regards, >> Sudeep >> >> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig >> index 41abdc54815e..80c963c60f13 100644 >> --- a/drivers/firmware/Kconfig >> +++ b/drivers/firmware/Kconfig >> @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL >> This protocol library provides interface for all the client drivers >> making use of the features offered by the SCP. >> >> +config ARM_SCPI_POWER_DOMAIN >> + tristate "SCPI power domain driver" >> + depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST >> + select PM_GENERIC_DOMAINS_OF > Actually I had something like below before and changed it before posting. config ARM_SCPI_POWER_DOMAIN tristate "SCPI power domain driver" depends on ARM_SCPI_PROTOCOL || COMPILE_TEST select PM_GENERIC_DOMAINS if PM select PM_GENERIC_DOMAINS_OF if PM The idea was to allow compilation of this even if PM was disabled. > That select doesn't work for me and gives: > > warning: (ARM_SCPI_POWER_DOMAIN) selects PM_GENERIC_DOMAINS_OF which has unmet direct dependencies (PM_GENERIC_DOMAINS && OF) > > Followed by link errors due to missing symbols. > > I think you need to select PM_GENERIC_DOMAINS as well. I agree, that's exactly what I had before. > Or perhaps just instead of, as PM_GENERIC_DOMAINS_OF defaults 'y' and isn't user > selectable. From kernel/power/Kconfig ... > Makes sense, I am fine with that too. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd 2016-06-06 15:53 ` [PATCH 3/3] firmware: scpi: add device power domain support using genpd Sudeep Holla 2016-06-07 13:18 ` Jon Medhurst (Tixy) @ 2016-06-10 16:19 ` Sudeep Holla 2016-06-15 13:05 ` Ulf Hansson 2 siblings, 0 replies; 16+ messages in thread From: Sudeep Holla @ 2016-06-10 16:19 UTC (permalink / raw) To: Rafael J. Wysocki, Kevin Hilman, Ulf Hansson Cc: linux-kernel, Sudeep Holla, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, linux-pm Hi, On 06/06/16 16:53, Sudeep Holla wrote: > This patch hooks up the support for device power domain provided by > SCPI using the Linux generic power domain infrastructure. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/Kconfig | 8 +++ > drivers/firmware/Makefile | 1 + > drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 161 insertions(+) > create mode 100644 drivers/firmware/scpi_pd.c > > Hi, > > Since most of the power controller drivers are place in drivers/soc/<soc_name>, > I am not sure where to put this SCPI power domain code as it can be used > on multiple SoC. I have placed it in drivers/firmware temporarily for > review. Please suggest the most apt place to put this driver. > Any suggestions ? Or drivers/firmware is OK for now ? -- Regards, Sudeep ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd 2016-06-06 15:53 ` [PATCH 3/3] firmware: scpi: add device power domain support using genpd Sudeep Holla 2016-06-07 13:18 ` Jon Medhurst (Tixy) 2016-06-10 16:19 ` Sudeep Holla @ 2016-06-15 13:05 ` Ulf Hansson 2016-06-15 13:23 ` Sudeep Holla 2 siblings, 1 reply; 16+ messages in thread From: Ulf Hansson @ 2016-06-15 13:05 UTC (permalink / raw) To: Sudeep Holla Cc: linux-kernel, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rafael J. Wysocki, Kevin Hilman, linux-pm On 6 June 2016 at 17:53, Sudeep Holla <sudeep.holla@arm.com> wrote: > This patch hooks up the support for device power domain provided by > SCPI using the Linux generic power domain infrastructure. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> For following versions, please keep me in the loop for the entire series. Including the cover-letter which I am unable to find. > --- > drivers/firmware/Kconfig | 8 +++ > drivers/firmware/Makefile | 1 + > drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 161 insertions(+) > create mode 100644 drivers/firmware/scpi_pd.c > > Hi, > > Since most of the power controller drivers are place in drivers/soc/<soc_name>, > I am not sure where to put this SCPI power domain code as it can be used > on multiple SoC. I have placed it in drivers/firmware temporarily for > review. Please suggest the most apt place to put this driver. To me, I think it makes sense to put this in the suggested directory, as it's not SoC specific code. > > Regards, > Sudeep > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > index 41abdc54815e..80c963c60f13 100644 > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL > This protocol library provides interface for all the client drivers > making use of the features offered by the SCP. > > +config ARM_SCPI_POWER_DOMAIN > + tristate "SCPI power domain driver" > + depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST > + select PM_GENERIC_DOMAINS_OF I think this is better: depends on (ARM_SCPI_PROTOCOL) || COMPILE_TEST select PM_GENERIC_DOMAINS if PM > + help > + This enables support for the SCPI power domains which can be > + enabled or disabled via the SCP firmware > + > config EDD > tristate "BIOS Enhanced Disk Drive calls determine boot disk" > depends on X86 > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile > index 474bada56fcd..24f7fe8e3fc3 100644 > --- a/drivers/firmware/Makefile > +++ b/drivers/firmware/Makefile > @@ -3,6 +3,7 @@ > # > obj-$(CONFIG_ARM_PSCI_FW) += psci.o > obj-$(CONFIG_ARM_SCPI_PROTOCOL) += arm_scpi.o > +obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pd.o > obj-$(CONFIG_DMI) += dmi_scan.o > obj-$(CONFIG_DMI_SYSFS) += dmi-sysfs.o > obj-$(CONFIG_EDD) += edd.o > diff --git a/drivers/firmware/scpi_pd.c b/drivers/firmware/scpi_pd.c Perhaps name it scpi_pm_domain.c instead as it gives a better description of its purpose. > new file mode 100644 > index 000000000000..fb24631d2b2e > --- /dev/null > +++ b/drivers/firmware/scpi_pd.c > @@ -0,0 +1,152 @@ > +/* > + * SCPI Generic power domain support. > + * > + * Copyright (C) 2016 ARM Ltd. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of_platform.h> > +#include <linux/pm_domain.h> > +#include <linux/scpi_protocol.h> > + > +struct scpi_pm_domain { > + struct generic_pm_domain genpd; > + struct scpi_ops *ops; > + u32 domain; > + char name[20]; > +}; > + > +enum scpi_power_domain_state { > + SCPI_PD_STATE_ON = 0, > + SCPI_PD_STATE_OFF = 3, > +}; > + > +#define to_scpi_pd(gpd) container_of(gpd, struct scpi_pm_domain, genpd) > + > +static int scpi_pd_power(struct scpi_pm_domain *pd, bool power_on) > +{ > + int ret; > + enum scpi_power_domain_state state; > + > + if (power_on) > + state = SCPI_PD_STATE_ON; > + else > + state = SCPI_PD_STATE_OFF; > + > + ret = pd->ops->device_set_power_state(pd->domain, state); > + if (ret) > + return ret; > + > + return (state == pd->ops->device_get_power_state(pd->domain)); > +} > + > +static int scpi_pd_power_on(struct generic_pm_domain *domain) > +{ > + struct scpi_pm_domain *pd = to_scpi_pd(domain); > + > + return scpi_pd_power(pd, true); > +} > + > +static int scpi_pd_power_off(struct generic_pm_domain *domain) > +{ > + struct scpi_pm_domain *pd = to_scpi_pd(domain); > + > + return scpi_pd_power(pd, false); > +} > + > +static int scpi_pm_domain_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct scpi_pm_domain *scpi_pd; > + struct genpd_onecell_data *scpi_pd_data; > + struct generic_pm_domain **domains; > + struct scpi_ops *scpi_ops; > + int ret, num_domains, i; > + > + scpi_ops = get_scpi_ops(); > + if (!scpi_ops) > + return -EPROBE_DEFER; > + > + if (!np) { > + dev_err(dev, "device tree node not found\n"); > + return -ENODEV; > + } > + > + ret = of_property_read_u32(np, "num-domains", &num_domains); > + if (ret) { > + dev_err(dev, "number of domains not found\n"); > + return -EINVAL; > + } > + > + scpi_pd = devm_kcalloc(dev, num_domains, sizeof(*scpi_pd), GFP_KERNEL); > + if (!scpi_pd) > + return -ENOMEM; > + > + scpi_pd_data = devm_kzalloc(dev, sizeof(*scpi_pd_data), GFP_KERNEL); > + if (!scpi_pd_data) > + return -ENOMEM; > + > + domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL); > + if (!domains) > + return -ENOMEM; > + > + for (i = 0; i < num_domains; i++, scpi_pd++) { > + domains[i] = &scpi_pd->genpd; > + > + scpi_pd->domain = i; > + scpi_pd->ops = scpi_ops; > + sprintf(scpi_pd->name, "%s.%d", np->name, i); > + scpi_pd->genpd.name = scpi_pd->name; > + scpi_pd->genpd.power_off = scpi_pd_power_off; > + scpi_pd->genpd.power_on = scpi_pd_power_on; > + > + /* > + * Treat all power domains as off at boot. > + * > + * The SCP firmware itself may have switched on some domains, > + * but for reference counting purpose, keep it this way. > + */ > + pm_genpd_init(&scpi_pd->genpd, NULL, true); > + } > + > + scpi_pd_data->domains = domains; > + scpi_pd_data->num_domains = num_domains; > + > + of_genpd_add_provider_onecell(np, scpi_pd_data); > + > + return 0; > +} > + > +static const struct of_device_id scpi_power_domain_ids[] = { > + { .compatible = "arm,scpi-power-domains", }, > + { /* sentinel */ } > +}; Actually I think you shouldn't implement this a standalone driver and thus you can remove this compatible. Instead, I think it's better if you let the arm_scpi driver to also initialize the PM domain. If you still want the PM domain code to be maintained in a separate file, just provide a header file which declares an "scpi_pm_domain_init()" function (and a stub when not supported), which the arm_scpi driver should call during ->probe(). > +MODULE_DEVICE_TABLE(of, scpi_power_domain_ids); > + > +static struct platform_driver scpi_power_domain_driver = { > + .driver = { > + .name = "scpi_power_domain", > + .of_match_table = scpi_power_domain_ids, > + }, > + .probe = scpi_pm_domain_probe, > +}; > +module_platform_driver(scpi_power_domain_driver); > + > +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>"); > +MODULE_DESCRIPTION("ARM SCPI power domain driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.7.4 > Kind regards Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd 2016-06-15 13:05 ` Ulf Hansson @ 2016-06-15 13:23 ` Sudeep Holla 2016-06-15 13:29 ` Ulf Hansson 0 siblings, 1 reply; 16+ messages in thread From: Sudeep Holla @ 2016-06-15 13:23 UTC (permalink / raw) To: Ulf Hansson Cc: Sudeep Holla, linux-kernel, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rafael J. Wysocki, Kevin Hilman, linux-pm On 15/06/16 14:05, Ulf Hansson wrote: > On 6 June 2016 at 17:53, Sudeep Holla <sudeep.holla@arm.com> wrote: >> This patch hooks up the support for device power domain provided by >> SCPI using the Linux generic power domain infrastructure. >> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Cc: Kevin Hilman <khilman@kernel.org> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> Cc: linux-pm@vger.kernel.org >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > For following versions, please keep me in the loop for the entire > series. Including the cover-letter which I am unable to find. > Ok, will do. >> --- >> drivers/firmware/Kconfig | 8 +++ >> drivers/firmware/Makefile | 1 + >> drivers/firmware/scpi_pd.c | 152 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 161 insertions(+) >> create mode 100644 drivers/firmware/scpi_pd.c >> >> Hi, >> >> Since most of the power controller drivers are place in drivers/soc/<soc_name>, >> I am not sure where to put this SCPI power domain code as it can be used >> on multiple SoC. I have placed it in drivers/firmware temporarily for >> review. Please suggest the most apt place to put this driver. > > To me, I think it makes sense to put this in the suggested directory, > as it's not SoC specific code. > Sure >> >> Regards, >> Sudeep >> >> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig >> index 41abdc54815e..80c963c60f13 100644 >> --- a/drivers/firmware/Kconfig >> +++ b/drivers/firmware/Kconfig >> @@ -27,6 +27,14 @@ config ARM_SCPI_PROTOCOL >> This protocol library provides interface for all the client drivers >> making use of the features offered by the SCP. >> >> +config ARM_SCPI_POWER_DOMAIN >> + tristate "SCPI power domain driver" >> + depends on (ARM_SCPI_PROTOCOL && PM) || COMPILE_TEST >> + select PM_GENERIC_DOMAINS_OF > > I think this is better: > depends on (ARM_SCPI_PROTOCOL) || COMPILE_TEST > select PM_GENERIC_DOMAINS if PM > Yes it's changed already like this after Tixy reported an issue. >> + help >> + This enables support for the SCPI power domains which can be >> + enabled or disabled via the SCP firmware >> + >> config EDD >> tristate "BIOS Enhanced Disk Drive calls determine boot disk" >> depends on X86 >> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile >> index 474bada56fcd..24f7fe8e3fc3 100644 >> --- a/drivers/firmware/Makefile >> +++ b/drivers/firmware/Makefile >> @@ -3,6 +3,7 @@ >> # >> obj-$(CONFIG_ARM_PSCI_FW) += psci.o >> obj-$(CONFIG_ARM_SCPI_PROTOCOL) += arm_scpi.o >> +obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pd.o >> obj-$(CONFIG_DMI) += dmi_scan.o >> obj-$(CONFIG_DMI_SYSFS) += dmi-sysfs.o >> obj-$(CONFIG_EDD) += edd.o >> diff --git a/drivers/firmware/scpi_pd.c b/drivers/firmware/scpi_pd.c > > Perhaps name it scpi_pm_domain.c instead as it gives a better > description of its purpose. > Agreed. [...] >> +static const struct of_device_id scpi_power_domain_ids[] = { >> + { .compatible = "arm,scpi-power-domains", }, >> + { /* sentinel */ } >> +}; > > Actually I think you shouldn't implement this a standalone driver and > thus you can remove this compatible. > While I tend to agree, I did this to keep it aligned with other SCPI users(clocks, sensors,.. for example). I assume remove compatible just from driver ? IMO, it doesn't make sense to add power domain provider without a compatible. > Instead, I think it's better if you let the arm_scpi driver to also > initialize the PM domain. > OK, I can do that. > If you still want the PM domain code to be maintained in a separate > file, just provide a header file which declares an > "scpi_pm_domain_init()" function (and a stub when not supported), > which the arm_scpi driver should call during ->probe(). > I am fine with that, just that it deviates from the approach taken in other subsystems as I mentioned above. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd 2016-06-15 13:23 ` Sudeep Holla @ 2016-06-15 13:29 ` Ulf Hansson 2016-06-15 13:44 ` Sudeep Holla 0 siblings, 1 reply; 16+ messages in thread From: Ulf Hansson @ 2016-06-15 13:29 UTC (permalink / raw) To: Sudeep Holla Cc: linux-kernel, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rafael J. Wysocki, Kevin Hilman, linux-pm [...] > >>> +static const struct of_device_id scpi_power_domain_ids[] = { >>> + { .compatible = "arm,scpi-power-domains", }, >>> + { /* sentinel */ } >>> +}; >> >> >> Actually I think you shouldn't implement this a standalone driver and >> thus you can remove this compatible. >> > > While I tend to agree, I did this to keep it aligned with other SCPI > users(clocks, sensors,.. for example). > > I assume remove compatible just from driver ? IMO, it doesn't make sense > to add power domain provider without a compatible. > >> Instead, I think it's better if you let the arm_scpi driver to also >> initialize the PM domain. >> > > OK, I can do that. > >> If you still want the PM domain code to be maintained in a separate >> file, just provide a header file which declares an >> "scpi_pm_domain_init()" function (and a stub when not supported), >> which the arm_scpi driver should call during ->probe(). >> > > I am fine with that, just that it deviates from the approach taken in > other subsystems as I mentioned above. If DT maintainers are happy with you adding a compatible for this, don't let me stop you from implementing this as standalone driver. I have no strong opinions about it, so perhaps it's then better to not deviate from other cases!? Kind regards Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] firmware: scpi: add device power domain support using genpd 2016-06-15 13:29 ` Ulf Hansson @ 2016-06-15 13:44 ` Sudeep Holla 0 siblings, 0 replies; 16+ messages in thread From: Sudeep Holla @ 2016-06-15 13:44 UTC (permalink / raw) To: Ulf Hansson Cc: Sudeep Holla, linux-kernel, Jon Medhurst, Mathieu Poirier, Suzuki K Poulose, Rafael J. Wysocki, Kevin Hilman, linux-pm On 15/06/16 14:29, Ulf Hansson wrote: > [...] > >> >>>> +static const struct of_device_id scpi_power_domain_ids[] = { >>>> + { .compatible = "arm,scpi-power-domains", }, >>>> + { /* sentinel */ } >>>> +}; >>> >>> >>> Actually I think you shouldn't implement this a standalone driver and >>> thus you can remove this compatible. >>> >> >> While I tend to agree, I did this to keep it aligned with other SCPI >> users(clocks, sensors,.. for example). >> >> I assume remove compatible just from driver ? IMO, it doesn't make sense >> to add power domain provider without a compatible. >> >>> Instead, I think it's better if you let the arm_scpi driver to also >>> initialize the PM domain. >>> >> >> OK, I can do that. >> >>> If you still want the PM domain code to be maintained in a separate >>> file, just provide a header file which declares an >>> "scpi_pm_domain_init()" function (and a stub when not supported), >>> which the arm_scpi driver should call during ->probe(). >>> >> >> I am fine with that, just that it deviates from the approach taken in >> other subsystems as I mentioned above. > > If DT maintainers are happy with you adding a compatible for this, > don't let me stop you from implementing this as standalone driver. > I assume compatibles are always preferred even if they are not used to make it future proof and may be that's why the binding was accepted. We need to have a node to specify phandle in the consumers anyways, it's always better to have separate node for each of the SCPI users/provider (like clock, sensors, power domains) instead of pointing all to the one SCPI node. Again that's just my view. > I have no strong opinions about it, so perhaps it's then better to not > deviate from other cases!? > OK, thanks. I will respin with Kconfig changes and retain the file in drivers/firmware for now. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-06-15 13:44 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-06 15:53 [PATCH 0/3] firmware: scpi: add device power domain support Sudeep Holla 2016-06-06 15:53 ` [PATCH 1/3] firmware: arm_scpi: add support for device power state management Sudeep Holla 2016-06-07 12:58 ` Jon Medhurst (Tixy) 2016-06-07 13:00 ` Sudeep Holla 2016-06-06 15:53 ` [PATCH 2/3] Documentation: add DT bindings for ARM SCPI power domains Sudeep Holla 2016-06-07 13:22 ` Mark Rutland 2016-06-07 13:39 ` Sudeep Holla 2016-06-07 14:45 ` Mark Rutland 2016-06-06 15:53 ` [PATCH 3/3] firmware: scpi: add device power domain support using genpd Sudeep Holla 2016-06-07 13:18 ` Jon Medhurst (Tixy) 2016-06-07 13:29 ` Sudeep Holla 2016-06-10 16:19 ` Sudeep Holla 2016-06-15 13:05 ` Ulf Hansson 2016-06-15 13:23 ` Sudeep Holla 2016-06-15 13:29 ` Ulf Hansson 2016-06-15 13:44 ` Sudeep Holla
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).