[PATCH] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors

[PATCH] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors

[Sudeep Holla]

Instead of type-casting the {tx,rx}.buf all over the place while
accessing them to read/write __le32 from/to the firmware, let's use
the nice existing {get,put}_unaligned_le32 accessors to hide all the
type cast ugliness.

Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/base.c    |  2 +-
 drivers/firmware/arm_scmi/clock.c   | 10 ++++------
 drivers/firmware/arm_scmi/common.h  |  2 ++
 drivers/firmware/arm_scmi/perf.c    |  8 ++++----
 drivers/firmware/arm_scmi/power.c   |  6 +++---
 drivers/firmware/arm_scmi/reset.c   |  2 +-
 drivers/firmware/arm_scmi/sensors.c | 12 +++++-------
 7 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index 204390297f4b..f804e8af6521 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -204,7 +204,7 @@ static int scmi_base_discover_agent_get(const struct scmi_handle *handle,
 	if (ret)
 		return ret;
 
-	*(__le32 *)t->tx.buf = cpu_to_le32(id);
+	put_unaligned_le32(id, t->tx.buf);
 
 	ret = scmi_do_xfer(handle, t);
 	if (!ret)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 4a32ae1822a3..199a668ea885 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -107,7 +107,7 @@ static int scmi_clock_attributes_get(const struct scmi_handle *handle,
 	if (ret)
 		return ret;
 
-	*(__le32 *)t->tx.buf = cpu_to_le32(clk_id);
+	put_unaligned_le32(clk_id, t->tx.buf);
 	attr = t->rx.buf;
 
 	ret = scmi_do_xfer(handle, t);
@@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value)
 	if (ret)
 		return ret;
 
-	*(__le32 *)t->tx.buf = cpu_to_le32(clk_id);
+	put_unaligned_le32(clk_id, t->tx.buf);
 
 	ret = scmi_do_xfer(handle, t);
 	if (!ret) {
-		__le32 *pval = t->rx.buf;
-
-		*value = le32_to_cpu(*pval);
-		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
+		*value = get_unaligned_le32(t->rx.buf);
+		*value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32;
 	}
 
 	scmi_xfer_put(handle, t);
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 43884e4ceac5..5237c2ff79fe 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -15,6 +15,8 @@
 #include <linux/scmi_protocol.h>
 #include <linux/types.h>
 
+#include <asm/unaligned.h>
+
 #define PROTOCOL_REV_MINOR_MASK	GENMASK(15, 0)
 #define PROTOCOL_REV_MAJOR_MASK	GENMASK(31, 16)
 #define PROTOCOL_REV_MAJOR(x)	(u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x)))
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index fb7f6cab2c11..9b338e66a24e 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -195,7 +195,7 @@ scmi_perf_domain_attributes_get(const struct scmi_handle *handle, u32 domain,
 	if (ret)
 		return ret;
 
-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 	attr = t->rx.buf;
 
 	ret = scmi_do_xfer(handle, t);
@@ -380,7 +380,7 @@ static int scmi_perf_mb_limits_get(const struct scmi_handle *handle, u32 domain,
 	if (ret)
 		return ret;
 
-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 
 	ret = scmi_do_xfer(handle, t);
 	if (!ret) {
@@ -459,11 +459,11 @@ static int scmi_perf_mb_level_get(const struct scmi_handle *handle, u32 domain,
 		return ret;
 
 	t->hdr.poll_completion = poll;
-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 
 	ret = scmi_do_xfer(handle, t);
 	if (!ret)
-		*level = le32_to_cpu(*(__le32 *)t->rx.buf);
+		*level = get_unaligned_le32(t->rx.buf);
 
 	scmi_xfer_put(handle, t);
 	return ret;
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 62f3401a1f01..5abef7079c0a 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -96,7 +96,7 @@ scmi_power_domain_attributes_get(const struct scmi_handle *handle, u32 domain,
 	if (ret)
 		return ret;
 
-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 	attr = t->rx.buf;
 
 	ret = scmi_do_xfer(handle, t);
@@ -147,11 +147,11 @@ scmi_power_state_get(const struct scmi_handle *handle, u32 domain, u32 *state)
 	if (ret)
 		return ret;
 
-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 
 	ret = scmi_do_xfer(handle, t);
 	if (!ret)
-		*state = le32_to_cpu(*(__le32 *)t->rx.buf);
+		*state = get_unaligned_le32(t->rx.buf);
 
 	scmi_xfer_put(handle, t);
 	return ret;
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 11cb8b5ccf34..c1d67a2af12f 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -88,7 +88,7 @@ scmi_reset_domain_attributes_get(const struct scmi_handle *handle, u32 domain,
 	if (ret)
 		return ret;
 
-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 	attr = t->rx.buf;
 
 	ret = scmi_do_xfer(handle, t);
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 7570308a16a0..5b330619a025 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -120,7 +120,7 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 
 	do {
 		/* Set the number of sensors to be skipped/already read */
-		*(__le32 *)t->tx.buf = cpu_to_le32(desc_index);
+		put_unaligned_le32(desc_index, t->tx.buf);
 
 		ret = scmi_do_xfer(handle, t);
 		if (ret)
@@ -217,7 +217,6 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 				   u32 sensor_id, u64 *value)
 {
 	int ret;
-	__le32 *pval;
 	struct scmi_xfer *t;
 	struct scmi_msg_sensor_reading_get *sensor;
 	struct sensors_info *si = handle->sensor_priv;
@@ -229,7 +228,6 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 	if (ret)
 		return ret;
 
-	pval = t->rx.buf;
 	sensor = t->tx.buf;
 	sensor->id = cpu_to_le32(sensor_id);
 
@@ -237,15 +235,15 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 		sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
 		ret = scmi_do_xfer_with_response(handle, t);
 		if (!ret) {
-			*value = le32_to_cpu(*(pval + 1));
-			*value |= (u64)le32_to_cpu(*(pval + 2)) << 32;
+			*value = get_unaligned_le32(t->rx.buf + 1);
+			*value |= (u64)get_unaligned_le32(t->rx.buf + 2) << 32;
 		}
 	} else {
 		sensor->flags = cpu_to_le32(0);
 		ret = scmi_do_xfer(handle, t);
 		if (!ret) {
-			*value = le32_to_cpu(*pval);
-			*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
+			*value = get_unaligned_le32(t->rx.buf);
+			*value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32;
 		}
 	}
 
-- 
2.17.1

Re: [PATCH] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors

[Philipp Zabel]

Hi Sudeep,

On Wed, 2019-08-07 at 14:00 +0100, Sudeep Holla wrote:
> Instead of type-casting the {tx,rx}.buf all over the place while
> accessing them to read/write __le32 from/to the firmware, let's use
> the nice existing {get,put}_unaligned_le32 accessors to hide all the
> type cast ugliness.
> 
> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/base.c    |  2 +-
>  drivers/firmware/arm_scmi/clock.c   | 10 ++++------
>  drivers/firmware/arm_scmi/common.h  |  2 ++
>  drivers/firmware/arm_scmi/perf.c    |  8 ++++----
>  drivers/firmware/arm_scmi/power.c   |  6 +++---
>  drivers/firmware/arm_scmi/reset.c   |  2 +-
>  drivers/firmware/arm_scmi/sensors.c | 12 +++++-------
>  7 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> index 204390297f4b..f804e8af6521 100644
> --- a/drivers/firmware/arm_scmi/base.c
> +++ b/drivers/firmware/arm_scmi/base.c
[...]
> @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value)
>  	if (ret)
>  		return ret;
>  
> -	*(__le32 *)t->tx.buf = cpu_to_le32(clk_id);
> +	put_unaligned_le32(clk_id, t->tx.buf);
>  
>  	ret = scmi_do_xfer(handle, t);
>  	if (!ret) {
> -		__le32 *pval = t->rx.buf;
> -
> -		*value = le32_to_cpu(*pval);
> -		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
> +		*value = get_unaligned_le32(t->rx.buf);
> +		*value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32;

Isn't t->rx.buf a void pointer? If I am not mistaken, you'd either have
to keep the pval local variables, or cast to (__le32 *) before doing
pointer arithmetic.

regards
Philipp

Re: [PATCH] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors

[Sudeep Holla]

On Wed, Aug 07, 2019 at 03:36:11PM +0200, Philipp Zabel wrote:
> Hi Sudeep,
>
> On Wed, 2019-08-07 at 14:00 +0100, Sudeep Holla wrote:
> > Instead of type-casting the {tx,rx}.buf all over the place while
> > accessing them to read/write __le32 from/to the firmware, let's use
> > the nice existing {get,put}_unaligned_le32 accessors to hide all the
> > type cast ugliness.
> >
> > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/base.c    |  2 +-
> >  drivers/firmware/arm_scmi/clock.c   | 10 ++++------
> >  drivers/firmware/arm_scmi/common.h  |  2 ++
> >  drivers/firmware/arm_scmi/perf.c    |  8 ++++----
> >  drivers/firmware/arm_scmi/power.c   |  6 +++---
> >  drivers/firmware/arm_scmi/reset.c   |  2 +-
> >  drivers/firmware/arm_scmi/sensors.c | 12 +++++-------
> >  7 files changed, 20 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > index 204390297f4b..f804e8af6521 100644
> > --- a/drivers/firmware/arm_scmi/base.c
> > +++ b/drivers/firmware/arm_scmi/base.c
> [...]
> > @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value)
> >  	if (ret)
> >  		return ret;
> >
> > -	*(__le32 *)t->tx.buf = cpu_to_le32(clk_id);
> > +	put_unaligned_le32(clk_id, t->tx.buf);
> >
> >  	ret = scmi_do_xfer(handle, t);
> >  	if (!ret) {
> > -		__le32 *pval = t->rx.buf;
> > -
> > -		*value = le32_to_cpu(*pval);
> > -		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
> > +		*value = get_unaligned_le32(t->rx.buf);
> > +		*value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32;
>
> Isn't t->rx.buf a void pointer? If I am not mistaken, you'd either have
> to keep the pval local variables, or cast to (__le32 *) before doing
> pointer arithmetic.
>

Ah right, that's the reason I added it at the first place. I will fix that.

--
Regards,
Sudeep

Re: [PATCH] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors

[Robin Murphy]

On 07/08/2019 14:57, Sudeep Holla wrote:
> On Wed, Aug 07, 2019 at 03:36:11PM +0200, Philipp Zabel wrote:
>> Hi Sudeep,
>>
>> On Wed, 2019-08-07 at 14:00 +0100, Sudeep Holla wrote:
>>> Instead of type-casting the {tx,rx}.buf all over the place while
>>> accessing them to read/write __le32 from/to the firmware, let's use
>>> the nice existing {get,put}_unaligned_le32 accessors to hide all the
>>> type cast ugliness.
>>>
>>> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>>   drivers/firmware/arm_scmi/base.c    |  2 +-
>>>   drivers/firmware/arm_scmi/clock.c   | 10 ++++------
>>>   drivers/firmware/arm_scmi/common.h  |  2 ++
>>>   drivers/firmware/arm_scmi/perf.c    |  8 ++++----
>>>   drivers/firmware/arm_scmi/power.c   |  6 +++---
>>>   drivers/firmware/arm_scmi/reset.c   |  2 +-
>>>   drivers/firmware/arm_scmi/sensors.c | 12 +++++-------
>>>   7 files changed, 20 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
>>> index 204390297f4b..f804e8af6521 100644
>>> --- a/drivers/firmware/arm_scmi/base.c
>>> +++ b/drivers/firmware/arm_scmi/base.c
>> [...]
>>> @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value)
>>>   	if (ret)
>>>   		return ret;
>>>
>>> -	*(__le32 *)t->tx.buf = cpu_to_le32(clk_id);
>>> +	put_unaligned_le32(clk_id, t->tx.buf);
>>>
>>>   	ret = scmi_do_xfer(handle, t);
>>>   	if (!ret) {
>>> -		__le32 *pval = t->rx.buf;
>>> -
>>> -		*value = le32_to_cpu(*pval);
>>> -		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
>>> +		*value = get_unaligned_le32(t->rx.buf);
>>> +		*value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32;
>>
>> Isn't t->rx.buf a void pointer? If I am not mistaken, you'd either have
>> to keep the pval local variables, or cast to (__le32 *) before doing
>> pointer arithmetic.
>>
> 
> Ah right, that's the reason I added it at the first place. I will fix that.

Couldn't you just use get_unaligned_le64() here anyway?

Robin.

Re: [PATCH] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors

[Sudeep Holla]

On Wed, Aug 07, 2019 at 03:07:39PM +0100, Robin Murphy wrote:
> On 07/08/2019 14:57, Sudeep Holla wrote:
> > On Wed, Aug 07, 2019 at 03:36:11PM +0200, Philipp Zabel wrote:
> > > Hi Sudeep,
> > > 
> > > On Wed, 2019-08-07 at 14:00 +0100, Sudeep Holla wrote:
> > > > Instead of type-casting the {tx,rx}.buf all over the place while
> > > > accessing them to read/write __le32 from/to the firmware, let's use
> > > > the nice existing {get,put}_unaligned_le32 accessors to hide all the
> > > > type cast ugliness.
> > > > 
> > > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > ---
> > > >   drivers/firmware/arm_scmi/base.c    |  2 +-
> > > >   drivers/firmware/arm_scmi/clock.c   | 10 ++++------
> > > >   drivers/firmware/arm_scmi/common.h  |  2 ++
> > > >   drivers/firmware/arm_scmi/perf.c    |  8 ++++----
> > > >   drivers/firmware/arm_scmi/power.c   |  6 +++---
> > > >   drivers/firmware/arm_scmi/reset.c   |  2 +-
> > > >   drivers/firmware/arm_scmi/sensors.c | 12 +++++-------
> > > >   7 files changed, 20 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > > > index 204390297f4b..f804e8af6521 100644
> > > > --- a/drivers/firmware/arm_scmi/base.c
> > > > +++ b/drivers/firmware/arm_scmi/base.c
> > > [...]
> > > > @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value)
> > > >   	if (ret)
> > > >   		return ret;
> > > > 
> > > > -	*(__le32 *)t->tx.buf = cpu_to_le32(clk_id);
> > > > +	put_unaligned_le32(clk_id, t->tx.buf);
> > > > 
> > > >   	ret = scmi_do_xfer(handle, t);
> > > >   	if (!ret) {
> > > > -		__le32 *pval = t->rx.buf;
> > > > -
> > > > -		*value = le32_to_cpu(*pval);
> > > > -		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
> > > > +		*value = get_unaligned_le32(t->rx.buf);
> > > > +		*value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32;
> > > 
> > > Isn't t->rx.buf a void pointer? If I am not mistaken, you'd either have
> > > to keep the pval local variables, or cast to (__le32 *) before doing
> > > pointer arithmetic.
> > > 
> > 
> > Ah right, that's the reason I added it at the first place. I will fix that.
> 
> Couldn't you just use get_unaligned_le64() here anyway?

Indeed, that's what I found as I wanted to avoid pval, testing now.

--
Regards,
Sudeep

RE: [PATCH] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors

[David Laight]

From: Sudeep Holla
> Sent: 07 August 2019 14:01
> 
> Instead of type-casting the {tx,rx}.buf all over the place while
> accessing them to read/write __le32 from/to the firmware, let's use
> the nice existing {get,put}_unaligned_le32 accessors to hide all the
> type cast ugliness.

Why the 'unaligned' accessors?

> -	*(__le32 *)t->tx.buf = cpu_to_le32(id);
> +	put_unaligned_le32(id, t->tx.buf);

These will be expensive if the cpu doesn't support them.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors

[Sudeep Holla]

On Wed, Aug 07, 2019 at 03:18:59PM +0000, David Laight wrote:
> From: Sudeep Holla
> > Sent: 07 August 2019 14:01
> >
> > Instead of type-casting the {tx,rx}.buf all over the place while
> > accessing them to read/write __le32 from/to the firmware, let's use
> > the nice existing {get,put}_unaligned_le32 accessors to hide all the
> > type cast ugliness.
>
> Why the 'unaligned' accessors?
>

Since the firmware run in LE, we do byte-swapping anyways.

> > -	*(__le32 *)t->tx.buf = cpu_to_le32(id);
> > +	put_unaligned_le32(id, t->tx.buf);
>

If you look at the generic definition for put_unaligned_le32, it's
exactly the same as what I am replacing with the call. So nothing
changes IIUC. In fact, I see that all the helper in unaligned/access_ok.h
just do the byte-swapping.

> These will be expensive if the cpu doesn't support them.

The SCMI is currently used only on ARM platforms which have
HAVE_EFFICIENT_UNALIGNED_ACCESS defined.

--
Regards,
Sudeep

[PATCH v2] firmware: arm_scmi: Use {get,put}_unaligned_le{32,64} accessors

[Sudeep Holla]

Instead of type-casting the {tx,rx}.buf all over the place while
accessing them to read/write __le{32,64} from/to the firmware, let's
use the existing {get,put}_unaligned_le{32,64} accessors to hide all
the type cast ugliness.

Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/base.c    |  2 +-
 drivers/firmware/arm_scmi/clock.c   | 12 ++++--------
 drivers/firmware/arm_scmi/common.h  |  2 ++
 drivers/firmware/arm_scmi/perf.c    |  8 ++++----
 drivers/firmware/arm_scmi/power.c   |  6 +++---
 drivers/firmware/arm_scmi/reset.c   |  2 +-
 drivers/firmware/arm_scmi/sensors.c | 17 ++++++-----------
 7 files changed, 21 insertions(+), 28 deletions(-)

v1->v2:
	- Dropped incorrect void ptr arithmetic and used unaligned_le64
	  accessors instead

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index 204390297f4b..f804e8af6521 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -204,7 +204,7 @@ static int scmi_base_discover_agent_get(const struct scmi_handle *handle,
 	if (ret)
 		return ret;

-	*(__le32 *)t->tx.buf = cpu_to_le32(id);
+	put_unaligned_le32(id, t->tx.buf);

 	ret = scmi_do_xfer(handle, t);
 	if (!ret)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 4a32ae1822a3..32526a793f3a 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -107,7 +107,7 @@ static int scmi_clock_attributes_get(const struct scmi_handle *handle,
 	if (ret)
 		return ret;

-	*(__le32 *)t->tx.buf = cpu_to_le32(clk_id);
+	put_unaligned_le32(clk_id, t->tx.buf);
 	attr = t->rx.buf;

 	ret = scmi_do_xfer(handle, t);
@@ -204,15 +204,11 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value)
 	if (ret)
 		return ret;

-	*(__le32 *)t->tx.buf = cpu_to_le32(clk_id);
+	put_unaligned_le32(clk_id, t->tx.buf);

 	ret = scmi_do_xfer(handle, t);
-	if (!ret) {
-		__le32 *pval = t->rx.buf;
-
-		*value = le32_to_cpu(*pval);
-		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
-	}
+	if (!ret)
+		*value = get_unaligned_le64(t->rx.buf);

 	scmi_xfer_put(handle, t);
 	return ret;
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 43884e4ceac5..5237c2ff79fe 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -15,6 +15,8 @@
 #include <linux/scmi_protocol.h>
 #include <linux/types.h>

+#include <asm/unaligned.h>
+
 #define PROTOCOL_REV_MINOR_MASK	GENMASK(15, 0)
 #define PROTOCOL_REV_MAJOR_MASK	GENMASK(31, 16)
 #define PROTOCOL_REV_MAJOR(x)	(u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x)))
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index fb7f6cab2c11..9b338e66a24e 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -195,7 +195,7 @@ scmi_perf_domain_attributes_get(const struct scmi_handle *handle, u32 domain,
 	if (ret)
 		return ret;

-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 	attr = t->rx.buf;

 	ret = scmi_do_xfer(handle, t);
@@ -380,7 +380,7 @@ static int scmi_perf_mb_limits_get(const struct scmi_handle *handle, u32 domain,
 	if (ret)
 		return ret;

-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);

 	ret = scmi_do_xfer(handle, t);
 	if (!ret) {
@@ -459,11 +459,11 @@ static int scmi_perf_mb_level_get(const struct scmi_handle *handle, u32 domain,
 		return ret;

 	t->hdr.poll_completion = poll;
-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);

 	ret = scmi_do_xfer(handle, t);
 	if (!ret)
-		*level = le32_to_cpu(*(__le32 *)t->rx.buf);
+		*level = get_unaligned_le32(t->rx.buf);

 	scmi_xfer_put(handle, t);
 	return ret;
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 62f3401a1f01..5abef7079c0a 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -96,7 +96,7 @@ scmi_power_domain_attributes_get(const struct scmi_handle *handle, u32 domain,
 	if (ret)
 		return ret;

-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 	attr = t->rx.buf;

 	ret = scmi_do_xfer(handle, t);
@@ -147,11 +147,11 @@ scmi_power_state_get(const struct scmi_handle *handle, u32 domain, u32 *state)
 	if (ret)
 		return ret;

-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);

 	ret = scmi_do_xfer(handle, t);
 	if (!ret)
-		*state = le32_to_cpu(*(__le32 *)t->rx.buf);
+		*state = get_unaligned_le32(t->rx.buf);

 	scmi_xfer_put(handle, t);
 	return ret;
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 11cb8b5ccf34..c1d67a2af12f 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -88,7 +88,7 @@ scmi_reset_domain_attributes_get(const struct scmi_handle *handle, u32 domain,
 	if (ret)
 		return ret;

-	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	put_unaligned_le32(domain, t->tx.buf);
 	attr = t->rx.buf;

 	ret = scmi_do_xfer(handle, t);
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 7570308a16a0..a400ea805fc2 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -120,7 +120,7 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,

 	do {
 		/* Set the number of sensors to be skipped/already read */
-		*(__le32 *)t->tx.buf = cpu_to_le32(desc_index);
+		put_unaligned_le32(desc_index, t->tx.buf);

 		ret = scmi_do_xfer(handle, t);
 		if (ret)
@@ -217,7 +217,6 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 				   u32 sensor_id, u64 *value)
 {
 	int ret;
-	__le32 *pval;
 	struct scmi_xfer *t;
 	struct scmi_msg_sensor_reading_get *sensor;
 	struct sensors_info *si = handle->sensor_priv;
@@ -229,24 +228,20 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 	if (ret)
 		return ret;

-	pval = t->rx.buf;
 	sensor = t->tx.buf;
 	sensor->id = cpu_to_le32(sensor_id);

 	if (s->async) {
 		sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
 		ret = scmi_do_xfer_with_response(handle, t);
-		if (!ret) {
-			*value = le32_to_cpu(*(pval + 1));
-			*value |= (u64)le32_to_cpu(*(pval + 2)) << 32;
-		}
+		if (!ret)
+			*value = get_unaligned_le64((void *)
+						    ((__le32 *)t->rx.buf + 1));
 	} else {
 		sensor->flags = cpu_to_le32(0);
 		ret = scmi_do_xfer(handle, t);
-		if (!ret) {
-			*value = le32_to_cpu(*pval);
-			*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
-		}
+		if (!ret)
+			*value = get_unaligned_le64(t->rx.buf);
 	}

 	scmi_xfer_put(handle, t);
--
2.17.1

Re: [PATCH v2] firmware: arm_scmi: Use {get,put}_unaligned_le{32,64} accessors

[Philipp Zabel]

On Wed, 2019-08-07 at 18:37 +0100, Sudeep Holla wrote:
> Instead of type-casting the {tx,rx}.buf all over the place while
> accessing them to read/write __le{32,64} from/to the firmware, let's
> use the existing {get,put}_unaligned_le{32,64} accessors to hide all
> the type cast ugliness.
> 
> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp