Re: [RESEND v17 2/7] dt-bindings: arm: mediatek: mmsys: add mt8195 SoC binding

Re: [RESEND v17 2/7] dt-bindings: arm: mediatek: mmsys: add mt8195 SoC binding

[AngeloGioacchino Del Regno]

Il 07/04/22 05:04, jason-jh.lin ha scritto:
> In the SoC before, such as mt8173, it has 2 pipelines binding to one
> mmsys with the same clock driver and the same power domain.
> 
> In mt8195, there are 4 pipelines binding to 4 different mmsys, such as
> vdosys0, vdosys1, vppsys0 and vppsys1.
> Each mmsys uses different clock drivers and different power domain.
> 
> Since each mmsys has its own clock, they could be identified
> by the different name of their clock.
> 
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

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

[Jason-JH Lin]

Hi CK,

Thanks for the reviews.

On Thu, 2022-04-07 at 13:45 +0800, CK Hu wrote:
> Hi, Jason:
> 
> On Thu, 2022-04-07 at 11:04 +0800, jason-jh.lin wrote:
> > 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"},
> > +};
> > +
> >  struct mtk_mmsys {
> >  	void __iomem *regs;
> >  	const struct mtk_mmsys_driver_data *data;
> > @@ -85,6 +100,45 @@ struct mtk_mmsys {
> >  	struct reset_controller_dev rcdev;
> >  };
> >  
> > +static int mtk_mmsys_get_match_data_by_clk_name(const struct
> > mtk_mmsys_driver_data **data,
> > +						struct device *dev)
> > +{
> > +	int i;
> > +	struct clk *clk;
> > +	const char *clk_name;
> > +	const struct of_device_id *of_id =
> > of_match_node(mtk_clk_platform_labels,
> > +							 dev->of_node);
> > +	const struct mtk_mmsys_driver_data *drvdata[] = {
> > +		&mt8195_vdosys0_driver_data,
> > +	};
> > +
> > +	if (!of_id || !of_id->data) {
> > +		dev_err(dev, "Can't find match clk platform labels\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(clk)) {
> > +		dev_err(dev, "failed to get mmsys clk\n");
> > +		return PTR_ERR(clk);
> > +	}
> > +
> > +	clk_name = __clk_get_name(clk);
> > +	if (!clk_name) {
> > +		dev_err(dev, "invalid mmsys clk name\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(drvdata); i++)
> > +		if (strncmp(drvdata[i]->clk_name, clk_name,
> > strlen(clk_name)) == 0 &&
> > +		    strncmp(drvdata[i]->clk_driver, of_id->data,
> > strlen(of_id->data)) == 0) {
> 
> I think clk_name is enough to identify the mmsys, why do you need
> clk_driver?

I think there might be another chip that needs to get driver data by
clk_name .
So I use "clk-mt8195" in clk_driver to identify the corresponding
platform whose clk_name of mmsys is also "cfg_vod0".

> > +			*data = drvdata[i];
> > +			return 0;
> > +		}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  void mtk_mmsys_ddp_connect(struct device *dev,
> >  			   enum mtk_ddp_comp_id cur,
> >  			   enum mtk_ddp_comp_id next)
> > @@ -206,6 +260,11 @@ static int mtk_mmsys_probe(struct
> > platform_device *pdev)
> >  	}
> >  
> >  	mmsys->data = of_device_get_match_data(&pdev->dev);
> > +	if (!mmsys->data &&
> > mtk_mmsys_get_match_data_by_clk_name(&mmsys->data, dev) < 0) {
> > +		dev_err(dev, "Couldn't get match driver data\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	platform_set_drvdata(pdev, mmsys);
> >  
> >  	clks = platform_device_register_data(&pdev->dev, mmsys->data-
> > > clk_driver,
> > 
> > @@ -260,6 +319,9 @@ static const struct of_device_id
> > of_match_mtk_mmsys[] = {
> >  		.compatible = "mediatek,mt8192-mmsys",
> >  		.data = &mt8192_mmsys_driver_data,
> >  	},
> > +	{
> > +		.compatible = "mediatek,mt8195-mmsys",
> > +	},
> >  	{
> >  		.compatible = "mediatek,mt8365-mmsys",
> >  		.data = &mt8365_mmsys_driver_data,
> > 
> 
> [snip]
> 
> > b/include/linux/soc/mediatek/mtk-mmsys.h
> > index 4bba275e235a..fb719fd1281c 100644
> > --- a/include/linux/soc/mediatek/mtk-mmsys.h
> > +++ b/include/linux/soc/mediatek/mtk-mmsys.h
> > @@ -16,14 +16,25 @@ enum mtk_ddp_comp_id {
> >  	DDP_COMPONENT_CCORR,
> >  	DDP_COMPONENT_COLOR0,
> >  	DDP_COMPONENT_COLOR1,
> > -	DDP_COMPONENT_DITHER,
> > +	DDP_COMPONENT_DITHER0,
> 
> I would like soc and drm modification to go through different tree,
> so
> this setting would not modify drm driver in this patch.
> 
> DDP_COMPONENT_DITHER0 = DDP_COMPONENT_DITHER,
> 
> Then modify drm driver after this patch.
> 
> Regards,
> CK

OK, I will use this modification at the next version.
Thanks!

Regards,
Jason-JH.Lin

> 
> > +	DDP_COMPONENT_DITHER1,
> > +	DDP_COMPONENT_DP_INTF0,
> > +	DDP_COMPONENT_DP_INTF1,
> >  	DDP_COMPONENT_DPI0,
> >  	DDP_COMPONENT_DPI1,
> > +	DDP_COMPONENT_DSC0,
> > +	DDP_COMPONENT_DSC1,
> >  	DDP_COMPONENT_DSI0,
> >  	DDP_COMPONENT_DSI1,
> >  	DDP_COMPONENT_DSI2,
> >  	DDP_COMPONENT_DSI3,
> >  	DDP_COMPONENT_GAMMA,
> > +	DDP_COMPONENT_MERGE0,
> > +	DDP_COMPONENT_MERGE1,
> > +	DDP_COMPONENT_MERGE2,
> > +	DDP_COMPONENT_MERGE3,
> > +	DDP_COMPONENT_MERGE4,
> > +	DDP_COMPONENT_MERGE5,
> >  	DDP_COMPONENT_OD0,
> >  	DDP_COMPONENT_OD1,
> >  	DDP_COMPONENT_OVL0,
> 
> 
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>

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

[CK Hu]

Hi, Jason:

On Thu, 2022-04-07 at 14:27 +0800, Jason-JH Lin wrote:
> Hi CK,
> 
> Thanks for the reviews.
> 
> On Thu, 2022-04-07 at 13:45 +0800, CK Hu wrote:
> > Hi, Jason:
> > 
> > On Thu, 2022-04-07 at 11:04 +0800, jason-jh.lin wrote:
> > > 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"},
> > > +};
> > > +
> > >  struct mtk_mmsys {
> > >  	void __iomem *regs;
> > >  	const struct mtk_mmsys_driver_data *data;
> > > @@ -85,6 +100,45 @@ struct mtk_mmsys {
> > >  	struct reset_controller_dev rcdev;
> > >  };
> > >  
> > > +static int mtk_mmsys_get_match_data_by_clk_name(const struct
> > > mtk_mmsys_driver_data **data,
> > > +						struct device *dev)
> > > +{
> > > +	int i;
> > > +	struct clk *clk;
> > > +	const char *clk_name;
> > > +	const struct of_device_id *of_id =
> > > of_match_node(mtk_clk_platform_labels,
> > > +							 dev->of_node);
> > > +	const struct mtk_mmsys_driver_data *drvdata[] = {
> > > +		&mt8195_vdosys0_driver_data,
> > > +	};
> > > +
> > > +	if (!of_id || !of_id->data) {
> > > +		dev_err(dev, "Can't find match clk platform labels\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	clk = devm_clk_get(dev, NULL);
> > > +	if (IS_ERR(clk)) {
> > > +		dev_err(dev, "failed to get mmsys clk\n");
> > > +		return PTR_ERR(clk);
> > > +	}
> > > +
> > > +	clk_name = __clk_get_name(clk);
> > > +	if (!clk_name) {
> > > +		dev_err(dev, "invalid mmsys clk name\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(drvdata); i++)
> > > +		if (strncmp(drvdata[i]->clk_name, clk_name,
> > > strlen(clk_name)) == 0 &&
> > > +		    strncmp(drvdata[i]->clk_driver, of_id->data,
> > > strlen(of_id->data)) == 0) {
> > 
> > I think clk_name is enough to identify the mmsys, why do you need
> > clk_driver?
> 
> I think there might be another chip that needs to get driver data by
> clk_name .
> So I use "clk-mt8195" in clk_driver to identify the corresponding
> platform whose clk_name of mmsys is also "cfg_vod0".

We usually don't care the future because the future may not happen. If
it's sure that would happen, I think clk_driver is not a good choice.
For now, the clk_driver name is different for each SoC, but it could be
the same for each SoC because only one clock driver would be compiled.
I think "compatible" would be different for each SoC.

Regards,
CK

> 
> > > +			*data = drvdata[i];
> > > +			return 0;
> > > +		}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  void mtk_mmsys_ddp_connect(struct device *dev,
> > >  			   enum mtk_ddp_comp_id cur,
> > >  			   enum mtk_ddp_comp_id next)
> > > @@ -206,6 +260,11 @@ static int mtk_mmsys_probe(struct
> > > platform_device *pdev)
> > >  	}
> > >  
> > >  	mmsys->data = of_device_get_match_data(&pdev->dev);
> > > +	if (!mmsys->data &&
> > > mtk_mmsys_get_match_data_by_clk_name(&mmsys->data, dev) < 0) {
> > > +		dev_err(dev, "Couldn't get match driver data\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	platform_set_drvdata(pdev, mmsys);
> > >  
> > >  	clks = platform_device_register_data(&pdev->dev, mmsys->data-
> > > > clk_driver,
> > > 
> > > @@ -260,6 +319,9 @@ static const struct of_device_id
> > > of_match_mtk_mmsys[] = {
> > >  		.compatible = "mediatek,mt8192-mmsys",
> > >  		.data = &mt8192_mmsys_driver_data,
> > >  	},
> > > +	{
> > > +		.compatible = "mediatek,mt8195-mmsys",
> > > +	},
> > >  	{
> > >  		.compatible = "mediatek,mt8365-mmsys",
> > >  		.data = &mt8365_mmsys_driver_data,
> > > 
> > 
> > [snip]
> > 
> > > b/include/linux/soc/mediatek/mtk-mmsys.h
> > > index 4bba275e235a..fb719fd1281c 100644
> > > --- a/include/linux/soc/mediatek/mtk-mmsys.h
> > > +++ b/include/linux/soc/mediatek/mtk-mmsys.h
> > > @@ -16,14 +16,25 @@ enum mtk_ddp_comp_id {
> > >  	DDP_COMPONENT_CCORR,
> > >  	DDP_COMPONENT_COLOR0,
> > >  	DDP_COMPONENT_COLOR1,
> > > -	DDP_COMPONENT_DITHER,
> > > +	DDP_COMPONENT_DITHER0,
> > 
> > I would like soc and drm modification to go through different tree,
> > so
> > this setting would not modify drm driver in this patch.
> > 
> > DDP_COMPONENT_DITHER0 = DDP_COMPONENT_DITHER,
> > 
> > Then modify drm driver after this patch.
> > 
> > Regards,
> > CK
> 
> OK, I will use this modification at the next version.
> Thanks!
> 
> Regards,
> Jason-JH.Lin
> 
> > 
> > > +	DDP_COMPONENT_DITHER1,
> > > +	DDP_COMPONENT_DP_INTF0,
> > > +	DDP_COMPONENT_DP_INTF1,
> > >  	DDP_COMPONENT_DPI0,
> > >  	DDP_COMPONENT_DPI1,
> > > +	DDP_COMPONENT_DSC0,
> > > +	DDP_COMPONENT_DSC1,
> > >  	DDP_COMPONENT_DSI0,
> > >  	DDP_COMPONENT_DSI1,
> > >  	DDP_COMPONENT_DSI2,
> > >  	DDP_COMPONENT_DSI3,
> > >  	DDP_COMPONENT_GAMMA,
> > > +	DDP_COMPONENT_MERGE0,
> > > +	DDP_COMPONENT_MERGE1,
> > > +	DDP_COMPONENT_MERGE2,
> > > +	DDP_COMPONENT_MERGE3,
> > > +	DDP_COMPONENT_MERGE4,
> > > +	DDP_COMPONENT_MERGE5,
> > >  	DDP_COMPONENT_OD0,
> > >  	DDP_COMPONENT_OD1,
> > >  	DDP_COMPONENT_OVL0,
> > 
> > 

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

[AngeloGioacchino Del Regno]

Il 08/04/22 03:28, CK Hu ha scritto:
> Hi, Jason:
> 
> On Thu, 2022-04-07 at 14:27 +0800, Jason-JH Lin wrote:
>> Hi CK,
>>
>> Thanks for the reviews.
>>
>> On Thu, 2022-04-07 at 13:45 +0800, CK Hu wrote:
>>> Hi, Jason:
>>>
>>> On Thu, 2022-04-07 at 11:04 +0800, jason-jh.lin wrote:
>>>> 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>
>>>>    */
>>>>   

..snip..

>>
>> I think there might be another chip that needs to get driver data by
>> clk_name .
>> So I use "clk-mt8195" in clk_driver to identify the corresponding
>> platform whose clk_name of mmsys is also "cfg_vod0".
> 
> We usually don't care the future because the future may not happen. If

Hello CK,

I'm sorry, but I really have to disagree here.
Sure, the future may not happen, but from what I can see, MediaTek's commitment
on upstreaming their SoCs is continuative and they care about the future.

Let's also not forget that these drivers are not on a downstream tree, where
you don't care about the past or the future, but on upstream, where you:
- Definitely care about the past
- Should care about the future, if you want to avoid commit noise and
   making big changes to your drivers everytime, which would slow down
   your upstreaming due to reviewers having to put 3x efforts on each
   iteration.

And let's also not forget that this being upstream means that these drivers
may (or may not) be extended even by passionate community developers, for
which, having such mechanisms there for other SoCs that MediaTek didn't try
to upstream yet can only be good - and when these are engineered with a
certain flexibility, while keeping the codebase solid, that can only be good.

Besides, if I've misunderstood your "don't care the future" statement,
pretend that I've never replied.


> it's sure that would happen, I think clk_driver is not a good choice.
> For now, the clk_driver name is different for each SoC, but it could be
> the same for each SoC because only one clock driver would be compiled.
> I think "compatible" would be different for each SoC.
> 

...but I agree on that one (and I gave my own review and suggestions on
how to improve that situation).

Regards,
Angelo

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

[CK Hu]

Hi, Angelo:

On Fri, 2022-04-08 at 10:49 +0200, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 03:28, CK Hu ha scritto:
> > Hi, Jason:
> > 
> > On Thu, 2022-04-07 at 14:27 +0800, Jason-JH Lin wrote:
> > > Hi CK,
> > > 
> > > Thanks for the reviews.
> > > 
> > > On Thu, 2022-04-07 at 13:45 +0800, CK Hu wrote:
> > > > Hi, Jason:
> > > > 
> > > > On Thu, 2022-04-07 at 11:04 +0800, jason-jh.lin wrote:
> > > > > 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>
> > > > >    */
> > > > >   
> 
> ..snip..
> 
> > > 
> > > I think there might be another chip that needs to get driver data
> > > by
> > > clk_name .
> > > So I use "clk-mt8195" in clk_driver to identify the corresponding
> > > platform whose clk_name of mmsys is also "cfg_vod0".
> > 
> > We usually don't care the future because the future may not happen.
> > If
> 
> Hello CK,
> 
> I'm sorry, but I really have to disagree here.
> Sure, the future may not happen, but from what I can see, MediaTek's
> commitment
> on upstreaming their SoCs is continuative and they care about the
> future.
> 
> Let's also not forget that these drivers are not on a downstream
> tree, where
> you don't care about the past or the future, but on upstream, where
> you:
> - Definitely care about the past
> - Should care about the future, if you want to avoid commit noise and
>    making big changes to your drivers everytime, which would slow
> down
>    your upstreaming due to reviewers having to put 3x efforts on each
>    iteration.
> 
> And let's also not forget that this being upstream means that these
> drivers
> may (or may not) be extended even by passionate community developers,
> for
> which, having such mechanisms there for other SoCs that MediaTek
> didn't try
> to upstream yet can only be good - and when these are engineered with
> a
> certain flexibility, while keeping the codebase solid, that can only
> be good.
> 
> Besides, if I've misunderstood your "don't care the future"
> statement,
> pretend that I've never replied.

OK, let's break this patch into two patches. The first is to support
mt8195 only with clock name identification. The second patch is to
identify SoC. In this series, we just need the first patch, so move the
second patch to the series of another SoC with multiple mmsys device.
Maybe another SoC with multiple mmsys device has new property which
could be used to identify SoC, so we have no information about what is
the better implementation of second patch. I do really care the future,
but I have no information about the future. Please public any hidden
information so we could have better decision.

Regards,
CK

> 
> 
> > it's sure that would happen, I think clk_driver is not a good
> > choice.
> > For now, the clk_driver name is different for each SoC, but it
> > could be
> > the same for each SoC because only one clock driver would be
> > compiled.
> > I think "compatible" would be different for each SoC.
> > 
> 
> ...but I agree on that one (and I gave my own review and suggestions
> on
> how to improve that situation).
> 
> Regards,
> Angelo

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

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

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

[Jason-JH Lin]

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>

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

[AngeloGioacchino Del Regno]

Il 08/04/22 04:42, Jason-JH Lin ha scritto:
> 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.
> 

Yes Jason, even if that looks strange, it is an accepted behavior... at
least, on Qualcomm drivers, it was done exactly like that.

Besides, I'm sure that you will definitely agree with me that operations
on strings are way slower than checking "a number" :) :) :)


By the way, check that one out, that'll probably help you:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c?h=next-20220408#n789


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

You're welcome! Looking forward to the next version!

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

Yes, and I agree: please keep doing this future-proofing, it's a good thing,
as long as the code keeps being readable and robust!


Cheers,
Angelo

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

[Jason-JH Lin]

Hi CK,

Thanks for the reviews.

On Thu, 2022-04-07 at 13:58 +0800, CK Hu wrote:
> Hi, Jason:
> 
> On Thu, 2022-04-07 at 11:04 +0800, jason-jh.lin wrote:
> > 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>
> > 
> 
> [snip]
> 
> >  
> > +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),
> > +};
> > +
> > 
> 
> [snip]
> 
> >  
> > +static int mtk_mmsys_get_match_data_by_clk_name(const struct
> > mtk_mmsys_driver_data **data,
> > +						struct device *dev)
> > +{
> > +	int i;
> > +	struct clk *clk;
> > +	const char *clk_name;
> > +	const struct of_device_id *of_id =
> > of_match_node(mtk_clk_platform_labels,
> > +							 dev->of_node);
> > +	const struct mtk_mmsys_driver_data *drvdata[] = {
> > +		&mt8195_vdosys0_driver_data,
> > +	};
> > +
> > +	if (!of_id || !of_id->data) {
> > +		dev_err(dev, "Can't find match clk platform labels\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(clk)) {
> > +		dev_err(dev, "failed to get mmsys clk\n");
> > +		return PTR_ERR(clk);
> > +	}
> > +
> > +	clk_name = __clk_get_name(clk);
> > +	if (!clk_name) {
> > +		dev_err(dev, "invalid mmsys clk name\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(drvdata); i++)
> > +		if (strncmp(drvdata[i]->clk_name, clk_name,
> > strlen(clk_name)) == 0 &&
> 
> Why not
> 
> strcmp(drvdata[i]->clk_name, clk_name) == 0
> 
> Regards,
> CK
> 

I would like to block strings without '\0', but it doesn't seem
necessary. So I will change to strcmp at the next version.

Regards,
Jason-JH.Lin

> > +		    strncmp(drvdata[i]->clk_driver, of_id->data,
> > strlen(of_id->data)) == 0) {
> > +			*data = drvdata[i];
> > +			return 0;
> > +		}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> 
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!0MJsfiFOYUann5GooGDcIv4Lgm1FaCXDl8dDFCwiONgD0zJn0PQwJuV05-tZNZwVBxVS$
>  
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>