[v11,3/5] soc: mediatek: Move mt8173 MMSYS to platform driver

Commit Message

Enric Balletbo i Serra March 2, 2020, 11:01 a.m. UTC

From: Matthias Brugger <[e-mail]>

There is no strong reason for this to use CLK_OF_DECLARE instead of
being a platform driver. Plus, this driver provides clocks but also
a shared register space for the mediatek-drm and the mediatek-mdp
driver. So move to drivers/soc/mediatek as a platform driver.

Signed-off-by: Matthias Brugger <[e-mail]>
Signed-off-by: Enric Balletbo i Serra <[e-mail]>
Reviewed-by: CK Hu <[e-mail]>
---

Changes in v11: None
Changes in v10:
- Renamed to be generic mtk-mmsys
- Add driver data support to be able to support diferent SoCs

Changes in v9:
- Move mmsys to drivers/soc/mediatek (CK)

Changes in v8:
- Be a builtin_platform_driver like other mediatek mmsys drivers.

Changes in v7:
- Free clk_data->clks as well
- Get rid of private data structure

 drivers/clk/mediatek/clk-mt8173.c | 104 --------------------
 drivers/soc/mediatek/Kconfig      |   7 ++
 drivers/soc/mediatek/Makefile     |   1 +
 drivers/soc/mediatek/mtk-mmsys.c  | 154 ++++++++++++++++++++++++++++++
 4 files changed, 162 insertions(+), 104 deletions(-)
 create mode 100644 drivers/soc/mediatek/mtk-mmsys.c

Comments

Stephen Boyd March 5, 2020, 9:01 p.m. UTC | #1

Quoting Enric Balletbo i Serra (2020-03-02 03:01:26)
> From: Matthias Brugger <[e-mail]>
> 
> There is no strong reason for this to use CLK_OF_DECLARE instead of
> being a platform driver.

Cool.

> Plus, this driver provides clocks but also
> a shared register space for the mediatek-drm and the mediatek-mdp
> driver. So move to drivers/soc/mediatek as a platform driver.
> 
> Signed-off-by: Matthias Brugger <[e-mail]>
> Signed-off-by: Enric Balletbo i Serra <[e-mail]>
> Reviewed-by: CK Hu <[e-mail]>
> ---
> 
> Changes in v11: None
> Changes in v10:
> - Renamed to be generic mtk-mmsys
> - Add driver data support to be able to support diferent SoCs
> 
> Changes in v9:
> - Move mmsys to drivers/soc/mediatek (CK)
> 
> Changes in v8:
> - Be a builtin_platform_driver like other mediatek mmsys drivers.
> 
> Changes in v7:
> - Free clk_data->clks as well
> - Get rid of private data structure
> 
>  drivers/clk/mediatek/clk-mt8173.c | 104 --------------------
>  drivers/soc/mediatek/Kconfig      |   7 ++
>  drivers/soc/mediatek/Makefile     |   1 +
>  drivers/soc/mediatek/mtk-mmsys.c  | 154 ++++++++++++++++++++++++++++++

Can you generate with -M so that we can see what has actually changed?

>  4 files changed, 162 insertions(+), 104 deletions(-)
>  create mode 100644 drivers/soc/mediatek/mtk-mmsys.c
> 
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index 2114b563478c..7a156944d50e 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -44,4 +44,11 @@ config MTK_SCPSYS
>           Say yes here to add support for the MediaTek SCPSYS power domain
>           driver.
>  
> +config MTK_MMSYS
> +       bool "MediaTek MMSYS Support"
> +       depends on COMMON_CLK_MT8173

Does it need some default so that defconfig updates don't break things?

> +       help
> +         Say yes here to add support for the MediaTek Multimedia
> +         Subsystem (MMSYS).
> +
>  endmenu
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index b01733074ad6..01f9f873634a 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
>  obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> +obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> new file mode 100644
> index 000000000000..473cdf732fb5
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -0,0 +1,154 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: James Liao <[e-mail]>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#include "../../clk/mediatek/clk-gate.h"
> +#include "../../clk/mediatek/clk-mtk.h"

Why not use include/linux/clk/?

But I also don't understand why the clk driver is moved outside of
drivers/clk/ into drivers/soc/. Commit text saying that it has shared
registers doesn't mean it can't still keep the clk driver part in the
drivers/clk/ area.

> +
> +#include <dt-bindings/clock/mt8173-clk.h>
> +
> +static const struct mtk_gate_regs mm0_cg_regs = {
> +       .set_ofs = 0x0104,
> +       .clr_ofs = 0x0108,
> +       .sta_ofs = 0x0100,
> +};
> +
> +static const struct mtk_gate_regs mm1_cg_regs = {
> +       .set_ofs = 0x0114,
> +       .clr_ofs = 0x0118,
> +       .sta_ofs = 0x0110,
> +};

Enric Balletbo i Serra March 6, 2020, 4:30 p.m. UTC | #2

Hi Stephen,

On 5/3/20 22:01, Stephen Boyd wrote:
> Quoting Enric Balletbo i Serra (2020-03-02 03:01:26)
>> From: Matthias Brugger <[e-mail]>
>>
>> There is no strong reason for this to use CLK_OF_DECLARE instead of
>> being a platform driver.
> 
> Cool.
> 
>> Plus, this driver provides clocks but also
>> a shared register space for the mediatek-drm and the mediatek-mdp
>> driver. So move to drivers/soc/mediatek as a platform driver.
>>
>> Signed-off-by: Matthias Brugger <[e-mail]>
>> Signed-off-by: Enric Balletbo i Serra <[e-mail]>
>> Reviewed-by: CK Hu <[e-mail]>
>> ---
>>
>> Changes in v11: None
>> Changes in v10:
>> - Renamed to be generic mtk-mmsys
>> - Add driver data support to be able to support diferent SoCs
>>
>> Changes in v9:
>> - Move mmsys to drivers/soc/mediatek (CK)
>>
>> Changes in v8:
>> - Be a builtin_platform_driver like other mediatek mmsys drivers.
>>
>> Changes in v7:
>> - Free clk_data->clks as well
>> - Get rid of private data structure
>>
>>  drivers/clk/mediatek/clk-mt8173.c | 104 --------------------
>>  drivers/soc/mediatek/Kconfig      |   7 ++
>>  drivers/soc/mediatek/Makefile     |   1 +
>>  drivers/soc/mediatek/mtk-mmsys.c  | 154 ++++++++++++++++++++++++++++++
> 
> Can you generate with -M so that we can see what has actually changed?
> 

Sure, sorry about that.

>>  4 files changed, 162 insertions(+), 104 deletions(-)
>>  create mode 100644 drivers/soc/mediatek/mtk-mmsys.c
>>
>> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
>> index 2114b563478c..7a156944d50e 100644
>> --- a/drivers/soc/mediatek/Kconfig
>> +++ b/drivers/soc/mediatek/Kconfig
>> @@ -44,4 +44,11 @@ config MTK_SCPSYS
>>           Say yes here to add support for the MediaTek SCPSYS power domain
>>           driver.
>>  
>> +config MTK_MMSYS
>> +       bool "MediaTek MMSYS Support"
>> +       depends on COMMON_CLK_MT8173
> 
> Does it need some default so that defconfig updates don't break things?
> 

Right.

>> +       help
>> +         Say yes here to add support for the MediaTek Multimedia
>> +         Subsystem (MMSYS).
>> +
>>  endmenu
>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
>> index b01733074ad6..01f9f873634a 100644
>> --- a/drivers/soc/mediatek/Makefile
>> +++ b/drivers/soc/mediatek/Makefile
>> @@ -3,3 +3,4 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
>>  obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
>>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>>  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
>> +obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
>> new file mode 100644
>> index 000000000000..473cdf732fb5
>> --- /dev/null
>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>> @@ -0,0 +1,154 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2014 MediaTek Inc.
>> + * Author: James Liao <[e-mail]>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "../../clk/mediatek/clk-gate.h"
>> +#include "../../clk/mediatek/clk-mtk.h"
> 
> Why not use include/linux/clk/?
> 

I can move these files to include, this will impact a lot more of drivers but,
yes, I think is the right way.

> But I also don't understand why the clk driver is moved outside of
> drivers/clk/ into drivers/soc/. Commit text saying that it has shared
> registers doesn't mean it can't still keep the clk driver part in the
> drivers/clk/ area.
> 

Actually moving this to the soc directory has been requested by CK (mediatek) as
a change in v8. You can see the discussion in [1]

Thanks,
 Enric

[1] https://patchwork.kernel.org/cover/11394709/

>> +
>> +#include <dt-bindings/clock/mt8173-clk.h>
>> +
>> +static const struct mtk_gate_regs mm0_cg_regs = {
>> +       .set_ofs = 0x0104,
>> +       .clr_ofs = 0x0108,
>> +       .sta_ofs = 0x0100,
>> +};
>> +
>> +static const struct mtk_gate_regs mm1_cg_regs = {
>> +       .set_ofs = 0x0114,
>> +       .clr_ofs = 0x0118,
>> +       .sta_ofs = 0x0110,
>> +};

Stephen Boyd March 6, 2020, 9:37 p.m. UTC | #3

Quoting Enric Balletbo i Serra (2020-03-06 08:30:16)
> Hi Stephen,
> 
> On 5/3/20 22:01, Stephen Boyd wrote:
> > Quoting Enric Balletbo i Serra (2020-03-02 03:01:26)
> >> From: Matthias Brugger <[e-mail]>
> >>
> >> There is no strong reason for this to use CLK_OF_DECLARE instead of
> >> being a platform driver.
> > 
> > Cool.
> > 
> >> Plus, this driver provides clocks but also
> >> a shared register space for the mediatek-drm and the mediatek-mdp
> >> driver. So move to drivers/soc/mediatek as a platform driver.
> >>
> >> Signed-off-by: Matthias Brugger <[e-mail]>
> >> Signed-off-by: Enric Balletbo i Serra <[e-mail]>
> >> Reviewed-by: CK Hu <[e-mail]>
> >> ---
> >>
> >> Changes in v11: None
> >> Changes in v10:
> >> - Renamed to be generic mtk-mmsys
> >> - Add driver data support to be able to support diferent SoCs
> >>
> >> Changes in v9:
> >> - Move mmsys to drivers/soc/mediatek (CK)
> >>
> >> Changes in v8:
> >> - Be a builtin_platform_driver like other mediatek mmsys drivers.
> >>
> >> Changes in v7:
> >> - Free clk_data->clks as well
> >> - Get rid of private data structure
> >>
> >>  drivers/clk/mediatek/clk-mt8173.c | 104 --------------------
> >>  drivers/soc/mediatek/Kconfig      |   7 ++
> >>  drivers/soc/mediatek/Makefile     |   1 +
> >>  drivers/soc/mediatek/mtk-mmsys.c  | 154 ++++++++++++++++++++++++++++++
> > 
> > Can you generate with -M so that we can see what has actually changed?
> > 
> 
> Sure, sorry about that.
> 
> >>  4 files changed, 162 insertions(+), 104 deletions(-)
> >>  create mode 100644 drivers/soc/mediatek/mtk-mmsys.c
> >>
> >> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> >> index 2114b563478c..7a156944d50e 100644
> >> --- a/drivers/soc/mediatek/Kconfig
> >> +++ b/drivers/soc/mediatek/Kconfig
> >> @@ -44,4 +44,11 @@ config MTK_SCPSYS
> >>           Say yes here to add support for the MediaTek SCPSYS power domain
> >>           driver.
> >>  
> >> +config MTK_MMSYS
> >> +       bool "MediaTek MMSYS Support"
> >> +       depends on COMMON_CLK_MT8173
> > 
> > Does it need some default so that defconfig updates don't break things?
> > 
> 
> Right.
> 
> >> +       help
> >> +         Say yes here to add support for the MediaTek Multimedia
> >> +         Subsystem (MMSYS).
> >> +
> >>  endmenu
> >> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> >> index b01733074ad6..01f9f873634a 100644
> >> --- a/drivers/soc/mediatek/Makefile
> >> +++ b/drivers/soc/mediatek/Makefile
> >> @@ -3,3 +3,4 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> >>  obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> >>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> >>  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> >> +obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
> >> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> >> new file mode 100644
> >> index 000000000000..473cdf732fb5
> >> --- /dev/null
> >> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> >> @@ -0,0 +1,154 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2014 MediaTek Inc.
> >> + * Author: James Liao <[e-mail]>
> >> + */
> >> +
> >> +#include <linux/clk-provider.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/platform_device.h>
> >> +
> >> +#include "../../clk/mediatek/clk-gate.h"
> >> +#include "../../clk/mediatek/clk-mtk.h"
> > 
> > Why not use include/linux/clk/?
> > 
> 
> I can move these files to include, this will impact a lot more of drivers but,
> yes, I think is the right way.
> 
> > But I also don't understand why the clk driver is moved outside of
> > drivers/clk/ into drivers/soc/. Commit text saying that it has shared
> > registers doesn't mean it can't still keep the clk driver part in the
> > drivers/clk/ area.
> > 
> 
> Actually moving this to the soc directory has been requested by CK (mediatek) as
> a change in v8. You can see the discussion in [1]
> 

I can reply there in that thread if necessary, but we shouldn't need to
force simple-mfd into DT bindings to support this. Match the compatible
string in drivers/soc/ and register devices in software for the
different pieces of this overall hardware block. If necessary, pass down
the ioremapped addresss down through device data to each logical driver
in the respective subsystem.

So yes, it looks like an MFD, but that doesn't mean we have to change
the DT binding or put it in drivers/mfd to support that. And we don't
have to fix any problems with allowing two drivers to probe the same
compatible string.

Enric Balletbo Serra March 6, 2020, 10:09 p.m. UTC | #4

Hi Stephen,

Missatge de Stephen Boyd <[e-mail]> del dia dv., 6 de març
2020 a les 22:37:
>
> Quoting Enric Balletbo i Serra (2020-03-06 08:30:16)
> > Hi Stephen,
> >
> > On 5/3/20 22:01, Stephen Boyd wrote:
> > > Quoting Enric Balletbo i Serra (2020-03-02 03:01:26)
> > >> From: Matthias Brugger <[e-mail]>
> > >>
> > >> There is no strong reason for this to use CLK_OF_DECLARE instead of
> > >> being a platform driver.
> > >
> > > Cool.
> > >
> > >> Plus, this driver provides clocks but also
> > >> a shared register space for the mediatek-drm and the mediatek-mdp
> > >> driver. So move to drivers/soc/mediatek as a platform driver.
> > >>
> > >> Signed-off-by: Matthias Brugger <[e-mail]>
> > >> Signed-off-by: Enric Balletbo i Serra <[e-mail]>
> > >> Reviewed-by: CK Hu <[e-mail]>
> > >> ---
> > >>
> > >> Changes in v11: None
> > >> Changes in v10:
> > >> - Renamed to be generic mtk-mmsys
> > >> - Add driver data support to be able to support diferent SoCs
> > >>
> > >> Changes in v9:
> > >> - Move mmsys to drivers/soc/mediatek (CK)
> > >>
> > >> Changes in v8:
> > >> - Be a builtin_platform_driver like other mediatek mmsys drivers.
> > >>
> > >> Changes in v7:
> > >> - Free clk_data->clks as well
> > >> - Get rid of private data structure
> > >>
> > >>  drivers/clk/mediatek/clk-mt8173.c | 104 --------------------
> > >>  drivers/soc/mediatek/Kconfig      |   7 ++
> > >>  drivers/soc/mediatek/Makefile     |   1 +
> > >>  drivers/soc/mediatek/mtk-mmsys.c  | 154 ++++++++++++++++++++++++++++++
> > >
> > > Can you generate with -M so that we can see what has actually changed?
> > >
> >
> > Sure, sorry about that.
> >
> > >>  4 files changed, 162 insertions(+), 104 deletions(-)
> > >>  create mode 100644 drivers/soc/mediatek/mtk-mmsys.c
> > >>
> > >> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > >> index 2114b563478c..7a156944d50e 100644
> > >> --- a/drivers/soc/mediatek/Kconfig
> > >> +++ b/drivers/soc/mediatek/Kconfig
> > >> @@ -44,4 +44,11 @@ config MTK_SCPSYS
> > >>           Say yes here to add support for the MediaTek SCPSYS power domain
> > >>           driver.
> > >>
> > >> +config MTK_MMSYS
> > >> +       bool "MediaTek MMSYS Support"
> > >> +       depends on COMMON_CLK_MT8173
> > >
> > > Does it need some default so that defconfig updates don't break things?
> > >
> >
> > Right.
> >
> > >> +       help
> > >> +         Say yes here to add support for the MediaTek Multimedia
> > >> +         Subsystem (MMSYS).
> > >> +
> > >>  endmenu
> > >> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > >> index b01733074ad6..01f9f873634a 100644
> > >> --- a/drivers/soc/mediatek/Makefile
> > >> +++ b/drivers/soc/mediatek/Makefile
> > >> @@ -3,3 +3,4 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> > >>  obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> > >>  obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> > >>  obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> > >> +obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
> > >> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> > >> new file mode 100644
> > >> index 000000000000..473cdf732fb5
> > >> --- /dev/null
> > >> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > >> @@ -0,0 +1,154 @@
> > >> +// SPDX-License-Identifier: GPL-2.0-only
> > >> +/*
> > >> + * Copyright (c) 2014 MediaTek Inc.
> > >> + * Author: James Liao <[e-mail]>
> > >> + */
> > >> +
> > >> +#include <linux/clk-provider.h>
> > >> +#include <linux/of_device.h>
> > >> +#include <linux/platform_device.h>
> > >> +
> > >> +#include "../../clk/mediatek/clk-gate.h"
> > >> +#include "../../clk/mediatek/clk-mtk.h"
> > >
> > > Why not use include/linux/clk/?
> > >
> >
> > I can move these files to include, this will impact a lot more of drivers but,
> > yes, I think is the right way.
> >
> > > But I also don't understand why the clk driver is moved outside of
> > > drivers/clk/ into drivers/soc/. Commit text saying that it has shared
> > > registers doesn't mean it can't still keep the clk driver part in the
> > > drivers/clk/ area.
> > >
> >
> > Actually moving this to the soc directory has been requested by CK (mediatek) as
> > a change in v8. You can see the discussion in [1]
> >
>
> I can reply there in that thread if necessary, but we shouldn't need to
> force simple-mfd into DT bindings to support this. Match the compatible
> string in drivers/soc/ and register devices in software for the
> different pieces of this overall hardware block. If necessary, pass down
> the ioremapped addresss down through device data to each logical driver
> in the respective subsystem.
>
> So yes, it looks like an MFD, but that doesn't mean we have to change
> the DT binding or put it in drivers/mfd to support that. And we don't
> have to fix any problems with allowing two drivers to probe the same
> compatible string.
>

That thread maybe has too much information and things evolved since
then. Note that the final solution is not an MFD neither we change the
bindings. I pointed to that thread just because CK (CK please correct
me if I'm wrong) thought that the driver is not a pure clock driver
and he preferred to move to drivers/soc/mediatek (in that thread, he
exposes his opinion on that).  Sorry to introduce more confusion.

You seem to be fine with the approach (just minor changes), so it
looks to me that the only problem is if this should be in drivers/clk
or drivers/soc. Honestly, this is not something I can't decide and
I'll let you (the soc and clk maintainers) decide. I don't really have
a strong opinion here. I don't mind move again to drivers/clk if that
is what we want but let's come to an agreement.

Thanks,
 Enric

Stephen Boyd March 9, 2020, 10:50 p.m. UTC | #5

Quoting Enric Balletbo Serra (2020-03-06 14:09:50)
> Missatge de Stephen Boyd <[e-mail]> del dia dv., 6 de mar
> 2020 a les 22:37:
> >
> > Quoting Enric Balletbo i Serra (2020-03-06 08:30:16)
> > > On 5/3/20 22:01, Stephen Boyd wrote:
> > > > Quoting Enric Balletbo i Serra (2020-03-02 03:01:26)
> > > >> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> > > >> new file mode 100644
> > > >> index 000000000000..473cdf732fb5
> > > >> --- /dev/null
> > > >> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > > >> @@ -0,0 +1,154 @@
> > > >> +// SPDX-License-Identifier: GPL-2.0-only
> > > >> +/*
> > > >> + * Copyright (c) 2014 MediaTek Inc.
> > > >> + * Author: James Liao <[e-mail]>
> > > >> + */
> > > >> +
> > > >> +#include <linux/clk-provider.h>
> > > >> +#include <linux/of_device.h>
> > > >> +#include <linux/platform_device.h>
> > > >> +
> > > >> +#include "../../clk/mediatek/clk-gate.h"
> > > >> +#include "../../clk/mediatek/clk-mtk.h"
> > > >
> > > > Why not use include/linux/clk/?
> > > >
> > >
> > > I can move these files to include, this will impact a lot more of drivers but,
> > > yes, I think is the right way.
> > >
> > > > But I also don't understand why the clk driver is moved outside of
> > > > drivers/clk/ into drivers/soc/. Commit text saying that it has shared
> > > > registers doesn't mean it can't still keep the clk driver part in the
> > > > drivers/clk/ area.
> > > >
> > >
> > > Actually moving this to the soc directory has been requested by CK (mediatek) as
> > > a change in v8. You can see the discussion in [1]
> > >
> >
> > I can reply there in that thread if necessary, but we shouldn't need to
> > force simple-mfd into DT bindings to support this. Match the compatible
> > string in drivers/soc/ and register devices in software for the
> > different pieces of this overall hardware block. If necessary, pass down
> > the ioremapped addresss down through device data to each logical driver
> > in the respective subsystem.
> >
> > So yes, it looks like an MFD, but that doesn't mean we have to change
> > the DT binding or put it in drivers/mfd to support that. And we don't
> > have to fix any problems with allowing two drivers to probe the same
> > compatible string.
> >
> 
> That thread maybe has too much information and things evolved since
> then. Note that the final solution is not an MFD neither we change the
> bindings. I pointed to that thread just because CK (CK please correct
> me if I'm wrong) thought that the driver is not a pure clock driver
> and he preferred to move to drivers/soc/mediatek (in that thread, he
> exposes his opinion on that).  Sorry to introduce more confusion.
> 
> You seem to be fine with the approach (just minor changes), so it
> looks to me that the only problem is if this should be in drivers/clk
> or drivers/soc. Honestly, this is not something I can't decide and
> I'll let you (the soc and clk maintainers) decide. I don't really have
> a strong opinion here. I don't mind move again to drivers/clk if that
> is what we want but let's come to an agreement.
> 

It's already in drivers/clk, so leave the clk part there and register
the clk device and any other devices by matching the compatible in
drivers/soc. That is my preferred solution. Can that be done?

Matthias Brugger March 10, 2020, 9:40 a.m. UTC | #6

On 09/03/2020 23:50, Stephen Boyd wrote:
> Quoting Enric Balletbo Serra (2020-03-06 14:09:50)
>> Missatge de Stephen Boyd <[e-mail]> del dia dv., 6 de mar
>> 2020 a les 22:37:
>>>
>>> Quoting Enric Balletbo i Serra (2020-03-06 08:30:16)
>>>> On 5/3/20 22:01, Stephen Boyd wrote:
>>>>> Quoting Enric Balletbo i Serra (2020-03-02 03:01:26)
>>>>>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..473cdf732fb5
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>>>>>> @@ -0,0 +1,154 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>> +/*
>>>>>> + * Copyright (c) 2014 MediaTek Inc.
>>>>>> + * Author: James Liao <[e-mail]>
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/clk-provider.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +
>>>>>> +#include "../../clk/mediatek/clk-gate.h"
>>>>>> +#include "../../clk/mediatek/clk-mtk.h"
>>>>>
>>>>> Why not use include/linux/clk/?
>>>>>
>>>>
>>>> I can move these files to include, this will impact a lot more of drivers but,
>>>> yes, I think is the right way.
>>>>
>>>>> But I also don't understand why the clk driver is moved outside of
>>>>> drivers/clk/ into drivers/soc/. Commit text saying that it has shared
>>>>> registers doesn't mean it can't still keep the clk driver part in the
>>>>> drivers/clk/ area.
>>>>>
>>>>
>>>> Actually moving this to the soc directory has been requested by CK (mediatek) as
>>>> a change in v8. You can see the discussion in [1]
>>>>
>>>
>>> I can reply there in that thread if necessary, but we shouldn't need to
>>> force simple-mfd into DT bindings to support this. Match the compatible
>>> string in drivers/soc/ and register devices in software for the
>>> different pieces of this overall hardware block. If necessary, pass down
>>> the ioremapped addresss down through device data to each logical driver
>>> in the respective subsystem.
>>>
>>> So yes, it looks like an MFD, but that doesn't mean we have to change
>>> the DT binding or put it in drivers/mfd to support that. And we don't
>>> have to fix any problems with allowing two drivers to probe the same
>>> compatible string.
>>>
>>
>> That thread maybe has too much information and things evolved since
>> then. Note that the final solution is not an MFD neither we change the
>> bindings. I pointed to that thread just because CK (CK please correct
>> me if I'm wrong) thought that the driver is not a pure clock driver
>> and he preferred to move to drivers/soc/mediatek (in that thread, he
>> exposes his opinion on that).  Sorry to introduce more confusion.
>>
>> You seem to be fine with the approach (just minor changes), so it
>> looks to me that the only problem is if this should be in drivers/clk
>> or drivers/soc. Honestly, this is not something I can't decide and
>> I'll let you (the soc and clk maintainers) decide. I don't really have
>> a strong opinion here. I don't mind move again to drivers/clk if that
>> is what we want but let's come to an agreement.
>>
> 
> It's already in drivers/clk, so leave the clk part there and register
> the clk device and any other devices by matching the compatible in
> drivers/soc. That is my preferred solution. Can that be done?
> 

I think we can once again create a platform device in drivers/soc which matches
the drivers/clk and then do the routing in drivers/soc. Enric any thoughts?

Enric Balletbo i Serra March 10, 2020, 9:50 a.m. UTC | #7

Hi all,

On 10/3/20 10:40, Matthias Brugger wrote:
> 
> 
> On 09/03/2020 23:50, Stephen Boyd wrote:
>> Quoting Enric Balletbo Serra (2020-03-06 14:09:50)
>>> Missatge de Stephen Boyd <[e-mail]> del dia dv., 6 de mar
>>> 2020 a les 22:37:
>>>>
>>>> Quoting Enric Balletbo i Serra (2020-03-06 08:30:16)
>>>>> On 5/3/20 22:01, Stephen Boyd wrote:
>>>>>> Quoting Enric Balletbo i Serra (2020-03-02 03:01:26)
>>>>>>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..473cdf732fb5
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>>>>>>> @@ -0,0 +1,154 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>>> +/*
>>>>>>> + * Copyright (c) 2014 MediaTek Inc.
>>>>>>> + * Author: James Liao <[e-mail]>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <linux/clk-provider.h>
>>>>>>> +#include <linux/of_device.h>
>>>>>>> +#include <linux/platform_device.h>
>>>>>>> +
>>>>>>> +#include "../../clk/mediatek/clk-gate.h"
>>>>>>> +#include "../../clk/mediatek/clk-mtk.h"
>>>>>>
>>>>>> Why not use include/linux/clk/?
>>>>>>
>>>>>
>>>>> I can move these files to include, this will impact a lot more of drivers but,
>>>>> yes, I think is the right way.
>>>>>
>>>>>> But I also don't understand why the clk driver is moved outside of
>>>>>> drivers/clk/ into drivers/soc/. Commit text saying that it has shared
>>>>>> registers doesn't mean it can't still keep the clk driver part in the
>>>>>> drivers/clk/ area.
>>>>>>
>>>>>
>>>>> Actually moving this to the soc directory has been requested by CK (mediatek) as
>>>>> a change in v8. You can see the discussion in [1]
>>>>>
>>>>
>>>> I can reply there in that thread if necessary, but we shouldn't need to
>>>> force simple-mfd into DT bindings to support this. Match the compatible
>>>> string in drivers/soc/ and register devices in software for the
>>>> different pieces of this overall hardware block. If necessary, pass down
>>>> the ioremapped addresss down through device data to each logical driver
>>>> in the respective subsystem.
>>>>
>>>> So yes, it looks like an MFD, but that doesn't mean we have to change
>>>> the DT binding or put it in drivers/mfd to support that. And we don't
>>>> have to fix any problems with allowing two drivers to probe the same
>>>> compatible string.
>>>>
>>>
>>> That thread maybe has too much information and things evolved since
>>> then. Note that the final solution is not an MFD neither we change the
>>> bindings. I pointed to that thread just because CK (CK please correct
>>> me if I'm wrong) thought that the driver is not a pure clock driver
>>> and he preferred to move to drivers/soc/mediatek (in that thread, he
>>> exposes his opinion on that).  Sorry to introduce more confusion.
>>>
>>> You seem to be fine with the approach (just minor changes), so it
>>> looks to me that the only problem is if this should be in drivers/clk
>>> or drivers/soc. Honestly, this is not something I can't decide and
>>> I'll let you (the soc and clk maintainers) decide. I don't really have
>>> a strong opinion here. I don't mind move again to drivers/clk if that
>>> is what we want but let's come to an agreement.
>>>
>>
>> It's already in drivers/clk, so leave the clk part there and register
>> the clk device and any other devices by matching the compatible in
>> drivers/soc. That is my preferred solution. Can that be done?
>>
> 
> I think we can once again create a platform device in drivers/soc which matches
> the drivers/clk and then do the routing in drivers/soc. Enric any thoughts?
> 

Yes, working already on a new version.

Thanks,
 Enric

Patch

diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 537a7f49b0f7..8f898ac476c0 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -753,93 +753,6 @@  static const struct mtk_gate img_clks[] __initconst = {
 	GATE_IMG(CLK_IMG_FD, "img_fd", "mm_sel", 11),
 };
 
-static const struct mtk_gate_regs mm0_cg_regs __initconst = {
-	.set_ofs = 0x0104,
-	.clr_ofs = 0x0108,
-	.sta_ofs = 0x0100,
-};
-
-static const struct mtk_gate_regs mm1_cg_regs __initconst = {
-	.set_ofs = 0x0114,
-	.clr_ofs = 0x0118,
-	.sta_ofs = 0x0110,
-};
-
-#define GATE_MM0(_id, _name, _parent, _shift) {			\
-		.id = _id,					\
-		.name = _name,					\
-		.parent_name = _parent,				\
-		.regs = &mm0_cg_regs,				\
-		.shift = _shift,				\
-		.ops = &mtk_clk_gate_ops_setclr,		\
-	}
-
-#define GATE_MM1(_id, _name, _parent, _shift) {			\
-		.id = _id,					\
-		.name = _name,					\
-		.parent_name = _parent,				\
-		.regs = &mm1_cg_regs,				\
-		.shift = _shift,				\
-		.ops = &mtk_clk_gate_ops_setclr,		\
-	}
-
-static const struct mtk_gate mm_clks[] __initconst = {
-	/* MM0 */
-	GATE_MM0(CLK_MM_SMI_COMMON, "mm_smi_common", "mm_sel", 0),
-	GATE_MM0(CLK_MM_SMI_LARB0, "mm_smi_larb0", "mm_sel", 1),
-	GATE_MM0(CLK_MM_CAM_MDP, "mm_cam_mdp", "mm_sel", 2),
-	GATE_MM0(CLK_MM_MDP_RDMA0, "mm_mdp_rdma0", "mm_sel", 3),
-	GATE_MM0(CLK_MM_MDP_RDMA1, "mm_mdp_rdma1", "mm_sel", 4),
-	GATE_MM0(CLK_MM_MDP_RSZ0, "mm_mdp_rsz0", "mm_sel", 5),
-	GATE_MM0(CLK_MM_MDP_RSZ1, "mm_mdp_rsz1", "mm_sel", 6),
-	GATE_MM0(CLK_MM_MDP_RSZ2, "mm_mdp_rsz2", "mm_sel", 7),
-	GATE_MM0(CLK_MM_MDP_TDSHP0, "mm_mdp_tdshp0", "mm_sel", 8),
-	GATE_MM0(CLK_MM_MDP_TDSHP1, "mm_mdp_tdshp1", "mm_sel", 9),
-	GATE_MM0(CLK_MM_MDP_WDMA, "mm_mdp_wdma", "mm_sel", 11),
-	GATE_MM0(CLK_MM_MDP_WROT0, "mm_mdp_wrot0", "mm_sel", 12),
-	GATE_MM0(CLK_MM_MDP_WROT1, "mm_mdp_wrot1", "mm_sel", 13),
-	GATE_MM0(CLK_MM_FAKE_ENG, "mm_fake_eng", "mm_sel", 14),
-	GATE_MM0(CLK_MM_MUTEX_32K, "mm_mutex_32k", "rtc_sel", 15),
-	GATE_MM0(CLK_MM_DISP_OVL0, "mm_disp_ovl0", "mm_sel", 16),
-	GATE_MM0(CLK_MM_DISP_OVL1, "mm_disp_ovl1", "mm_sel", 17),
-	GATE_MM0(CLK_MM_DISP_RDMA0, "mm_disp_rdma0", "mm_sel", 18),
-	GATE_MM0(CLK_MM_DISP_RDMA1, "mm_disp_rdma1", "mm_sel", 19),
-	GATE_MM0(CLK_MM_DISP_RDMA2, "mm_disp_rdma2", "mm_sel", 20),
-	GATE_MM0(CLK_MM_DISP_WDMA0, "mm_disp_wdma0", "mm_sel", 21),
-	GATE_MM0(CLK_MM_DISP_WDMA1, "mm_disp_wdma1", "mm_sel", 22),
-	GATE_MM0(CLK_MM_DISP_COLOR0, "mm_disp_color0", "mm_sel", 23),
-	GATE_MM0(CLK_MM_DISP_COLOR1, "mm_disp_color1", "mm_sel", 24),
-	GATE_MM0(CLK_MM_DISP_AAL, "mm_disp_aal", "mm_sel", 25),
-	GATE_MM0(CLK_MM_DISP_GAMMA, "mm_disp_gamma", "mm_sel", 26),
-	GATE_MM0(CLK_MM_DISP_UFOE, "mm_disp_ufoe", "mm_sel", 27),
-	GATE_MM0(CLK_MM_DISP_SPLIT0, "mm_disp_split0", "mm_sel", 28),
-	GATE_MM0(CLK_MM_DISP_SPLIT1, "mm_disp_split1", "mm_sel", 29),
-	GATE_MM0(CLK_MM_DISP_MERGE, "mm_disp_merge", "mm_sel", 30),
-	GATE_MM0(CLK_MM_DISP_OD, "mm_disp_od", "mm_sel", 31),
-	/* MM1 */
-	GATE_MM1(CLK_MM_DISP_PWM0MM, "mm_disp_pwm0mm", "mm_sel", 0),
-	GATE_MM1(CLK_MM_DISP_PWM026M, "mm_disp_pwm026m", "pwm_sel", 1),
-	GATE_MM1(CLK_MM_DISP_PWM1MM, "mm_disp_pwm1mm", "mm_sel", 2),
-	GATE_MM1(CLK_MM_DISP_PWM126M, "mm_disp_pwm126m", "pwm_sel", 3),
-	GATE_MM1(CLK_MM_DSI0_ENGINE, "mm_dsi0_engine", "mm_sel", 4),
-	GATE_MM1(CLK_MM_DSI0_DIGITAL, "mm_dsi0_digital", "dsi0_dig", 5),
-	GATE_MM1(CLK_MM_DSI1_ENGINE, "mm_dsi1_engine", "mm_sel", 6),
-	GATE_MM1(CLK_MM_DSI1_DIGITAL, "mm_dsi1_digital", "dsi1_dig", 7),
-	GATE_MM1(CLK_MM_DPI_PIXEL, "mm_dpi_pixel", "dpi0_sel", 8),
-	GATE_MM1(CLK_MM_DPI_ENGINE, "mm_dpi_engine", "mm_sel", 9),
-	GATE_MM1(CLK_MM_DPI1_PIXEL, "mm_dpi1_pixel", "lvds_pxl", 10),
-	GATE_MM1(CLK_MM_DPI1_ENGINE, "mm_dpi1_engine", "mm_sel", 11),
-	GATE_MM1(CLK_MM_HDMI_PIXEL, "mm_hdmi_pixel", "dpi0_sel", 12),
-	GATE_MM1(CLK_MM_HDMI_PLLCK, "mm_hdmi_pllck", "hdmi_sel", 13),
-	GATE_MM1(CLK_MM_HDMI_AUDIO, "mm_hdmi_audio", "apll1", 14),
-	GATE_MM1(CLK_MM_HDMI_SPDIF, "mm_hdmi_spdif", "apll2", 15),
-	GATE_MM1(CLK_MM_LVDS_PIXEL, "mm_lvds_pixel", "lvds_pxl", 16),
-	GATE_MM1(CLK_MM_LVDS_CTS, "mm_lvds_cts", "lvds_cts", 17),
-	GATE_MM1(CLK_MM_SMI_LARB4, "mm_smi_larb4", "mm_sel", 18),
-	GATE_MM1(CLK_MM_HDMI_HDCP, "mm_hdmi_hdcp", "hdcp_sel", 19),
-	GATE_MM1(CLK_MM_HDMI_HDCP24M, "mm_hdmi_hdcp24m", "hdcp_24m_sel", 20),
-};
-
 static const struct mtk_gate_regs vdec0_cg_regs __initconst = {
 	.set_ofs = 0x0000,
 	.clr_ofs = 0x0004,
@@ -1144,23 +1057,6 @@  static void __init mtk_imgsys_init(struct device_node *node)
 }
 CLK_OF_DECLARE(mtk_imgsys, "mediatek,mt8173-imgsys", mtk_imgsys_init);
 
-static void __init mtk_mmsys_init(struct device_node *node)
-{
-	struct clk_onecell_data *clk_data;
-	int r;
-
-	clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
-
-	mtk_clk_register_gates(node, mm_clks, ARRAY_SIZE(mm_clks),
-						clk_data);
-
-	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
-	if (r)
-		pr_err("%s(): could not register clock provider: %d\n",
-			__func__, r);
-}
-CLK_OF_DECLARE(mtk_mmsys, "mediatek,mt8173-mmsys", mtk_mmsys_init);
-
 static void __init mtk_vdecsys_init(struct device_node *node)
 {
 	struct clk_onecell_data *clk_data;
diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index 2114b563478c..7a156944d50e 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -44,4 +44,11 @@  config MTK_SCPSYS
 	  Say yes here to add support for the MediaTek SCPSYS power domain
 	  driver.
 
+config MTK_MMSYS
+	bool "MediaTek MMSYS Support"
+	depends on COMMON_CLK_MT8173
+	help
+	  Say yes here to add support for the MediaTek Multimedia
+	  Subsystem (MMSYS).
+
 endmenu
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index b01733074ad6..01f9f873634a 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -3,3 +3,4 @@  obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
 obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
 obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
 obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
+obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
new file mode 100644
index 000000000000..473cdf732fb5
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-mmsys.c
@@ -0,0 +1,154 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: James Liao <[e-mail]>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "../../clk/mediatek/clk-gate.h"
+#include "../../clk/mediatek/clk-mtk.h"
+
+#include <dt-bindings/clock/mt8173-clk.h>
+
+static const struct mtk_gate_regs mm0_cg_regs = {
+	.set_ofs = 0x0104,
+	.clr_ofs = 0x0108,
+	.sta_ofs = 0x0100,
+};
+
+static const struct mtk_gate_regs mm1_cg_regs = {
+	.set_ofs = 0x0114,
+	.clr_ofs = 0x0118,
+	.sta_ofs = 0x0110,
+};
+
+#define GATE_MM0(_id, _name, _parent, _shift) {			\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &mm0_cg_regs,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr,		\
+	}
+
+#define GATE_MM1(_id, _name, _parent, _shift) {			\
+		.id = _id,					\
+		.name = _name,					\
+		.parent_name = _parent,				\
+		.regs = &mm1_cg_regs,				\
+		.shift = _shift,				\
+		.ops = &mtk_clk_gate_ops_setclr,		\
+	}
+
+static const struct mtk_gate mt8173_mm_clks[] = {
+	/* MM0 */
+	GATE_MM0(CLK_MM_SMI_COMMON, "mm_smi_common", "mm_sel", 0),
+	GATE_MM0(CLK_MM_SMI_LARB0, "mm_smi_larb0", "mm_sel", 1),
+	GATE_MM0(CLK_MM_CAM_MDP, "mm_cam_mdp", "mm_sel", 2),
+	GATE_MM0(CLK_MM_MDP_RDMA0, "mm_mdp_rdma0", "mm_sel", 3),
+	GATE_MM0(CLK_MM_MDP_RDMA1, "mm_mdp_rdma1", "mm_sel", 4),
+	GATE_MM0(CLK_MM_MDP_RSZ0, "mm_mdp_rsz0", "mm_sel", 5),
+	GATE_MM0(CLK_MM_MDP_RSZ1, "mm_mdp_rsz1", "mm_sel", 6),
+	GATE_MM0(CLK_MM_MDP_RSZ2, "mm_mdp_rsz2", "mm_sel", 7),
+	GATE_MM0(CLK_MM_MDP_TDSHP0, "mm_mdp_tdshp0", "mm_sel", 8),
+	GATE_MM0(CLK_MM_MDP_TDSHP1, "mm_mdp_tdshp1", "mm_sel", 9),
+	GATE_MM0(CLK_MM_MDP_WDMA, "mm_mdp_wdma", "mm_sel", 11),
+	GATE_MM0(CLK_MM_MDP_WROT0, "mm_mdp_wrot0", "mm_sel", 12),
+	GATE_MM0(CLK_MM_MDP_WROT1, "mm_mdp_wrot1", "mm_sel", 13),
+	GATE_MM0(CLK_MM_FAKE_ENG, "mm_fake_eng", "mm_sel", 14),
+	GATE_MM0(CLK_MM_MUTEX_32K, "mm_mutex_32k", "rtc_sel", 15),
+	GATE_MM0(CLK_MM_DISP_OVL0, "mm_disp_ovl0", "mm_sel", 16),
+	GATE_MM0(CLK_MM_DISP_OVL1, "mm_disp_ovl1", "mm_sel", 17),
+	GATE_MM0(CLK_MM_DISP_RDMA0, "mm_disp_rdma0", "mm_sel", 18),
+	GATE_MM0(CLK_MM_DISP_RDMA1, "mm_disp_rdma1", "mm_sel", 19),
+	GATE_MM0(CLK_MM_DISP_RDMA2, "mm_disp_rdma2", "mm_sel", 20),
+	GATE_MM0(CLK_MM_DISP_WDMA0, "mm_disp_wdma0", "mm_sel", 21),
+	GATE_MM0(CLK_MM_DISP_WDMA1, "mm_disp_wdma1", "mm_sel", 22),
+	GATE_MM0(CLK_MM_DISP_COLOR0, "mm_disp_color0", "mm_sel", 23),
+	GATE_MM0(CLK_MM_DISP_COLOR1, "mm_disp_color1", "mm_sel", 24),
+	GATE_MM0(CLK_MM_DISP_AAL, "mm_disp_aal", "mm_sel", 25),
+	GATE_MM0(CLK_MM_DISP_GAMMA, "mm_disp_gamma", "mm_sel", 26),
+	GATE_MM0(CLK_MM_DISP_UFOE, "mm_disp_ufoe", "mm_sel", 27),
+	GATE_MM0(CLK_MM_DISP_SPLIT0, "mm_disp_split0", "mm_sel", 28),
+	GATE_MM0(CLK_MM_DISP_SPLIT1, "mm_disp_split1", "mm_sel", 29),
+	GATE_MM0(CLK_MM_DISP_MERGE, "mm_disp_merge", "mm_sel", 30),
+	GATE_MM0(CLK_MM_DISP_OD, "mm_disp_od", "mm_sel", 31),
+	/* MM1 */
+	GATE_MM1(CLK_MM_DISP_PWM0MM, "mm_disp_pwm0mm", "mm_sel", 0),
+	GATE_MM1(CLK_MM_DISP_PWM026M, "mm_disp_pwm026m", "pwm_sel", 1),
+	GATE_MM1(CLK_MM_DISP_PWM1MM, "mm_disp_pwm1mm", "mm_sel", 2),
+	GATE_MM1(CLK_MM_DISP_PWM126M, "mm_disp_pwm126m", "pwm_sel", 3),
+	GATE_MM1(CLK_MM_DSI0_ENGINE, "mm_dsi0_engine", "mm_sel", 4),
+	GATE_MM1(CLK_MM_DSI0_DIGITAL, "mm_dsi0_digital", "dsi0_dig", 5),
+	GATE_MM1(CLK_MM_DSI1_ENGINE, "mm_dsi1_engine", "mm_sel", 6),
+	GATE_MM1(CLK_MM_DSI1_DIGITAL, "mm_dsi1_digital", "dsi1_dig", 7),
+	GATE_MM1(CLK_MM_DPI_PIXEL, "mm_dpi_pixel", "dpi0_sel", 8),
+	GATE_MM1(CLK_MM_DPI_ENGINE, "mm_dpi_engine", "mm_sel", 9),
+	GATE_MM1(CLK_MM_DPI1_PIXEL, "mm_dpi1_pixel", "lvds_pxl", 10),
+	GATE_MM1(CLK_MM_DPI1_ENGINE, "mm_dpi1_engine", "mm_sel", 11),
+	GATE_MM1(CLK_MM_HDMI_PIXEL, "mm_hdmi_pixel", "dpi0_sel", 12),
+	GATE_MM1(CLK_MM_HDMI_PLLCK, "mm_hdmi_pllck", "hdmi_sel", 13),
+	GATE_MM1(CLK_MM_HDMI_AUDIO, "mm_hdmi_audio", "apll1", 14),
+	GATE_MM1(CLK_MM_HDMI_SPDIF, "mm_hdmi_spdif", "apll2", 15),
+	GATE_MM1(CLK_MM_LVDS_PIXEL, "mm_lvds_pixel", "lvds_pxl", 16),
+	GATE_MM1(CLK_MM_LVDS_CTS, "mm_lvds_cts", "lvds_cts", 17),
+	GATE_MM1(CLK_MM_SMI_LARB4, "mm_smi_larb4", "mm_sel", 18),
+	GATE_MM1(CLK_MM_HDMI_HDCP, "mm_hdmi_hdcp", "hdcp_sel", 19),
+	GATE_MM1(CLK_MM_HDMI_HDCP24M, "mm_hdmi_hdcp24m", "hdcp_24m_sel", 20),
+};
+
+struct mtk_mmsys_driver_data {
+	const struct mtk_gate *gates_clk;
+	int gates_num;
+};
+
+static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
+	.gates_clk = mt8173_mm_clks,
+	.gates_num = ARRAY_SIZE(mt8173_mm_clks),
+};
+
+static int mtk_mmsys_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	const struct mtk_mmsys_driver_data *data;
+	struct clk_onecell_data *clk_data;
+	int ret;
+
+	clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
+	if (!clk_data)
+		return -ENOMEM;
+
+	data = of_device_get_match_data(&pdev->dev);
+
+	ret = mtk_clk_register_gates(node, data->gates_clk, data->gates_num,
+				     clk_data);
+	if (ret)
+		return ret;
+
+	ret = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id of_match_mtk_mmsys[] = {
+	{
+		.compatible = "mediatek,mt8173-mmsys",
+		.data = &mt8173_mmsys_driver_data,
+	},
+	{ }
+};
+
+static struct platform_driver mtk_mmsys_drv = {
+	.driver = {
+		.name = "mtk-mmsys",
+		.of_match_table = of_match_mtk_mmsys,
+	},
+	.probe = mtk_mmsys_probe,
+};
+
+builtin_platform_driver(mtk_mmsys_drv);