[04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message
diff mbox

Message ID 1471515066-3626-5-git-send-email-narmstrong@baylibre.com
State Superseded
Headers show

Commit Message

Neil Armstrong Aug. 18, 2016, 10:10 a.m. UTC
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(+)

Comments

Sudeep Holla Aug. 19, 2016, 4:22 p.m. UTC | #1
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.
Neil Armstrong Aug. 23, 2016, 8:19 a.m. UTC | #2
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
Sudeep Holla Aug. 23, 2016, 2:47 p.m. UTC | #3
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.

Patch
diff mbox

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)
 {