Re: [RESEND v17 3/7] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

From: Jason-JH Lin
Date: Thu Apr 07 2022 - 22:43:05 EST

Hi Angelo,

Thanks for the reviews.

On Thu, 2022-04-07 at 11:11 +0200, AngeloGioacchino Del Regno wrote:
> Il 07/04/22 05:04, jason-jh.lin ha scritto:
> > 1. Add mt8195 mmsys compatible for vdosys0.
> > 2. Add mt8195 routing table settings and fix build fail.
> > 3. Add clock name, clock driver name and routing table into the
> > driver data
> > of mt8195 vdosys0.
> > 4. Add get match data by clock name function and clock platform
> > labels
> > to identify which mmsys node is corresponding to vdosys0.
> >
> > Signed-off-by: jason-jh.lin <jason-jh.lin@xxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 2 +-
> > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 6 +-
> > drivers/soc/mediatek/mt8167-mmsys.h | 2 +-
> > drivers/soc/mediatek/mt8183-mmsys.h | 2 +-
> > drivers/soc/mediatek/mt8186-mmsys.h | 4 +-
> > drivers/soc/mediatek/mt8192-mmsys.h | 4 +-
> > drivers/soc/mediatek/mt8195-mmsys.h | 370
> > ++++++++++++++++++++
> > drivers/soc/mediatek/mt8365-mmsys.h | 4 +-
> > drivers/soc/mediatek/mtk-mmsys.c | 62 ++++
> > drivers/soc/mediatek/mtk-mmsys.h | 1 +
> > drivers/soc/mediatek/mtk-mutex.c | 8 +-
> > include/linux/soc/mediatek/mtk-mmsys.h | 13 +-
> > 12 files changed, 461 insertions(+), 17 deletions(-)
> > create mode 100644 drivers/soc/mediatek/mt8195-mmsys.h
> >
>
> ..snip..
>
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > b/drivers/soc/mediatek/mtk-mmsys.c
> > index 4fc4c2c9ea20..b2fa239c5f5f 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -4,6 +4,8 @@
> > * Author: James Liao <jamesjj.liao@xxxxxxxxxxxx>
> > */
> >
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > #include <linux/delay.h>
> > #include <linux/device.h>
> > #include <linux/io.h>
> > @@ -17,6 +19,7 @@
> > #include "mt8183-mmsys.h"
> > #include "mt8186-mmsys.h"
> > #include "mt8192-mmsys.h"
> > +#include "mt8195-mmsys.h"
> > #include "mt8365-mmsys.h"
> >
> > static const struct mtk_mmsys_driver_data
> > mt2701_mmsys_driver_data = {
> > @@ -72,12 +75,24 @@ static const struct mtk_mmsys_driver_data
> > mt8192_mmsys_driver_data = {
> > .num_routes = ARRAY_SIZE(mmsys_mt8192_routing_table),
> > };
> >
> > +static const struct mtk_mmsys_driver_data
> > mt8195_vdosys0_driver_data = {
> > + .clk_name = "cfg_vdo0",
> > + .clk_driver = "clk-mt8195-vdo0",
> > + .routes = mmsys_mt8195_routing_table,
> > + .num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
> > +};
> > +
> > static const struct mtk_mmsys_driver_data
> > mt8365_mmsys_driver_data = {
> > .clk_driver = "clk-mt8365-mm",
> > .routes = mt8365_mmsys_routing_table,
> > .num_routes = ARRAY_SIZE(mt8365_mmsys_routing_table),
> > };
> >
> > +static const struct of_device_id mtk_clk_platform_labels[] = {
> > + { .compatible = "mediatek,mt8195-mmsys",
> > + .data = (void *)"clk-mt8195"},
>
> I have a hunch that MT8195 won't be the first and last SoC having
> multiple
> mmsys channels. I would tend to think that there will be more....
>

Yes, there will be another SoC with multiple mmsys channels...

> ....so, to make it clean from the beginning, I think that you should,
> at
> this point, assign a struct to that .data pointer, instead of
> declaring a
> drvdata struct into mtk_mmsys_get_match_data_by_clk_name().
>
> Besides, I think that this kind of usage for __clk_get_name() may be
> an API
> abuse... but I'm not sure about that... in any case:
> - if it's not an abuse, then you should simply pass
> mt8195_vdosys0_driver_data,
> or an array of pointers to mtk_mmsys_driver_data;
> - if this is an abuse, you can do the same checks by looking at the
> iostart
> (mmio base address) of the vdosys{0,1} node(s).

Do you mean that I should change clk_name to iostart like this?

mt8195_vdosys0_driver_data = {
.iostart = 0x1c01a000, // instead of clk_name
.clk_driver = "clk-mt8195-vdo0",
.routes = mmsys_mt8195_routing_table,
.num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
};

Just to confirm that address information can be disclosed here.
If it is not appropriate to use address here, I'll keep using clk_name.

> Honestly, though, I'm not even sure that you need this different
> of_device_id
> array here... since you could simply wrap the mtk_mmsys_driver_data
> in the
> of_match_mtk_mmsys that you have below... here's another idea:
>
> struct mtk_mmsys_match_data {
> const struct mtk_mmsys_driver_data *drv_data[];
> unsigned short num_drv_data;
> };
>
> ...so that:
>
> static int some_function_handling_multi_mmsys(struct mtk_mmsys
> *mmsys,
> struct
> mtk_mmsys_match_data *match)
> {
> int i;
>
> i = [ logic to find the right match->drv_data entry here ]
>
> return i;
> }
>
> static int mtk_mmsys_probe()
> {
> .... variables, something else ....
>
> if (match_data->num_drv_data > 1) {
> /* This SoC has multiple mmsys channels */
> ret = some_function_handling_multi_mmsys(mmsys);
> if (ret < 0)
> return ret;
>
> mmsys->data = match_data->drv_data[ret];
> } else {
> dev_dbg(dev, "Using single mmsys channel\n");
> mmsys->data = match_data->drv_data[0];
> }
>
> ...everything else that mtk_mmsys_probe does ...
> }

I've tried this idea in my local environment and it looks good.
So I'll apply this at the next version. Thanks for your idea!

> What I'm trying to communicate with this is that the currently chosen
> solution
> looks a bit fragile and needs to be made robust.
> In comparison, even if it's not technically right to have two
> different compatibles
> for the same hardware (and shall not be done), the former solution,
> even if wrong,
> was more robust than this one, imo.
>
> Regards,
> Angelo

Because we don't have a property to identify the different mmsys
directly (not using multi-mmsys handle function).

Although it make the code more complicated and not robust, but I think
this time it should be implemented for other multi-mmsys SoC in the
feature.


Regards,
Jason-JH.Lin

-
Jason-JH Lin <jason-jh.lin@xxxxxxxxxxxx>