Re: [PATCH v5 7/8] clk: sunxi-ng: a733: Add bus clock gates

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 10/5/26 06:41, Junhui Liu wrote:
> Hi Chen-Yu,
> 
> On Sun Oct 4, 2026 at 7:03 PM CST, Chen-Yu Tsai wrote:
>> On Tue, Sep 29, 2026 at 7:28 PM Junhui Liu <junhui.liu@pigmoral.tech> wrote:
>>>
>>> Add the bus clock gates that control access to the devices' register
>>> interface on the Allwinner A733 SoC. These clocks are typically
>>> single-bit controls in the BGR registers, covering UARTs, SPI, I2C, and
>>> various multimedia engines. It also includes bus gates for system
>>> components like the IOMMU and MSI-lite interfaces.
>>>
>>> Also mark the ahb-store, mbus-store and ahb-cpus clocks as critical.
>>> Disabling either store gate breaks access to boot/storage devices such
>>> as MMC and SPI NOR, while gating ahb-cpus hangs any access to the CPUS
>>> power domain registers when unused clocks are disabled.
>>>
>>> Tested-by: Jerome Brunet <jbrunet@baylibre.com>
>>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
>>> ---
>>>   drivers/clk/sunxi-ng/ccu-sun60i-a733.c | 492 ++++++++++++++++++++++++++++++++-
>>>   1 file changed, 491 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
>>> index 1b2d4d36ec4c..b077e5d2f32c 100644
>>> --- a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
>>> +++ b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
>>> @@ -4,6 +4,10 @@
>>>    * Copyright (C) 2026 Junhui Liu <junhui.liu@pigmoral.tech>
>>>    * Based on the A523 CCU driver:
>>>    *   Copyright (C) 2023-2024 Arm Ltd.
>>> + *
>>> + * TODO: The real parents of some bus gates, including its-pcie0-aclk,
>>> + * msi-lite, npu, ufs, sgpio, lpc and i2spcm, are not documented in the
>>> + * manual. For now, follow the vendor BSP, which keeps them on hosc.
>>>    */
>>
>> [...]
>>
>>> @@ -510,9 +522,118 @@ static SUNXI_CCU_M_DATA_WITH_MUX_GATE_FEAT(mbus_clk, "mbus", mbus_parents, 0x588
>>>                                             BIT(31),     /* gate */
>>>                                             CLK_IS_CRITICAL,
>>>                                             CCU_FEATURE_UPDATE_BIT);
>>> +static const struct clk_hw *mbus_hws[] = { &mbus_clk.common.hw };
>>> +
>>> +static SUNXI_CCU_GATE_HWS(mbus_iommu0_sys_clk, "mbus-iommu0-sys", mbus_hws, 0x58c, BIT(0), 0);
>>> +static SUNXI_CCU_GATE_HWS(apb_iommu0_sys_clk, "apb-iommu0-sys", apb0_hws, 0x58c, BIT(1), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_iommu0_sys_clk, "ahb-iommu0-sys", ahb_hws, 0x58c, BIT(2), 0);
>>> +
>>> +static SUNXI_CCU_GATE_DATA(bus_msi_lite0_clk, "bus-msi-lite0", hosc, 0x594, BIT(0), 0);
>>> +static SUNXI_CCU_GATE_DATA(bus_msi_lite1_clk, "bus-msi-lite1", hosc, 0x59c, BIT(0), 0);
>>> +static SUNXI_CCU_GATE_DATA(bus_msi_lite2_clk, "bus-msi-lite2", hosc, 0x5a4, BIT(0), 0);
>>> +
>>> +static SUNXI_CCU_GATE_HWS(mbus_iommu1_sys_clk, "mbus-iommu1-sys", mbus_hws, 0x5b4, BIT(0), 0);
>>> +static SUNXI_CCU_GATE_HWS(apb_iommu1_sys_clk, "apb-iommu1-sys", apb0_hws, 0x5b4, BIT(1), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_iommu1_sys_clk, "ahb-iommu1-sys", ahb_hws, 0x5b4, BIT(2), 0);
>>
>>> +static SUNXI_CCU_GATE_HWS(ahb_ve_dec_clk, "ahb-ve-dec", ahb_hws,
>>> +                         0x5c0, BIT(0), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_ve_enc_clk, "ahb-ve-enc", ahb_hws,
>>> +                         0x5c0, BIT(1), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_vid_in_clk, "ahb-vid-in", ahb_hws,
>>> +                         0x5c0, BIT(2), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_vid_cout0_clk, "ahb-vid-cout0", ahb_hws,
>>> +                         0x5c0, BIT(3), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_vid_cout1_clk, "ahb-vid-cout1", ahb_hws,
>>> +                         0x5c0, BIT(4), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_de_clk, "ahb-de", ahb_hws,
>>> +                         0x5c0, BIT(5), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_npu_clk, "ahb-npu", ahb_hws,
>>> +                         0x5c0, BIT(6), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_gpu0_clk, "ahb-gpu0", ahb_hws,
>>> +                         0x5c0, BIT(7), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_serdes_clk, "ahb-serdes", ahb_hws,
>>> +                         0x5c0, BIT(8), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_usb_sys_clk, "ahb-usb-sys", ahb_hws,
>>> +                         0x5c0, BIT(9), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_msi_lite0_clk, "ahb-msi-lite0", ahb_hws,
>>> +                         0x5c0, BIT(16), 0);
>>> +static SUNXI_CCU_GATE_HWS(ahb_store_clk, "ahb-store", ahb_hws,
>>> +                         0x5c0, BIT(24), CLK_IS_CRITICAL);
>>> +static SUNXI_CCU_GATE_HWS(ahb_cpus_clk, "ahb-cpus", ahb_hws,
>>> +                         0x5c0, BIT(28), CLK_IS_CRITICAL);
>>
>> I suspect these refer to a AHB-AHB bridge for the various subsystems shown
>> in the memory map. That would explain why when the "ahb-store" clock is
>> gated, the storage bits stop working.
>>
>> If that's the case, we should describe it as the parent of each storage
>> peripheral bus gate's parent. Same would go for the other types. I haven't
>> checked what the BSP does though.
> 
> As Norman mentioned in his reply, the vendor BSP also does not model
> this hierarchy and instead models these gates as independent clocks
> parented to the oscillator. The manual does not describe the hierarchy
> either, so I don't think we have enough information to model it
> differently at this point.

Well, I wouldn't use the BSP as a good example on how to model the clock 
tree, they have been very misguided in the past.
What the BSP does is to add just more input/gate MBUS clocks for each 
device, which is something I think we should avoid.
To that regard using them as parent clocks for the existing gates, as 
Chen-Yu suggested, sounds like a good idea: We keep a single clock for 
the devices, but still can turn both clocks off (I think, not tested).

Cheers,
Andre

> I will add comments explaining why ahb_store_clk, mbus_store_clk, and
> ahb_cpus_clk are marked critical.
> 
>>
>>> +static SUNXI_CCU_GATE_HWS(mbus_iommu0_clk, "mbus-iommu0", mbus_hws,
>>> +                         0x5e0, BIT(0), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_iommu1_clk, "mbus-iommu1", mbus_hws,
>>> +                         0x5e0, BIT(1), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_desys_clk, "mbus-desys", mbus_hws,
>>> +                         0x5e0, BIT(11), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_ve_enc0_gate_clk, "mbus-ve-enc0-gate", mbus_hws,
>>> +                         0x5e0, BIT(12), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_ve_dec0_gate_clk, "mbus-ve-dec0-gate", mbus_hws,
>>> +                         0x5e0, BIT(14), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_gpu0_clk, "mbus-gpu0", mbus_hws,
>>> +                         0x5e0, BIT(16), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_npu_clk, "mbus-npu", mbus_hws,
>>> +                         0x5e0, BIT(18), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_vid_in_clk, "mbus-vid-in", mbus_hws,
>>> +                         0x5e0, BIT(24), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_serdes_clk, "mbus-serdes", mbus_hws,
>>> +                         0x5e0, BIT(28), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_msi_lite0_clk, "mbus-msi-lite0", mbus_hws,
>>> +                         0x5e0, BIT(29), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_store_clk, "mbus-store", mbus_hws,
>>> +                         0x5e0, BIT(30), CLK_IS_CRITICAL);
>>
>> Same thing for the MBUS clocks.
>>
>>> +static SUNXI_CCU_GATE_HWS(mbus_msi_lite2_clk, "mbus-msi-lite2", mbus_hws,
>>> +                         0x5e0, BIT(31), 0);
>>> +
>>> +static SUNXI_CCU_GATE_HWS(mbus_dma0_clk, "mbus-dma0", mbus_hws,
>>> +                         0x5e4, BIT(0), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_ve_enc0_clk, "mbus-ve-enc0", mbus_hws,
>>> +                         0x5e4, BIT(1), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_ce_clk, "mbus-ce", mbus_hws,
>>> +                         0x5e4, BIT(2), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_dma1_clk, "mbus-dma1", mbus_hws,
>>> +                         0x5e4, BIT(3), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_nand_clk, "mbus-nand", mbus_hws,
>>> +                         0x5e4, BIT(5), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_csi_clk, "mbus-csi", mbus_hws,
>>> +                         0x5e4, BIT(8), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_isp_clk, "mbus-isp", mbus_hws,
>>> +                         0x5e4, BIT(9), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_gmac0_clk, "mbus-gmac0", mbus_hws,
>>> +                         0x5e4, BIT(11), 0);
>>> +/* Undocumented, taken from the vendor kernel. */
>>> +static SUNXI_CCU_GATE_HWS(mbus_gmac1_clk, "mbus-gmac1", mbus_hws,
>>> +                         0x5e4, BIT(12), 0);
>>> +static SUNXI_CCU_GATE_HWS(mbus_ve_dec0_clk, "mbus-ve-dec0", mbus_hws,
>>> +                         0x5e4, BIT(18), 0);
>>
>> [...]
>>
>> The remaining definitions look correct (minus the vendor BSP ones which I
>> couldn't check at this time).
>>
>> I didn't check the clock list additions.
>>
>> Consider this
>>
>> Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
>>
> 
> Thanks!
> 
>>
>> We can change the AHB / MBUS clock parents later if we do figure it out.
>>
>>
>> ChenYu
>