mbox series

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

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

Message

Sudeep Holla Sept. 26, 2023, 12:27 p.m. UTC
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,

Comments

Huisong Li Sept. 27, 2023, 2:07 a.m. UTC | #1
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);
>
Huisong Li Sept. 27, 2023, 2:11 a.m. UTC | #2
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,
Sudeep Holla Sept. 27, 2023, 1:59 p.m. UTC | #3
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.
Sudeep Holla Sept. 27, 2023, 2 p.m. UTC | #4
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.
Rafael J. Wysocki Sept. 27, 2023, 2:19 p.m. UTC | #5
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?
Sudeep Holla Sept. 27, 2023, 2:45 p.m. UTC | #6
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
Rafael J. Wysocki Sept. 27, 2023, 3:07 p.m. UTC | #7
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. :-)
Huisong Li Sept. 28, 2023, 3:49 a.m. UTC | #8
在 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.
>
Huisong Li Sept. 28, 2023, 11:28 a.m. UTC | #9
在 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.
>