2023-09-26 12:39:53

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 0/3] ACPI: PCC: Define and use the common PCC shared memory regions related macros

This set of 3 small patches intend to consolidate and replace the existing
locally defined macros within couple of PCC client drivers when accessing
the command and status bitfields.

Signed-off-by: Sudeep Holla <[email protected]>
---
Sudeep Holla (3):
ACPI: PCC: Add PCC shared memory region command and status bitfields
i2c: xgene-slimpro: Migrate to use generic PCC shmem related macros
hwmon: (xgene) Migrate to use generic PCC shmem related macros

drivers/hwmon/xgene-hwmon.c | 16 +++++-----------
drivers/i2c/busses/i2c-xgene-slimpro.c | 16 ++++------------
include/acpi/pcc.h | 11 +++++++++++
3 files changed, 20 insertions(+), 23 deletions(-)
---
base-commit: 6465e260f48790807eef06b583b38ca9789b6072
change-id: 20230926-pcc_defines-24be5e33b6f3

Best regards,
--
Regards,
Sudeep


2023-09-26 12:39:55

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 1/3] ACPI: PCC: Add PCC shared memory region command and status bitfields

Define the common macros to use when referring to various bitfields in
the PCC generic communications channel command and status fields.

Currently different drivers that need to use these bitfields have defined
these locally. This common macro is intended to consolidate and replace
those.

Cc: Rafael J. Wysocki <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
include/acpi/pcc.h | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 73e806fe7ce7..66d9934c2ee4 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -18,7 +18,18 @@ struct pcc_mbox_chan {
u16 min_turnaround_time;
};

+/* Generic Communications Channel Shared Memory Region */
+#define PCC_SIGNATURE 0x50424300
+/* Generic Communications Channel Command Field */
+#define PCC_CMD_GENERATE_DB_INTR BIT(15)
+/* Generic Communications Channel Status Field */
+#define PCC_STATUS_CMD_COMPLETE BIT(0)
+#define PCC_STATUS_SCI_DOORBELL BIT(1)
+#define PCC_STATUS_ERROR BIT(2)
+#define PCC_STATUS_PLATFORM_NOTIFY BIT(3)
+
#define MAX_PCC_SUBSPACES 256
+
#ifdef CONFIG_PCC
extern struct pcc_mbox_chan *
pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id);

--
2.42.0

2023-09-26 12:39:59

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 2/3] i2c: xgene-slimpro: Migrate to use generic PCC shmem related macros

Use the newly defined common and generic PCC shared memory region
related macros in this driver to replace the locally defined ones.

Cc: Andi Shyti <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/i2c/busses/i2c-xgene-slimpro.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
index fbc1ffbd2fa7..658396c9eeab 100644
--- a/drivers/i2c/busses/i2c-xgene-slimpro.c
+++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
@@ -91,14 +91,6 @@

#define SLIMPRO_IIC_MSG_DWORD_COUNT 3

-/* PCC related defines */
-#define PCC_SIGNATURE 0x50424300
-#define PCC_STS_CMD_COMPLETE BIT(0)
-#define PCC_STS_SCI_DOORBELL BIT(1)
-#define PCC_STS_ERR BIT(2)
-#define PCC_STS_PLAT_NOTIFY BIT(3)
-#define PCC_CMD_GENERATE_DB_INT BIT(15)
-
struct slimpro_i2c_dev {
struct i2c_adapter adapter;
struct device *dev;
@@ -160,11 +152,11 @@ static void slimpro_i2c_pcc_rx_cb(struct mbox_client *cl, void *msg)

/* Check if platform sends interrupt */
if (!xgene_word_tst_and_clr(&generic_comm_base->status,
- PCC_STS_SCI_DOORBELL))
+ PCC_STATUS_SCI_DOORBELL))
return;

if (xgene_word_tst_and_clr(&generic_comm_base->status,
- PCC_STS_CMD_COMPLETE)) {
+ PCC_STATUS_CMD_COMPLETE)) {
msg = generic_comm_base + 1;

/* Response message msg[1] contains the return value. */
@@ -186,10 +178,10 @@ static void slimpro_i2c_pcc_tx_prepare(struct slimpro_i2c_dev *ctx, u32 *msg)
cpu_to_le32(PCC_SIGNATURE | ctx->mbox_idx));

WRITE_ONCE(generic_comm_base->command,
- cpu_to_le16(SLIMPRO_MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INT));
+ cpu_to_le16(SLIMPRO_MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INTR));

status = le16_to_cpu(READ_ONCE(generic_comm_base->status));
- status &= ~PCC_STS_CMD_COMPLETE;
+ status &= ~PCC_STATUS_CMD_COMPLETE;
WRITE_ONCE(generic_comm_base->status, cpu_to_le16(status));

/* Copy the message to the PCC comm space */

--
2.42.0

2023-09-26 12:40:02

by Sudeep Holla

[permalink] [raw]
Subject: [PATCH 3/3] hwmon: (xgene) Migrate to use generic PCC shmem related macros

Use the newly defined common and generic PCC shared memory region
related macros in this driver to replace the locally defined ones.

Cc: Jean Delvare <[email protected]>
Cc: Guenter Roeck <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/hwmon/xgene-hwmon.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index 78d9f52e2a71..1ccdd61b6d13 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -57,12 +57,6 @@
(MSG_TYPE_SET(MSG_TYPE_PWRMGMT) | \
MSG_SUBTYPE_SET(hndl) | TPC_CMD_SET(cmd) | type)

-/* PCC defines */
-#define PCC_SIGNATURE_MASK 0x50424300
-#define PCCC_GENERATE_DB_INT BIT(15)
-#define PCCS_CMD_COMPLETE BIT(0)
-#define PCCS_SCI_DOORBEL BIT(1)
-#define PCCS_PLATFORM_NOTIFICATION BIT(3)
/*
* Arbitrary retries in case the remote processor is slow to respond
* to PCC commands
@@ -142,15 +136,15 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)

/* Write signature for subspace */
WRITE_ONCE(generic_comm_base->signature,
- cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
+ cpu_to_le32(PCC_SIGNATURE | ctx->mbox_idx));

/* Write to the shared command region */
WRITE_ONCE(generic_comm_base->command,
- cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
+ cpu_to_le16(MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INTR));

/* Flip CMD COMPLETE bit */
val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
- val &= ~PCCS_CMD_COMPLETE;
+ val &= ~PCC_STATUS_CMD_COMPLETE;
WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));

/* Copy the message to the PCC comm space */
@@ -544,7 +538,7 @@ static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg)
msg = generic_comm_base + 1;
/* Check if platform sends interrupt */
if (!xgene_word_tst_and_clr(&generic_comm_base->status,
- PCCS_SCI_DOORBEL))
+ PCC_STATUS_SCI_DOORBELL))
return;

/*
@@ -566,7 +560,7 @@ static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg)
TPC_CMD(((u32 *)msg)[0]) == TPC_ALARM))) {
/* Check if platform completes command */
if (xgene_word_tst_and_clr(&generic_comm_base->status,
- PCCS_CMD_COMPLETE)) {
+ PCC_STATUS_CMD_COMPLETE)) {
ctx->sync_msg.msg = ((u32 *)msg)[0];
ctx->sync_msg.param1 = ((u32 *)msg)[1];
ctx->sync_msg.param2 = ((u32 *)msg)[2];

--
2.42.0

2023-09-26 13:44:20

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 3/3] hwmon: (xgene) Migrate to use generic PCC shmem related macros

On Tue, Sep 26, 2023 at 01:28:02PM +0100, Sudeep Holla wrote:
> Use the newly defined common and generic PCC shared memory region
> related macros in this driver to replace the locally defined ones.
>
> Cc: Jean Delvare <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Signed-off-by: Sudeep Holla <[email protected]>

Acked-by: Guenter Roeck <[email protected]>

> ---
> drivers/hwmon/xgene-hwmon.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index 78d9f52e2a71..1ccdd61b6d13 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -57,12 +57,6 @@
> (MSG_TYPE_SET(MSG_TYPE_PWRMGMT) | \
> MSG_SUBTYPE_SET(hndl) | TPC_CMD_SET(cmd) | type)
>
> -/* PCC defines */
> -#define PCC_SIGNATURE_MASK 0x50424300
> -#define PCCC_GENERATE_DB_INT BIT(15)
> -#define PCCS_CMD_COMPLETE BIT(0)
> -#define PCCS_SCI_DOORBEL BIT(1)
> -#define PCCS_PLATFORM_NOTIFICATION BIT(3)
> /*
> * Arbitrary retries in case the remote processor is slow to respond
> * to PCC commands
> @@ -142,15 +136,15 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>
> /* Write signature for subspace */
> WRITE_ONCE(generic_comm_base->signature,
> - cpu_to_le32(PCC_SIGNATURE_MASK | ctx->mbox_idx));
> + cpu_to_le32(PCC_SIGNATURE | ctx->mbox_idx));
>
> /* Write to the shared command region */
> WRITE_ONCE(generic_comm_base->command,
> - cpu_to_le16(MSG_TYPE(msg[0]) | PCCC_GENERATE_DB_INT));
> + cpu_to_le16(MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INTR));
>
> /* Flip CMD COMPLETE bit */
> val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
> - val &= ~PCCS_CMD_COMPLETE;
> + val &= ~PCC_STATUS_CMD_COMPLETE;
> WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));
>
> /* Copy the message to the PCC comm space */
> @@ -544,7 +538,7 @@ static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg)
> msg = generic_comm_base + 1;
> /* Check if platform sends interrupt */
> if (!xgene_word_tst_and_clr(&generic_comm_base->status,
> - PCCS_SCI_DOORBEL))
> + PCC_STATUS_SCI_DOORBELL))
> return;
>
> /*
> @@ -566,7 +560,7 @@ static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg)
> TPC_CMD(((u32 *)msg)[0]) == TPC_ALARM))) {
> /* Check if platform completes command */
> if (xgene_word_tst_and_clr(&generic_comm_base->status,
> - PCCS_CMD_COMPLETE)) {
> + PCC_STATUS_CMD_COMPLETE)) {
> ctx->sync_msg.msg = ((u32 *)msg)[0];
> ctx->sync_msg.param1 = ((u32 *)msg)[1];
> ctx->sync_msg.param2 = ((u32 *)msg)[2];
>
> --
> 2.42.0
>

2023-09-26 22:13:28

by Andi Shyti

[permalink] [raw]
Subject: Re: [PATCH 2/3] i2c: xgene-slimpro: Migrate to use generic PCC shmem related macros

Hi Sudeep,

> Use the newly defined common and generic PCC shared memory region
> related macros in this driver to replace the locally defined ones.
>
> Cc: Andi Shyti <[email protected]>
> Signed-off-by: Sudeep Holla <[email protected]>

Reviewed-by: Andi Shyti <[email protected]>

Thanks,
Andi

2023-09-27 04:23:41

by Huisong Li

[permalink] [raw]
Subject: Re: [PATCH 1/3] ACPI: PCC: Add PCC shared memory region command and status bitfields

Hi Sudeep,

在 2023/9/26 20:28, Sudeep Holla 写道:
> Define the common macros to use when referring to various bitfields in
> the PCC generic communications channel command and status fields.
Can you define the bit0 macros in the "flags" for Extended PCC Subspace
Shared Memory Region?
>
> Currently different drivers that need to use these bitfields have defined
> these locally. This common macro is intended to consolidate and replace
> those.
>
> Cc: Rafael J. Wysocki <[email protected]>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> include/acpi/pcc.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 73e806fe7ce7..66d9934c2ee4 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -18,7 +18,18 @@ struct pcc_mbox_chan {
> u16 min_turnaround_time;
> };
>
> +/* Generic Communications Channel Shared Memory Region */
> +#define PCC_SIGNATURE 0x50424300
Why is this signature 0x50424300?
In ACPI spec, this signature is all 0x50434303.
> +/* Generic Communications Channel Command Field */
> +#define PCC_CMD_GENERATE_DB_INTR BIT(15)
> +/* Generic Communications Channel Status Field */
> +#define PCC_STATUS_CMD_COMPLETE BIT(0)
> +#define PCC_STATUS_SCI_DOORBELL BIT(1)
> +#define PCC_STATUS_ERROR BIT(2)
> +#define PCC_STATUS_PLATFORM_NOTIFY BIT(3)
> +
> #define MAX_PCC_SUBSPACES 256
> +
> #ifdef CONFIG_PCC
> extern struct pcc_mbox_chan *
> pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id);
>

2023-09-27 04:37:40

by Huisong Li

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI: PCC: Define and use the common PCC shared memory regions related macros

Hi Sudeep,

Could you please use these new common macros for kunpeng_hccs?

在 2023/9/26 20:27, Sudeep Holla 写道:
> This set of 3 small patches intend to consolidate and replace the existing
> locally defined macros within couple of PCC client drivers when accessing
> the command and status bitfields.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> Sudeep Holla (3):
> ACPI: PCC: Add PCC shared memory region command and status bitfields
> i2c: xgene-slimpro: Migrate to use generic PCC shmem related macros
> hwmon: (xgene) Migrate to use generic PCC shmem related macros
>
> drivers/hwmon/xgene-hwmon.c | 16 +++++-----------
> drivers/i2c/busses/i2c-xgene-slimpro.c | 16 ++++------------
> include/acpi/pcc.h | 11 +++++++++++
> 3 files changed, 20 insertions(+), 23 deletions(-)
> ---
> base-commit: 6465e260f48790807eef06b583b38ca9789b6072
> change-id: 20230926-pcc_defines-24be5e33b6f3
>
> Best regards,

2023-09-27 14:11:55

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI: PCC: Define and use the common PCC shared memory regions related macros

On Wed, Sep 27, 2023 at 10:11:21AM +0800, lihuisong (C) wrote:
> Hi Sudeep,
>
> Could you please use these new common macros for kunpeng_hccs?

Sure, sorry for missing that. I had these changes in a branch for a while,
did check for new additions when I decided to post them.

--
Regards,
Sudeep

2023-09-27 14:27:50

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 1/3] ACPI: PCC: Add PCC shared memory region command and status bitfields

On Wed, Sep 27, 2023 at 10:07:15AM +0800, lihuisong (C) wrote:
> Hi Sudeep,
>
> 在 2023/9/26 20:28, Sudeep Holla 写道:
> > Define the common macros to use when referring to various bitfields in
> > the PCC generic communications channel command and status fields.
>
> Can you define the bit0 macros in the "flags" for Extended PCC Subspace
> Shared Memory Region?

Sure I will take a look and include it in v2 if applicable.

> >
> > Currently different drivers that need to use these bitfields have defined
> > these locally. This common macro is intended to consolidate and replace
> > those.
> >
> > Cc: Rafael J. Wysocki <[email protected]>
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > include/acpi/pcc.h | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> > index 73e806fe7ce7..66d9934c2ee4 100644
> > --- a/include/acpi/pcc.h
> > +++ b/include/acpi/pcc.h
> > @@ -18,7 +18,18 @@ struct pcc_mbox_chan {
> > u16 min_turnaround_time;
> > };
> > +/* Generic Communications Channel Shared Memory Region */
> > +#define PCC_SIGNATURE 0x50424300
> Why is this signature 0x50424300?

It is as per the specification.

> In ACPI spec, this signature is all 0x50434303.

No, not exactly. It is just an example.
The PCC signature - The signature of a subspace is computed by a bitwise-or
of the value 0x50434300 with the subspace ID. For example, subspace 3 has
signature 0x50434303

And I see the driver you mentioned(drivers/soc/hisilicon/kunpeng_hccs.c)
is doing the right thing. I am bit confused as why you being the author
of the driver are now confused.

--
Regards,
Sudeep

2023-09-27 14:34:20

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI: PCC: Define and use the common PCC shared memory regions related macros

On Tue, Sep 26, 2023 at 2:33 PM Sudeep Holla <[email protected]> wrote:
>
> This set of 3 small patches intend to consolidate and replace the existing
> locally defined macros within couple of PCC client drivers when accessing
> the command and status bitfields.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> Sudeep Holla (3):
> ACPI: PCC: Add PCC shared memory region command and status bitfields
> i2c: xgene-slimpro: Migrate to use generic PCC shmem related macros
> hwmon: (xgene) Migrate to use generic PCC shmem related macros
>
> drivers/hwmon/xgene-hwmon.c | 16 +++++-----------
> drivers/i2c/busses/i2c-xgene-slimpro.c | 16 ++++------------
> include/acpi/pcc.h | 11 +++++++++++
> 3 files changed, 20 insertions(+), 23 deletions(-)
> ---

This is fine with me.

How do you want to route it?

2023-09-27 15:13:52

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI: PCC: Define and use the common PCC shared memory regions related macros

On Wed, Sep 27, 2023 at 4:47 PM Sudeep Holla <[email protected]> wrote:
>
> On Wed, Sep 27, 2023 at 04:19:21PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Sep 26, 2023 at 2:33 PM Sudeep Holla <[email protected]> wrote:
> > >
> > > This set of 3 small patches intend to consolidate and replace the existing
> > > locally defined macros within couple of PCC client drivers when accessing
> > > the command and status bitfields.
> > >
> > > Signed-off-by: Sudeep Holla <[email protected]>
> > > ---
> > > Sudeep Holla (3):
> > > ACPI: PCC: Add PCC shared memory region command and status bitfields
> > > i2c: xgene-slimpro: Migrate to use generic PCC shmem related macros
> > > hwmon: (xgene) Migrate to use generic PCC shmem related macros
> > >
> > > drivers/hwmon/xgene-hwmon.c | 16 +++++-----------
> > > drivers/i2c/busses/i2c-xgene-slimpro.c | 16 ++++------------
> > > include/acpi/pcc.h | 11 +++++++++++
> > > 3 files changed, 20 insertions(+), 23 deletions(-)
> > > ---
> >
> > This is fine with me.
> >
> > How do you want to route it?
>
> I have to respin this to include another driver.
>
> I also have 2 PCC mailbox driver changes that I wanted to send a pull request
> to you. I can make this part of that PR or you can take it directly. Both
> hwmon and i2c maintainers have acked now.

A PR would be convenient. :-)

2023-09-27 16:56:13

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI: PCC: Define and use the common PCC shared memory regions related macros

On Wed, Sep 27, 2023 at 04:19:21PM +0200, Rafael J. Wysocki wrote:
> On Tue, Sep 26, 2023 at 2:33 PM Sudeep Holla <[email protected]> wrote:
> >
> > This set of 3 small patches intend to consolidate and replace the existing
> > locally defined macros within couple of PCC client drivers when accessing
> > the command and status bitfields.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > Sudeep Holla (3):
> > ACPI: PCC: Add PCC shared memory region command and status bitfields
> > i2c: xgene-slimpro: Migrate to use generic PCC shmem related macros
> > hwmon: (xgene) Migrate to use generic PCC shmem related macros
> >
> > drivers/hwmon/xgene-hwmon.c | 16 +++++-----------
> > drivers/i2c/busses/i2c-xgene-slimpro.c | 16 ++++------------
> > include/acpi/pcc.h | 11 +++++++++++
> > 3 files changed, 20 insertions(+), 23 deletions(-)
> > ---
>
> This is fine with me.
>
> How do you want to route it?

I have to respin this to include another driver.

I also have 2 PCC mailbox driver changes that I wanted to send a pull request
to you. I can make this part of that PR or you can take it directly. Both
hwmon and i2c maintainers have acked now.

--
Regards,
Sudeep

2023-09-28 03:57:12

by Huisong Li

[permalink] [raw]
Subject: Re: [PATCH 1/3] ACPI: PCC: Add PCC shared memory region command and status bitfields


在 2023/9/27 21:59, Sudeep Holla 写道:
> On Wed, Sep 27, 2023 at 10:07:15AM +0800, lihuisong (C) wrote:
>> Hi Sudeep,
>>
>> 在 2023/9/26 20:28, Sudeep Holla 写道:
>>> Define the common macros to use when referring to various bitfields in
>>> the PCC generic communications channel command and status fields.
>> Can you define the bit0 macros in the "flags" for Extended PCC Subspace
>> Shared Memory Region?
> Sure I will take a look and include it in v2 if applicable.
Thanks
>
>>> Currently different drivers that need to use these bitfields have defined
>>> these locally. This common macro is intended to consolidate and replace
>>> those.
>>>
>>> Cc: Rafael J. Wysocki <[email protected]>
>>> Signed-off-by: Sudeep Holla <[email protected]>
>>> ---
>>> include/acpi/pcc.h | 11 +++++++++++
>>> 1 file changed, 11 insertions(+)
>>>
>>> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
>>> index 73e806fe7ce7..66d9934c2ee4 100644
>>> --- a/include/acpi/pcc.h
>>> +++ b/include/acpi/pcc.h
>>> @@ -18,7 +18,18 @@ struct pcc_mbox_chan {
>>> u16 min_turnaround_time;
>>> };
>>> +/* Generic Communications Channel Shared Memory Region */
>>> +#define PCC_SIGNATURE 0x50424300
>> Why is this signature 0x50424300?
> It is as per the specification.
>
>> In ACPI spec, this signature is all 0x50434303.
> No, not exactly. It is just an example.
> The PCC signature - The signature of a subspace is computed by a bitwise-or
> of the value 0x50434300 with the subspace ID. For example, subspace 3 has
> signature 0x50434303
Sorry for my mistake. I know this.
I mean, why doesn't the following macro follow spec and define this
signature as 0x504*3*430.
"#define PCC_SIGNATURE **0x504*2*4300*"*
Because it seems that all version of ACPI spec is 0x5043430.
>
> And I see the driver you mentioned(drivers/soc/hisilicon/kunpeng_hccs.c)
> is doing the right thing. I am bit confused as why you being the author
> of the driver are now confused.
I used 0x50424300 instead of 0x50424300 according to the spec.
>

2023-09-28 11:23:20

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 1/3] ACPI: PCC: Add PCC shared memory region command and status bitfields

On Thu, Sep 28, 2023 at 11:49:25AM +0800, lihuisong (C) wrote:
>
> 在 2023/9/27 21:59, Sudeep Holla 写道:
> > On Wed, Sep 27, 2023 at 10:07:15AM +0800, lihuisong (C) wrote:

[...]

> > > > +/* Generic Communications Channel Shared Memory Region */
> > > > +#define PCC_SIGNATURE 0x50424300
> > > Why is this signature 0x50424300?
> > It is as per the specification.
> >
> > > In ACPI spec, this signature is all 0x50434303.
> > No, not exactly. It is just an example.
> > The PCC signature - The signature of a subspace is computed by a bitwise-or
> > of the value 0x50434300 with the subspace ID. For example, subspace 3 has
> > signature 0x50434303
> Sorry for my mistake. I know this.
> I mean, why doesn't the following macro follow spec and define this
> signature as 0x504*3*430.
> "#define PCC_SIGNATURE **0x504*2*4300*"*
> Because it seems that all version of ACPI spec is 0x5043430.

Sorry my mistake. Stupidly the existing drivers have it wrong and I just
copied them without paying much attention. I will fix it up. It must be
0x50434300 instead of 0x50424300. If you have no other comments, can you
please ack v2 patch 4/4 changing soc kunpeng_hccs driver. I will fixup
the PCC_SIGNATURE and send it as part of my PR to Rafael.

Refer [1] for the change in PCC_SIGNATURE, sorry for missing the point. I
was confident that the existing code is correct :), but I am wrong.

--
Regards,
Sudeep

[1] https://git.kernel.org/sudeep.holla/l/pcc_defines

2023-09-28 11:39:00

by Huisong Li

[permalink] [raw]
Subject: Re: [PATCH 1/3] ACPI: PCC: Add PCC shared memory region command and status bitfields


在 2023/9/28 19:11, Sudeep Holla 写道:
> On Thu, Sep 28, 2023 at 11:49:25AM +0800, lihuisong (C) wrote:
>> 在 2023/9/27 21:59, Sudeep Holla 写道:
>>> On Wed, Sep 27, 2023 at 10:07:15AM +0800, lihuisong (C) wrote:
> [...]
>
>>>>> +/* Generic Communications Channel Shared Memory Region */
>>>>> +#define PCC_SIGNATURE 0x50424300
>>>> Why is this signature 0x50424300?
>>> It is as per the specification.
>>>
>>>> In ACPI spec, this signature is all 0x50434303.
>>> No, not exactly. It is just an example.
>>> The PCC signature - The signature of a subspace is computed by a bitwise-or
>>> of the value 0x50434300 with the subspace ID. For example, subspace 3 has
>>> signature 0x50434303
>> Sorry for my mistake. I know this.
>> I mean, why doesn't the following macro follow spec and define this
>> signature as 0x504*3*430.
>> "#define PCC_SIGNATURE **0x504*2*4300*"*
>> Because it seems that all version of ACPI spec is 0x5043430.
> Sorry my mistake. Stupidly the existing drivers have it wrong and I just
> copied them without paying much attention. I will fix it up. It must be
> 0x50434300 instead of 0x50424300. If you have no other comments, can you
They are very similar.????
> please ack v2 patch 4/4 changing soc kunpeng_hccs driver. I will fixup
> the PCC_SIGNATURE and send it as part of my PR to Rafael.
ok
>
> Refer [1] for the change in PCC_SIGNATURE, sorry for missing the point. I
> was confident that the existing code is correct :), but I am wrong.
>