On 21/10/2024 16:12, Tudor Ambarus wrote:
> Hi, Krzysztof,
>
> On 10/21/24 12:52 PM, Krzysztof Kozlowski wrote:
>> On 17/10/2024 18:36, Tudor Ambarus wrote:
>>> ACPM (Alive Clock and Power Manager) is a firmware that operates on the
>>> APM (Active Power Management) module that handles overall power management
>>> activities. ACPM and masters regard each other as independent
>>> hardware component and communicate with each other using mailbox messages
>>> and shared memory.
>>>
>>> The mailbox channels are initialized based on the configuration data
>>> found at a specific offset into the shared memory (mmio-sram). The
>>> configuration data consists of channel id, message and queue lengths,
>>> pointers to the RX and TX queues which are also part of the SRAM, and
>>> whether RX works by polling or interrupts. All known clients of this
>>> driver are using polling channels, thus the driver implements for now
>>> just polling mode.
>>>
>>> Add support for the exynos acpm core driver. Helper drivers will follow.
>>> These will construct the mailbox messages in the format expected by the
>>> firmware.
>>
>> 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.
>
>>
>> I did not perform full review yet, just skimmed over the code. I will
>> take a look a bit later.
>>
>
> No worries, thanks for doing it. I agree with all the suggestions from
> below and I'll address them after we get an agreement with Jassi on how
> to extend the mailbox core.
>
> More answers below.
>
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
>>> ---
>>> drivers/firmware/Kconfig | 1 +
>>> drivers/firmware/Makefile | 1 +
>>> drivers/firmware/samsung/Kconfig | 11 +
>>> drivers/firmware/samsung/Makefile | 3 +
>>> drivers/firmware/samsung/exynos-acpm.c | 703 ++++++++++++++++++++
>>
>> Please add directory to the Samsung Exynos SoC maintainer entry. I also
>> encourage adding separate entry for the driver where you would be listed
>> as maintainer.
>
> ok
>
>>
>> There is no firmware tree, so this will be going via Samsung SoC.
>
> I noticed afterwards, thanks.
>
>>
>>> include/linux/mailbox/exynos-acpm-message.h | 21 +
>>> 6 files changed, 740 insertions(+)
>>> create mode 100644 drivers/firmware/samsung/Kconfig
>>> create mode 100644 drivers/firmware/samsung/Makefile
>>> create mode 100644 drivers/firmware/samsung/exynos-acpm.c
>>> create mode 100644 include/linux/mailbox/exynos-acpm-message.h
>>>
>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>> index 71d8b26c4103..24edb956831b 100644
>>> --- a/drivers/firmware/Kconfig
>>> +++ b/drivers/firmware/Kconfig
>>> @@ -267,6 +267,7 @@ source "drivers/firmware/meson/Kconfig"
>>> source "drivers/firmware/microchip/Kconfig"
>>> source "drivers/firmware/psci/Kconfig"
>>> source "drivers/firmware/qcom/Kconfig"
>>> +source "drivers/firmware/samsung/Kconfig"
>>> source "drivers/firmware/smccc/Kconfig"
>>> source "drivers/firmware/tegra/Kconfig"
>>> source "drivers/firmware/xilinx/Kconfig"
>>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>>> index 7a8d486e718f..91efcc868a05 100644
>>> --- a/drivers/firmware/Makefile
>>> +++ b/drivers/firmware/Makefile
>>> @@ -33,6 +33,7 @@ obj-y += efi/
>>> obj-y += imx/
>>> obj-y += psci/
>>> obj-y += qcom/
>>> +obj-y += samsung/
>>> obj-y += smccc/
>>> obj-y += tegra/
>>> obj-y += xilinx/
>>> diff --git a/drivers/firmware/samsung/Kconfig b/drivers/firmware/samsung/Kconfig
>>> new file mode 100644
>>> index 000000000000..f908773c1441
>>> --- /dev/null
>>> +++ b/drivers/firmware/samsung/Kconfig
>>> @@ -0,0 +1,11 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +config EXYNOS_ACPM
>>> + tristate "Exynos ACPM (Alive Clock and Power Manager) driver support"
>>
>> depends ARCH_EXYNOS || COMPILE_TEST
>
> oh yes.
>>
>> Please also send a arm64 defconfig change making it a module.
>
> will do
>
> 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;
>>
>> Why are you storing addresses as u32? Shouldn't these be __iomem*?
>
> This structure defines the offsets in SRAM that describe the channel
> parameters. Instances of this struct shall be declared indeed as:
> struct exynos_acpm_shmem_chan __iomem *shmem_chan;
> I missed that in v2, but will update in v2.
>
>>
>> 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?
>
> cut
>
>>> +
>>> + spin_lock_irqsave(&chan->tx_lock, flags);
>>> +
>>> + tx_front = readl_relaxed(chan->tx.front);
>>> + idx = (tx_front + 1) % chan->qlen;
>>> +
>>> + ret = exynos_acpm_wait_for_queue_slots(mbox_chan, idx);
>>> + if (ret)
>>> + goto exit;
>>> +
>>> + exynos_acpm_prepare_request(mbox_chan, req);
>>> +
>>> + /* Write TX command. */
>>> + __iowrite32_copy(chan->tx.base + chan->mlen * tx_front, tx->cmd,
>>> + req->txlen / 4);
>>> +
>>> + /* Advance TX front. */
>>> + writel_relaxed(idx, chan->tx.front);
>>> +
>>> + /* Flush SRAM posted writes. */
>>> + readl_relaxed(chan->tx.front);
>>> +
>>
>> How does this flush work? Maybe you just miss here barries (s/relaxed//)?
>
> I think _relaxed() accessors should be fine in this driver as there are
> no DMA accesses involved. _relaxed() accessors are fully ordered for
> accesses to the same device/endpoint.
>
> I'm worried however about the posted writes, the buses the devices sit
> on may themselves have asynchronicity. So I issue a read from the same
> device to ensure that the write has occured.
Hm, ok, it seems it is actually standard way for posted bus.
>
> There is something that I haven't dimistified though. You'll notice that
> the writes from above are on SRAM. I enqueue on the TX queue then
> advance the head/front of the queue and then I read back to make sure
> that the writes occured. Below I write to the controller's interrupt
> register (different device/endpoint) to raise the interrupt for the
> counterpart and inform that TX queue advanced. I'm not sure whether I
> need a barrier here to make sure that the CPU does not reorder the
> accesses and raise the interrupt before advancing the TX queue. If
> someone already knows the answer, please say, I'll do some more reading
> in the meantime.
I think it is fine.
Best regards,
Krzysztof
