Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1465093 > unrolled thread

[PATCH 00/13] scpi: Add support for legacy SCPI protocol

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/13] scpi: Add support for legacy SCPI protocol Neil Armstrong <narmstrong@baylibre.com> - 2016-08-18 12:20 +0200
    [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure Neil Armstrong <narmstrong@baylibre.com> - 2016-08-18 12:20 +0200
      Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure Sudeep Holla <sudeep.holla@arm.com> - 2016-08-19 18:50 +0200
        Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure Neil Armstrong <narmstrong@baylibre.com> - 2016-08-23 10:30 +0200
          Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure Sudeep Holla <sudeep.holla@arm.com> - 2016-08-23 17:00 +0200
    [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message Neil Armstrong <narmstrong@baylibre.com> - 2016-08-18 12:20 +0200
      Re: [PATCH 04/13] scpi: Add legacy SCP functions calling  legacy_scpi_send_message Sudeep Holla <sudeep.holla@arm.com> - 2016-08-19 18:30 +0200
        Re: [PATCH 04/13] scpi: Add legacy SCP functions calling  legacy_scpi_send_message Neil Armstrong <narmstrong@baylibre.com> - 2016-08-23 10:20 +0200
          Re: [PATCH 04/13] scpi: Add legacy SCP functions calling  legacy_scpi_send_message Sudeep Holla <sudeep.holla@arm.com> - 2016-08-23 16:50 +0200
    [PATCH 09/13] scpi: implement rockchip support via the vendor_msg mechanism Neil Armstrong <narmstrong@baylibre.com> - 2016-08-18 12:20 +0200
    [PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands Neil Armstrong <narmstrong@baylibre.com> - 2016-08-18 12:20 +0200
      Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to  vendor commands Sudeep Holla <sudeep.holla@arm.com> - 2016-08-19 03:30 +0200
        Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to  vendor commands Neil Armstrong <narmstrong@baylibre.com> - 2016-08-19 10:10 +0200
          Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to  vendor commands Sudeep Holla <sudeep.holla@arm.com> - 2016-08-19 12:40 +0200
    [PATCH 11/13] dt-bindings: Add support for Amlogic GXBB SCPI Interface Neil Armstrong <narmstrong@baylibre.com> - 2016-08-18 12:20 +0200
      Re: [PATCH 11/13] dt-bindings: Add support for Amlogic GXBB SCPI  Interface Rob Herring <robh@kernel.org> - 2016-08-19 15:50 +0200
    [PATCH 08/13] scpi: add a vendor_msg mechanism in case the mailbox message differs Neil Armstrong <narmstrong@baylibre.com> - 2016-08-18 12:20 +0200
      Re: [PATCH 08/13] scpi: add a vendor_msg mechanism in case the  mailbox message differs Sudeep Holla <sudeep.holla@arm.com> - 2016-08-19 18:50 +0200
    [PATCH 07/13] scpi: ignore init_versions failure if reported not supported Neil Armstrong <narmstrong@baylibre.com> - 2016-08-18 12:20 +0200
      Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not  supported Sudeep Holla <sudeep.holla@arm.com> - 2016-08-19 18:50 +0200
        Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not  supported Neil Armstrong <narmstrong@baylibre.com> - 2016-08-23 10:30 +0200
          Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not  supported Sudeep Holla <sudeep.holla@arm.com> - 2016-08-23 17:00 +0200
            Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not  supported Sudeep Holla <sudeep.holla@arm.com> - 2016-08-23 17:10 +0200
            Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not  supported Neil Armstrong <narmstrong@baylibre.com> - 2016-08-23 17:10 +0200

Page 1 of 2  [1] 2  Next page →

#1465093 — [PATCH 00/13] scpi: Add support for legacy SCPI protocol

This patchset aims to support the legacy SCPI firmware implementation that was
delivered as early technology preview for the JUNO platform.

Finally a stable, maintained and public implementation for the SCPI protocol
has been upstreamed part of the JUNO support and it is the recommended way
of implementing SCP communication on ARMv8 platforms.

The Amlogic GXBB platform is using this legacy protocol, as the RK3368 & RK3399
platforms.

This patchset add support for the legacy protocol in the arm_scpi.c file,
avoiding code duplication.

Last RFC discution tread can be found at : https://lkml.org/lkml/2016/8/9/210

The first patch is here to provide vendor commands on the official SCPI protocol,
it can be delayed to another patchset.

Patches 8 & 9 are only here to demo how Rockchip support could be implemented, these
patches should be delayed to a rockchip specific patchset.

The last patch depends on the "Platform MHU" dtsi patch.

Neil Armstrong (13):
  scpi: Add vendor_send_message to enable access to vendor commands
  scpi: Add alternative legacy structures and macros
  scpi: Add legacy send, prepare and handle remote functions
  scpi: Add legacy SCP functions calling legacy_scpi_send_message
  scpi: move of_match table before probe functions
  scpi: add priv_scpi_ops and fill legacy structure
  scpi: ignore init_versions failure if reported not supported
  scpi: add a vendor_msg mechanism in case the mailbox message differs
  scpi: implement rockchip support via the vendor_msg mechanism
  scpi: grow MAX_DVFS_OPPS to 16 entries
  dt-bindings: Add support for Amlogic GXBB SCPI Interface
  ARM64: dts: meson-gxbb: Add SRAM node
  ARM64: dts: meson-gxbb: Add SCPI with cpufreq & sensors Nodes

 Documentation/devicetree/bindings/arm/arm,scpi.txt |   8 +-
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  45 ++
 drivers/firmware/arm_scpi.c                        | 455 ++++++++++++++++++++-
 include/linux/scpi_protocol.h                      |   4 +
 4 files changed, 490 insertions(+), 22 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1465096 — [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure

In order to use the legacy functions variants, add a new priv_scpi_ops
structure that will contain the internal alterne functions and then use these
alternate call in the probe function.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 68 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index b0d911b..3fe39fe 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -213,6 +213,7 @@ struct scpi_drvinfo {
 	struct scpi_ops *scpi_ops;
 	struct scpi_chan *channels;
 	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
+	const struct priv_scpi_ops *ops;
 };
 
 /*
@@ -299,6 +300,17 @@ struct dev_pstate_set {
 	u8 pstate;
 } __packed;
 
+struct priv_scpi_ops {
+	/* Internal Specific Ops */
+	void (*handle_remote_msg)(struct mbox_client *c, void *msg);
+	void (*tx_prepare)(struct mbox_client *c, void *msg);
+	/* Message Specific Ops */
+	int (*init_versions)(struct scpi_drvinfo *info);
+	int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf);
+	/* System wide Ops */
+	struct scpi_ops *scpi_ops;
+};
+
 static struct scpi_drvinfo *scpi_info;
 
 static int scpi_linux_errmap[SCPI_ERR_MAX] = {
@@ -695,9 +707,12 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
 	if (scpi_info->dvfs[domain])	/* data already populated */
 		return scpi_info->dvfs[domain];
 
-	ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, &domain, sizeof(domain),
+	if (scpi_info->ops && scpi_info->ops->dvfs_get_info)
+		ret = scpi_info->ops->dvfs_get_info(domain, &buf);
+	else
+		ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO,
+					&domain, sizeof(domain),
 				&buf, sizeof(buf));
-
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = {
 	.vendor_send_message = scpi_ext_send_message,
 };
 
+static struct scpi_ops legacy_scpi_ops = {
+	.get_version = scpi_get_version,
+	.clk_get_range = NULL,
+	.clk_get_val = legacy_scpi_clk_get_val,
+	.clk_set_val = legacy_scpi_clk_set_val,
+	.dvfs_get_idx = legacy_scpi_dvfs_get_idx,
+	.dvfs_set_idx = legacy_scpi_dvfs_set_idx,
+	.dvfs_get_info = scpi_dvfs_get_info,
+	.sensor_get_capability = legacy_scpi_sensor_get_capability,
+	.sensor_get_info = legacy_scpi_sensor_get_info,
+	.sensor_get_value = legacy_scpi_sensor_get_value,
+	.device_get_power_state = NULL,
+	.device_set_power_state = NULL,
+	.vendor_send_message = legacy_scpi_send_message,
+};
+
 struct scpi_ops *get_scpi_ops(void)
 {
 	return scpi_info ? scpi_info->scpi_ops : NULL;
@@ -972,8 +1003,17 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
 	return 0;
 }
 
+static const struct priv_scpi_ops scpi_legacy_ops = {
+	.handle_remote_msg = legacy_scpi_handle_remote_msg,
+	.tx_prepare = legacy_scpi_tx_prepare,
+	.init_versions = legacy_scpi_init_versions,
+	.dvfs_get_info = legacy_scpi_dvfs_get_info,
+	.scpi_ops = &legacy_scpi_ops,
+};
+
 static const struct of_device_id scpi_of_match[] = {
 	{.compatible = "arm,scpi"},
+	{.compatible = "amlogic,meson-gxbb-scpi", .data = &scpi_legacy_ops},
 	{},
 };
 
@@ -986,11 +1026,18 @@ static int scpi_probe(struct platform_device *pdev)
 	struct scpi_chan *scpi_chan;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	const struct of_device_id *match;
+
+	match = of_match_device(scpi_of_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
 
 	scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
 	if (!scpi_info)
 		return -ENOMEM;
 
+	scpi_info->ops = match->data;
+
 	count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
 	if (count < 0) {
 		dev_err(dev, "no mboxes property in '%s'\n", np->full_name);
@@ -1023,7 +1070,13 @@ static int scpi_probe(struct platform_device *pdev)
 		pchan->tx_payload = pchan->rx_payload + (size >> 1);
 
 		cl->dev = dev;
-		cl->rx_callback = scpi_handle_remote_msg;
+		if (scpi_info->ops && scpi_info->ops->handle_remote_msg)
+			cl->rx_callback = scpi_info->ops->handle_remote_msg;
+		else
+			cl->rx_callback = scpi_handle_remote_msg;
+		if (scpi_info->ops && scpi_info->ops->tx_prepare)
+			cl->tx_prepare = scpi_info->ops->tx_prepare;
+		else
 		cl->tx_prepare = scpi_tx_prepare;
 		cl->tx_block = true;
 		cl->tx_tout = 20;
@@ -1054,6 +1107,9 @@ err:
 	scpi_info->num_chans = count;
 	platform_set_drvdata(pdev, scpi_info);
 
+	if (scpi_info->ops && scpi_info->ops->init_versions)
+		ret = scpi_info->ops->init_versions(scpi_info);
+	else
 	ret = scpi_init_versions(scpi_info);
 	if (ret) {
 		dev_err(dev, "incorrect or no SCP firmware found\n");
@@ -1067,7 +1123,11 @@ err:
 		  FW_REV_MAJOR(scpi_info->firmware_version),
 		  FW_REV_MINOR(scpi_info->firmware_version),
 		  FW_REV_PATCH(scpi_info->firmware_version));
-	scpi_info->scpi_ops = &scpi_ops;
+
+	if (scpi_info->ops && scpi_info->ops->scpi_ops)
+		scpi_info->scpi_ops = scpi_info->ops->scpi_ops;
+	else
+		scpi_info->scpi_ops = &scpi_ops;
 
 	ret = sysfs_create_groups(&dev->kobj, versions_groups);
 	if (ret)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1466556 — Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure

On 18/08/16 11:10, Neil Armstrong wrote:
> In order to use the legacy functions variants, add a new priv_scpi_ops
> structure that will contain the internal alterne functions and then use these
> alternate call in the probe function.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 68 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index b0d911b..3fe39fe 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -213,6 +213,7 @@ struct scpi_drvinfo {
>  	struct scpi_ops *scpi_ops;
>  	struct scpi_chan *channels;
>  	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
> +	const struct priv_scpi_ops *ops;
>  };
>
>  /*
> @@ -299,6 +300,17 @@ struct dev_pstate_set {
>  	u8 pstate;
>  } __packed;
>
> +struct priv_scpi_ops {
> +	/* Internal Specific Ops */
> +	void (*handle_remote_msg)(struct mbox_client *c, void *msg);
> +	void (*tx_prepare)(struct mbox_client *c, void *msg);
> +	/* Message Specific Ops */
> +	int (*init_versions)(struct scpi_drvinfo *info);
> +	int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf);
> +	/* System wide Ops */
> +	struct scpi_ops *scpi_ops;
> +};
> +


I fail to understand the need for this. Can you please explain the issue
you would face without this ?

>  static struct scpi_drvinfo *scpi_info;
>
>  static int scpi_linux_errmap[SCPI_ERR_MAX] = {
> @@ -695,9 +707,12 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>  	if (scpi_info->dvfs[domain])	/* data already populated */
>  		return scpi_info->dvfs[domain];
>
> -	ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, &domain, sizeof(domain),
> +	if (scpi_info->ops && scpi_info->ops->dvfs_get_info)
> +		ret = scpi_info->ops->dvfs_get_info(domain, &buf);
> +	else
> +		ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO,
> +					&domain, sizeof(domain),
>  				&buf, sizeof(buf));
> -
>  	if (ret)
>  		return ERR_PTR(ret);
>
> @@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = {
>  	.vendor_send_message = scpi_ext_send_message,
>  };
>
> +static struct scpi_ops legacy_scpi_ops = {
> +	.get_version = scpi_get_version,
> +	.clk_get_range = NULL,
> +	.clk_get_val = legacy_scpi_clk_get_val,
> +	.clk_set_val = legacy_scpi_clk_set_val,
> +	.dvfs_get_idx = legacy_scpi_dvfs_get_idx,
> +	.dvfs_set_idx = legacy_scpi_dvfs_set_idx,
> +	.dvfs_get_info = scpi_dvfs_get_info,
> +	.sensor_get_capability = legacy_scpi_sensor_get_capability,
> +	.sensor_get_info = legacy_scpi_sensor_get_info,
> +	.sensor_get_value = legacy_scpi_sensor_get_value,
> +	.device_get_power_state = NULL,
> +	.device_set_power_state = NULL,
> +	.vendor_send_message = legacy_scpi_send_message,

I think we need not have this at all if you follow the suggestion I had
in the previous patch. Try and let's see how it would look.

> +};
> +
>  struct scpi_ops *get_scpi_ops(void)
>  {
>  	return scpi_info ? scpi_info->scpi_ops : NULL;
> @@ -972,8 +1003,17 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
>  	return 0;
>  }
>
> +static const struct priv_scpi_ops scpi_legacy_ops = {
> +	.handle_remote_msg = legacy_scpi_handle_remote_msg,
> +	.tx_prepare = legacy_scpi_tx_prepare,
> +	.init_versions = legacy_scpi_init_versions,
> +	.dvfs_get_info = legacy_scpi_dvfs_get_info,
> +	.scpi_ops = &legacy_scpi_ops,
> +};
> +

Ditto, can go away.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1468374 — Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure

On 08/19/2016 06:39 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> In order to use the legacy functions variants, add a new priv_scpi_ops
>> structure that will contain the internal alterne functions and then use these
>> alternate call in the probe function.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 68 ++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 64 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index b0d911b..3fe39fe 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -213,6 +213,7 @@ struct scpi_drvinfo {
>>      struct scpi_ops *scpi_ops;
>>      struct scpi_chan *channels;
>>      struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
>> +    const struct priv_scpi_ops *ops;
>>  };
>>
>>  /*
>> @@ -299,6 +300,17 @@ struct dev_pstate_set {
>>      u8 pstate;
>>  } __packed;
>>
>> +struct priv_scpi_ops {
>> +    /* Internal Specific Ops */
>> +    void (*handle_remote_msg)(struct mbox_client *c, void *msg);
>> +    void (*tx_prepare)(struct mbox_client *c, void *msg);
>> +    /* Message Specific Ops */
>> +    int (*init_versions)(struct scpi_drvinfo *info);
>> +    int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf);
>> +    /* System wide Ops */
>> +    struct scpi_ops *scpi_ops;
>> +};
>> +
> 
> 
> I fail to understand the need for this. Can you please explain the issue
> you would face without this ?
> 
>>  static struct scpi_drvinfo *scpi_info;
>>
>>  static int scpi_linux_errmap[SCPI_ERR_MAX] = {
>> @@ -695,9 +707,12 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>>      if (scpi_info->dvfs[domain])    /* data already populated */
>>          return scpi_info->dvfs[domain];
>>
>> -    ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, &domain, sizeof(domain),
>> +    if (scpi_info->ops && scpi_info->ops->dvfs_get_info)
>> +        ret = scpi_info->ops->dvfs_get_info(domain, &buf);
>> +    else
>> +        ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO,
>> +                    &domain, sizeof(domain),
>>                  &buf, sizeof(buf));
>> -
>>      if (ret)
>>          return ERR_PTR(ret);
>>
>> @@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = {
>>      .vendor_send_message = scpi_ext_send_message,
>>  };
>>
>> +static struct scpi_ops legacy_scpi_ops = {
>> +    .get_version = scpi_get_version,
>> +    .clk_get_range = NULL,
>> +    .clk_get_val = legacy_scpi_clk_get_val,
>> +    .clk_set_val = legacy_scpi_clk_set_val,
>> +    .dvfs_get_idx = legacy_scpi_dvfs_get_idx,
>> +    .dvfs_set_idx = legacy_scpi_dvfs_set_idx,
>> +    .dvfs_get_info = scpi_dvfs_get_info,
>> +    .sensor_get_capability = legacy_scpi_sensor_get_capability,
>> +    .sensor_get_info = legacy_scpi_sensor_get_info,
>> +    .sensor_get_value = legacy_scpi_sensor_get_value,
>> +    .device_get_power_state = NULL,
>> +    .device_set_power_state = NULL,
>> +    .vendor_send_message = legacy_scpi_send_message,
> 
> I think we need not have this at all if you follow the suggestion I had
> in the previous patch. Try and let's see how it would look.

If you confirm you want the if/else as said in patch 4.

But clk_get_range, device_get/set_power_state are not available in legacy,
I think we should still have this alternate structure.

>> +};
>> +
>>  struct scpi_ops *get_scpi_ops(void)
>>  {
>>      return scpi_info ? scpi_info->scpi_ops : NULL;
>> @@ -972,8 +1003,17 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
>>      return 0;
>>  }
>>
>> +static const struct priv_scpi_ops scpi_legacy_ops = {
>> +    .handle_remote_msg = legacy_scpi_handle_remote_msg,
>> +    .tx_prepare = legacy_scpi_tx_prepare,
>> +    .init_versions = legacy_scpi_init_versions,
>> +    .dvfs_get_info = legacy_scpi_dvfs_get_info,
>> +    .scpi_ops = &legacy_scpi_ops,
>> +};
>> +
> 
> Ditto, can go away.
> 

Yes, in the if/else is_legacy variant this should go away.

Thanks,
Neil

[toc] | [prev] | [next] | [standalone]

#1468613 — Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure

On 23/08/16 09:22, Neil Armstrong wrote:
> On 08/19/2016 06:39 PM, Sudeep Holla wrote:
>>
>>
>> On 18/08/16 11:10, Neil Armstrong wrote:
>>> In order to use the legacy functions variants, add a new priv_scpi_ops
>>> structure that will contain the internal alterne functions and then use these
>>> alternate call in the probe function.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/firmware/arm_scpi.c | 68 ++++++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 64 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>> index b0d911b..3fe39fe 100644
>>> --- a/drivers/firmware/arm_scpi.c
>>> +++ b/drivers/firmware/arm_scpi.c
>>> @@ -213,6 +213,7 @@ struct scpi_drvinfo {
>>>      struct scpi_ops *scpi_ops;
>>>      struct scpi_chan *channels;
>>>      struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
>>> +    const struct priv_scpi_ops *ops;
>>>  };
>>>
>>>  /*
>>> @@ -299,6 +300,17 @@ struct dev_pstate_set {
>>>      u8 pstate;
>>>  } __packed;
>>>
>>> +struct priv_scpi_ops {
>>> +    /* Internal Specific Ops */
>>> +    void (*handle_remote_msg)(struct mbox_client *c, void *msg);
>>> +    void (*tx_prepare)(struct mbox_client *c, void *msg);
>>> +    /* Message Specific Ops */
>>> +    int (*init_versions)(struct scpi_drvinfo *info);
>>> +    int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf);
>>> +    /* System wide Ops */
>>> +    struct scpi_ops *scpi_ops;
>>> +};
>>> +
>>
>>
>> I fail to understand the need for this. Can you please explain the issue
>> you would face without this ?
>>
>>>  static struct scpi_drvinfo *scpi_info;
>>>
>>>  static int scpi_linux_errmap[SCPI_ERR_MAX] = {
>>> @@ -695,9 +707,12 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>>>      if (scpi_info->dvfs[domain])    /* data already populated */
>>>          return scpi_info->dvfs[domain];
>>>
>>> -    ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, &domain, sizeof(domain),
>>> +    if (scpi_info->ops && scpi_info->ops->dvfs_get_info)
>>> +        ret = scpi_info->ops->dvfs_get_info(domain, &buf);
>>> +    else
>>> +        ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO,
>>> +                    &domain, sizeof(domain),
>>>                  &buf, sizeof(buf));
>>> -
>>>      if (ret)
>>>          return ERR_PTR(ret);
>>>
>>> @@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = {
>>>      .vendor_send_message = scpi_ext_send_message,
>>>  };
>>>
>>> +static struct scpi_ops legacy_scpi_ops = {
>>> +    .get_version = scpi_get_version,
>>> +    .clk_get_range = NULL,
>>> +    .clk_get_val = legacy_scpi_clk_get_val,
>>> +    .clk_set_val = legacy_scpi_clk_set_val,
>>> +    .dvfs_get_idx = legacy_scpi_dvfs_get_idx,
>>> +    .dvfs_set_idx = legacy_scpi_dvfs_set_idx,
>>> +    .dvfs_get_info = scpi_dvfs_get_info,
>>> +    .sensor_get_capability = legacy_scpi_sensor_get_capability,
>>> +    .sensor_get_info = legacy_scpi_sensor_get_info,
>>> +    .sensor_get_value = legacy_scpi_sensor_get_value,
>>> +    .device_get_power_state = NULL,
>>> +    .device_set_power_state = NULL,
>>> +    .vendor_send_message = legacy_scpi_send_message,
>>
>> I think we need not have this at all if you follow the suggestion I had
>> in the previous patch. Try and let's see how it would look.
>
> If you confirm you want the if/else as said in patch 4.
>
> But clk_get_range, device_get/set_power_state are not available in legacy,
> I think we should still have this alternate structure.
>

I was thinking of overriding the pointers accordingly at the probe time
as the common list is bigger than the one that differs.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1465098 — [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message

In order to support legacy SCP functions from kernel-wide driver, add legacy
functions using the legacy command enums and calling legacy_scpi_send_message.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 118 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 50b1297..bb9965f 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
 	return ret;
 }
 
+/* scpi_clk_get_range not available for legacy */
+
 static unsigned long scpi_clk_get_val(u16 clk_id)
 {
 	int ret;
@@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
 	return ret ? ret : le32_to_cpu(clk.rate);
 }
 
+static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
+{
+	int ret;
+	struct clk_get_value clk;
+	__le16 le_clk_id = cpu_to_le16(clk_id);
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_GET_CLOCK_VALUE,
+				       &le_clk_id, sizeof(le_clk_id),
+				       &clk, sizeof(clk));
+	return ret ? ret : le32_to_cpu(clk.rate);
+}
+
 static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
 {
 	int stat;
@@ -601,6 +615,19 @@ static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
 				 &stat, sizeof(stat));
 }
 
+static int legacy_scpi_clk_set_val(u16 clk_id, unsigned long rate)
+{
+	int stat;
+	struct legacy_clk_set_value clk = {
+		.id = cpu_to_le16(clk_id),
+		.rate = cpu_to_le32(rate)
+	};
+
+	return legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
+					&clk, sizeof(clk),
+					&stat, sizeof(stat));
+}
+
 static int scpi_dvfs_get_idx(u8 domain)
 {
 	int ret;
@@ -611,6 +638,17 @@ static int scpi_dvfs_get_idx(u8 domain)
 	return ret ? ret : dvfs_idx;
 }
 
+static int legacy_scpi_dvfs_get_idx(u8 domain)
+{
+	int ret;
+	u8 dvfs_idx;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_GET_DVFS,
+				       &domain, sizeof(domain),
+				       &dvfs_idx, sizeof(dvfs_idx));
+	return ret ? ret : dvfs_idx;
+}
+
 static int scpi_dvfs_set_idx(u8 domain, u8 index)
 {
 	int stat;
@@ -620,6 +658,16 @@ static int scpi_dvfs_set_idx(u8 domain, u8 index)
 				 &stat, sizeof(stat));
 }
 
+static int legacy_scpi_dvfs_set_idx(u8 domain, u8 index)
+{
+	int stat;
+	struct dvfs_set dvfs = {domain, index};
+
+	return legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_DVFS,
+					&dvfs, sizeof(dvfs),
+					&stat, sizeof(stat));
+}
+
 static int opp_cmp_func(const void *opp1, const void *opp2)
 {
 	const struct scpi_opp *t1 = opp1, *t2 = opp2;
@@ -627,6 +675,13 @@ static int opp_cmp_func(const void *opp1, const void *opp2)
 	return t1->freq - t2->freq;
 }
 
+static int legacy_scpi_dvfs_get_info(u8 domain, struct dvfs_info *buf)
+{
+	return legacy_scpi_send_message(LEGACY_SCPI_CMD_GET_DVFS_INFO,
+					&domain, sizeof(domain),
+					buf, sizeof(*buf));
+}
+
 static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
 {
 	struct scpi_dvfs_info *info;
@@ -683,6 +738,20 @@ static int scpi_sensor_get_capability(u16 *sensors)
 	return ret;
 }
 
+static int legacy_scpi_sensor_get_capability(u16 *sensors)
+{
+	struct sensor_capabilities cap_buf;
+	int ret;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_SENSOR_CAPABILITIES,
+				       NULL, 0,
+				       &cap_buf, sizeof(cap_buf));
+	if (!ret)
+		*sensors = le16_to_cpu(cap_buf.sensors);
+
+	return ret;
+}
+
 static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
 {
 	__le16 id = cpu_to_le16(sensor_id);
@@ -699,6 +768,24 @@ static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
 	return ret;
 }
 
+static int legacy_scpi_sensor_get_info(u16 sensor_id,
+				       struct scpi_sensor_info *info)
+{
+	__le16 id = cpu_to_le16(sensor_id);
+	struct _scpi_sensor_info _info;
+	int ret;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_SENSOR_INFO,
+				       &id, sizeof(id),
+				       &_info, sizeof(_info));
+	if (!ret) {
+		memcpy(info, &_info, sizeof(*info));
+		info->sensor_id = le16_to_cpu(_info.sensor_id);
+	}
+
+	return ret;
+}
+
 static int scpi_sensor_get_value(u16 sensor, u64 *val)
 {
 	__le16 id = cpu_to_le16(sensor);
@@ -714,6 +801,21 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
 	return ret;
 }
 
+static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
+{
+	__le16 id = cpu_to_le16(sensor);
+	struct legacy_sensor_value buf;
+	int ret;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_SENSOR_VALUE,
+				       &id, sizeof(id),
+				       &buf, sizeof(buf));
+	if (!ret)
+		*val = (u64)le32_to_cpu(buf.val);
+
+	return ret;
+}
+
 static int scpi_device_get_power_state(u16 dev_id)
 {
 	int ret;
@@ -773,6 +875,22 @@ static int scpi_init_versions(struct scpi_drvinfo *info)
 	return ret;
 }
 
+static int legacy_scpi_init_versions(struct scpi_drvinfo *info)
+{
+	int ret;
+	struct scp_capabilities caps;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_SCPI_CAPABILITIES,
+				       NULL, 0,
+				       &caps, sizeof(caps));
+	if (!ret) {
+		info->protocol_version = le32_to_cpu(caps.protocol_version);
+		info->firmware_version = le32_to_cpu(caps.platform_version);
+	}
+
+	return ret;
+}
+
 static ssize_t protocol_version_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1466546 — Re: [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message

On 18/08/16 11:10, Neil Armstrong wrote:
> In order to support legacy SCP functions from kernel-wide driver, add legacy
> functions using the legacy command enums and calling legacy_scpi_send_message.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 118 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 50b1297..bb9965f 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>  	return ret;
>  }
>
> +/* scpi_clk_get_range not available for legacy */
> +
>  static unsigned long scpi_clk_get_val(u16 clk_id)
>  {
>  	int ret;
> @@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
>  	return ret ? ret : le32_to_cpu(clk.rate);
>  }
>
> +static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
> +{
> +	int ret;
> +	struct clk_get_value clk;
> +	__le16 le_clk_id = cpu_to_le16(clk_id);
> +
> +	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_GET_CLOCK_VALUE,
> +				       &le_clk_id, sizeof(le_clk_id),
> +				       &clk, sizeof(clk));
> +	return ret ? ret : le32_to_cpu(clk.rate);
> +}
> +
>  static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
>  {
>  	int stat;
> @@ -601,6 +615,19 @@ static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
>  				 &stat, sizeof(stat));
>  }
>
> +static int legacy_scpi_clk_set_val(u16 clk_id, unsigned long rate)
> +{
> +	int stat;
> +	struct legacy_clk_set_value clk = {
> +		.id = cpu_to_le16(clk_id),
> +		.rate = cpu_to_le32(rate)
> +	};
> +
> +	return legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
> +					&clk, sizeof(clk),
> +					&stat, sizeof(stat));

Except this one which has a different structure format, why do we need
to define legacy versions of other functions ? Can't we play with
function pointer or have a boolean in drvinfo structure and use then in
the existing functions as I had shown in one of the earlier emails.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1468372 — Re: [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message

On 08/19/2016 06:22 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> In order to support legacy SCP functions from kernel-wide driver, add legacy
>> functions using the legacy command enums and calling legacy_scpi_send_message.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 118 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 118 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 50b1297..bb9965f 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>>      return ret;
>>  }
>>
>> +/* scpi_clk_get_range not available for legacy */
>> +
>>  static unsigned long scpi_clk_get_val(u16 clk_id)
>>  {
>>      int ret;
>> @@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
>>      return ret ? ret : le32_to_cpu(clk.rate);
>>  }
>>
>> +static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
>> +{
>> +    int ret;
>> +    struct clk_get_value clk;
>> +    __le16 le_clk_id = cpu_to_le16(clk_id);
>> +
>> +    ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_GET_CLOCK_VALUE,
>> +                       &le_clk_id, sizeof(le_clk_id),
>> +                       &clk, sizeof(clk));
>> +    return ret ? ret : le32_to_cpu(clk.rate);
>> +}
>> +
>>  static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
>>  {
>>      int stat;
>> @@ -601,6 +615,19 @@ static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
>>                   &stat, sizeof(stat));
>>  }
>>
>> +static int legacy_scpi_clk_set_val(u16 clk_id, unsigned long rate)
>> +{
>> +    int stat;
>> +    struct legacy_clk_set_value clk = {
>> +        .id = cpu_to_le16(clk_id),
>> +        .rate = cpu_to_le32(rate)
>> +    };
>> +
>> +    return legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
>> +                    &clk, sizeof(clk),
>> +                    &stat, sizeof(stat));
> 
> Except this one which has a different structure format, why do we need
> to define legacy versions of other functions ? Can't we play with
> function pointer or have a boolean in drvinfo structure and use then in
> the existing functions as I had shown in one of the earlier emails.
> 

The main problem is that the command indexes deviates starting at
SCPI_CMD_SET_CSS_PWR_STATE, I'll be pleased to know how to implement it.

Should I add a test :
if (scpi_drvinfo->is_legacy)
	legacy_scpi_send_message(...)
else
	scpi_send_message(...)

In each function ?

My strategy was to leave the "final" function untouched ans provide
alternatives to legacy.
I can add this "is_legacy" if/else instead of ops structures.

Please tell me how you'll implement this, so I'll adapt the merge.

Neil

[toc] | [prev] | [next] | [standalone]

#1468607 — Re: [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message

On 23/08/16 09:19, Neil Armstrong wrote:
> On 08/19/2016 06:22 PM, Sudeep Holla wrote:
>>
>>
>> On 18/08/16 11:10, Neil Armstrong wrote:
>>> In order to support legacy SCP functions from kernel-wide driver, add legacy
>>> functions using the legacy command enums and calling legacy_scpi_send_message.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/firmware/arm_scpi.c | 118 ++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 118 insertions(+)
>>>
>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>> index 50b1297..bb9965f 100644
>>> --- a/drivers/firmware/arm_scpi.c
>>> +++ b/drivers/firmware/arm_scpi.c
>>> @@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>>>      return ret;
>>>  }
>>>
>>> +/* scpi_clk_get_range not available for legacy */
>>> +
>>>  static unsigned long scpi_clk_get_val(u16 clk_id)
>>>  {
>>>      int ret;
>>> @@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
>>>      return ret ? ret : le32_to_cpu(clk.rate);
>>>  }
>>>
>>> +static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
>>> +{
>>> +    int ret;
>>> +    struct clk_get_value clk;
>>> +    __le16 le_clk_id = cpu_to_le16(clk_id);
>>> +
>>> +    ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_GET_CLOCK_VALUE,
>>> +                       &le_clk_id, sizeof(le_clk_id),
>>> +                       &clk, sizeof(clk));
>>> +    return ret ? ret : le32_to_cpu(clk.rate);
>>> +}
>>> +
>>>  static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
>>>  {
>>>      int stat;
>>> @@ -601,6 +615,19 @@ static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
>>>                   &stat, sizeof(stat));
>>>  }
>>>
>>> +static int legacy_scpi_clk_set_val(u16 clk_id, unsigned long rate)
>>> +{
>>> +    int stat;
>>> +    struct legacy_clk_set_value clk = {
>>> +        .id = cpu_to_le16(clk_id),
>>> +        .rate = cpu_to_le32(rate)
>>> +    };
>>> +
>>> +    return legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
>>> +                    &clk, sizeof(clk),
>>> +                    &stat, sizeof(stat));
>>
>> Except this one which has a different structure format, why do we need
>> to define legacy versions of other functions ? Can't we play with
>> function pointer or have a boolean in drvinfo structure and use then in
>> the existing functions as I had shown in one of the earlier emails.
>>
>
> The main problem is that the command indexes deviates starting at
> SCPI_CMD_SET_CSS_PWR_STATE, I'll be pleased to know how to implement it.
>

Yes, I was thinking of some kind of mapping to new index using an array.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1465100 — [PATCH 09/13] scpi: implement rockchip support via the vendor_msg mechanism

In order to support the Rockchip legacy procotol, implement the vendor
mailbox specific functions and add ops to the of_match table.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 275feef..d73db7c 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -455,6 +455,37 @@ static int legacy_scpi_get_chan(u8 cmd)
 	return 0;
 }
 
+/* Rockchip SoCs needs a special structure as a message */
+
+struct rockchip_scpi_xfer {
+	u32 cmd;
+	int rx_size;
+};
+
+static int rockchip_init(struct device *dev, struct scpi_chan *chan)
+{
+	chan->vendor_data = devm_kmalloc(dev,
+					 sizeof(struct rockchip_scpi_xfer),
+					 GFP_KERNEL);
+	if (!chan->vendor_data)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int rockchip_prepare(struct scpi_chan *chan)
+{
+	struct scpi_xfer *msg = chan->t;
+	struct rockchip_scpi_xfer *xfer = chan->vendor_data;
+
+	xfer->cmd = msg->cmd;
+	xfer->rx_size = msg->rx_len;
+
+	msg->vendor_msg = xfer;
+
+	return 0;
+}
+
 static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
 {
 	struct scpi_xfer *t;
@@ -1026,9 +1057,21 @@ static const struct priv_scpi_ops scpi_legacy_ops = {
 	.scpi_ops = &legacy_scpi_ops,
 };
 
+static const struct priv_scpi_ops scpi_rockchip_ops = {
+	.init = rockchip_init,
+	.prepare = rockchip_prepare,
+	.handle_remote_msg = legacy_scpi_handle_remote_msg,
+	.tx_prepare = legacy_scpi_tx_prepare,
+	.init_versions = legacy_scpi_init_versions,
+	.dvfs_get_info = legacy_scpi_dvfs_get_info,
+	.scpi_ops = &legacy_scpi_ops,
+};
+
 static const struct of_device_id scpi_of_match[] = {
 	{.compatible = "arm,scpi"},
 	{.compatible = "amlogic,meson-gxbb-scpi", .data = &scpi_legacy_ops},
+	{.compatible = "rockchip,rk3368-scpi", .data = &scpi_rockchip_ops},
+	{.compatible = "rockchip,rk3399-scpi", .data = &scpi_rockchip_ops},
 	{},
 };
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1465101 — [PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands

Adds an optional vendor_send_message to the scpi to enable sending
vendor platform specific commands to the SCP firmware.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c   | 26 +++++++++++++++++++++++---
 include/linux/scpi_protocol.h |  4 ++++
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 4388937..403783a 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -46,6 +46,8 @@
 
 #define CMD_ID_SHIFT		0
 #define CMD_ID_MASK		0x7f
+#define CMD_SET_SHIFT		7
+#define CMD_SET_MASK		0x1
 #define CMD_TOKEN_ID_SHIFT	8
 #define CMD_TOKEN_ID_MASK	0xff
 #define CMD_DATA_SIZE_SHIFT	16
@@ -53,6 +55,10 @@
 #define PACK_SCPI_CMD(cmd_id, tx_sz)			\
 	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |	\
 	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
+#define PACK_EXT_SCPI_CMD(cmd_id, tx_sz)		\
+	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |	\
+	(CMD_SET_MASK << CMD_SET_SHIFT) |		\
+	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
 #define ADD_SCPI_TOKEN(cmd, token)			\
 	((cmd) |= (((token) & CMD_TOKEN_ID_MASK) << CMD_TOKEN_ID_SHIFT))
 
@@ -344,8 +350,8 @@ static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
 	mutex_unlock(&ch->xfers_lock);
 }
 
-static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
-			     void *rx_buf, unsigned int rx_len)
+static int __scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+			       void *rx_buf, unsigned int rx_len, bool extn)
 {
 	int ret;
 	u8 chan;
@@ -360,7 +366,8 @@ static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
 		return -ENOMEM;
 
 	msg->slot = BIT(SCPI_SLOT);
-	msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
+	msg->cmd = extn ? PACK_EXT_SCPI_CMD(cmd, tx_len) :
+			  PACK_SCPI_CMD(cmd, tx_len);
 	msg->tx_buf = tx_buf;
 	msg->tx_len = tx_len;
 	msg->rx_buf = rx_buf;
@@ -385,6 +392,18 @@ out:
 	return ret > 0 ? scpi_to_linux_errno(ret) : ret;
 }
 
+static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+			     void *rx_buf, unsigned int rx_len)
+{
+	return __scpi_send_message(cmd, tx_buf, tx_len, rx_buf, rx_len, false);
+}
+
+static int scpi_ext_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+				 void *rx_buf, unsigned int rx_len)
+{
+	return __scpi_send_message(cmd, tx_buf, tx_len, rx_buf, rx_len, true);
+}
+
 static u32 scpi_get_version(void)
 {
 	return scpi_info->protocol_version;
@@ -578,6 +597,7 @@ static struct scpi_ops scpi_ops = {
 	.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,
+	.vendor_send_message = scpi_ext_send_message,
 };
 
 struct scpi_ops *get_scpi_ops(void)
diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
index dc5f989..2b68581 100644
--- a/include/linux/scpi_protocol.h
+++ b/include/linux/scpi_protocol.h
@@ -58,6 +58,8 @@ struct scpi_sensor_info {
  *	OPP is an index to the list return by @dvfs_get_info
  * @dvfs_get_info: returns the DVFS capabilities of the given power
  *	domain. It includes the OPP list and the latency information
+ * @vendor_send_message: vendor specific message sending, arg can specify
+ *	a scpi implementation specific argument
  */
 struct scpi_ops {
 	u32 (*get_version)(void);
@@ -72,6 +74,8 @@ struct scpi_ops {
 	int (*sensor_get_value)(u16, u64 *);
 	int (*device_get_power_state)(u16);
 	int (*device_set_power_state)(u16, u8);
+	int (*vendor_send_message)(u8 cmd, void *tx_buf, unsigned int tx_len,
+				   void *rx_buf, unsigned int rx_len);
 };
 
 #if IS_REACHABLE(CONFIG_ARM_SCPI_PROTOCOL)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1465793 — Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands

On 18/08/16 11:10, Neil Armstrong wrote:
> Adds an optional vendor_send_message to the scpi to enable sending
> vendor platform specific commands to the SCP firmware.
>

I don't see any users of vendor_send_message in this series, so I prefer
it to be dropped and introduced when required.

Also I had a different view on how to introduce this[1]. I would rather
wait until the requirement comes that enables us to make use of it.
Looks like you took parts of it and introduces vendor_send_message
allowing users to send any data which I don't like especially without
knowing how will it be (ab)used.

-- 
Regards,
Sudeep

[1] http://www.spinics.net/lists/kernel/msg2263649.html

[toc] | [prev] | [next] | [standalone]

#1466212 — Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands

On 08/18/2016 05:53 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> Adds an optional vendor_send_message to the scpi to enable sending
>> vendor platform specific commands to the SCP firmware.
>>
> 
> I don't see any users of vendor_send_message in this series, so I prefer
> it to be dropped and introduced when required.
> 
> Also I had a different view on how to introduce this[1]. I would rather
> wait until the requirement comes that enables us to make use of it.
> Looks like you took parts of it and introduces vendor_send_message
> allowing users to send any data which I don't like especially without
> knowing how will it be (ab)used.
> 

Hi Sudeep,

Indeed, I won't ask you to merge patches 1, 8 and 9, they need refactoring and
a separate patchset.

Vendor commands are not necessary for Amlogic, this is a nice to have since they
have a "user" command that can be extended by a firmware loaded by u-boot.

Could you review the following patches and I'll post a reduced v2 ASAP.

Thanks,
Neil

[toc] | [prev] | [next] | [standalone]

#1466272 — Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands

On 19/08/16 09:00, Neil Armstrong wrote:
> On 08/18/2016 05:53 PM, Sudeep Holla wrote:
>>
>>
>> On 18/08/16 11:10, Neil Armstrong wrote:
>>> Adds an optional vendor_send_message to the scpi to enable sending
>>> vendor platform specific commands to the SCP firmware.
>>>
>>
>> I don't see any users of vendor_send_message in this series, so I prefer
>> it to be dropped and introduced when required.
>>
>> Also I had a different view on how to introduce this[1]. I would rather
>> wait until the requirement comes that enables us to make use of it.
>> Looks like you took parts of it and introduces vendor_send_message
>> allowing users to send any data which I don't like especially without
>> knowing how will it be (ab)used.
>>
>
> Hi Sudeep,
>
> Indeed, I won't ask you to merge patches 1, 8 and 9, they need refactoring and
> a separate patchset.
>

Ah OK,then better if you put then at the end. Otherwise it makes it
difficult to review.

> Vendor commands are not necessary for Amlogic, this is a nice to have
> since they have a "user" command that can be extended by a firmware
> loaded by u-boot.
>

Good :)

> Could you review the following patches and I'll post a reduced v2
> ASAP.
>

Yes I was doing that yesterday, sorry got distracted. I will continue today.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1465104 — [PATCH 11/13] dt-bindings: Add support for Amlogic GXBB SCPI Interface

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
index faa4b44..04bc171 100644
--- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
@@ -7,7 +7,7 @@ by Linux to initiate various system control and power operations.
 
 Required properties:
 
-- compatible : should be "arm,scpi"
+- compatible : should be "arm,scpi" or "amlogic,meson-gxbb-scpi"
 - mboxes: List of phandle and mailbox channel specifiers
 	  All the channels reserved by remote SCP firmware for use by
 	  SCPI message protocol should be specified in any order
@@ -60,7 +60,8 @@ A small area of SRAM is reserved for SCPI communication between application
 processors and SCP.
 
 Required properties:
-- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno
+- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno,
+		or "amlogic,meson-gxbb-sram" for Amlogic GXBB SoC.
 
 The rest of the properties should follow the generic mmio-sram description
 found in ../../sram/sram.txt
@@ -70,7 +71,8 @@ Each sub-node represents the reserved area for SCPI.
 Required sub-node properties:
 - reg : The base offset and size of the reserved area with the SRAM
 - compatible : should be "arm,juno-scp-shmem" for Non-secure SRAM based
-	       shared memory on Juno platforms
+	       shared memory on Juno platforms or
+	       "amlogic,meson-gxbb-scp-shmem" for Amlogic GXBB SoC.
 
 Sensor bindings for the sensors based on SCPI Message Protocol
 --------------------------------------------------------------
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1466432 — Re: [PATCH 11/13] dt-bindings: Add support for Amlogic GXBB SCPI Interface

On Thu, Aug 18, 2016 at 12:11:04PM +0200, Neil Armstrong wrote:
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1465106 — [PATCH 08/13] scpi: add a vendor_msg mechanism in case the mailbox message differs

In case the mailbox message pointer must contain a specific structure.
add a mechanism to override the pointer sent to the mailbox by a
vendor specific data initialized by a vendor speficic function.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index d3be4c5..275feef 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -189,6 +189,7 @@ struct scpi_xfer {
 	unsigned int rx_len;
 	struct list_head node;
 	struct completion done;
+	void *vendor_msg;
 };
 
 struct scpi_chan {
@@ -203,6 +204,7 @@ struct scpi_chan {
 	struct mutex xfers_lock;
 	u8 token;
 	struct scpi_xfer *t;
+	void *vendor_data;
 };
 
 struct scpi_drvinfo {
@@ -302,6 +304,8 @@ struct dev_pstate_set {
 
 struct priv_scpi_ops {
 	/* Internal Specific Ops */
+	int (*init)(struct device *dev, struct scpi_chan *chan);
+	int (*prepare)(struct scpi_chan *chan);
 	void (*handle_remote_msg)(struct mbox_client *c, void *msg);
 	void (*tx_prepare)(struct mbox_client *c, void *msg);
 	/* Message Specific Ops */
@@ -498,7 +502,18 @@ static int legacy_scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
 	init_completion(&msg->done);
 	scpi_chan->t = msg;
 
-	ret = mbox_send_message(scpi_chan->chan, &msg->cmd);
+	/* Call the prepare hook to eventually set the vendor_msg */
+	if (scpi_info->ops &&
+	    scpi_info->ops->prepare) {
+		ret = scpi_info->ops->prepare(scpi_chan);
+		if (ret) {
+			mutex_unlock(&scpi_chan->xfers_lock);
+			return ret;
+		}
+	} else
+		msg->vendor_msg = &msg->cmd;
+
+	ret = mbox_send_message(scpi_chan->chan, msg->vendor_msg);
 	if (ret < 0)
 		goto out;
 
@@ -1069,6 +1084,12 @@ static int scpi_probe(struct platform_device *pdev)
 		}
 		pchan->tx_payload = pchan->rx_payload + (size >> 1);
 
+		if (scpi_info->ops && scpi_info->ops->init) {
+			ret = scpi_info->ops->init(dev, pchan);
+			if (ret)
+				goto err;
+		}
+
 		cl->dev = dev;
 		if (scpi_info->ops && scpi_info->ops->handle_remote_msg)
 			cl->rx_callback = scpi_info->ops->handle_remote_msg;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1466555 — Re: [PATCH 08/13] scpi: add a vendor_msg mechanism in case the mailbox message differs

On 18/08/16 11:11, Neil Armstrong wrote:
> In case the mailbox message pointer must contain a specific structure.
> add a mechanism to override the pointer sent to the mailbox by a
> vendor specific data initialized by a vendor speficic function.
>

As discussed, I won't look at this and next patch. You can drop from now.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1465107 — [PATCH 07/13] scpi: ignore init_versions failure if reported not supported

In Amlogic GXBB Legacy SCPI, the LEGACY_SCPI_CMD_SCPI_CAPABILITIES report
as SCPI_ERR_SUPPORT, so do not fail if this command is not supported.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 3fe39fe..d3be4c5 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -1111,12 +1111,13 @@ err:
 		ret = scpi_info->ops->init_versions(scpi_info);
 	else
 	ret = scpi_init_versions(scpi_info);
-	if (ret) {
+	if (ret && ret != -EOPNOTSUPP) {
 		dev_err(dev, "incorrect or no SCP firmware found\n");
 		scpi_remove(pdev);
 		return ret;
 	}
 
+	if (ret != -EOPNOTSUPP) {
 	_dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
 		  PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
 		  PROTOCOL_REV_MINOR(scpi_info->protocol_version),
@@ -1124,15 +1125,16 @@ err:
 		  FW_REV_MINOR(scpi_info->firmware_version),
 		  FW_REV_PATCH(scpi_info->firmware_version));
 
+		ret = sysfs_create_groups(&dev->kobj, versions_groups);
+		if (ret)
+			dev_err(dev, "unable to create sysfs version group\n");
+	}
+
 	if (scpi_info->ops && scpi_info->ops->scpi_ops)
 		scpi_info->scpi_ops = scpi_info->ops->scpi_ops;
 	else
 		scpi_info->scpi_ops = &scpi_ops;
 
-	ret = sysfs_create_groups(&dev->kobj, versions_groups);
-	if (ret)
-		dev_err(dev, "unable to create sysfs version group\n");
-
 	return of_platform_populate(dev->of_node, NULL, NULL, dev);
 }
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1466554 — Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not supported

On 18/08/16 11:11, Neil Armstrong wrote:
> In Amlogic GXBB Legacy SCPI, the LEGACY_SCPI_CMD_SCPI_CAPABILITIES report
> as SCPI_ERR_SUPPORT, so do not fail if this command is not supported.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 3fe39fe..d3be4c5 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -1111,12 +1111,13 @@ err:
>  		ret = scpi_info->ops->init_versions(scpi_info);
>  	else
>  	ret = scpi_init_versions(scpi_info);
> -	if (ret) {
> +	if (ret && ret != -EOPNOTSUPP) {
>  		dev_err(dev, "incorrect or no SCP firmware found\n");
>  		scpi_remove(pdev);
>  		return ret;
>  	}
>

Why not deal it in init_versions itself.

> +	if (ret != -EOPNOTSUPP) {
>  	_dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
>  		  PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
>  		  PROTOCOL_REV_MINOR(scpi_info->protocol_version),

Why not have default value like 0.0 ? Just add a comment. Since get
version is exported out, IMO having default value makes more sense. What
do you think ?

> @@ -1124,15 +1125,16 @@ err:
>  		  FW_REV_MINOR(scpi_info->firmware_version),
>  		  FW_REV_PATCH(scpi_info->firmware_version));
>
> +		ret = sysfs_create_groups(&dev->kobj, versions_groups);
> +		if (ret)
> +			dev_err(dev, "unable to create sysfs version group\n");
> +	}
> +

Again this can stay as is if we have default.

-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web