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

Fri Mar 6 22:09:50 UTC 2020

Hi Stephen,

Missatge de Stephen Boyd <sboyd at kernel.org> 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 <mbrugger at suse.com>
> > >>
> > >> 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 <mbrugger at suse.com>
> > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo at collabora.com>
> > >> Reviewed-by: CK Hu <ck.hu at mediatek.com>
> > >> ---
> > >>
> > >> 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 <jamesjj.liao at 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.
> >
> > > 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel