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

[AngeloGioacchino Del Regno]

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....

....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).

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 ...
}

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