| Message ID | 20240712023626.1010559-2-admiyo@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | MCTP over PCC | expand |
On Thu, 11 Jul 2024 22:36:24 -0400 admiyo@os.amperecomputing.com wrote: > From: Adam Young <admiyo@os.amperecomputing.com> > > Type 4 PCC channels have an option to send back a response > to the platform when they are done processing the request. > The flag to indicate whether or not to respond is inside > the message body, and thus is not available to the pcc > mailbox. Hi Adam, I've been meaning to look at this for a while, but finally getting time for review catchup. Would be good to have an explicit specification reference to make it easy for reviewers to find the bit to compare with. > > In order to read the flag, this patch maps the shared > buffer to virtual memory. > > Signed-off-by: Adam Young <admiyo@os.amperecomputing.com> > --- > drivers/mailbox/pcc.c | 32 ++++++++++++++++++++++++-------- > include/acpi/pcc.h | 8 ++++++++ > 2 files changed, 32 insertions(+), 8 deletions(-) > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > index 94885e411085..4a588f1b6ec2 100644 > --- a/drivers/mailbox/pcc.c > +++ b/drivers/mailbox/pcc.c > @@ -90,6 +90,7 @@ struct pcc_chan_reg { > * @cmd_complete: PCC register bundle for the command complete check register > * @cmd_update: PCC register bundle for the command complete update register > * @error: PCC register bundle for the error status register > + * @shmem_base_addr: the virtual memory address of the shared buffer If you are only going to map this from this pointer for the initiator/responder shared memory region, maybe it would benefit from a more specific name? > * @plat_irq: platform interrupt > * @type: PCC subspace type > * @plat_irq_flags: platform interrupt flags > @@ -107,6 +108,7 @@ struct pcc_chan_info { > struct pcc_chan_reg cmd_complete; > struct pcc_chan_reg cmd_update; > struct pcc_chan_reg error; > + void __iomem *shmem_base_addr; > int plat_irq; > u8 type; > unsigned int plat_irq_flags; > @@ -269,6 +271,24 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) > return !!val; > } > > +static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan) > +{ > + struct pcc_extended_type_hdr pcc_hdr; I'd avoid name pcc_hdr, as it's only the header on a paricular type of pcc subspace. Either go super generic with hdr or pcc_rsp_subspc_hdr or something along those lines. > + > + if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) > + return; I'd put a blank line here. Next bit is unrelated to the error check so white space will help with readability (a little bit anyway!) > + memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr, > + sizeof(struct pcc_extended_type_hdr)); sizeof(pcc_hdr) > + /* > + * The PCC slave subspace channel needs to set the command complete bit > + * and ring doorbell after processing message. > + * > + * The PCC master subspace channel clears chan_in_use to free channel. > + */ > + if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK) > + pcc_send_data(chan, NULL); > +} > + > /** > * pcc_mbox_irq - PCC mailbox interrupt handler > * @irq: interrupt number > @@ -306,14 +326,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > > mbox_chan_received_data(chan, NULL); > > - /* > - * The PCC slave subspace channel needs to set the command complete bit > - * and ring doorbell after processing message. > - * > - * The PCC master subspace channel clears chan_in_use to free channel. > - */ > - if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) > - pcc_send_data(chan, NULL); > + check_and_ack(pchan, chan); > pchan->chan_in_use = false; > > return IRQ_HANDLED; > @@ -352,6 +365,9 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id) > if (rc) > return ERR_PTR(rc); > > + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev, > + pchan->chan.shmem_base_addr, > + pchan->chan.shmem_size); devm doesn't seem appropriate here given we have manual management of other resources, so the ordering will be different in remove vs probe. So I'd handle release of this manually in mbox_free_channel() > return &pchan->chan; > } > EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); > diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h > index 9b373d172a77..0bcb86dc4de7 100644 > --- a/include/acpi/pcc.h > +++ b/include/acpi/pcc.h > @@ -18,6 +18,13 @@ struct pcc_mbox_chan { > u16 min_turnaround_time; > }; > > +struct pcc_extended_type_hdr { Spec reference would be useful for this. Looks to be Table 14.12 in acpi 6.5. I can't remember convention in this file for whether you need to find earliest spec for references or not. > + __le32 signature; > + __le32 flags; > + __le32 length; > + char command[4]; > +}; > + > /* Generic Communications Channel Shared Memory Region */ > #define PCC_SIGNATURE 0x50434300 > /* Generic Communications Channel Command Field */ > @@ -31,6 +38,7 @@ struct pcc_mbox_chan { > #define PCC_CMD_COMPLETION_NOTIFY BIT(0) > > #define MAX_PCC_SUBSPACES 256 > +#define PCC_ACK_FLAG_MASK 0x1 Maybe this should be something like PCC_EXT_FLAGS_ACK_MASK to give a hint of where it applies. Also, can we keep name closer to the spec (even though it's meaning is that we must ack the command when done) PCC_EXT_FLAGS_NOTIFY_ON_COMPLETION_MASK > > #ifdef CONFIG_PCC > extern struct pcc_mbox_chan *
>>+ * @shmem_base_addr: the virtual memory address of the shared buffer >If you are only going to map this from this pointer for the >initiator/responder shared memory region, maybe it would benefit >from a more specific name? I am not certain what would be more correct. On 8/1/24 07:41, Jonathan Cameron wrote: >> + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev, >> + pchan->chan.shmem_base_addr, >> + pchan->chan.shmem_size); > devm doesn't seem appropriate here given we have manual management > of other resources, so the ordering will be different in remove > vs probe. > > So I'd handle release of this manually in mbox_free_channel() How fixed are you on this? mbox_free_channel is the parent code, and knows nothing about this resource. It does no specific resource cleanup. The only place we could release it is in the pcc_mbox_free, but that is essentially a call to the parent function. All other comments should be addressed in the next version.
On Fri, 13 Sep 2024 17:21:06 -0400 Adam Young <admiyo@amperemail.onmicrosoft.com> wrote: > >>+ * @shmem_base_addr: the virtual memory address of the shared buffer > > >If you are only going to map this from this pointer for the > >initiator/responder shared memory region, maybe it would benefit > >from a more specific name? > > > I am not certain what would be more correct. > > > On 8/1/24 07:41, Jonathan Cameron wrote: > > >> + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev, > >> + pchan->chan.shmem_base_addr, > >> + pchan->chan.shmem_size); > > devm doesn't seem appropriate here given we have manual management > > of other resources, so the ordering will be different in remove > > vs probe. > > > > So I'd handle release of this manually in mbox_free_channel() > > > How fixed are you on this? mbox_free_channel is the parent code, and > knows nothing about this resource. It does no specific resource cleanup. I've lost context on this unfortunately and don't have time to look back at it this week. Maybe right answer is a cleanup callback? > > The only place we could release it is in the pcc_mbox_free, but that is > essentially a call to the parent function. > > All other comments should be addressed in the next version. > >
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index 94885e411085..4a588f1b6ec2 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -90,6 +90,7 @@ struct pcc_chan_reg { * @cmd_complete: PCC register bundle for the command complete check register * @cmd_update: PCC register bundle for the command complete update register * @error: PCC register bundle for the error status register + * @shmem_base_addr: the virtual memory address of the shared buffer * @plat_irq: platform interrupt * @type: PCC subspace type * @plat_irq_flags: platform interrupt flags @@ -107,6 +108,7 @@ struct pcc_chan_info { struct pcc_chan_reg cmd_complete; struct pcc_chan_reg cmd_update; struct pcc_chan_reg error; + void __iomem *shmem_base_addr; int plat_irq; u8 type; unsigned int plat_irq_flags; @@ -269,6 +271,24 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) return !!val; } +static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan) +{ + struct pcc_extended_type_hdr pcc_hdr; + + if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) + return; + memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr, + sizeof(struct pcc_extended_type_hdr)); + /* + * The PCC slave subspace channel needs to set the command complete bit + * and ring doorbell after processing message. + * + * The PCC master subspace channel clears chan_in_use to free channel. + */ + if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK) + pcc_send_data(chan, NULL); +} + /** * pcc_mbox_irq - PCC mailbox interrupt handler * @irq: interrupt number @@ -306,14 +326,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) mbox_chan_received_data(chan, NULL); - /* - * The PCC slave subspace channel needs to set the command complete bit - * and ring doorbell after processing message. - * - * The PCC master subspace channel clears chan_in_use to free channel. - */ - if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) - pcc_send_data(chan, NULL); + check_and_ack(pchan, chan); pchan->chan_in_use = false; return IRQ_HANDLED; @@ -352,6 +365,9 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id) if (rc) return ERR_PTR(rc); + pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev, + pchan->chan.shmem_base_addr, + pchan->chan.shmem_size); return &pchan->chan; } EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h index 9b373d172a77..0bcb86dc4de7 100644 --- a/include/acpi/pcc.h +++ b/include/acpi/pcc.h @@ -18,6 +18,13 @@ struct pcc_mbox_chan { u16 min_turnaround_time; }; +struct pcc_extended_type_hdr { + __le32 signature; + __le32 flags; + __le32 length; + char command[4]; +}; + /* Generic Communications Channel Shared Memory Region */ #define PCC_SIGNATURE 0x50434300 /* Generic Communications Channel Command Field */ @@ -31,6 +38,7 @@ struct pcc_mbox_chan { #define PCC_CMD_COMPLETION_NOTIFY BIT(0) #define MAX_PCC_SUBSPACES 256 +#define PCC_ACK_FLAG_MASK 0x1 #ifdef CONFIG_PCC extern struct pcc_mbox_chan *