public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}
@ 2023-09-25 10:15 Sudeep Holla
  2023-09-27 11:15 ` Cristian Marussi
  2023-09-27 13:06 ` Sudeep Holla
  0 siblings, 2 replies; 6+ messages in thread
From: Sudeep Holla @ 2023-09-25 10:15 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: Sudeep Holla, Cristian Marussi

It is very confusing to use *_v2 for everything applicable until SCMI
clock protocol version v2.0 including v1.0 for example. So let us rename
such that *_v2 is used only for SCMI clock protocol v2.1 onwards. Also
add comment to indicate the same explicitly.

Cc: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 41 +++++++++++++++++--------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index d18bf789fc24..9c0e33c1efab 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -46,12 +46,13 @@ struct scmi_msg_resp_clock_attributes {
 	__le32 clock_enable_latency;
 };
 
-struct scmi_msg_clock_config_set_v2 {
+struct scmi_msg_clock_config_set {
 	__le32 id;
 	__le32 attributes;
 };
 
-struct scmi_msg_clock_config_set_v21 {
+/* Valid only from SCMI clock v2.1 */
+struct scmi_msg_clock_config_set_v2 {
 	__le32 id;
 	__le32 attributes;
 #define NULL_OEM_TYPE			0
@@ -429,13 +430,13 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
 }
 
 static int
-scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
-			 enum clk_state state, u8 __unused0, u32 __unused1,
-			 bool atomic)
+scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
+		      enum clk_state state, u8 __unused0, u32 __unused1,
+		      bool atomic)
 {
 	int ret;
 	struct scmi_xfer *t;
-	struct scmi_msg_clock_config_set_v2 *cfg;
+	struct scmi_msg_clock_config_set *cfg;
 
 	if (state >= CLK_STATE_RESERVED)
 		return -EINVAL;
@@ -457,15 +458,16 @@ scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
 	return ret;
 }
 
+/* For SCMI clock v2.1 and onwards */
 static int
-scmi_clock_config_set_v21(const struct scmi_protocol_handle *ph, u32 clk_id,
-			  enum clk_state state, u8 oem_type, u32 oem_val,
-			  bool atomic)
+scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
+			 enum clk_state state, u8 oem_type, u32 oem_val,
+			 bool atomic)
 {
 	int ret;
 	u32 attrs;
 	struct scmi_xfer *t;
-	struct scmi_msg_clock_config_set_v21 *cfg;
+	struct scmi_msg_clock_config_set_v2 *cfg;
 
 	if (state == CLK_STATE_RESERVED ||
 	    (!oem_type && state == CLK_STATE_UNCHANGED))
@@ -513,10 +515,11 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
 				    NULL_OEM_TYPE, 0, atomic);
 }
 
+/* For SCMI clock v2.1 and onwards */
 static int
-scmi_clock_config_get_v21(const struct scmi_protocol_handle *ph, u32 clk_id,
-			  u8 oem_type, u32 *attributes, bool *enabled,
-			  u32 *oem_val, bool atomic)
+scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
+			 u8 oem_type, u32 *attributes, bool *enabled,
+			 u32 *oem_val, bool atomic)
 {
 	int ret;
 	u32 flags;
@@ -556,9 +559,9 @@ scmi_clock_config_get_v21(const struct scmi_protocol_handle *ph, u32 clk_id,
 }
 
 static int
-scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
-			 u8 oem_type, u32 *attributes, bool *enabled,
-			 u32 *oem_val, bool atomic)
+scmi_clock_config_get(const struct scmi_protocol_handle *ph, u32 clk_id,
+		      u8 oem_type, u32 *attributes, bool *enabled,
+		      u32 *oem_val, bool atomic)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -781,11 +784,11 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 
 	if (PROTOCOL_REV_MAJOR(version) >= 0x2 &&
 	    PROTOCOL_REV_MINOR(version) >= 0x1) {
-		cinfo->clock_config_set = scmi_clock_config_set_v21;
-		cinfo->clock_config_get = scmi_clock_config_get_v21;
-	} else {
 		cinfo->clock_config_set = scmi_clock_config_set_v2;
 		cinfo->clock_config_get = scmi_clock_config_get_v2;
+	} else {
+		cinfo->clock_config_set = scmi_clock_config_set;
+		cinfo->clock_config_get = scmi_clock_config_get;
 	}
 
 	cinfo->version = version;
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}
  2023-09-25 10:15 [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21} Sudeep Holla
@ 2023-09-27 11:15 ` Cristian Marussi
  2023-09-27 11:50   ` Sudeep Holla
  2023-09-27 13:06 ` Sudeep Holla
  1 sibling, 1 reply; 6+ messages in thread
From: Cristian Marussi @ 2023-09-27 11:15 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-arm-kernel

On Mon, Sep 25, 2023 at 11:15:57AM +0100, Sudeep Holla wrote:
> It is very confusing to use *_v2 for everything applicable until SCMI
> clock protocol version v2.0 including v1.0 for example. So let us rename
> such that *_v2 is used only for SCMI clock protocol v2.1 onwards. Also
> add comment to indicate the same explicitly.
> 

Hi Sudeep,

looking back at this, indeed, I remember being unsure if it was better
to use the v2/v21 naming scheme or the one that this patch propose.

Revisiting this now, I have to say that I agree with you, but why you
have also renamed _v21 to v2 ? The idea was to match the exact protocol
version ( I see that you added a comment anyway...)

IOW, the day some further new non-backward compatible features will be
possibly introduced (say clock v3), we could go like:

 - _config_set_v21: only v2.1 (the one you have renamed to v2)
 - _config_set_v3: only v3
 - _config_set : everything else, i.e. up to v2.0 (as you've renamed
   now)

I have no string opinions anyway, so I am fine also with this version of
the patch...better than the original brain-dead naming scheme that I had
chosen indeed :P

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}
  2023-09-27 11:15 ` Cristian Marussi
@ 2023-09-27 11:50   ` Sudeep Holla
  2023-09-27 12:43     ` Cristian Marussi
  0 siblings, 1 reply; 6+ messages in thread
From: Sudeep Holla @ 2023-09-27 11:50 UTC (permalink / raw)
  To: Cristian Marussi; +Cc: linux-arm-kernel, Sudeep Holla

On Wed, Sep 27, 2023 at 12:15:16PM +0100, Cristian Marussi wrote:
> On Mon, Sep 25, 2023 at 11:15:57AM +0100, Sudeep Holla wrote:
> > It is very confusing to use *_v2 for everything applicable until SCMI
> > clock protocol version v2.0 including v1.0 for example. So let us rename
> > such that *_v2 is used only for SCMI clock protocol v2.1 onwards. Also
> > add comment to indicate the same explicitly.
> > 
> 
> Hi Sudeep,
> 
> looking back at this, indeed, I remember being unsure if it was better
> to use the v2/v21 naming scheme or the one that this patch propose.
> 
> Revisiting this now, I have to say that I agree with you, but why you
> have also renamed _v21 to v2 ? The idea was to match the exact protocol
> version ( I see that you added a comment anyway...)
>

OK we can do that too. I was thinking of continuous increment in the structure
version independent of the spec version. Having _v21, _v53, ...etc looks odd
to me. I was thinking more like _v2(v2.1 onwards) and _v3(v5.3 onwards) for
example. Hope that clarifies and let me know if that is still confusing in
your opinion.

> IOW, the day some further new non-backward compatible features will be
> possibly introduced (say clock v3), we could go like:
> 
>  - _config_set_v21: only v2.1 (the one you have renamed to v2)
>  - _config_set_v3: only v3
>  - _config_set : everything else, i.e. up to v2.0 (as you've renamed
>    now)
>

Correct. My main worry is if things get changed at random minor version
like v2.1, v4.5, v5.3, ...etc. Unlikely to happen but not ruled out 😉,
blame spec authors.

> I have no string opinions anyway, so I am fine also with this version of
> the patch...better than the original brain-dead naming scheme that I had
> chosen indeed :P
> 

I am not saying what I did is easier to follow, but a comment with the
driver structure version must help.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}
  2023-09-27 11:50   ` Sudeep Holla
@ 2023-09-27 12:43     ` Cristian Marussi
  2023-09-27 12:51       ` Sudeep Holla
  0 siblings, 1 reply; 6+ messages in thread
From: Cristian Marussi @ 2023-09-27 12:43 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-arm-kernel

On Wed, Sep 27, 2023 at 12:50:23PM +0100, Sudeep Holla wrote:
> On Wed, Sep 27, 2023 at 12:15:16PM +0100, Cristian Marussi wrote:
> > On Mon, Sep 25, 2023 at 11:15:57AM +0100, Sudeep Holla wrote:
> > > It is very confusing to use *_v2 for everything applicable until SCMI
> > > clock protocol version v2.0 including v1.0 for example. So let us rename
> > > such that *_v2 is used only for SCMI clock protocol v2.1 onwards. Also
> > > add comment to indicate the same explicitly.
> > > 
> > 

Hi,

> > Hi Sudeep,
> > 
> > looking back at this, indeed, I remember being unsure if it was better
> > to use the v2/v21 naming scheme or the one that this patch propose.
> > 
> > Revisiting this now, I have to say that I agree with you, but why you
> > have also renamed _v21 to v2 ? The idea was to match the exact protocol
> > version ( I see that you added a comment anyway...)
> >
> 
> OK we can do that too. I was thinking of continuous increment in the structure
> version independent of the spec version. Having _v21, _v53, ...etc looks odd
> to me. I was thinking more like _v2(v2.1 onwards) and _v3(v5.3 onwards) for
> example. Hope that clarifies and let me know if that is still confusing in
> your opinion.
> 
> > IOW, the day some further new non-backward compatible features will be
> > possibly introduced (say clock v3), we could go like:
> > 
> >  - _config_set_v21: only v2.1 (the one you have renamed to v2)
> >  - _config_set_v3: only v3
> >  - _config_set : everything else, i.e. up to v2.0 (as you've renamed
> >    now)
> >
> 
> Correct. My main worry is if things get changed at random minor version
> like v2.1, v4.5, v5.3, ...etc. Unlikely to happen but not ruled out 😉,
> blame spec authors.
>

I think using a progressive continuos increment plus a comment should be
fine indeed (like this series does): this is anyway a special case, there
are anyway already many other places in the stack where we have so small
changes from one version to another that can be handled within the same
function version just with an if-check

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}
  2023-09-27 12:43     ` Cristian Marussi
@ 2023-09-27 12:51       ` Sudeep Holla
  0 siblings, 0 replies; 6+ messages in thread
From: Sudeep Holla @ 2023-09-27 12:51 UTC (permalink / raw)
  To: Cristian Marussi; +Cc: linux-arm-kernel, Sudeep Holla

On Wed, Sep 27, 2023 at 01:43:17PM +0100, Cristian Marussi wrote:
> On Wed, Sep 27, 2023 at 12:50:23PM +0100, Sudeep Holla wrote:
> > On Wed, Sep 27, 2023 at 12:15:16PM +0100, Cristian Marussi wrote:
> > > On Mon, Sep 25, 2023 at 11:15:57AM +0100, Sudeep Holla wrote:
> > > > It is very confusing to use *_v2 for everything applicable until SCMI
> > > > clock protocol version v2.0 including v1.0 for example. So let us rename
> > > > such that *_v2 is used only for SCMI clock protocol v2.1 onwards. Also
> > > > add comment to indicate the same explicitly.
> > > > 
> > > 
> 
> Hi,
> 
> > > Hi Sudeep,
> > > 
> > > looking back at this, indeed, I remember being unsure if it was better
> > > to use the v2/v21 naming scheme or the one that this patch propose.
> > > 
> > > Revisiting this now, I have to say that I agree with you, but why you
> > > have also renamed _v21 to v2 ? The idea was to match the exact protocol
> > > version ( I see that you added a comment anyway...)
> > >
> > 
> > OK we can do that too. I was thinking of continuous increment in the structure
> > version independent of the spec version. Having _v21, _v53, ...etc looks odd
> > to me. I was thinking more like _v2(v2.1 onwards) and _v3(v5.3 onwards) for
> > example. Hope that clarifies and let me know if that is still confusing in
> > your opinion.
> > 
> > > IOW, the day some further new non-backward compatible features will be
> > > possibly introduced (say clock v3), we could go like:
> > > 
> > >  - _config_set_v21: only v2.1 (the one you have renamed to v2)
> > >  - _config_set_v3: only v3
> > >  - _config_set : everything else, i.e. up to v2.0 (as you've renamed
> > >    now)
> > >
> > 
> > Correct. My main worry is if things get changed at random minor version
> > like v2.1, v4.5, v5.3, ...etc. Unlikely to happen but not ruled out 😉,
> > blame spec authors.
> >
> 
> I think using a progressive continuos increment plus a comment should be
> fine indeed (like this series does): this is anyway a special case, there
> are anyway already many other places in the stack where we have so small
> changes from one version to another that can be handled within the same
> function version just with an if-check
>

Yes that would be ideal. Only when memory layout format changes we need
these structure versioning.

> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
>

Thanks!

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}
  2023-09-25 10:15 [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21} Sudeep Holla
  2023-09-27 11:15 ` Cristian Marussi
@ 2023-09-27 13:06 ` Sudeep Holla
  1 sibling, 0 replies; 6+ messages in thread
From: Sudeep Holla @ 2023-09-27 13:06 UTC (permalink / raw)
  To: linux-arm-kernel, Sudeep Holla; +Cc: Cristian Marussi

On Mon, 25 Sep 2023 11:15:57 +0100, Sudeep Holla wrote:
> It is very confusing to use *_v2 for everything applicable until SCMI
> clock protocol version v2.0 including v1.0 for example. So let us rename
> such that *_v2 is used only for SCMI clock protocol v2.1 onwards. Also
> add comment to indicate the same explicitly.
>

Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!

[1/1] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}
      https://git.kernel.org/sudeep.holla/c/8b6022be4c6e
--
Regards,
Sudeep


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-27 13:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-25 10:15 [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21} Sudeep Holla
2023-09-27 11:15 ` Cristian Marussi
2023-09-27 11:50   ` Sudeep Holla
2023-09-27 12:43     ` Cristian Marussi
2023-09-27 12:51       ` Sudeep Holla
2023-09-27 13:06 ` Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox