Re: [PATCH v2 2/2] firmware: add exynos acpm driver

[Tudor Ambarus <tudor.ambarus@linaro.org>]

Hi, Krzysztof,

On 10/22/24 5:38 AM, Krzysztof Kozlowski wrote:

cut

>>> I skimmed through the driver and I do not understand why this is
>>> firmware. You are implementing a mailbox provider/controller.
>>
>> In my case the mailbox hardware is used just to raise the interrupt to
>> the other side. Then there's the SRAM which contains the channels
>> configuration data and the TX/RX queues. The enqueue/deque is done
>> in/from SRAM. This resembles a lot with drivers/firmware/arm_scmi/, see:
>>
>> drivers/firmware/arm_scmi/shmem.c
>> drivers/firmware/arm_scmi/transports/mailbox.c
> 
> Wait, SCMI is an interface. Not the case here.
> 
>>
>> After the SRAM and mailbox/transport code I'll come up with two helper
>> drivers that construct the mailbox messages in the format expected by
>> the firmware. There are 2 types of messages recognized by the ACPM
>> firmware: PMIC and DVFS. The client drivers will use these helper
>> drivers to prepare a specific message. Then they will use the mailbox
>> core to send the message and they'll wait for the answer.
>>
>> This layered structure and the use of SRAM resembles with arm_scmi and
>> made me think that the ACPM driver it's better suited for
>> drivers/firmware. I'm opened for suggestions though.
> 
> Sure, but then this driver cannot perform mbox_controller_register().
> Only mailbox providers, so drivers in mailbox, use it.
> 

Okay, I can move the driver to drivers/mailbox/.

cut

>>>> +/**
>>>> + * struct exynos_acpm_shmem_chan - descriptor of a shared memory channel.
>>>> + *
>>>> + * @id:			channel ID.
>>>> + * @reserved:		reserved for future use.
>>>> + * @rx_rear:		rear pointer of RX queue.
>>>> + * @rx_front:		front pointer of RX queue.
>>>> + * @rx_base:		base address of RX queue.
>>>> + * @reserved1:		reserved for future use.
>>>> + * @tx_rear:		rear pointer of TX queue.
>>>> + * @tx_front:		front pointer of TX queue.
>>>> + * @tx_base:		base address of TX queue.
>>>> + * @qlen:		queue length. Applies to both TX/RX queues.
>>>> + * @mlen:		message length. Applies to both TX/RX queues.
>>>> + * @reserved2:		reserved for future use.
>>>> + * @polling:		true when the channel works on polling.
>>>> + */
>>>> +struct exynos_acpm_shmem_chan {
>>>> +	u32 id;
>>>> +	u32 reserved[3];
>>>> +	u32 rx_rear;
>>>> +	u32 rx_front;
>>>> +	u32 rx_base;
>>>> +	u32 reserved1[3];
>>>> +	u32 tx_rear;
>>>> +	u32 tx_front;
>>>> +	u32 tx_base;
>>>> +	u32 qlen;
>>>> +	u32 mlen;
>>>> +	u32 reserved2[2];
>>>> +	u32 polling;
>>>

cut

>>>
>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>
>> I'm not writing any SRAM configuration fields, these fields are used to
>> read/retrive the channel parameters from SRAM.
> 
> I meany tx_base is always 0. Where is this property set? Ever?

It's not zero. My assumption is it is set in the acpm firmware, but I
don't have access to that to verify. Here are some debug prints made in
the linux driver:

[    0.069575][    T1] gs-acpm-ipc 17610000.mailbox:
exynos_mbox_chan_init ID = 2 poll = 1, mlen = 16, qlen = 5
[    0.069927][    T1] gs-acpm-ipc 17610000.mailbox:
exynos_mbox_chan_init ID = 2 offsets: rx_base = 0x00038290 rx_front =
0x0003828c, rx_rear = 0x00038288
[    0.070449][    T1] gs-acpm-ipc 17610000.mailbox:
exynos_mbox_chan_init ID = 2 offsets: tx_base = 0x000382f0 tx_front =
0x000382ec, tx_rear = 0x000382e8


tx_base contains the SRAM offset of the RX queue used in linux. The
offset is relative to the base address of the SRAM config data.

tx_base is seen/named from the firmware's point of view, thus named TX.
I assume the same struct is defined in the acpm firmware.


Somewhere below in the linux driver I get the RX ring base address by doing:

rx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_base);

where base is the SRAM base address of the channels configuration data.

static void __iomem *exynos_acpm_get_iomem_addr(void __iomem *base,


                                                void __iomem *addr)


{


        u32 offset;





        offset = readl_relaxed(addr);


        return base + offset;


}

Hope this clarifies a bit these struct members.
Cheers,
ta