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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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


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