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

[Jason-JH Lin <jason-jh.lin@mediatek.com>]

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


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek