Message-ID: <CAKfTPtDSRXiganNfMwCWh1K2G2k0kP4h2zwbMqr==CdAJ+h21A@mail.gmail.com>
Date: Tue, 22 Oct 2024 09:27:20 +0200
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Tudor Ambarus <tudor.ambarus@...aro.org>, jassisinghbrar@...il.com, 
	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, daniel.lezcano@...aro.org
Subject: Re: [PATCH v2 2/2] firmware: add exynos acpm driver

On Tue, 22 Oct 2024 at 06:38, Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> 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.

How is it different from SCMI ? They both use mailbox and shared
memory to set a message in the SRAM and ping the other side with the
mailbox. The only diff is that SCMI is an public spec whereas this one
is specific to some SoC

>
> >
> > 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.

Yes, the mailbox driver part should go into mailbox and this part
should then use mbox_request_channel() to get a channel

>
> >
> >>
> >> 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