Message-ID: <17b12339c2ac5231cbbd88358691cca7f58f8e3f.camel@mediatek.com>
Date: Wed, 2 Apr 2025 04:06:04 +0000
From: Paul-pl Chen (陳柏霖) <Paul-pl.Chen@...iatek.com>
To: "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...labora.com>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>
CC: Sunny Shen (沈姍姍) <Sunny.Shen@...iatek.com>,
	Sirius Wang (王皓昱) <Sirius.Wang@...iatek.com>,
	Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
	Xiandong Wang (王先冬)
	<Xiandong.Wang@...iatek.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, Project_Global_Chrome_Upstream_Group
	<Project_Global_Chrome_Upstream_Group@...iatek.com>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"fshao@...omium.org" <fshao@...omium.org>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, Singo Chang (張興國)
	<Singo.Chang@...iatek.com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "treapking@...omium.org" <treapking@...omium.org>
Subject: Re: [PATCH v2 06/15] soc: mediatek: add mmsys support for MT8196

On Mon, 2025-03-24 at 18:09 +0100, AngeloGioacchino Del Regno wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 21/03/25 10:33, paul-pl.chen ha scritto:
> > From: Nancy Lin <nancy.lin@...iatek.com>
> > 
> > 1. Defining driver data and adding compatible string
> > for different subsystems
> > (DISPSYS0, DISPSYS1, OVLSYS0, OVLSYS1, VDISP_AO)
> > 2. Adding functions to control top clocks and ddp clocks.
> > 3. Updating the probe function to initialize clocks and
> > enable runtime PM if its node has the power-domains property.
> > 4. Adding functions to configure ddp components and
> > set default configurations.
> > 5. Adding the routing table for each mmsys in MT8196.
> 
> You need at least two commits for all that you're doing here... and
> adding MT8196
> tables should be the last one.
> 
Hi AngeloGioacchino,

Thank you for your feedback. I appreciate your suggestion to split the
changes into at least two commits. Based on your advice, I'm
considering dividing the changes as follows:

Commit 1: Add mmsys support
This commit would include:
(1) Defining driver data and adding compatible strings for different
subsystems (DISPSYS0, DISPSYS1, OVLSYS0, OVLSYS1, VDISP_AO)
(2) Adding functions to control top clocks and ddp clocks
(3) Updating the probe function to initialize clocks and enable runtime
PM if its node has the power-domains property
(4) Adding functions to configure ddp components and set default
configurations

Commit 2: Add mmsys tables support for MT8196
This commit would focus on:
(5) Adding the routing table for each mmsys in MT8196

Does this division align with your expectations?
> > 
> > Signed-off-by: Nancy Lin <nancy.lin@...iatek.com>
> > Signed-off-by: Paul-pl Chen <paul-pl.chen@...iatek.com>
> > ---
> >   drivers/soc/mediatek/mt8196-mmsys.h    | 451
> > +++++++++++++++++++++++++
> >   drivers/soc/mediatek/mtk-mmsys.c       | 203 ++++++++++-
> >   drivers/soc/mediatek/mtk-mmsys.h       |  18 +
> >   include/linux/soc/mediatek/mtk-mmsys.h |  60 ++++
> >   4 files changed, 731 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/soc/mediatek/mt8196-mmsys.h
> > 
> > diff --git a/drivers/soc/mediatek/mt8196-mmsys.h
> > b/drivers/soc/mediatek/mt8196-mmsys.h
> > new file mode 100644
> > index 000000000000..ff841ae9939a
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mt8196-mmsys.h
> > @@ -0,0 +1,451 @@
> 
> ..snip..
> 
> > +static const struct mtk_mmsys_default
> > mmsys_mt8196_disp0_default_table[] = {
> > +     {MT8196_OVLSYS_GCE_EVENT_SEL, MT8196_EVENT_GCE_EN, GENMASK(1,
> > 0)},
> > +     {MT8196_DISP0_BYPASS_MUX_SHADOW,
> > +      MT8196_CB_BYPASS_MUX_SHADOW | MT8196_BYPASS_MUX_SHADOW,
> > +      MT8196_CB_BYPASS_MUX_SHADOW | MT8196_BYPASS_MUX_SHADOW},
> > +     {MT8196_DISP0_DLI_RELAY0, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +     {MT8196_DISP0_DLI_RELAY1, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +     {MT8196_DISP0_DLI_RELAY8, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +     {MT8196_DISP0_DLO_RELAY1, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +     {MT8196_DISP0_DLO_RELAY2, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +     {MT8196_DISP0_DLO_RELAY3, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +};
> > +
> > +static const struct mtk_mmsys_default
> > mmsys_mt8196_disp1_default_table[] = {
> > +     {MT8196_OVLSYS_GCE_EVENT_SEL, MT8196_EVENT_GCE_EN, GENMASK(1,
> > 0)},
> > +     {MT8196_DISP1_INT_MERGE, 0, BIT(0)},
> > +     {MT8196_DISP1_BYPASS_MUX_SHADOW,
> > +      MT8196_CB_BYPASS_MUX_SHADOW | MT8196_BYPASS_MUX_SHADOW,
> > +      MT8196_CB_BYPASS_MUX_SHADOW | MT8196_BYPASS_MUX_SHADOW},
> > +     {MT8196_DISP1_DLI_RELAY21, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +     {MT8196_DISP1_DLI_RELAY22, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +     {MT8196_DISP1_DLI_RELAY23, MT8196_DLI_RELAY_1T2P, GENMASK(31,
> > 30)},
> > +     {MT8196_DISP1_GCE_FRAME_DONE_SEL0, MT8196_FRAME_DONE_DVO,
> > GENMASK(5, 0)},
> > +     {MT8196_DISP1_GCE_FRAME_DONE_SEL1,
> > MT8196_FRAME_DONE_DP_INTF0, GENMASK(5, 0)},
> > +};
> > +
> > +static const struct mtk_mmsys_routes
> > mmsys_mt8196_ovl0_routing_table[] = {
> > +     {
> > +             DDP_COMPONENT_OVL0_EXDMA2,
> > DDP_COMPONENT_OVL0_BLENDER1,
> 
> There's a new MMSYS_ROUTE macro that was introduced because tables
> contained
> wrong values in multiple instances and in multiple SoCs: please use
> it here and
> for all routing tables.
> 
Sure, I will use the new MMSYS_ROUTE macro " MMSYS_ROUTE " to replace
the old routing table.
> > +             MT8196_OVL_RSZ_IN_CB2_MOUT_EN,
> > MT8196_DISP_OVL_EXDMA2_1_TO_OVL_EXDMA_OUT_CB3,
> > +             MT8196_DISP_OVL_EXDMA2_1_TO_OVL_EXDMA_OUT_CB3
> 
> ..snip..
> 
> > +};
> > +#endif /* __SOC_MEDIATEK_MT8196_MMSYS_H */
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > b/drivers/soc/mediatek/mtk-mmsys.c
> > index bb4639ca0b8c..1d3ca4f9f237 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -4,12 +4,14 @@
> >    * Author: James Liao <jamesjj.liao@...iatek.com>
> >    */
> > 
> > +#include <linux/clk.h>
> >   #include <linux/delay.h>
> >   #include <linux/device.h>
> >   #include <linux/io.h>
> >   #include <linux/module.h>
> >   #include <linux/of.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >   #include <linux/reset-controller.h>
> >   #include <linux/soc/mediatek/mtk-mmsys.h>
> > 
> > @@ -21,6 +23,7 @@
> >   #include "mt8188-mmsys.h"
> >   #include "mt8192-mmsys.h"
> >   #include "mt8195-mmsys.h"
> > +#include "mt8196-mmsys.h"
> >   #include "mt8365-mmsys.h"
> > 
> >   #define MMSYS_SW_RESET_PER_REG 32
> > @@ -144,6 +147,54 @@ static const struct mtk_mmsys_driver_data
> > mt8195_vppsys1_driver_data = {
> >       .is_vppsys = true,
> >   };
> > 
> > +static const struct mtk_mmsys_driver_data
> > mt8196_dispsys0_driver_data = {
> > +     .clk_driver = "clk-mt8196-disp0",
> > +     .routes = mmsys_mt8196_disp0_routing_table,
> > +     .num_routes = ARRAY_SIZE(mmsys_mt8196_disp0_routing_table),
> > +     .async_info = mmsys_mt8196_disp0_async_comp_table,
> > +     .num_async_info =
> > ARRAY_SIZE(mmsys_mt8196_disp0_async_comp_table),
> > +     .def_config = mmsys_mt8196_disp0_default_table,
> > +     .num_def_config =
> > ARRAY_SIZE(mmsys_mt8196_disp0_default_table),
> > +     .num_top_clk = 1,
> > +};
> > +
> > +static const struct mtk_mmsys_driver_data
> > mt8196_dispsys1_driver_data = {
> > +     .clk_driver = "clk-mt8196-disp1",
> > +     .routes = mmsys_mt8196_disp1_routing_table,
> > +     .num_routes = ARRAY_SIZE(mmsys_mt8196_disp1_routing_table),
> > +     .async_info = mmsys_mt8196_disp1_async_comp_table,
> > +     .num_async_info =
> > ARRAY_SIZE(mmsys_mt8196_disp1_async_comp_table),
> > +     .def_config = mmsys_mt8196_disp1_default_table,
> > +     .num_def_config =
> > ARRAY_SIZE(mmsys_mt8196_disp1_default_table),
> > +     .num_top_clk = 1,
> > +};
> > +
> > +static const struct mtk_mmsys_driver_data
> > mt8196_ovlsys0_driver_data = {
> > +     .clk_driver = "clk-mt8196-ovl0",
> > +     .routes = mmsys_mt8196_ovl0_routing_table,
> > +     .num_routes = ARRAY_SIZE(mmsys_mt8196_ovl0_routing_table),
> > +     .async_info = mmsys_mt8196_ovl0_async_comp_table,
> > +     .num_async_info =
> > ARRAY_SIZE(mmsys_mt8196_ovl0_async_comp_table),
> > +     .def_config = mmsys_mt8196_ovl0_default_table,
> > +     .num_def_config =
> > ARRAY_SIZE(mmsys_mt8196_ovl0_default_table),
> > +};
> > +
> > +static const struct mtk_mmsys_driver_data
> > mt8196_ovlsys1_driver_data = {
> > +     .clk_driver = "clk-mt8196-ovl1",
> > +     .routes = mmsys_mt8196_ovl1_routing_table,
> > +     .num_routes = ARRAY_SIZE(mmsys_mt8196_ovl1_routing_table),
> > +     .async_info = mmsys_mt8196_ovl1_async_comp_table,
> > +     .num_async_info =
> > ARRAY_SIZE(mmsys_mt8196_ovl1_async_comp_table),
> > +     .def_config = mmsys_mt8196_ovl0_default_table,
> > +     .num_def_config =
> > ARRAY_SIZE(mmsys_mt8196_ovl0_default_table),
> > +};
> > +
> > +static const struct mtk_mmsys_driver_data
> > mt8196_vdisp_ao_driver_data = {
> > +     .clk_driver = "clk-mt8196-vdisp_ao",
> > +     .def_config = mmsys_mt8196_vdisp_ao_default_table,
> > +     .num_def_config =
> > ARRAY_SIZE(mmsys_mt8196_vdisp_ao_default_table),
> > +};
> > +
> >   static const struct mtk_mmsys_driver_data
> > mt8365_mmsys_driver_data = {
> >       .clk_driver = "clk-mt8365-mm",
> >       .routes = mt8365_mmsys_routing_table,
> > @@ -158,6 +209,9 @@ struct mtk_mmsys {
> >       spinlock_t lock; /* protects mmsys_sw_rst_b reg */
> >       struct reset_controller_dev rcdev;
> >       struct cmdq_client_reg cmdq_base;
> > +     struct clk **async_clk;
> > +     int num_async_clk;
> > +     struct clk **top_clk;
> >   };
> > 
> >   static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32
> > offset, u32 mask, u32 val,
> > @@ -180,6 +234,99 @@ static void mtk_mmsys_update_bits(struct
> > mtk_mmsys *mmsys, u32 offset, u32 mask,
> >       writel_relaxed(tmp, mmsys->regs + offset);
> >   }
> > 
> > +int mtk_mmsys_top_clk_enable(struct device *dev)
> > +{
> > +     struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +     int ret, i;
> > +
> > +     if (!mmsys->data->num_top_clk)
> > +             return 0;
> > +
> > +     for (i = 0; i < mmsys->data->num_top_clk; i++)
> > +             ret = clk_prepare_enable(mmsys->top_clk[i]);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_top_clk_enable);
> > +
> > +void mtk_mmsys_top_clk_disable(struct device *dev)
> > +{
> > +     struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +     int i;
> > +
> > +     for (i = 0; i < mmsys->data->num_top_clk; i++)
> > +             clk_disable_unprepare(mmsys->top_clk[i]);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_top_clk_disable);
> > +
> > +int mtk_mmsys_ddp_clk_enable(struct device *dev, enum
> > mtk_ddp_comp_id comp_id)
> > +{
> > +     struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +     const struct mtk_mmsys_async_info *async = mmsys->data-
> > >async_info;
> > +
> > +     int i;
> > +
> > +     if (!mmsys->data->num_async_info)
> > +             return 0;
> > +
> > +     for (i = 0; i < mmsys->data->num_async_info; i++)
> > +             if (comp_id == async[i].comp_id)
> > +                     return clk_prepare_enable(mmsys-
> > >async_clk[async[i].index]);
> 
> Why can't you add the clocks in the mediatek-drm nodes and handle
> enablement in the
> drm driver?!
> 
The "async" is not like other components under mediate-drm that have
independent functions.It only controls which clocks need to be enabled
when the path MUX goes through certain routes.

That's why it's placed in mtk-mmsys. It's only activated when it needs
to be connected to the necessary path MUX.

Currently, the path order is represented through component IDs.
Therefore, to indicate its relative position on the DDP path, we
defined a component ID for it to use.

> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_clk_enable);
> > +
> > +void mtk_mmsys_ddp_clk_disable(struct device *dev, enum
> > mtk_ddp_comp_id comp_id)
> > +{
> > +     struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +     const struct mtk_mmsys_async_info *async = mmsys->data-
> > >async_info;
> > +     int i;
> > +
> > +     if (!mmsys->data->num_async_info)
> > +             return;
> > +
> > +     for (i = 0; i < mmsys->data->num_async_info; i++)
> > +             if (comp_id == async[i].comp_id)
> > +                     clk_disable_unprepare(mmsys-
> > >async_clk[async[i].index]);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_clk_disable);
> > +
> > +void mtk_mmsys_ddp_config(struct device *dev, enum mtk_ddp_comp_id
> > comp_id,
> > +                       int width, int height, struct cmdq_pkt
> > *cmdq_pkt)
> > +{
> > +     struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +     const struct mtk_mmsys_async_info *async = mmsys->data-
> > >async_info;
> > +     int i;
> > +
> > +     if (!mmsys->data->num_async_info)
> > +             return;
> > +
> > +     for (i = 0; i < mmsys->data->num_async_info; i++)
> > +             if (comp_id == async[i].comp_id)
> > +                     break;
> > +
> > +     if (i == mmsys->data->num_async_info)
> > +             return;
> > +
> > +     mtk_mmsys_update_bits(mmsys, async[i].offset, async[i].mask,
> > +                           height << 16 | width, cmdq_pkt);
> 
> linux/bitfield.h provides macros that you should use for those
> register fields.
> 

We use "mtk_mmsys_update_bits" to check if a cmdq packet exists.
If the cmdq packet exists, the function will make settings through
cmdq.
If cmdq is NULL, it will directly make settings through the CPU.

Regarding your comment
"linux/bitfield.h provides macros that you should use for those
register fields", are they suggesting that:

[1] We should use the macros provided by linux/bitfield.h to replace
expressions like "height << 16" 

or [2] We should rewrite mtk_mmsys_update_bits using kernel APIs
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_config);
> > +
> > +void mtk_mmsys_default_config(struct device *dev)
> > +{
> > +     struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +     const struct mtk_mmsys_default *def_config = mmsys->data-
> > >def_config;
> > +     int i;
> > +
> > +     if (!mmsys->data->num_def_config)
> > +             return;
> > +
> > +     for (i = 0; i < mmsys->data->num_def_config; i++)
> > +             mtk_mmsys_update_bits(mmsys, def_config[i].offset,
> > def_config[i].mask,
> > +                                   def_config[i].val, NULL);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_mmsys_default_config);
> > +
> >   void mtk_mmsys_ddp_connect(struct device *dev,
> >                          enum mtk_ddp_comp_id cur,
> >                          enum mtk_ddp_comp_id next)
> > @@ -390,7 +537,7 @@ static int mtk_mmsys_probe(struct
> > platform_device *pdev)
> >       struct platform_device *clks;
> >       struct platform_device *drm;
> >       struct mtk_mmsys *mmsys;
> > -     int ret;
> > +     int ret, i;
> > 
> >       mmsys = devm_kzalloc(dev, sizeof(*mmsys), GFP_KERNEL);
> >       if (!mmsys)
> > @@ -432,6 +579,49 @@ static int mtk_mmsys_probe(struct
> > platform_device *pdev)
> >               return PTR_ERR(clks);
> >       mmsys->clks_pdev = clks;
> > 
> > +     if (mmsys->data->num_top_clk) {
> > +             struct device_node *node;
> > +
> > +             node = of_get_child_by_name(dev->of_node, "top");
> 
> No, you can't do that if there's no binding to support that.
> 
We will add the "async" component in the MMSYS YAML file.
"async" is a nessceary clock in the specific routing display 
path MUX configuration
> 
> > +             if (!node) {
> > +                     dev_err(&pdev->dev, "Couldn't find top
> > node\n");
> > +                     return -EINVAL;
> > +             }
> > +
> > +             mmsys->top_clk = devm_kmalloc_array(dev, mmsys->data-
> > >num_top_clk,
> > +                                                 sizeof(*mmsys-
> > >top_clk), GFP_KERNEL);
> > +             if (!mmsys->top_clk)
> > +                     return -ENOMEM;
> > +
> > +             for (i = 0; i < mmsys->data->num_top_clk; i++) {
> > +                     mmsys->top_clk[i] = of_clk_get(node, i);
> > +                     if (IS_ERR(mmsys->top_clk[i]))
> > +                             return PTR_ERR(mmsys->top_clk[i]);
> > +             }
> > +     }
> > +
> > +     if (mmsys->data->num_async_info) {
> > +             struct device_node *node;
> > +
> > +             node = of_get_child_by_name(dev->of_node, "async");
> > +             if (!node) {
> > +                     dev_err(&pdev->dev, "Couldn't find async
> > node\n");
> > +                     return -EINVAL;
> > +             }
> > +
> 
> Also this looks like you have children providing only clocks?!
> I really need to look at the bindings to decide, but this looks
> wrong.
> 
We will add the "async" component in the MMSYS YAML file.
"async" is a nessceary clock in the specific routing display 
path MUX configuration
> Regards,
> Angelo
> 
> 
> Best Regards, 
Paul

Powered by blists - more mailing lists