Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

[Sricharan Ramabadhran <quic_srichara@quicinc.com>]

On 4/26/2025 3:19 PM, Konrad Dybcio wrote:
> On 3/27/25 7:17 PM, Sricharan R wrote:
>> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>>
>> This mailbox facilitates the communication between the TMEL server
>> subsystem (Trust Management Engine Lite) and the TMEL client
>> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
>> authentication, enable/disable efuses, crypto services etc. Each client in
>> the SoC has its own block of message RAM and IRQ for communication with the
>> TMEL SS. The protocol used to communicate in the message RAM is known as
>> Qualcomm Messaging Protocol (QMP).
>>
>> Remote proc driver subscribes to this mailbox and uses the
>> mbox_send_message to use TMEL to securely authenticate/teardown the images.
>>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> ---
> 
> [...]
> 
>> +
>> +#define QMP_NUM_CHANS		0x1
> 
> Quantities make more sense in decimal, but since this is effectively
> a single-use value, you can put in the '1' literal in num_chans and use
> devm_kzalloc instead of devm_kcalloc in the other use
> 
ok

>> +#define QMP_TOUT_MS		1000
> 
> "TIMEOUT"
> 
ok

>> +#define QMP_CTRL_DATA_SIZE	4
>> +#define QMP_MAX_PKT_SIZE	0x18
> 
> This is very handwavy, please structurize all data that comes in and
> out of the mailbox.
> 
ok

>> +#define QMP_UCORE_DESC_OFFSET	0x1000
>> +#define QMP_SEND_TIMEOUT	30000
> 
> Please include the unit in the macro name - although 30s is quite a
> timeout for a couple bytes..
> 
ok

> [...]
> 
>> +#define QMP_HW_MBOX_SIZE		32
>> +#define QMP_MBOX_RSV_SIZE		4
>> +#define QMP_MBOX_IPC_PACKET_SIZE	(QMP_HW_MBOX_SIZE - QMP_CTRL_DATA_SIZE - QMP_MBOX_RSV_SIZE)
>> +#define QMP_MBOX_IPC_MAX_PARAMS		5
>> +
>> +#define QMP_MAX_PARAM_IN_PARAM_ID	14
>> +#define QMP_PARAM_CNT_FOR_OUTBUF	3
>> +#define QMP_SRAM_IPC_MAX_PARAMS		(QMP_MAX_PARAM_IN_PARAM_ID * QMP_PARAM_CNT_FOR_OUTBUF)
>> +#define QMP_SRAM_IPC_MAX_BUF_SIZE	(QMP_SRAM_IPC_MAX_PARAMS * sizeof(u32))
> 
> These should be expressed in terms of structures and sizeof() instead,
> as well
> 
ok

>> +
>> +#define TMEL_ERROR_GENERIC		(0x1u)
>> +#define TMEL_ERROR_NOT_SUPPORTED	(0x2u)
>> +#define TMEL_ERROR_BAD_PARAMETER	(0x3u)
>> +#define TMEL_ERROR_BAD_MESSAGE		(0x4u)
>> +#define TMEL_ERROR_BAD_ADDRESS		(0x5u)
>> +#define TMEL_ERROR_TMELCOM_FAILURE	(0x6u)
>> +#define TMEL_ERROR_TMEL_BUSY		(0x7u)
> 
> Oh I didn't notice this during the first review.. I assume these are
> returned by the mbox. Please create a dictionary such as:
> 
> u32 tmel_error_dict[] = {
> 	[TMEL_ERROR_GENERIC] = EINVAL,
> 	[TMEL_ERROR_NOT_SUPPORTED] = EOPNOTSUPP
> 	...
> };
> 
> that we can then plug into the function down below that currently does
> error ? -EINVAL : 0
> 

Hmm ok, will try this mapping.

Regards,
  Sricharan