| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGb2v67-s5kygq=4fUPKA_6csiUpkLd77Mh-4dFW4WnNFb20jw@mail.gmail.com>
Date: Sat, 6 Sep 2025 00:13:33 +0800
From: Chen-Yu Tsai <wens@...nel.org>
To: Andre Przywara <andre.przywara@....com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Stephen Boyd <sboyd@...nel.org>,
Jernej Skrabec <jernej@...nel.org>, Samuel Holland <samuel@...lland.org>, linux-sunxi@...ts.linux.dev,
linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/8] clk: sunxi-ng: add support for the A523/T527 MCU CCU
On Fri, Sep 5, 2025 at 11:14 PM Andre Przywara <andre.przywara@....com> wrote:
>
> On Sun, 31 Aug 2025 01:08:59 +0800
> Chen-Yu Tsai <wens@...nel.org> wrote:
>
> Hi Chen-Yu,
>
> many thanks for this patch, I feel with you when it comes to model
> Allwinner CCUs in the kernel ;-)
>
> > From: Chen-Yu Tsai <wens@...e.org>
> >
> > The A523/T527 SoCs have a new MCU PRCM, which has more clocks and reset
> > controls for the RISC-V MCU and other peripherals. There is no visible
> > bus in this part, but there is a second audio PLL. The BSP driver uses
> > the 24MHz main oscillator as the parent for all the bus clocks.
>
> So my copy of the T527 manual (v0.92) shows the system but tree of the
> MCU_PRCM in figure 2-24, and there some peripherals are on AHB_DEC0, while
> others are on APBS0. Shall we model this correctly, then?
The figure was a bit misleading as it had at its root "CPU" and "MCU_AHB"
instead of "CPUX". I guess given that we can actually access these devices,
they should be the same. It was weird because there were no bus clock
dividers in this block.
> > Add a driver to support this part. Unlike the BSP driver, the SoC's main
> > MBUS clock is chosen as the parent for the MCU MBUS clock, and the
> > latter then serves as the parent of the MCU DMA controller's MBUS clock.
> >
> > Signed-off-by: Chen-Yu Tsai <wens@...e.org>
> > ---
> > drivers/clk/sunxi-ng/Kconfig | 5 +
> > drivers/clk/sunxi-ng/Makefile | 2 +
> > drivers/clk/sunxi-ng/ccu-sun55i-a523-mcu.c | 447 +++++++++++++++++++++
> > 3 files changed, 454 insertions(+)
> > create mode 100644 drivers/clk/sunxi-ng/ccu-sun55i-a523-mcu.c
> >
> > diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
> > index 8896fd052ef1..6af2d020e03e 100644
> > --- a/drivers/clk/sunxi-ng/Kconfig
> > +++ b/drivers/clk/sunxi-ng/Kconfig
> > @@ -57,6 +57,11 @@ config SUN55I_A523_CCU
> > default ARCH_SUNXI
> > depends on ARM64 || COMPILE_TEST
> >
> > +config SUN55I_A523_MCU_CCU
> > + tristate "Support for the Allwinner A523/T527 MCU CCU"
> > + default ARCH_SUNXI
> > + depends on ARM64 || COMPILE_TEST
> > +
> > config SUN55I_A523_R_CCU
> > tristate "Support for the Allwinner A523/T527 PRCM CCU"
> > default ARCH_SUNXI
> > diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
> > index 82e471036de6..a1c4087d7241 100644
> > --- a/drivers/clk/sunxi-ng/Makefile
> > +++ b/drivers/clk/sunxi-ng/Makefile
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_SUN50I_H6_CCU) += sun50i-h6-ccu.o
> > obj-$(CONFIG_SUN50I_H6_R_CCU) += sun50i-h6-r-ccu.o
> > obj-$(CONFIG_SUN50I_H616_CCU) += sun50i-h616-ccu.o
> > obj-$(CONFIG_SUN55I_A523_CCU) += sun55i-a523-ccu.o
> > +obj-$(CONFIG_SUN55I_A523_MCU_CCU) += sun55i-a523-mcu-ccu.o
> > obj-$(CONFIG_SUN55I_A523_R_CCU) += sun55i-a523-r-ccu.o
> > obj-$(CONFIG_SUN4I_A10_CCU) += sun4i-a10-ccu.o
> > obj-$(CONFIG_SUN5I_CCU) += sun5i-ccu.o
> > @@ -61,6 +62,7 @@ sun50i-h6-ccu-y += ccu-sun50i-h6.o
> > sun50i-h6-r-ccu-y += ccu-sun50i-h6-r.o
> > sun50i-h616-ccu-y += ccu-sun50i-h616.o
> > sun55i-a523-ccu-y += ccu-sun55i-a523.o
> > +sun55i-a523-mcu-ccu-y += ccu-sun55i-a523-mcu.o
> > sun55i-a523-r-ccu-y += ccu-sun55i-a523-r.o
> > sun4i-a10-ccu-y += ccu-sun4i-a10.o
> > sun5i-ccu-y += ccu-sun5i.o
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun55i-a523-mcu.c b/drivers/clk/sunxi-ng/ccu-sun55i-a523-mcu.c
> > new file mode 100644
> > index 000000000000..6105485837c9
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu-sun55i-a523-mcu.c
> > @@ -0,0 +1,447 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2025 Chen-Yu Tsai <wens@...e.org>
> > + *
> > + * Based on the A523 CCU driver:
> > + * Copyright (C) 2023-2024 Arm Ltd.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <dt-bindings/clock/sun55i-a523-mcu-ccu.h>
> > +#include <dt-bindings/reset/sun55i-a523-mcu-ccu.h>
> > +
> > +#include "ccu_common.h"
> > +#include "ccu_reset.h"
> > +
> > +#include "ccu_div.h"
> > +#include "ccu_gate.h"
> > +#include "ccu_mp.h"
> > +#include "ccu_mult.h"
> > +#include "ccu_nm.h"
> > +
> > +static const struct clk_parent_data osc24M[] = {
> > + { .fw_name = "hosc" }
> > +};
> > +
> > +#define SUN55I_A523_PLL_AUDIO1_REG 0x00c
> > +static struct ccu_sdm_setting pll_audio1_sdm_table[] = {
> > + { .rate = 2167603200, .pattern = 0xa000a234, .m = 1, .n = 90 }, /* div2->22.5792 */
> > + { .rate = 2359296000, .pattern = 0xa0009ba6, .m = 1, .n = 98 }, /* div2->24.576 */
> > + { .rate = 1806336000, .pattern = 0xa000872b, .m = 1, .n = 75 }, /* div5->22.576 */
> > +};
> > +
> > +static struct ccu_nm pll_audio1_clk = {
> > + .enable = BIT(27),
> > + .lock = BIT(28),
> > + .n = _SUNXI_CCU_MULT_MIN(8, 8, 11),
> > + .m = _SUNXI_CCU_DIV(1, 1),
> > + .sdm = _SUNXI_CCU_SDM(pll_audio1_sdm_table, BIT(24),
> > + 0x010, BIT(31)),
> > + .min_rate = 180000000U,
> > + .max_rate = 3500000000U,
> > + .common = {
> > + .reg = 0x00c,
> > + .features = CCU_FEATURE_SIGMA_DELTA_MOD,
> > + .hw.init = CLK_HW_INIT_PARENTS_DATA("pll-audio1",
> > + osc24M, &ccu_nm_ops,
> > + CLK_SET_RATE_GATE),
> > + },
> > +};
> > +
> > +static const struct clk_hw *pll_audio1_div_parents[] = { &pll_audio1_clk.common.hw };
> > +static CLK_FIXED_FACTOR_HWS(pll_periph1_div2_clk, "pll-audio1-div2",
> > + pll_audio1_div_parents, 2, 1,
> > + CLK_SET_RATE_PARENT);
>
> I admit that those "fixed programmable" dividers are odd, but there are
> fields in the PLL control register that we should use, so it's not a
> fixed divider clock, but a programmable divider, using
> SUNXI_CCU_M_HWS().
As you found out below, this is programmed to be fixed.
> And I think you want the struct name to contain audio1, not periph1?
Correct. Copy-paste thing.
> > +static CLK_FIXED_FACTOR_HWS(pll_periph1_div5_clk, "pll-audio1-div5",
> > + pll_audio1_div_parents, 5, 1,
> > + CLK_SET_RATE_PARENT);
>
> Same here.
>
> > +
> > +static SUNXI_CCU_M_WITH_GATE(audio_out_clk, "audio-out",
> > + "pll-audio1-div2", 0x01c,
> > + 0, 5, BIT(31), CLK_SET_RATE_PARENT);
>
> I wonder if CLK_SET_RATE_PARENT is a good idea here. IIUC, then the
> idea would be that the PLL is running at a fixed high rate (3072 MHz),
> and gets divided down here to something more audio-y, like 48 or 96 MHz?
No. For audio there are only two classes of clock rates that matter:
- multiple of 24.576 MHz for 48 KHz family of sample rates
- multiple of 22.5792 MHz for 44.1 KHz family of sample rates
The PLL has to be able to switch between these. The SDM table below
contains predefined values from the BSP.
> Do you have an idea what this clock is supposed to be used for? I
> don't see it used anywhere, neither in this series, nor in the manual's
> other clock descriptions or even pins.
I've no idea, but didn't want to leave it out and then later on someone
has to add it.
> > +
> > +static const struct clk_parent_data dsp_parents[] = {
> > + { .fw_name = "hosc" },
> > + { .fw_name = "losc" },
> > + { .fw_name = "iosc" },
> > + { .hw = &pll_periph1_div5_clk.hw },
> > + { .hw = &pll_periph1_div2_clk.hw },
>
> The manual says that parent 0b011 is the DIV2 clock, and 0b100 is the
> DIV5 clock, so those lines should be swapped.
BSP has this order, so I'm confused as you are. I can just leave a note
here saying the order is from the BSP, which is different from the manual.
Until someone actually plays with the DSP, we won't know which one is correct.
> > + { .fw_name = "dsp" },
> > +};
> > +static SUNXI_CCU_M_DATA_WITH_MUX_GATE(dsp_clk, "mcu-dsp", dsp_parents, 0x0020,
> > + 0, 5, /* M */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + 0);
> > +
> > +static const struct clk_parent_data i2s_parents[] = {
> > + { .fw_name = "pll-audio0-4x" },
> > + { .hw = &pll_periph1_div2_clk.hw },
> > + { .hw = &pll_periph1_div5_clk.hw },
> > +};
> > +
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(i2s0_clk, "i2s0", i2s_parents, 0x02c,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
>
> Same question about CLK_SET_RATE_PARENT here. Does the flag mean that any
> rate request is only to be fulfilled by the parent? Couldn't find a good
> explanation for that.
AFAIK the flag is _supposed_ to mean that during a rate change, the clk
provider is free to request a different rate from the parent, one that
is suitable for its own use. This actually depends on clk drivers correctly
implementing it in their .determine_rate callbacks though.
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(i2s1_clk, "i2s1", i2s_parents, 0x030,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(i2s2_clk, "i2s2", i2s_parents, 0x034,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(i2s3_clk, "i2s3", i2s_parents, 0x038,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +
> > +static const struct clk_parent_data i2s3_asrc_parents[] = {
> > + { .fw_name = "pll-periph0-300m" },
> > + { .hw = &pll_periph1_div2_clk.hw },
> > + { .hw = &pll_periph1_div5_clk.hw },
> > +};
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(i2s3_asrc_clk, "i2s3-asrc",
> > + i2s3_asrc_parents, 0x038,
>
> that address should be 0x03c
It might have been a "copy-paste and then forget to update it" situation.
It was around midnight when I wrote this driver.
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +
> > +static SUNXI_CCU_GATE_DATA(bus_i2s0_clk, "bus-i2s0", osc24M, 0x040, BIT(0), 0);
>
> My manual says that APBS0 is the bus clock for the I2S peripherals. I
> guess another one for the list of "clocks" in the DT binding :-(
> This applies to the other bus clocks as well, they should be either APBS0
> or AHB(_DEC0?).
OK.
> > +static SUNXI_CCU_GATE_DATA(bus_i2s1_clk, "bus-i2s1", osc24M, 0x040, BIT(1), 0);
> > +static SUNXI_CCU_GATE_DATA(bus_i2s2_clk, "bus-i2s2", osc24M, 0x040, BIT(2), 0);
> > +static SUNXI_CCU_GATE_DATA(bus_i2s3_clk, "bus-i2s3", osc24M, 0x040, BIT(3), 0);
> > +
> > +static const struct clk_parent_data audio_parents[] = {
> > + { .fw_name = "pll-audio0-4x" },
> > + { .hw = &pll_periph1_div2_clk.hw },
> > + { .hw = &pll_periph1_div5_clk.hw },
> > +};
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(spdif_tx_clk, "spdif-tx",
> > + audio_parents, 0x044,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(spdif_rx_clk, "spdif-rx",
> > + i2s3_asrc_parents, 0x048,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +
> > +static SUNXI_CCU_GATE_DATA(bus_spdif_clk, "bus-spdif", osc24M, 0x04c, BIT(0), 0);
> > +
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(dmic_clk, "dmic", audio_parents, 0x050,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +
> > +static SUNXI_CCU_GATE_DATA(bus_dmic_clk, "bus-dmic", osc24M, 0x054, BIT(0), 0);
> > +
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(audio_dac_clk, "audio-dac",
> > + audio_parents, 0x058,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +static SUNXI_CCU_DUALDIV_MUX_GATE(audio_adc_clk, "audio-adc",
> > + audio_parents, 0x05c,
> > + 0, 5, /* M */
> > + 5, 5, /* P */
> > + 24, 3, /* mux */
> > + BIT(31), /* gate */
> > + CLK_SET_RATE_PARENT);
> > +
> > +static SUNXI_CCU_GATE_DATA(bus_audio_codec_clk, "bus-audio-codec",
> > + osc24M, 0x060, BIT(0), 0);
> > +
> > +static SUNXI_CCU_GATE_DATA(bus_dsp_msgbox_clk, "bus-dsp-msgbox",
> > + osc24M, 0x068, BIT(0), 0);
> > +static SUNXI_CCU_GATE_DATA(bus_dsp_cfg_clk, "bus-dsp-cfg",
> > + osc24M, 0x06c, BIT(0), 0);
> > +
> > +static SUNXI_CCU_GATE_DATA(bus_npu_hclk, "bus-npu-hclk", osc24M, 0x070, BIT(1), 0);
> > +static SUNXI_CCU_GATE_DATA(bus_npu_aclk, "bus-npu-aclk", osc24M, 0x070, BIT(2), 0);
> > +
> > +static const struct clk_parent_data timer_parents[] = {
> > + { .fw_name = "hosc" },
> > + { .fw_name = "losc" },
> > + { .fw_name = "iosc" },
> > + { .fw_name = "r-ahb" }
> > +};
> > +static SUNXI_CCU_P_DATA_WITH_MUX_GATE(mcu_timer0_clk, "mcu-timer0", timer_parents,
> > + 0x074,
> > + 1, 3, /* P */
> > + 4, 2, /* mux */
> > + BIT(0), /* gate */
> > + 0);
> > +static SUNXI_CCU_P_DATA_WITH_MUX_GATE(mcu_timer1_clk, "mcu-timer1", timer_parents,
> > + 0x078,
> > + 1, 3, /* P */
> > + 4, 2, /* mux */
> > + BIT(0), /* gate */
> > + 0);
> > +static SUNXI_CCU_P_DATA_WITH_MUX_GATE(mcu_timer2_clk, "mcu-timer2", timer_parents,
> > + 0x07c,
> > + 1, 3, /* P */
> > + 4, 2, /* mux */
> > + BIT(0), /* gate */
> > + 0);
> > +static SUNXI_CCU_P_DATA_WITH_MUX_GATE(mcu_timer3_clk, "mcu-timer3", timer_parents,
> > + 0x080,
> > + 1, 3, /* P */
> > + 4, 2, /* mux */
> > + BIT(0), /* gate */
> > + 0);
> > +static SUNXI_CCU_P_DATA_WITH_MUX_GATE(mcu_timer4_clk, "mcu-timer4", timer_parents,
> > + 0x084,
> > + 1, 3, /* P */
> > + 4, 2, /* mux */
> > + BIT(0), /* gate */
> > + 0);
> > +static SUNXI_CCU_P_DATA_WITH_MUX_GATE(mcu_timer5_clk, "mcu-timer5", timer_parents,
> > + 0x088,
> > + 1, 3, /* P */
> > + 4, 2, /* mux */
> > + BIT(0), /* gate */
> > + 0);
> > +static SUNXI_CCU_GATE_DATA(bus_mcu_timer_clk, "bus-mcu-timer", osc24M, 0x08c, BIT(0), 0);
> > +static SUNXI_CCU_GATE_DATA(bus_mcu_dma_clk, "bus-mcu-dma", osc24M, 0x104, BIT(0), 0);
> > +static SUNXI_CCU_GATE_DATA(tzma0_clk, "tzma0", osc24M, 0x108, BIT(0), 0);
> > +static SUNXI_CCU_GATE_DATA(tzma1_clk, "tzma1", osc24M, 0x10c, BIT(0), 0);
>
> Where did you find those two? I guess in the BSP code? Can you maybe add a
> comment about it then?
OK.
> > +static SUNXI_CCU_GATE_DATA(bus_pubsram_clk, "bus-pubsram", osc24M, 0x114, BIT(0), 0);
> > +
> > +/*
> > + * user manual has "mbus" clock as parent of both clocks below,
> > + * but this makes more sense, since BSP MCU DMA controller has
> > + * reference to both of them, likely needing both enabled.
> > + */
> > +static SUNXI_CCU_GATE_FW(mbus_mcu_clk, "mbus-mcu", "mbus", 0x11c, BIT(1), 0);
> > +static SUNXI_CCU_GATE_HW(mbus_mcu_dma_clk, "mbus-mcu-dma",
> > + &mbus_mcu_clk.common.hw, 0x11c, BIT(0), 0);
> > +
> > +static const struct clk_parent_data riscv_pwm_parents[] = {
>
> Where does the pwm part come from? Is the clock below actually the RISC-V
> PWM clock? Which would make more sense, since I don't see a PLL or any
> other fast clock in there.
I'm reusing the parent list here, since the list is the same for the riscv
mod clock and the pwm0 mod clock, hence the name has riscv + pwm.
> > + { .fw_name = "hosc" },
> > + { .fw_name = "losc" },
> > + { .fw_name = "iosc" },
> > +};
> > +
> > +static SUNXI_CCU_MUX_DATA_WITH_GATE(riscv_clk, "riscv",
>
> Related to above: what RISC-V clock is this exactly? Is that some PWM
> clock source, as the parents name suggests, or the main clock, which looks
> rather slow then? Or is that to select the root of the RISC-V clock tree?
>From https://github.com/radxa/allwinner-bsp/blob/product-t527-linux/configs/linux-5.15/sun55iw3p1.dtsi#L2981
it looks like it is the actual clock for the risc-v core. Given that it
is supposed to be a low power standby thing, I guess Allwinner didn't
think it was necessary to have anything faster.
> > + riscv_pwm_parents, 0x120,
> > + 27, 3, BIT(31), 0);
> > +static SUNXI_CCU_GATE_DATA(bus_riscv_cfg_clk, "bus-riscv-cfg", osc24M,
> > + 0x124, BIT(0), 0);
> > +static SUNXI_CCU_GATE_DATA(bus_riscv_msgbox_clk, "bus-riscv-msgbox", osc24M,
> > + 0x128, BIT(0), 0);
> > +
> > +static SUNXI_CCU_MUX_DATA_WITH_GATE(mcu_pwmmcu0_clk, "mcu-pwm0",
> > + riscv_pwm_parents, 0x130,
> > + 27, 3, BIT(31), 0);
>
> The mux fields for this clock start at bit 24.
Oops.
> > +static SUNXI_CCU_GATE_DATA(bus_mcu_pwm0_clk, "bus-mcu-pwm0", osc24M,
> > + 0x128, BIT(0), 0);
>
> The register offset is 0x134.
(facepalm)
> > +
> > +/*
> > + * Contains all clocks that are controlled by a hardware register. They
> > + * have a (sunxi) .common member, which needs to be initialised by the common
> > + * sunxi CCU code, to be filled with the MMIO base address and the shared lock.
> > + */
> > +static struct ccu_common *sun55i_a523_mcu_ccu_clks[] = {
> > + &pll_audio1_clk.common,
> > + &audio_out_clk.common,
> > + &dsp_clk.common,
> > + &i2s0_clk.common,
> > + &i2s1_clk.common,
> > + &i2s2_clk.common,
> > + &i2s3_clk.common,
> > + &i2s3_asrc_clk.common,
> > + &bus_i2s0_clk.common,
> > + &bus_i2s1_clk.common,
> > + &bus_i2s2_clk.common,
> > + &bus_i2s3_clk.common,
> > + &spdif_tx_clk.common,
> > + &spdif_rx_clk.common,
> > + &bus_spdif_clk.common,
> > + &dmic_clk.common,
> > + &bus_dmic_clk.common,
> > + &audio_dac_clk.common,
> > + &audio_adc_clk.common,
> > + &bus_audio_codec_clk.common,
> > + &bus_dsp_msgbox_clk.common,
> > + &bus_dsp_cfg_clk.common,
> > + &bus_npu_aclk.common,
> > + &bus_npu_hclk.common,
> > + &mcu_timer0_clk.common,
> > + &mcu_timer1_clk.common,
> > + &mcu_timer2_clk.common,
> > + &mcu_timer3_clk.common,
> > + &mcu_timer4_clk.common,
> > + &mcu_timer5_clk.common,
> > + &bus_mcu_timer_clk.common,
> > + &bus_mcu_dma_clk.common,
> > + &tzma0_clk.common,
> > + &tzma1_clk.common,
> > + &bus_pubsram_clk.common,
> > + &mbus_mcu_dma_clk.common,
> > + &mbus_mcu_clk.common,
> > + &riscv_clk.common,
> > + &bus_riscv_cfg_clk.common,
> > + &bus_riscv_msgbox_clk.common,
> > + &mcu_pwm0_clk.common,
> > + &bus_mcu_pwm0_clk.common,
> > +};
> > +
> > +static struct clk_hw_onecell_data sun55i_a523_mcu_hw_clks = {
> > + .num = CLK_BUS_MCU_PWM0 + 1,
>
> like of the NPU patch, can we use ".num = NUM_CLOCKS," here, and
> #define NUM_CLOCKS at the beginning of the file, right after including the
> binding headers?
> Or alternatively, put .num after the .hws definitions, so that last clock
> and number are closer together?
As mentioned in that patch, I prefer the latter.
> > + .hws = {
> > + [CLK_MCU_PLL_AUDIO1] = &pll_audio1_clk.common.hw,
> > + [CLK_MCU_PLL_AUDIO1_DIV2] = &pll_periph1_div2_clk.hw,
> > + [CLK_MCU_PLL_AUDIO1_DIV5] = &pll_periph1_div5_clk.hw,
> > + [CLK_MCU_AUDIO_OUT] = &audio_out_clk.common.hw,
> > + [CLK_MCU_DSP] = &dsp_clk.common.hw,
> > + [CLK_MCU_I2S0] = &i2s0_clk.common.hw,
> > + [CLK_MCU_I2S1] = &i2s1_clk.common.hw,
> > + [CLK_MCU_I2S2] = &i2s2_clk.common.hw,
> > + [CLK_MCU_I2S3] = &i2s3_clk.common.hw,
> > + [CLK_MCU_I2S3_ASRC] = &i2s3_asrc_clk.common.hw,
> > + [CLK_BUS_MCU_I2S0] = &bus_i2s0_clk.common.hw,
> > + [CLK_BUS_MCU_I2S1] = &bus_i2s1_clk.common.hw,
> > + [CLK_BUS_MCU_I2S2] = &bus_i2s2_clk.common.hw,
> > + [CLK_BUS_MCU_I2S3] = &bus_i2s3_clk.common.hw,
> > + [CLK_MCU_SPDIF_TX] = &spdif_tx_clk.common.hw,
> > + [CLK_MCU_SPDIF_RX] = &spdif_rx_clk.common.hw,
> > + [CLK_BUS_MCU_SPDIF] = &bus_spdif_clk.common.hw,
> > + [CLK_MCU_DMIC] = &dmic_clk.common.hw,
> > + [CLK_BUS_MCU_DMIC] = &bus_dmic_clk.common.hw,
> > + [CLK_MCU_AUDIO_CODEC_DAC] = &audio_dac_clk.common.hw,
> > + [CLK_MCU_AUDIO_CODEC_ADC] = &audio_adc_clk.common.hw,
> > + [CLK_BUS_MCU_AUDIO_CODEC] = &bus_audio_codec_clk.common.hw,
> > + [CLK_BUS_MCU_DSP_MSGBOX] = &bus_dsp_msgbox_clk.common.hw,
> > + [CLK_BUS_MCU_DSP_CFG] = &bus_dsp_cfg_clk.common.hw,
> > + [CLK_BUS_MCU_NPU_HCLK] = &bus_npu_hclk.common.hw,
> > + [CLK_BUS_MCU_NPU_ACLK] = &bus_npu_aclk.common.hw,
> > + [CLK_MCU_TIMER0] = &mcu_timer0_clk.common.hw,
> > + [CLK_MCU_TIMER1] = &mcu_timer1_clk.common.hw,
> > + [CLK_MCU_TIMER2] = &mcu_timer2_clk.common.hw,
> > + [CLK_MCU_TIMER3] = &mcu_timer3_clk.common.hw,
> > + [CLK_MCU_TIMER4] = &mcu_timer4_clk.common.hw,
> > + [CLK_MCU_TIMER5] = &mcu_timer5_clk.common.hw,
> > + [CLK_BUS_MCU_TIMER] = &bus_mcu_timer_clk.common.hw,
> > + [CLK_BUS_MCU_DMA] = &bus_mcu_dma_clk.common.hw,
> > + [CLK_MCU_TZMA0] = &tzma0_clk.common.hw,
> > + [CLK_MCU_TZMA1] = &tzma1_clk.common.hw,
> > + [CLK_BUS_MCU_PUBSRAM] = &bus_pubsram_clk.common.hw,
> > + [CLK_MCU_MBUS_DMA] = &mbus_mcu_dma_clk.common.hw,
> > + [CLK_MCU_MBUS] = &mbus_mcu_clk.common.hw,
> > + [CLK_MCU_RISCV] = &riscv_clk.common.hw,
> > + [CLK_BUS_MCU_RISCV_CFG] = &bus_riscv_cfg_clk.common.hw,
> > + [CLK_BUS_MCU_RISCV_MSGBOX] = &bus_riscv_msgbox_clk.common.hw,
> > + [CLK_MCU_PWM0] = &mcu_pwm0_clk.common.hw,
> > + [CLK_BUS_MCU_PWM0] = &bus_mcu_pwm0_clk.common.hw,
> > + },
> > +};
> > +
> > +static struct ccu_reset_map sun55i_a523_mcu_ccu_resets[] = {
> > + [RST_BUS_MCU_I2S0] = { 0x0040, BIT(16) },
> > + [RST_BUS_MCU_I2S1] = { 0x0040, BIT(17) },
> > + [RST_BUS_MCU_I2S2] = { 0x0040, BIT(18) },
> > + [RST_BUS_MCU_I2S3] = { 0x0040, BIT(19) },
> > + [RST_BUS_MCU_SPDIF] = { 0x004c, BIT(16) },
> > + [RST_BUS_MCU_DMIC] = { 0x0054, BIT(16) },
> > + [RST_BUS_MCU_AUDIO_CODEC] = { 0x0060, BIT(16) },
> > + [RST_BUS_MCU_DSP_MSGBOX] = { 0x0068, BIT(16) },
> > + [RST_BUS_MCU_DSP_CFG] = { 0x006c, BIT(16) },
> > + [RST_BUS_MCU_NPU] = { 0x0070, BIT(16) },
> > + [RST_BUS_MCU_TIMER] = { 0x008c, BIT(16) },
> > + [RST_BUS_MCU_DSP_DEBUG] = { 0x0100, BIT(16) },
> > + [RST_BUS_MCU_DSP] = { 0x0100, BIT(17) },
>
> I don't see those two in the manual.
>
> > + [RST_BUS_MCU_DMA] = { 0x0104, BIT(16) },
> > + [RST_BUS_MCU_PUBSRAM] = { 0x0114, BIT(16) },
>
> The manual shows a reset bit in register 0x120, in the same register as
> this ominous riscv_clk above.
Hmm. I don't see this in the T527 manual (0.92), nor the A523 manuals
(both 1.1 and 1.4), nor the BSP code.
> > + [RST_BUS_MCU_RISCV_CFG] = { 0x0124, BIT(16) },
> > + [RST_BUS_MCU_RISCV_DEBUG] = { 0x0124, BIT(17) },
> > + [RST_BUS_MCU_RISCV_CORE] = { 0x0124, BIT(18) },
> > + [RST_BUS_MCU_RISCV_MSGBOX] = { 0x0128, BIT(16) },
>
> There is a reset bit for the PWM0 clock in the manual, register 0x130,
> same as the mcu_pwmmcu0_clk above.
I don't see this one either.
> > + [RST_BUS_MCU_PWM0] = { 0x0134, BIT(16) },
>
> Verified that all of the names defined in the binding headers appear here,
> and all definitions here are mentioned in the binding headers.
>
> > +};
> > +
> > +static const struct sunxi_ccu_desc sun55i_a523_mcu_ccu_desc = {
> > + .ccu_clks = sun55i_a523_mcu_ccu_clks,
> > + .num_ccu_clks = ARRAY_SIZE(sun55i_a523_mcu_ccu_clks),
> > +
> > + .hw_clks = &sun55i_a523_mcu_hw_clks,
> > +
> > + .resets = sun55i_a523_mcu_ccu_resets,
> > + .num_resets = ARRAY_SIZE(sun55i_a523_mcu_ccu_resets),
> > +};
> > +
> > +static int sun55i_a523_mcu_ccu_probe(struct platform_device *pdev)
> > +{
> > + void __iomem *reg;
> > + u32 val;
> > + int ret;
> > +
> > + reg = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(reg))
> > + return PTR_ERR(reg);
> > +
> > + val = readl(reg + SUN55I_A523_PLL_AUDIO1_REG);
> > +
> > + /*
> > + * The PLL clock code does not model all bits, for instance it does
> > + * not support a separate enable and gate bit. We present the
> > + * gate bit(27) as the enable bit, but then have to set the
> > + * PLL Enable, LDO Enable, and Lock Enable bits on all PLLs here.
> > + */
> > + val |= BIT(31) | BIT(30) | BIT(29);
> > +
> > + /* Enforce p1 = 5, p0 = 2 (the default) for PLL_AUDIO1 */
> > + val &= ~(GENMASK(22, 20) | GENMASK(18, 16));
> > + val |= (4 << 20) | (1 << 16);
>
> Ah, I see, here you set those fixed dividers. I still think we should
> model them properly, as suggested above.
I'm not sure why it's designed this way, especially having a value that
is only close but not exactly 22.5792 MHz on the 5-divider.
Nevertheless, since the outputs are named this way, I think we should
just follow it. It causes less confusion. I can add a comment above
the PLL part.
Thanks
ChenYu
> Cheers,
> Andre
>
> > +
> > + writel(val, reg + SUN55I_A523_PLL_AUDIO1_REG);
> > +
> > + ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun55i_a523_mcu_ccu_desc);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id sun55i_a523_mcu_ccu_ids[] = {
> > + { .compatible = "allwinner,sun55i-a523-mcu-ccu" },
> > + { }
> > +};
> > +
> > +static struct platform_driver sun55i_a523_mcu_ccu_driver = {
> > + .probe = sun55i_a523_mcu_ccu_probe,
> > + .driver = {
> > + .name = "sun55i-a523-mcu-ccu",
> > + .suppress_bind_attrs = true,
> > + .of_match_table = sun55i_a523_mcu_ccu_ids,
> > + },
> > +};
> > +module_platform_driver(sun55i_a523_mcu_ccu_driver);
> > +
> > +MODULE_IMPORT_NS("SUNXI_CCU");
> > +MODULE_DESCRIPTION("Support for the Allwinner A523 MCU CCU");
> > +MODULE_LICENSE("GPL");
>
Powered by blists - more mailing lists