Re: [PATCH v4 15/27] clk: mediatek: Add MT8196 ufssys clock support

[Laura Nao <laura.nao@collabora.com>]

On 8/15/25 05:50, Chen-Yu Tsai wrote:
> On Tue, Aug 5, 2025 at 10:55 PM Laura Nao <laura.nao@collabora.com> wrote:
>>
>> Add support for the MT8196 ufssys clock controller, which provides clock
>> gate control for UFS.
>>
>> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>> Co-developed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> Signed-off-by: Laura Nao <laura.nao@collabora.com>
>> ---
>>  drivers/clk/mediatek/Kconfig             |   7 ++
>>  drivers/clk/mediatek/Makefile            |   1 +
>>  drivers/clk/mediatek/clk-mt8196-ufs_ao.c | 109 +++++++++++++++++++++++
>>  3 files changed, 117 insertions(+)
>>  create mode 100644 drivers/clk/mediatek/clk-mt8196-ufs_ao.c
>>
>> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
>> index 1e0c6f177ecd..d99c39a7f10e 100644
>> --- a/drivers/clk/mediatek/Kconfig
>> +++ b/drivers/clk/mediatek/Kconfig
>> @@ -1010,6 +1010,13 @@ config COMMON_CLK_MT8196
>>         help
>>           This driver supports MediaTek MT8196 basic clocks.
>>
>> +config COMMON_CLK_MT8196_UFSSYS
>> +       tristate "Clock driver for MediaTek MT8196 ufssys"
>> +       depends on COMMON_CLK_MT8196
>> +       default COMMON_CLK_MT8196
>> +       help
>> +         This driver supports MediaTek MT8196 ufssys clocks.
>> +
>>  config COMMON_CLK_MT8365
>>         tristate "Clock driver for MediaTek MT8365"
>>         depends on ARCH_MEDIATEK || COMPILE_TEST
>> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
>> index 8888ffd3d7ba..1a497de00846 100644
>> --- a/drivers/clk/mediatek/Makefile
>> +++ b/drivers/clk/mediatek/Makefile
>> @@ -153,6 +153,7 @@ obj-$(CONFIG_COMMON_CLK_MT8195_WPESYS) += clk-mt8195-wpe.o
>>  obj-$(CONFIG_COMMON_CLK_MT8196) += clk-mt8196-apmixedsys.o clk-mt8196-topckgen.o \
>>                                    clk-mt8196-topckgen2.o clk-mt8196-vlpckgen.o \
>>                                    clk-mt8196-peri_ao.o
>> +obj-$(CONFIG_COMMON_CLK_MT8196_UFSSYS) += clk-mt8196-ufs_ao.o
>>  obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-mt8365.o
>>  obj-$(CONFIG_COMMON_CLK_MT8365_APU) += clk-mt8365-apu.o
>>  obj-$(CONFIG_COMMON_CLK_MT8365_CAM) += clk-mt8365-cam.o
>> diff --git a/drivers/clk/mediatek/clk-mt8196-ufs_ao.c b/drivers/clk/mediatek/clk-mt8196-ufs_ao.c
>> new file mode 100644
>> index 000000000000..858706b3ba6f
>> --- /dev/null
>> +++ b/drivers/clk/mediatek/clk-mt8196-ufs_ao.c
>> @@ -0,0 +1,109 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2025 MediaTek Inc.
>> + *                    Guangjie Song <guangjie.song@mediatek.com>
>> + * Copyright (c) 2025 Collabora Ltd.
>> + *                    Laura Nao <laura.nao@collabora.com>
>> + */
>> +#include <dt-bindings/clock/mediatek,mt8196-clock.h>
>> +#include <dt-bindings/reset/mediatek,mt8196-resets.h>
>
> Nit: add empty line here for separation.
>

Ack.

>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "clk-gate.h"
>> +#include "clk-mtk.h"
>> +
>> +#define MT8196_UFSAO_RST0_SET_OFFSET   0x48
>> +#define MT8196_UFSAO_RST1_SET_OFFSET   0x148
>> +
>> +static const struct mtk_gate_regs ufsao0_cg_regs = {
>> +       .set_ofs = 0x108,
>> +       .clr_ofs = 0x10c,
>> +       .sta_ofs = 0x104,
>> +};
>> +
>> +static const struct mtk_gate_regs ufsao1_cg_regs = {
>> +       .set_ofs = 0x8,
>> +       .clr_ofs = 0xc,
>> +       .sta_ofs = 0x4,
>> +};
>> +
>> +#define GATE_UFSAO0(_id, _name, _parent, _shift) {     \
>> +               .id = _id,                              \
>> +               .name = _name,                          \
>> +               .parent_name = _parent,                 \
>> +               .regs = &ufsao0_cg_regs,                \
>> +               .shift = _shift,                        \
>> +               .flags = CLK_OPS_PARENT_ENABLE,         \
>
> This probably doesn't work correctly, since not every clock defined
> below has the "ufs" clock as its parent. If the requirement is that
> the "ufs" clock be enabled for accessing this register, it is going
> to fail (badly).
>

Thanks for pointing this out - I missed this because, as you noted,
other drivers using this flag defined gates with the same parent and
worked correctly (i.e., I2C). I don’t have a way to justify keeping this
flag at the moment, and I can’t test whether the ufs clock is required
for register access at this stage of upstream support (same goes for the
other drivers with this issue).

I’m thinking of removing it here and in the other affected drivers, and
revisit the issue once we have actual users for these clocks upstream.

Best,

Laura

> ChenYu
>
>> +               .ops = &mtk_clk_gate_ops_setclr,        \
>> +       }
>> +
>> +#define GATE_UFSAO1(_id, _name, _parent, _shift) {     \
>> +               .id = _id,                              \
>> +               .name = _name,                          \
>> +               .parent_name = _parent,                 \
>> +               .regs = &ufsao1_cg_regs,                \
>> +               .shift = _shift,                        \
>> +               .flags = CLK_OPS_PARENT_ENABLE,         \
>> +               .ops = &mtk_clk_gate_ops_setclr,        \
>> +       }
>> +
>> +static const struct mtk_gate ufsao_clks[] = {
>> +       /* UFSAO0 */
>> +       GATE_UFSAO0(CLK_UFSAO_UFSHCI_UFS, "ufsao_ufshci_ufs", "ufs", 0),
>> +       GATE_UFSAO0(CLK_UFSAO_UFSHCI_AES, "ufsao_ufshci_aes", "aes_ufsfde", 1),
>> +       /* UFSAO1 */
>> +       GATE_UFSAO1(CLK_UFSAO_UNIPRO_TX_SYM, "ufsao_unipro_tx_sym", "clk26m", 0),
>> +       GATE_UFSAO1(CLK_UFSAO_UNIPRO_RX_SYM0, "ufsao_unipro_rx_sym0", "clk26m", 1),
>> +       GATE_UFSAO1(CLK_UFSAO_UNIPRO_RX_SYM1, "ufsao_unipro_rx_sym1", "clk26m", 2),
>> +       GATE_UFSAO1(CLK_UFSAO_UNIPRO_SYS, "ufsao_unipro_sys", "ufs", 3),
>> +       GATE_UFSAO1(CLK_UFSAO_UNIPRO_SAP, "ufsao_unipro_sap", "clk26m", 4),
>> +       GATE_UFSAO1(CLK_UFSAO_PHY_SAP, "ufsao_phy_sap", "clk26m", 8),
>> +};
>> +
>> +static u16 ufsao_rst_ofs[] = {
>> +       MT8196_UFSAO_RST0_SET_OFFSET,
>> +       MT8196_UFSAO_RST1_SET_OFFSET
>> +};
>> +
>> +static u16 ufsao_rst_idx_map[] = {
>> +       [MT8196_UFSAO_RST0_UFS_MPHY] = 8,
>> +       [MT8196_UFSAO_RST1_UFS_UNIPRO] = 1 * RST_NR_PER_BANK + 0,
>> +       [MT8196_UFSAO_RST1_UFS_CRYPTO] = 1 * RST_NR_PER_BANK + 1,
>> +       [MT8196_UFSAO_RST1_UFSHCI] = 1 * RST_NR_PER_BANK + 2,
>> +};
>> +
>> +static const struct mtk_clk_rst_desc ufsao_rst_desc = {
>> +       .version = MTK_RST_SET_CLR,
>> +       .rst_bank_ofs = ufsao_rst_ofs,
>> +       .rst_bank_nr = ARRAY_SIZE(ufsao_rst_ofs),
>> +       .rst_idx_map = ufsao_rst_idx_map,
>> +       .rst_idx_map_nr = ARRAY_SIZE(ufsao_rst_idx_map),
>> +};
>> +
>> +static const struct mtk_clk_desc ufsao_mcd = {
>> +       .clks = ufsao_clks,
>> +       .num_clks = ARRAY_SIZE(ufsao_clks),
>> +       .rst_desc = &ufsao_rst_desc,
>> +};
>> +
>> +static const struct of_device_id of_match_clk_mt8196_ufs_ao[] = {
>> +       { .compatible = "mediatek,mt8196-ufscfg-ao", .data = &ufsao_mcd },
>> +       { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_ufs_ao);
>> +
>> +static struct platform_driver clk_mt8196_ufs_ao_drv = {
>> +       .probe = mtk_clk_simple_probe,
>> +       .remove = mtk_clk_simple_remove,
>> +       .driver = {
>> +               .name = "clk-mt8196-ufs-ao",
>> +               .of_match_table = of_match_clk_mt8196_ufs_ao,
>> +       },
>> +};
>> +
>> +module_platform_driver(clk_mt8196_ufs_ao_drv);
>> +MODULE_DESCRIPTION("MediaTek MT8196 ufs_ao clocks driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.39.5
>>