| Message ID | 20230926-pcc_defines-v1-0-0f925a1658fd@arm.com |
|---|---|
| Headers | show |
| Series | ACPI: PCC: Define and use the common PCC shared memory regions related macros | expand |
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 <rafael.j.wysocki@intel.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > 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); >
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 <sudeep.holla@arm.com> > --- > 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,
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 <rafael.j.wysocki@intel.com> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > --- > > 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.
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.
On Tue, Sep 26, 2023 at 2:33 PM Sudeep Holla <sudeep.holla@arm.com> 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 <sudeep.holla@arm.com> > --- > 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?
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 <sudeep.holla@arm.com> 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 <sudeep.holla@arm.com> > > --- > > 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
On Wed, Sep 27, 2023 at 4:47 PM Sudeep Holla <sudeep.holla@arm.com> 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 <sudeep.holla@arm.com> 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 <sudeep.holla@arm.com> > > > --- > > > 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/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 <rafael.j.wysocki@intel.com> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> --- >>> 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/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. >
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 <sudeep.holla@arm.com> --- 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,