Message-ID: <bb544194-d8af-4086-b569-4a4b4befd6ad@amperemail.onmicrosoft.com>
Date: Tue, 22 Jul 2025 13:10:01 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: admiyo@...amperecomputing.com, Sudeep Holla <sudeep.holla@....com>,
 Jassi Brar <jassisinghbrar@...il.com>, "Rafael J. Wysocki"
 <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Robert Moore <robert.moore@...el.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Jeremy Kerr <jk@...econstruct.com.au>,
 Matt Johnston <matt@...econstruct.com.au>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the
 shared buffer


On 7/14/25 20:10, admiyo@...amperecomputing.com wrote:
> From: Adam Young <admiyo@...amperecomputing.com>
>
> Define a new, optional, callback that allows the driver to
> specify how the return data buffer is allocated.  If that callback
> is set,  mailbox/pcc.c is now responsible for reading from and
> writing to the PCC shared buffer.
>
> This also allows for proper checks of the Commnand complete flag
> between the PCC sender and receiver.
>
> For Type 4 channels, initialize the command complete flag prior
> to accepting messages.
>
> Since the mailbox does not know what memory allocation scheme
> to use for response messages, the client now has an optional
> callback that allows it to allocate the buffer for a response
> message.
>
> When an outbound message is written to the buffer, the mailbox
> checks for the flag indicating the client wants an tx complete
> notification via IRQ.  Upon receipt of the interrupt It will
> pair it with the outgoing message. The expected use is to
> free the kernel memory buffer for the previous outgoing message.
>
> Signed-off-by: Adam Young <admiyo@...amperecomputing.com>
> ---
>   drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++++--
>   include/acpi/pcc.h    |  29 ++++++++++++
>   2 files changed, 127 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index f6714c233f5a..0a00719b2482 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -306,6 +306,22 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
>   		pcc_chan_reg_read_modify_write(&pchan->db);
>   }
>   
> +static void *write_response(struct pcc_chan_info *pchan)
> +{
> +	struct pcc_header pcc_header;
> +	void *buffer;
> +	int data_len;
> +
> +	memcpy_fromio(&pcc_header, pchan->chan.shmem,
> +		      sizeof(pcc_header));
> +	data_len = pcc_header.length - sizeof(u32) + sizeof(struct pcc_header);
> +
> +	buffer = pchan->chan.rx_alloc(pchan->chan.mchan->cl, data_len);
> +	if (buffer != NULL)
> +		memcpy_fromio(buffer, pchan->chan.shmem, data_len);
> +	return buffer;
> +}
> +
>   /**
>    * pcc_mbox_irq - PCC mailbox interrupt handler
>    * @irq:	interrupt number
> @@ -317,6 +333,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   {
>   	struct pcc_chan_info *pchan;
>   	struct mbox_chan *chan = p;
> +	struct pcc_header *pcc_header = chan->active_req;

OK, so it looks a little strange to re-initialize this later. Would it 
be better to not have it initialized?


> +	void *handle = NULL;
>   
>   	pchan = chan->con_priv;
>   
> @@ -340,7 +358,17 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	 * required to avoid any possible race in updatation of this flag.
>   	 */
>   	pchan->chan_in_use = false;
> -	mbox_chan_received_data(chan, NULL);
> +
> +	if (pchan->chan.rx_alloc)
> +		handle = write_response(pchan);
> +
> +	if (chan->active_req) {
> +		pcc_header = chan->active_req;
> +		if (pcc_header->flags & PCC_CMD_COMPLETION_NOTIFY)

Note that this is the counterpoint to my earlier patch that only 
notifies the platform if the platform requests it.  This is part of the 
specification.

> +			mbox_chan_txdone(chan, 0);
> +	}
> +
> +	mbox_chan_received_data(chan, handle);
>   
>   	pcc_chan_acknowledge(pchan);
>   
> @@ -384,9 +412,24 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	pcc_mchan = &pchan->chan;
>   	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
>   					   pcc_mchan->shmem_size);
> -	if (pcc_mchan->shmem)
> -		return pcc_mchan;
> +	if (!pcc_mchan->shmem)
> +		goto err;
> +
> +	pcc_mchan->manage_writes = false;
> +
> +	/* This indicates that the channel is ready to accept messages.
> +	 * This needs to happen after the channel has registered
> +	 * its callback. There is no access point to do that in
> +	 * the mailbox API. That implies that the mailbox client must
> +	 * have set the allocate callback function prior to
> +	 * sending any messages.
> +	 */
> +	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
Is there a better  way to do this?  The flag is not accessable from the 
driver.
> +
> +	return pcc_mchan;
>   
> +err:
>   	mbox_free_channel(chan);
>   	return ERR_PTR(-ENXIO);
>   }
> @@ -417,8 +460,38 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>   
> +static int pcc_write_to_buffer(struct mbox_chan *chan, void *data)
> +{
> +	struct pcc_chan_info *pchan = chan->con_priv;
> +	struct pcc_mbox_chan *pcc_mbox_chan = &pchan->chan;
> +	struct pcc_header *pcc_header = data;
> +
> +	if (!pchan->chan.manage_writes)
> +		return 0;
> +
> +	/* The PCC header length includes the command field
> +	 * but not the other values from the header.
> +	 */
> +	int len = pcc_header->length - sizeof(u32) + sizeof(struct pcc_header);
> +	u64 val;
> +
> +	pcc_chan_reg_read(&pchan->cmd_complete, &val);
> +	if (!val) {
> +		pr_info("%s pchan->cmd_complete not set", __func__);
> +		return -1;
> +	}
> +	memcpy_toio(pcc_mbox_chan->shmem,  data, len);
> +	return 0;
> +}
> +

I think this is the pattern that we  want all of the PCC mailbox clients 
to migrate to.  Is there any reason it was not implmented this way 
originally?-

> +
>   /**
> - * pcc_send_data - Called from Mailbox Controller code. Used
> + * pcc_send_data - Called from Mailbox Controller code. If
> + *		pchan->chan.rx_alloc is set, then the command complete
> + *		flag is checked and the data is written to the shared
> + *		buffer io memory.
> + *
> + *		If pchan->chan.rx_alloc is not set, then it is used
>    *		here only to ring the channel doorbell. The PCC client
>    *		specific read/write is done in the client driver in
>    *		order to maintain atomicity over PCC channel once
> @@ -434,17 +507,37 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
>   	int ret;
>   	struct pcc_chan_info *pchan = chan->con_priv;
>   
> +	ret = pcc_write_to_buffer(chan, data);
> +	if (ret)
> +		return ret;
> +
>   	ret = pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>   	if (ret)
>   		return ret;
>   
>   	ret = pcc_chan_reg_read_modify_write(&pchan->db);
> +
>   	if (!ret && pchan->plat_irq > 0)
>   		pchan->chan_in_use = true;
>   
>   	return ret;
>   }
>   
> +
> +static bool pcc_last_tx_done(struct mbox_chan *chan)
> +{
> +	struct pcc_chan_info *pchan = chan->con_priv;
> +	u64 val;
> +
> +	pcc_chan_reg_read(&pchan->cmd_complete, &val);
> +	if (!val)
> +		return false;
> +	else
> +		return true;
> +}
> +
> +
> +
>   /**
>    * pcc_startup - Called from Mailbox Controller code. Used here
>    *		to request the interrupt.
> @@ -490,6 +583,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>   	.send_data = pcc_send_data,
>   	.startup = pcc_startup,
>   	.shutdown = pcc_shutdown,
> +	.last_tx_done = pcc_last_tx_done,
>   };
>   
>   /**
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 840bfc95bae3..9af3b502f839 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -17,6 +17,35 @@ struct pcc_mbox_chan {
>   	u32 latency;
>   	u32 max_access_rate;
>   	u16 min_turnaround_time;
> +
> +	/* Set to true to indicate that the mailbox should manage
> +	 * writing the dat to the shared buffer. This differs from
> +	 * the case where the drivesr are writing to the buffer and
> +	 * using send_data only to  ring the doorbell.  If this flag
> +	 * is set, then the void * data parameter of send_data must
> +	 * point to a kernel-memory buffer formatted in accordance with
> +	 * the PCC specification.
> +	 *
> +	 * The active buffer management will include reading the
> +	 * notify_on_completion flag, and will then
> +	 * call mbox_chan_txdone when the acknowledgment interrupt is
> +	 * received.
> +	 */
> +	bool manage_writes;
> +
> +	/* Optional callback that allows the driver
> +	 * to allocate the memory used for receiving
> +	 * messages.  The return value is the location
> +	 * inside the buffer where the mailbox should write the data.
> +	 */
> +	void *(*rx_alloc)(struct mbox_client *cl,  int size);
> +};
> +
> +struct pcc_header {
> +	u32 signature;
> +	u32 flags;
> +	u32 length;
> +	u32 command;
>   };

Fairly certain these should not be explicitly little endian IAW the 
spec. It has been a source of discussion in the past.


>   
>   /* Generic Communications Channel Shared Memory Region */

Powered by blists - more mailing lists