Message-ID: <d8ac05a9-9229-49a4-b7f2-8d92060ccc63@amperemail.onmicrosoft.com>
Date: Thu, 31 Jul 2025 15:35:53 -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

Is there some reason that this patch is not showing up in the 
maintainers queues for review?

On 7/22/25 13:10, Adam Young wrote:
>
> 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