Message-ID: <1ece02e6-bf78-443a-8143-a54e94dd744c@kernel.org>
Date: Tue, 22 Oct 2024 06:38:39 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Tudor Ambarus <tudor.ambarus@...aro.org>, jassisinghbrar@...il.com
Cc: alim.akhtar@...sung.com, mst@...hat.com, javierm@...hat.com,
 tzimmermann@...e.de, bartosz.golaszewski@...aro.org,
 luzmaximilian@...il.com, sudeep.holla@....com, conor.dooley@...rochip.com,
 bjorn@...osinc.com, ulf.hansson@...aro.org,
 linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, marcan@...can.st, neal@...pa.dev,
 alyssa@...enzweig.io, broonie@...nel.org, andre.draszik@...aro.org,
 willmcvicker@...gle.com, peter.griffin@...aro.org, kernel-team@...roid.com,
 vincent.guittot@...aro.org, daniel.lezcano@...aro.org
Subject: Re: [PATCH v2 2/2] firmware: add exynos acpm driver

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@...aro.org>
>>> ---
>>>  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

Powered by blists - more mailing lists