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

Hi Stephen,

Missatge de Stephen Boyd [off-list ref] del dia dv., 6 de març
2020 a les 22:37:

Quoting Enric Balletbo i Serra (2020-03-06 08:30:16)

quoted

Hi Stephen,

On 5/3/20 22:01, Stephen Boyd wrote:

quoted

Quoting Enric Balletbo i Serra (2020-03-02 03:01:26)

quoted

From: Matthias Brugger <mbrugger@suse.com>

There is no strong reason for this to use CLK_OF_DECLARE instead of
being a platform driver.

Cool.

quoted

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 <mbrugger@suse.com>
Signed-off-by: Enric Balletbo i Serra <redacted>
Reviewed-by: CK Hu <redacted>
---

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.

quoted

quoted

 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.

quoted

quoted

+       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 <jamesjj.liao@mediatek.com>
+ */
+
+#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.

quoted

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

_______________________________________________

dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel