This series adds support for the "hardware message box" in sun8i, sun9i,
and sun50i SoCs, used for communication with the ARISC management
processor (the platform's equivalent of the ARM SCP). The end goal is to
use the arm_scpi driver as a client, communicating with firmware running
on the AR100 CPU, or to use the mailbox to forward NMIs that the
firmware picks up from R_INTC.
Unfortunately, the ARM SCPI client no longer works with this driver
since it now exposes all 8 hardware FIFOs individually. The SCPI client
could be made to work (and I posted proof-of-concept code to that effect
with v1 of this series), but that is a low priority, as Linux does not
directly use SCPI with the current firmware version; all SCPI use goes
through ATF via PSCI.
As requested in the comments to v3 of this patchset, a demo client is
provided in the final patch. This demo goes along with a toy firmware
which shows that the driver does indeed work for two-way communication
on all channels. To build the firmware component, run:
git clone https://github.com/crust-firmware/meta meta
git clone -b mailbox-demo https://github.com/crust-firmware/crust meta/crust
cd meta
make
That will by default produce a U-Boot + ATF + SCP firmware image in
[meta/]build/pinebook/u-boot-sunxi-with-spl.bin. See the top-level
README.md for more information, such as cross-compiler setup.
I've now used this driver with three separate clients over the past two
years, and they all work. If there are no remaining concerns with the
driver, I'd like it to get merged.
Even without the driver, the clock patches (1-2) can go in at any time.
Changes from v3:
- Rebased on sunxi-next
- Added Rob's Reviewed-by for patch 3
- Fixed a crash when receiving a message on a disabled channel
- Cleaned up some comments/formatting in the driver
- Fixed #mbox-cells in sunxi-h3-h5.dtsi (patch 7)
- Removed the irqchip example (no longer relevant to the fw design)
- Added a demo/example client that uses the driver and a toy firmware
Changes from v2:
- Merge patches 1-3
- Add a comment in the code explaining the CLK_IS_CRITICAL usage
- Add a patch to mark the AR100 clocks as critical
- Use YAML for the device tree binding
- Include a not-for-merge example usage of the mailbox
Changes from v1:
- Marked message box clocks as critical instead of hacks in the driver
- 8 unidirectional channels instead of 4 bidirectional pairs
- Use per-SoC compatible strings and an A31 fallback compatible
- Dropped the mailbox framework patch
- Include DT patches for SoCs that document the message box
Samuel Holland (10):
clk: sunxi-ng: Mark msgbox clocks as critical
clk: sunxi-ng: Mark AR100 clocks as critical
dt-bindings: mailbox: Add a sunxi message box binding
mailbox: sunxi-msgbox: Add a new mailbox driver
ARM: dts: sunxi: a80: Add msgbox node
ARM: dts: sunxi: a83t: Add msgbox node
ARM: dts: sunxi: h3/h5: Add msgbox node
arm64: dts: allwinner: a64: Add msgbox node
arm64: dts: allwinner: h6: Add msgbox node
[DO NOT MERGE] drivers: firmware: msgbox demo
.../mailbox/allwinner,sunxi-msgbox.yaml | 79 +++++
arch/arm/boot/dts/sun8i-a83t.dtsi | 10 +
arch/arm/boot/dts/sun9i-a80.dtsi | 10 +
arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 +
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 34 ++
arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 24 ++
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 +
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 +-
drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 2 +-
drivers/clk/sunxi-ng/ccu-sun50i-h6.c | 3 +-
drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 3 +-
drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 3 +-
drivers/clk/sunxi-ng/ccu-sun8i-a83t.c | 3 +-
drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 3 +-
drivers/clk/sunxi-ng/ccu-sun8i-r.c | 2 +-
drivers/clk/sunxi-ng/ccu-sun9i-a80.c | 3 +-
drivers/firmware/Kconfig | 6 +
drivers/firmware/Makefile | 1 +
drivers/firmware/sunxi_msgbox_demo.c | 307 +++++++++++++++++
drivers/mailbox/Kconfig | 10 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/sunxi-msgbox.c | 323 ++++++++++++++++++
22 files changed, 842 insertions(+), 9 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sunxi-msgbox.yaml
create mode 100644 drivers/firmware/sunxi_msgbox_demo.c
create mode 100644 drivers/mailbox/sunxi-msgbox.c
--
2.21.0
On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
provide additional features, such as thermal monitoring and poweron/off
support for boards without a PMIC.
Since the AR100 may be running critical firmware, even if Linux does not
know about it or directly interact with it (all requests may go through
an intermediary interface such as PSCI), Linux must not turn off its
clock.
At this time, such power management firmware only exists for the A64 and
H5 SoCs. However, it makes sense to take care of all CCU drivers now
for consistency, and to ease the transition in the future once firmware
is ported to the other SoCs.
Leaving the clock running is safe even if no firmware is present, since
the AR100 stays in reset by default. In most cases, the AR100 clock is
kept enabled by Linux anyway, since it is the parent of all APB0 bus
peripherals. This change only prevents Linux from turning off the AR100
clock in the rare case that no peripherals are in use.
Signed-off-by: Samuel Holland <[email protected]>
---
drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 2 +-
drivers/clk/sunxi-ng/ccu-sun8i-r.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
index 45a1ed3fe674..adf907020951 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
@@ -45,7 +45,7 @@ static struct ccu_div ar100_clk = {
.hw.init = CLK_HW_INIT_PARENTS("ar100",
ar100_r_apb2_parents,
&ccu_div_ops,
- 0),
+ CLK_IS_CRITICAL),
},
};
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r.c b/drivers/clk/sunxi-ng/ccu-sun8i-r.c
index 4646fdc61053..feef4f750943 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-r.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-r.c
@@ -45,7 +45,7 @@ static struct ccu_div ar100_clk = {
.hw.init = CLK_HW_INIT_PARENTS_DATA("ar100",
ar100_parents,
&ccu_div_ops,
- 0),
+ CLK_IS_CRITICAL),
},
};
--
2.21.0
The msgbox clock is critical because the hardware it controls is shared
between Linux and system firmware. The message box may be used by the
EL3 secure monitor's PSCI implementation. On 64-bit sunxi SoCs, this is
provided by ARM TF-A; 32-bit SoCs use a different implementation. The
secure monitor uses the message box to forward requests to power
management firmware running on a separate CPU.
It is not enough for the secure monitor to enable the clock each time
Linux performs a SMC into EL3, as both the firmware and Linux can run
concurrently on separate CPUs. So it is never safe for Linux to turn
this clock off, and it should be marked as critical.
At this time, such power management firmware only exists for the A64 and
H5 SoCs. However, it makes sense to take care of all CCU drivers now
for consistency, and to ease the transition in the future once firmware
is ported to the other SoCs.
Signed-off-by: Samuel Holland <[email protected]>
---
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 ++-
drivers/clk/sunxi-ng/ccu-sun50i-h6.c | 3 ++-
drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 3 ++-
drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 3 ++-
drivers/clk/sunxi-ng/ccu-sun8i-a83t.c | 3 ++-
drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 3 ++-
drivers/clk/sunxi-ng/ccu-sun9i-a80.c | 3 ++-
7 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 49bd7a4c015c..045121b50da3 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -342,8 +342,9 @@ static SUNXI_CCU_GATE(bus_de_clk, "bus-de", "ahb1",
0x064, BIT(12), 0);
static SUNXI_CCU_GATE(bus_gpu_clk, "bus-gpu", "ahb1",
0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
static SUNXI_CCU_GATE(bus_msgbox_clk, "bus-msgbox", "ahb1",
- 0x064, BIT(21), 0);
+ 0x064, BIT(21), CLK_IS_CRITICAL);
static SUNXI_CCU_GATE(bus_spinlock_clk, "bus-spinlock", "ahb1",
0x064, BIT(22), 0);
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
index aebef4af9861..14f39bc4180f 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
@@ -340,8 +340,9 @@ static SUNXI_CCU_GATE(bus_vp9_clk, "bus-vp9", "psi-ahb1-ahb2",
static SUNXI_CCU_GATE(bus_dma_clk, "bus-dma", "psi-ahb1-ahb2",
0x70c, BIT(0), 0);
+/* Used for communication between firmware components at runtime */
static SUNXI_CCU_GATE(bus_msgbox_clk, "bus-msgbox", "psi-ahb1-ahb2",
- 0x71c, BIT(0), 0);
+ 0x71c, BIT(0), CLK_IS_CRITICAL);
static SUNXI_CCU_GATE(bus_spinlock_clk, "bus-spinlock", "psi-ahb1-ahb2",
0x72c, BIT(0), 0);
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
index 103aa504f6c8..5a28583f57e2 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
@@ -255,8 +255,9 @@ static SUNXI_CCU_GATE(bus_de_fe_clk, "bus-de-fe", "ahb1",
0x064, BIT(14), 0);
static SUNXI_CCU_GATE(bus_gpu_clk, "bus-gpu", "ahb1",
0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
static SUNXI_CCU_GATE(bus_msgbox_clk, "bus-msgbox", "ahb1",
- 0x064, BIT(21), 0);
+ 0x064, BIT(21), CLK_IS_CRITICAL);
static SUNXI_CCU_GATE(bus_spinlock_clk, "bus-spinlock", "ahb1",
0x064, BIT(22), 0);
static SUNXI_CCU_GATE(bus_drc_clk, "bus-drc", "ahb1",
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
index 91838cd11037..50cf3726ef30 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
@@ -267,8 +267,9 @@ static SUNXI_CCU_GATE(bus_de_fe_clk, "bus-de-fe", "ahb1",
0x064, BIT(14), 0);
static SUNXI_CCU_GATE(bus_gpu_clk, "bus-gpu", "ahb1",
0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
static SUNXI_CCU_GATE(bus_msgbox_clk, "bus-msgbox", "ahb1",
- 0x064, BIT(21), 0);
+ 0x064, BIT(21), CLK_IS_CRITICAL);
static SUNXI_CCU_GATE(bus_spinlock_clk, "bus-spinlock", "ahb1",
0x064, BIT(22), 0);
static SUNXI_CCU_GATE(bus_drc_clk, "bus-drc", "ahb1",
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a83t.c b/drivers/clk/sunxi-ng/ccu-sun8i-a83t.c
index 2b434521c5cc..4ab3a76f4ffa 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a83t.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a83t.c
@@ -339,8 +339,9 @@ static SUNXI_CCU_GATE(bus_de_clk, "bus-de", "ahb1",
0x064, BIT(12), 0);
static SUNXI_CCU_GATE(bus_gpu_clk, "bus-gpu", "ahb1",
0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
static SUNXI_CCU_GATE(bus_msgbox_clk, "bus-msgbox", "ahb1",
- 0x064, BIT(21), 0);
+ 0x064, BIT(21), CLK_IS_CRITICAL);
static SUNXI_CCU_GATE(bus_spinlock_clk, "bus-spinlock", "ahb1",
0x064, BIT(22), 0);
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index 6b636362379e..7429d3fe8fb7 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -273,8 +273,9 @@ static SUNXI_CCU_GATE(bus_de_clk, "bus-de", "ahb1",
0x064, BIT(12), 0);
static SUNXI_CCU_GATE(bus_gpu_clk, "bus-gpu", "ahb1",
0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
static SUNXI_CCU_GATE(bus_msgbox_clk, "bus-msgbox", "ahb1",
- 0x064, BIT(21), 0);
+ 0x064, BIT(21), CLK_IS_CRITICAL);
static SUNXI_CCU_GATE(bus_spinlock_clk, "bus-spinlock", "ahb1",
0x064, BIT(22), 0);
diff --git a/drivers/clk/sunxi-ng/ccu-sun9i-a80.c b/drivers/clk/sunxi-ng/ccu-sun9i-a80.c
index dcac1391767f..47d1d18b6f38 100644
--- a/drivers/clk/sunxi-ng/ccu-sun9i-a80.c
+++ b/drivers/clk/sunxi-ng/ccu-sun9i-a80.c
@@ -748,8 +748,9 @@ static SUNXI_CCU_GATE(bus_usb_clk, "bus-usb", "ahb1",
0x584, BIT(1), 0);
static SUNXI_CCU_GATE(bus_gmac_clk, "bus-gmac", "ahb1",
0x584, BIT(17), 0);
+/* Used for communication between firmware components at runtime */
static SUNXI_CCU_GATE(bus_msgbox_clk, "bus-msgbox", "ahb1",
- 0x584, BIT(21), 0);
+ 0x584, BIT(21), CLK_IS_CRITICAL);
static SUNXI_CCU_GATE(bus_spinlock_clk, "bus-spinlock", "ahb1",
0x584, BIT(22), 0);
static SUNXI_CCU_GATE(bus_hstimer_clk, "bus-hstimer", "ahb1",
--
2.21.0
Hi,
On Mon, Aug 19, 2019 at 10:23:03PM -0500, Samuel Holland wrote:
> On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
> firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
> provide additional features, such as thermal monitoring and poweron/off
> support for boards without a PMIC.
>
> Since the AR100 may be running critical firmware, even if Linux does not
> know about it or directly interact with it (all requests may go through
> an intermediary interface such as PSCI), Linux must not turn off its
> clock.
>
> At this time, such power management firmware only exists for the A64 and
> H5 SoCs. However, it makes sense to take care of all CCU drivers now
> for consistency, and to ease the transition in the future once firmware
> is ported to the other SoCs.
>
> Leaving the clock running is safe even if no firmware is present, since
> the AR100 stays in reset by default. In most cases, the AR100 clock is
> kept enabled by Linux anyway, since it is the parent of all APB0 bus
> peripherals. This change only prevents Linux from turning off the AR100
> clock in the rare case that no peripherals are in use.
>
> Signed-off-by: Samuel Holland <[email protected]>
So I'm not really sure where you want to go with this.
That clock is only useful where you're having a firmware running on
the AR100, and that firmware would have a device tree node of its own,
where we could list the clocks needed for the firmware to keep
running, if it ever runs. If the driver has not been compiled in /
loaded, then we don't care either.
But more fundamentally, if we're going to use SCPI, then those clocks
will not be handled by that driver anyway, but by the firmware, right?
So I'm not really sure that we should do it statically this way, and
that we should do it at all.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 8/20/19 2:11 AM, Maxime Ripard wrote:
> Hi,
>
> On Mon, Aug 19, 2019 at 10:23:03PM -0500, Samuel Holland wrote:
>> On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
>> firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
>> provide additional features, such as thermal monitoring and poweron/off
>> support for boards without a PMIC.
>>
>> Since the AR100 may be running critical firmware, even if Linux does not
>> know about it or directly interact with it (all requests may go through
>> an intermediary interface such as PSCI), Linux must not turn off its
>> clock.
This paragraph here is the key. The firmware won't necessarily have a device
tree node, and in the current design it will not, since Linux never communicates
with it directly. All communication goes through ATF via PSCI.
>> At this time, such power management firmware only exists for the A64 and
>> H5 SoCs. However, it makes sense to take care of all CCU drivers now
>> for consistency, and to ease the transition in the future once firmware
>> is ported to the other SoCs.
>>
>> Leaving the clock running is safe even if no firmware is present, since
>> the AR100 stays in reset by default. In most cases, the AR100 clock is
>> kept enabled by Linux anyway, since it is the parent of all APB0 bus
>> peripherals. This change only prevents Linux from turning off the AR100
>> clock in the rare case that no peripherals are in use.
>>
>> Signed-off-by: Samuel Holland <[email protected]>
>
> So I'm not really sure where you want to go with this.
>
> That clock is only useful where you're having a firmware running on
> the AR100, and that firmware would have a device tree node of its own,
> where we could list the clocks needed for the firmware to keep
> running, if it ever runs. If the driver has not been compiled in /
> loaded, then we don't care either.
See above. I don't expect that the firmware would have a device tree node,
because the firmware doesn't need any Linux drivers.
> But more fundamentally, if we're going to use SCPI, then those clocks
> will not be handled by that driver anyway, but by the firmware, right?
In the future, we might use SCPI clocks/sensors/regulators/etc. from Linux, but
that's not the plan at the moment. Given that it's already been two years since
I started this project, I'm trying to limit its scope so I can get at least some
part merged. The first step is to integrate a firmware that provides
suspend/resume functionality only. That firmware does implement SCPI, and if the
top-level Linux SCPI driver worked with multiple mailbox channels, it could
query the firmware's version and fetures. But all of the SCPI commands used for
suspend/resume must go through ATF via PSCI.
> So I'm not really sure that we should do it statically this way, and
> that we should do it at all.
Do you have a better way to model "firmware uses this clock behind the scenes,
so Linux please don't touch it"? It's unfortunate that we have Linux and
firmware fighting over the R_CCU, but since we didn't have firmware (e.g. SCPI
clocks) in the beginning, it's where we are today.
The AR100 clock doesn't actually have a gate, and it generally has dependencies
like R_INTC in use. So as I mentioned in the commit message, the clock will
normally be on anyway. The goal was to model the fact that there are users of
this clock that Linux doesn't/can't know about.
> Maxime
Thanks,
Samuel
On Tue, Aug 20, 2019 at 08:02:55AM -0500, Samuel Holland wrote:
> On 8/20/19 2:11 AM, Maxime Ripard wrote:
> > On Mon, Aug 19, 2019 at 10:23:03PM -0500, Samuel Holland wrote:
> >> On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
> >> firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
> >> provide additional features, such as thermal monitoring and poweron/off
> >> support for boards without a PMIC.
> >>
> >> Since the AR100 may be running critical firmware, even if Linux does not
> >> know about it or directly interact with it (all requests may go through
> >> an intermediary interface such as PSCI), Linux must not turn off its
> >> clock.
>
> This paragraph here is the key. The firmware won't necessarily have a device
> tree node, and in the current design it will not, since Linux never communicates
> with it directly. All communication goes through ATF via PSCI.
Sorry, I'm a bit lost in all those ARM firmware interfaces.
I thought SCPI was supposed to be a separate interface that had
nothing to do with PSCI?
Anyway, my main concern is that I don't really want to make exceptions
to the clock handling for everyone's usecase, and this creates a
precedent (and not even a permanent one, since eventually the plan is
to have all the clock handling happening in the firmware, right?).
There's the protected-clocks property in the DT though that will
achieve the same goal. The code to deal with it is not generic at the
moment, but it could be made that way. Would patching the DT to
protect the clock you care about, when you care about protecting them,
be an option for you?
> >> At this time, such power management firmware only exists for the A64 and
> >> H5 SoCs. However, it makes sense to take care of all CCU drivers now
> >> for consistency, and to ease the transition in the future once firmware
> >> is ported to the other SoCs.
> >>
> >> Leaving the clock running is safe even if no firmware is present, since
> >> the AR100 stays in reset by default. In most cases, the AR100 clock is
> >> kept enabled by Linux anyway, since it is the parent of all APB0 bus
> >> peripherals. This change only prevents Linux from turning off the AR100
> >> clock in the rare case that no peripherals are in use.
> >>
> >> Signed-off-by: Samuel Holland <[email protected]>
> >
> > So I'm not really sure where you want to go with this.
> >
> > That clock is only useful where you're having a firmware running on
> > the AR100, and that firmware would have a device tree node of its own,
> > where we could list the clocks needed for the firmware to keep
> > running, if it ever runs. If the driver has not been compiled in /
> > loaded, then we don't care either.
>
> See above. I don't expect that the firmware would have a device tree node,
> because the firmware doesn't need any Linux drivers.
>
> > But more fundamentally, if we're going to use SCPI, then those clocks
> > will not be handled by that driver anyway, but by the firmware, right?
>
> In the future, we might use SCPI clocks/sensors/regulators/etc. from Linux, but
> that's not the plan at the moment. Given that it's already been two years since
> I started this project, I'm trying to limit its scope so I can get at least some
> part merged. The first step is to integrate a firmware that provides
> suspend/resume functionality only. That firmware does implement SCPI, and if the
> top-level Linux SCPI driver worked with multiple mailbox channels, it could
> query the firmware's version and fetures. But all of the SCPI commands used for
> suspend/resume must go through ATF via PSCI.
I didn't know that you could / should do that with PSCI / SCPI.
> > So I'm not really sure that we should do it statically this way, and
> > that we should do it at all.
>
> Do you have a better way to model "firmware uses this clock behind the scenes,
> so Linux please don't touch it"? It's unfortunate that we have Linux and
> firmware fighting over the R_CCU, but since we didn't have firmware (e.g. SCPI
> clocks) in the beginning, it's where we are today.
>
> The AR100 clock doesn't actually have a gate, and it generally has dependencies
> like R_INTC in use. So as I mentioned in the commit message, the clock will
> normally be on anyway. The goal was to model the fact that there are users of
> this clock that Linux doesn't/can't know about.
Like I said, if that's an option, I'd prefer to have protected-clocks
work for everyone / for sunxi.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Quoting Maxime Ripard (2019-08-21 05:24:36)
> On Tue, Aug 20, 2019 at 08:02:55AM -0500, Samuel Holland wrote:
> > On 8/20/19 2:11 AM, Maxime Ripard wrote:
> > > So I'm not really sure that we should do it statically this way, and
> > > that we should do it at all.
> >
> > Do you have a better way to model "firmware uses this clock behind the scenes,
> > so Linux please don't touch it"? It's unfortunate that we have Linux and
> > firmware fighting over the R_CCU, but since we didn't have firmware (e.g. SCPI
> > clocks) in the beginning, it's where we are today.
> >
> > The AR100 clock doesn't actually have a gate, and it generally has dependencies
> > like R_INTC in use. So as I mentioned in the commit message, the clock will
> > normally be on anyway. The goal was to model the fact that there are users of
> > this clock that Linux doesn't/can't know about.
>
> Like I said, if that's an option, I'd prefer to have protected-clocks
> work for everyone / for sunxi.
>
Yes. Use protected-clocks to indicate what shouldn't be touched by the
kernel. It's not super easy to make it "generic" right now, but I
suppose we can work the flag into the core framework more so that we
still register the clks but otherwise make the 'clk_get()' operation
fail on them somehow and the disable unused operation skip them. I just
took the easy way out for qcom for the time being and didn't register
them from the driver.
Hello Samuel,
On Mon, Aug 19, 2019 at 10:23:01PM -0500, Samuel Holland wrote:
> This series adds support for the "hardware message box" in sun8i, sun9i,
> and sun50i SoCs, used for communication with the ARISC management
> processor (the platform's equivalent of the ARM SCP). The end goal is to
> use the arm_scpi driver as a client, communicating with firmware running
> on the AR100 CPU, or to use the mailbox to forward NMIs that the
> firmware picks up from R_INTC.
>
> Unfortunately, the ARM SCPI client no longer works with this driver
> since it now exposes all 8 hardware FIFOs individually. The SCPI client
> could be made to work (and I posted proof-of-concept code to that effect
> with v1 of this series), but that is a low priority, as Linux does not
> directly use SCPI with the current firmware version; all SCPI use goes
> through ATF via PSCI.
>
> As requested in the comments to v3 of this patchset, a demo client is
> provided in the final patch. This demo goes along with a toy firmware
> which shows that the driver does indeed work for two-way communication
> on all channels. To build the firmware component, run:
I've tried using this driver with mainline arm_scpi driver (which is probably
an expected future use, since crust provides SCPI interface).
The problem I've found is that arm_scpi expects message box to be
bi-directional, but this driver provides uni-directional interface.
What do you think about making this driver provide bi-directional interface?
We could halve the number of channels to 4 and mandate TX/RX configuration
(from main CPU's PoV) as ABI.
Otherwise it's impossible to use it with the arm_scpi driver.
Or do you have any other ideas? I guess arm_scpi can be fixed to add a
property that would make it possible to use single shmem with two
mailboxes, one for rx and one for tx, but making sun6i mailbox have
bi-directional interface sounds easier.
regards,
o.
> git clone https://github.com/crust-firmware/meta meta
> git clone -b mailbox-demo https://github.com/crust-firmware/crust meta/crust
> cd meta
> make
>
> That will by default produce a U-Boot + ATF + SCP firmware image in
> [meta/]build/pinebook/u-boot-sunxi-with-spl.bin. See the top-level
> README.md for more information, such as cross-compiler setup.
>
> I've now used this driver with three separate clients over the past two
> years, and they all work. If there are no remaining concerns with the
> driver, I'd like it to get merged.
>
> Even without the driver, the clock patches (1-2) can go in at any time.
>
> Changes from v3:
> - Rebased on sunxi-next
> - Added Rob's Reviewed-by for patch 3
> - Fixed a crash when receiving a message on a disabled channel
> - Cleaned up some comments/formatting in the driver
> - Fixed #mbox-cells in sunxi-h3-h5.dtsi (patch 7)
> - Removed the irqchip example (no longer relevant to the fw design)
> - Added a demo/example client that uses the driver and a toy firmware
>
> Changes from v2:
> - Merge patches 1-3
> - Add a comment in the code explaining the CLK_IS_CRITICAL usage
> - Add a patch to mark the AR100 clocks as critical
> - Use YAML for the device tree binding
> - Include a not-for-merge example usage of the mailbox
>
> Changes from v1:
> - Marked message box clocks as critical instead of hacks in the driver
> - 8 unidirectional channels instead of 4 bidirectional pairs
> - Use per-SoC compatible strings and an A31 fallback compatible
> - Dropped the mailbox framework patch
> - Include DT patches for SoCs that document the message box
>
> Samuel Holland (10):
> clk: sunxi-ng: Mark msgbox clocks as critical
> clk: sunxi-ng: Mark AR100 clocks as critical
> dt-bindings: mailbox: Add a sunxi message box binding
> mailbox: sunxi-msgbox: Add a new mailbox driver
> ARM: dts: sunxi: a80: Add msgbox node
> ARM: dts: sunxi: a83t: Add msgbox node
> ARM: dts: sunxi: h3/h5: Add msgbox node
> arm64: dts: allwinner: a64: Add msgbox node
> arm64: dts: allwinner: h6: Add msgbox node
> [DO NOT MERGE] drivers: firmware: msgbox demo
>
> .../mailbox/allwinner,sunxi-msgbox.yaml | 79 +++++
> arch/arm/boot/dts/sun8i-a83t.dtsi | 10 +
> arch/arm/boot/dts/sun9i-a80.dtsi | 10 +
> arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 +
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 34 ++
> arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 24 ++
> arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 +
> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 +-
> drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 2 +-
> drivers/clk/sunxi-ng/ccu-sun50i-h6.c | 3 +-
> drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 3 +-
> drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 3 +-
> drivers/clk/sunxi-ng/ccu-sun8i-a83t.c | 3 +-
> drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 3 +-
> drivers/clk/sunxi-ng/ccu-sun8i-r.c | 2 +-
> drivers/clk/sunxi-ng/ccu-sun9i-a80.c | 3 +-
> drivers/firmware/Kconfig | 6 +
> drivers/firmware/Makefile | 1 +
> drivers/firmware/sunxi_msgbox_demo.c | 307 +++++++++++++++++
> drivers/mailbox/Kconfig | 10 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/sunxi-msgbox.c | 323 ++++++++++++++++++
> 22 files changed, 842 insertions(+), 9 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sunxi-msgbox.yaml
> create mode 100644 drivers/firmware/sunxi_msgbox_demo.c
> create mode 100644 drivers/mailbox/sunxi-msgbox.c
>
> --
> 2.21.0
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 9/8/19 10:22 PM, Ondřej Jirman wrote:
> Hello Samuel,
>
> On Mon, Aug 19, 2019 at 10:23:01PM -0500, Samuel Holland wrote:
>> This series adds support for the "hardware message box" in sun8i, sun9i,
>> and sun50i SoCs, used for communication with the ARISC management
>> processor (the platform's equivalent of the ARM SCP). The end goal is to
>> use the arm_scpi driver as a client, communicating with firmware running
>> on the AR100 CPU, or to use the mailbox to forward NMIs that the
>> firmware picks up from R_INTC.
>>
>> Unfortunately, the ARM SCPI client no longer works with this driver
>> since it now exposes all 8 hardware FIFOs individually. The SCPI client
>> could be made to work (and I posted proof-of-concept code to that effect
>> with v1 of this series), but that is a low priority, as Linux does not
>> directly use SCPI with the current firmware version; all SCPI use goes
>> through ATF via PSCI.
>>
>> As requested in the comments to v3 of this patchset, a demo client is
>> provided in the final patch. This demo goes along with a toy firmware
>> which shows that the driver does indeed work for two-way communication
>> on all channels. To build the firmware component, run:
>
> I've tried using this driver with mainline arm_scpi driver (which is probably
> an expected future use, since crust provides SCPI interface).
If you've verified in some way that this driver works on A83T, I'd appreciate
your Tested-by, so I can send a patch for the A83T device tree node.
> The problem I've found is that arm_scpi expects message box to be
> bi-directional, but this driver provides uni-directional interface.
>
> What do you think about making this driver provide bi-directional interface?
> We could halve the number of channels to 4 and mandate TX/RX configuration
> (from main CPU's PoV) as ABI.
Funny you mention that. That's what I did originally for v1, but it got NAKed by
Maxime, Andre, and Jassi:
https://lkml.org/lkml/2018/2/28/125
https://lkml.org/lkml/2018/2/28/944
> Otherwise it's impossible to use it with the arm_scpi driver.
>
> Or do you have any other ideas? I guess arm_scpi can be fixed to add a
> property that would make it possible to use single shmem with two
> mailboxes, one for rx and one for tx, but making sun6i mailbox have
> bi-directional interface sounds easier.
Yes, you can use the existence of the mbox-names property to determine if the
driver needs one mailbox or two, as I did in this driver:
https://lkml.org/lkml/2019/3/1/789
I'll have a patch available soon that implements this for arm_scpi.
Cheers,
Samuel
> regards,
> o.
>
>> git clone https://github.com/crust-firmware/meta meta
>> git clone -b mailbox-demo https://github.com/crust-firmware/crust meta/crust
>> cd meta
>> make
>>
>> That will by default produce a U-Boot + ATF + SCP firmware image in
>> [meta/]build/pinebook/u-boot-sunxi-with-spl.bin. See the top-level
>> README.md for more information, such as cross-compiler setup.
>>
>> I've now used this driver with three separate clients over the past two
>> years, and they all work. If there are no remaining concerns with the
>> driver, I'd like it to get merged.
>>
>> Even without the driver, the clock patches (1-2) can go in at any time.
>>
>> Changes from v3:
>> - Rebased on sunxi-next
>> - Added Rob's Reviewed-by for patch 3
>> - Fixed a crash when receiving a message on a disabled channel
>> - Cleaned up some comments/formatting in the driver
>> - Fixed #mbox-cells in sunxi-h3-h5.dtsi (patch 7)
>> - Removed the irqchip example (no longer relevant to the fw design)
>> - Added a demo/example client that uses the driver and a toy firmware
>>
>> Changes from v2:
>> - Merge patches 1-3
>> - Add a comment in the code explaining the CLK_IS_CRITICAL usage
>> - Add a patch to mark the AR100 clocks as critical
>> - Use YAML for the device tree binding
>> - Include a not-for-merge example usage of the mailbox
>>
>> Changes from v1:
>> - Marked message box clocks as critical instead of hacks in the driver
>> - 8 unidirectional channels instead of 4 bidirectional pairs
>> - Use per-SoC compatible strings and an A31 fallback compatible
>> - Dropped the mailbox framework patch
>> - Include DT patches for SoCs that document the message box
>>
>> Samuel Holland (10):
>> clk: sunxi-ng: Mark msgbox clocks as critical
>> clk: sunxi-ng: Mark AR100 clocks as critical
>> dt-bindings: mailbox: Add a sunxi message box binding
>> mailbox: sunxi-msgbox: Add a new mailbox driver
>> ARM: dts: sunxi: a80: Add msgbox node
>> ARM: dts: sunxi: a83t: Add msgbox node
>> ARM: dts: sunxi: h3/h5: Add msgbox node
>> arm64: dts: allwinner: a64: Add msgbox node
>> arm64: dts: allwinner: h6: Add msgbox node
>> [DO NOT MERGE] drivers: firmware: msgbox demo
>>
>> .../mailbox/allwinner,sunxi-msgbox.yaml | 79 +++++
>> arch/arm/boot/dts/sun8i-a83t.dtsi | 10 +
>> arch/arm/boot/dts/sun9i-a80.dtsi | 10 +
>> arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 +
>> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 34 ++
>> arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 24 ++
>> arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 +
>> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 +-
>> drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 2 +-
>> drivers/clk/sunxi-ng/ccu-sun50i-h6.c | 3 +-
>> drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 3 +-
>> drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 3 +-
>> drivers/clk/sunxi-ng/ccu-sun8i-a83t.c | 3 +-
>> drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 3 +-
>> drivers/clk/sunxi-ng/ccu-sun8i-r.c | 2 +-
>> drivers/clk/sunxi-ng/ccu-sun9i-a80.c | 3 +-
>> drivers/firmware/Kconfig | 6 +
>> drivers/firmware/Makefile | 1 +
>> drivers/firmware/sunxi_msgbox_demo.c | 307 +++++++++++++++++
>> drivers/mailbox/Kconfig | 10 +
>> drivers/mailbox/Makefile | 2 +
>> drivers/mailbox/sunxi-msgbox.c | 323 ++++++++++++++++++
>> 22 files changed, 842 insertions(+), 9 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sunxi-msgbox.yaml
>> create mode 100644 drivers/firmware/sunxi_msgbox_demo.c
>> create mode 100644 drivers/mailbox/sunxi-msgbox.c
>>
>> --
>> 2.21.0
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi,
On Sun, Sep 08, 2019 at 10:54:17PM -0500, Samuel Holland wrote:
> On 9/8/19 10:22 PM, Ondřej Jirman wrote:
> > Hello Samuel,
> >
> > On Mon, Aug 19, 2019 at 10:23:01PM -0500, Samuel Holland wrote:
> >> This series adds support for the "hardware message box" in sun8i, sun9i,
> >> and sun50i SoCs, used for communication with the ARISC management
> >> processor (the platform's equivalent of the ARM SCP). The end goal is to
> >> use the arm_scpi driver as a client, communicating with firmware running
> >> on the AR100 CPU, or to use the mailbox to forward NMIs that the
> >> firmware picks up from R_INTC.
> >>
> >> Unfortunately, the ARM SCPI client no longer works with this driver
> >> since it now exposes all 8 hardware FIFOs individually. The SCPI client
> >> could be made to work (and I posted proof-of-concept code to that effect
> >> with v1 of this series), but that is a low priority, as Linux does not
> >> directly use SCPI with the current firmware version; all SCPI use goes
> >> through ATF via PSCI.
> >>
> >> As requested in the comments to v3 of this patchset, a demo client is
> >> provided in the final patch. This demo goes along with a toy firmware
> >> which shows that the driver does indeed work for two-way communication
> >> on all channels. To build the firmware component, run:
> >
> > I've tried using this driver with mainline arm_scpi driver (which is probably
> > an expected future use, since crust provides SCPI interface).
>
> If you've verified in some way that this driver works on A83T, I'd appreciate
> your Tested-by, so I can send a patch for the A83T device tree node.
Tested-by: Ondrej Jirman <[email protected]>
(on A83T)
> > The problem I've found is that arm_scpi expects message box to be
> > bi-directional, but this driver provides uni-directional interface.
> >
> > What do you think about making this driver provide bi-directional interface?
> > We could halve the number of channels to 4 and mandate TX/RX configuration
> > (from main CPU's PoV) as ABI.
>
> Funny you mention that. That's what I did originally for v1, but it got NAKed by
> Maxime, Andre, and Jassi:
>
> https://lkml.org/lkml/2018/2/28/125
> https://lkml.org/lkml/2018/2/28/944
>
> > Otherwise it's impossible to use it with the arm_scpi driver.
> >
> > Or do you have any other ideas? I guess arm_scpi can be fixed to add a
> > property that would make it possible to use single shmem with two
> > mailboxes, one for rx and one for tx, but making sun6i mailbox have
> > bi-directional interface sounds easier.
>
> Yes, you can use the existence of the mbox-names property to determine if the
> driver needs one mailbox or two, as I did in this driver:
>
> https://lkml.org/lkml/2019/3/1/789
>
> I'll have a patch available soon that implements this for arm_scpi.
Yeah, I've patched arm_scpi too. :)
https://megous.com/git/linux/commit/?h=tbs-5.3&id=69a0cd0093a63039ace2f763e8d82009c50ff03c
(but that's just for the test, because it breaks the existing interface for
other uses)
Anyway, using mbox-names looks like a nice solution! Thanks! Though,
arm_scpi driver has a bit more complicated existing interface, where it can use
multiple mailboxes and rotates through them after every message.
BTW, I'm slowly laboring through understanding how to get suspend to ram working
on one A83T tablet. https://xnux.eu/tablet-hacking/ Which is how I tested this
driver.
regards,
o.
> Cheers,
> Samuel
>
> > regards,
> > o.
> >
> >> git clone https://github.com/crust-firmware/meta meta
> >> git clone -b mailbox-demo https://github.com/crust-firmware/crust meta/crust
> >> cd meta
> >> make
> >>
> >> That will by default produce a U-Boot + ATF + SCP firmware image in
> >> [meta/]build/pinebook/u-boot-sunxi-with-spl.bin. See the top-level
> >> README.md for more information, such as cross-compiler setup.
> >>
> >> I've now used this driver with three separate clients over the past two
> >> years, and they all work. If there are no remaining concerns with the
> >> driver, I'd like it to get merged.
> >>
> >> Even without the driver, the clock patches (1-2) can go in at any time.
> >>
> >> Changes from v3:
> >> - Rebased on sunxi-next
> >> - Added Rob's Reviewed-by for patch 3
> >> - Fixed a crash when receiving a message on a disabled channel
> >> - Cleaned up some comments/formatting in the driver
> >> - Fixed #mbox-cells in sunxi-h3-h5.dtsi (patch 7)
> >> - Removed the irqchip example (no longer relevant to the fw design)
> >> - Added a demo/example client that uses the driver and a toy firmware
> >>
> >> Changes from v2:
> >> - Merge patches 1-3
> >> - Add a comment in the code explaining the CLK_IS_CRITICAL usage
> >> - Add a patch to mark the AR100 clocks as critical
> >> - Use YAML for the device tree binding
> >> - Include a not-for-merge example usage of the mailbox
> >>
> >> Changes from v1:
> >> - Marked message box clocks as critical instead of hacks in the driver
> >> - 8 unidirectional channels instead of 4 bidirectional pairs
> >> - Use per-SoC compatible strings and an A31 fallback compatible
> >> - Dropped the mailbox framework patch
> >> - Include DT patches for SoCs that document the message box
> >>
> >> Samuel Holland (10):
> >> clk: sunxi-ng: Mark msgbox clocks as critical
> >> clk: sunxi-ng: Mark AR100 clocks as critical
> >> dt-bindings: mailbox: Add a sunxi message box binding
> >> mailbox: sunxi-msgbox: Add a new mailbox driver
> >> ARM: dts: sunxi: a80: Add msgbox node
> >> ARM: dts: sunxi: a83t: Add msgbox node
> >> ARM: dts: sunxi: h3/h5: Add msgbox node
> >> arm64: dts: allwinner: a64: Add msgbox node
> >> arm64: dts: allwinner: h6: Add msgbox node
> >> [DO NOT MERGE] drivers: firmware: msgbox demo
> >>
> >> .../mailbox/allwinner,sunxi-msgbox.yaml | 79 +++++
> >> arch/arm/boot/dts/sun8i-a83t.dtsi | 10 +
> >> arch/arm/boot/dts/sun9i-a80.dtsi | 10 +
> >> arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 +
> >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 34 ++
> >> arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 24 ++
> >> arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 +
> >> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 +-
> >> drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 2 +-
> >> drivers/clk/sunxi-ng/ccu-sun50i-h6.c | 3 +-
> >> drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 3 +-
> >> drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 3 +-
> >> drivers/clk/sunxi-ng/ccu-sun8i-a83t.c | 3 +-
> >> drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 3 +-
> >> drivers/clk/sunxi-ng/ccu-sun8i-r.c | 2 +-
> >> drivers/clk/sunxi-ng/ccu-sun9i-a80.c | 3 +-
> >> drivers/firmware/Kconfig | 6 +
> >> drivers/firmware/Makefile | 1 +
> >> drivers/firmware/sunxi_msgbox_demo.c | 307 +++++++++++++++++
> >> drivers/mailbox/Kconfig | 10 +
> >> drivers/mailbox/Makefile | 2 +
> >> drivers/mailbox/sunxi-msgbox.c | 323 ++++++++++++++++++
> >> 22 files changed, 842 insertions(+), 9 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sunxi-msgbox.yaml
> >> create mode 100644 drivers/firmware/sunxi_msgbox_demo.c
> >> create mode 100644 drivers/mailbox/sunxi-msgbox.c
> >>
> >> --
> >> 2.21.0
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> [email protected]
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel