Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers
Hi Matthias and Chun-Kuang, On 11/10/20 4:22, Chun-Kuang Hu wrote: > Hi, Matthias: > > Matthias Brugger 於 2020年10月8日 週四 下午6:22寫道： >> >> Hi Enric and CK, >> >> On 06/10/2020 21:33, Enric Balletbo i Serra wrote: >>> From: CK Hu >>> >>> Actually, setting the registers for routing, use multiple 'if-else' for >>> different >>> routes, but this code would be more and more complicated while we >>> support more and more SoCs. Change that and use a table per SoC so the >>> code will be more portable and clear. >>> >> >> I'd like to have some clarifications on this patch. Now we use one big >> if-else >> structure to decide which value and address we need. As this values work for >> a >> large set of SoCs I suppose the hardware block is the same. >> >> When we add new HW like mt8183, mt8192 or mt6779 we will need different >> values. >> But my question is, if the HW between theses new platforms will be different >> (read values change for every SoC). Or will it be the same HW block for all >> these SoCs so that we could add three new functions ddp_mout_en, ddp_sout_sel >> and ddp_sel_in. >> > > As I know, routes in mt8173, mt2701, mt2712 are different. That means > in the same register address, it control different input/output > selection for each SoC. But they use a very tricky way to use the same > table: some register's default value (the default routes) meet their > requirement, they do not set it and just set the register whose > default value does not meet the requirement. But this tricky way fail > when support more SoC, so mt8183 and mt8192 need an independent route. > So we have better have different route for each SoC, but we don't have > the complete route information for these three SoC, so just keep them > in the same table. After we've more information, we could separate > mt2701, mt2712 to an independent table. > >> Why I'm asking? Because right now it seems like double the work we have to do >> for every SoC. We will need to define the components in the DRM driver and >> then >> we need to add the values for the routing. > > The double work are two different function work. mmsys driver provide > full routing control and drm choose the route according to its > application. For example: > > mmsys provide the three route: > > rdma0 -> dsi0 > rdma1 -> dsi0 > rdma2 -> dsi0 > > and drm could only choose one at some moment. Even drm now just choose > rdma0 -> dsi0, I think mmsys still should provide the full route > control like clock driver (even though some clock is not use by client > driver, clock driver should implement all clock control function) > >> >> More comments below. >> >>> Signed-off-by: CK Hu >>> Signed-off-by: Enric Balletbo i Serra >>> --- >>> >>> drivers/soc/mediatek/mtk-mmsys.c | 393 +-- >>> 1 file changed, 210 insertions(+), 183 deletions(-) >>> >>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c >>> b/drivers/soc/mediatek/mtk-mmsys.c >>> index da2de8f6969e..f00d6d08c9c5 100644 >>> --- a/drivers/soc/mediatek/mtk-mmsys.c >>> +++ b/drivers/soc/mediatek/mtk-mmsys.c >>> @@ -42,39 +42,61 @@ >>> #define RDMA0_SOUT_DSI1 0x1 >>> #define RDMA0_SOUT_DSI2 0x4 >>> #define RDMA0_SOUT_DSI3 0x5 >>> +#define RDMA0_SOUT_MASK 0x7 >>> #define RDMA1_SOUT_DPI0 0x2 >>> #define RDMA1_SOUT_DPI1 0x3 >>> #define RDMA1_SOUT_DSI1 0x1 >>> #define RDMA1_SOUT_DSI2 0x4 >>> #define RDMA1_SOUT_DSI3 0x5 >>> +#define RDMA1_SOUT_MASK 0x7 >>> #define RDMA2_SOUT_DPI0 0x2 >>> #define RDMA2_SOUT_DPI1 0x3 >>> #define RDMA2_SOUT_DSI1 0x1 >>> #define RDMA2_SOUT_DSI2 0x4 >>> #define RDMA2_SOUT_DSI3 0x5 >>> +#define RDMA2_SOUT_MASK 0x7 >>> #define DPI0_SEL_IN_RDMA1 0x1 >>> #define DPI0_SEL_IN_RDMA2 0x3 >>> +#define DPI0_SEL_IN_MASK 0x3 >>> #define DPI1_SEL_IN_RDMA1 (0x1 << 8) >>> #define DPI1_SEL_IN_RDMA2 (0x3 << 8) >>> +#define DPI1_SEL_IN_MASK (0x3 << 8) >>> #define DSI0_SEL_IN_RDMA1 0x1 >>> #define DSI0_SEL_IN_RDMA2 0x4 >>> +#define DSI0_SEL_IN_MASK 0x7 >>> #define DSI1_SEL_IN_RDMA1 0x1 >>> #define DSI1_SEL_IN_RDMA2 0x4 >>> +#define DSI1_SEL_IN_MASK 0x7 >>> #define DSI2_SEL_IN_RDMA1 (0x1 << 16) >>> #define DSI2_SEL_IN_RDMA2 (0x4 << 16) >>> +#define DSI2_SEL_IN_MASK (0x7 << 16) >>> #define DSI3_SEL_IN_RDMA1
Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers
Hi, Matthias: Matthias Brugger 於 2020年10月8日 週四 下午6:22寫道： > > Hi Enric and CK, > > On 06/10/2020 21:33, Enric Balletbo i Serra wrote: > > From: CK Hu > > > > Actually, setting the registers for routing, use multiple 'if-else' for > > different > > routes, but this code would be more and more complicated while we > > support more and more SoCs. Change that and use a table per SoC so the > > code will be more portable and clear. > > > > I'd like to have some clarifications on this patch. Now we use one big if-else > structure to decide which value and address we need. As this values work for a > large set of SoCs I suppose the hardware block is the same. > > When we add new HW like mt8183, mt8192 or mt6779 we will need different > values. > But my question is, if the HW between theses new platforms will be different > (read values change for every SoC). Or will it be the same HW block for all > these SoCs so that we could add three new functions ddp_mout_en, ddp_sout_sel > and ddp_sel_in. > As I know, routes in mt8173, mt2701, mt2712 are different. That means in the same register address, it control different input/output selection for each SoC. But they use a very tricky way to use the same table: some register's default value (the default routes) meet their requirement, they do not set it and just set the register whose default value does not meet the requirement. But this tricky way fail when support more SoC, so mt8183 and mt8192 need an independent route. So we have better have different route for each SoC, but we don't have the complete route information for these three SoC, so just keep them in the same table. After we've more information, we could separate mt2701, mt2712 to an independent table. > Why I'm asking? Because right now it seems like double the work we have to do > for every SoC. We will need to define the components in the DRM driver and > then > we need to add the values for the routing. The double work are two different function work. mmsys driver provide full routing control and drm choose the route according to its application. For example: mmsys provide the three route: rdma0 -> dsi0 rdma1 -> dsi0 rdma2 -> dsi0 and drm could only choose one at some moment. Even drm now just choose rdma0 -> dsi0, I think mmsys still should provide the full route control like clock driver (even though some clock is not use by client driver, clock driver should implement all clock control function) > > More comments below. > > > Signed-off-by: CK Hu > > Signed-off-by: Enric Balletbo i Serra > > --- > > > > drivers/soc/mediatek/mtk-mmsys.c | 393 +-- > > 1 file changed, 210 insertions(+), 183 deletions(-) > > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c > > b/drivers/soc/mediatek/mtk-mmsys.c > > index da2de8f6969e..f00d6d08c9c5 100644 > > --- a/drivers/soc/mediatek/mtk-mmsys.c > > +++ b/drivers/soc/mediatek/mtk-mmsys.c > > @@ -42,39 +42,61 @@ > > #define RDMA0_SOUT_DSI1 0x1 > > #define RDMA0_SOUT_DSI2 0x4 > > #define RDMA0_SOUT_DSI3 0x5 > > +#define RDMA0_SOUT_MASK 0x7 > > #define RDMA1_SOUT_DPI0 0x2 > > #define RDMA1_SOUT_DPI1 0x3 > > #define RDMA1_SOUT_DSI1 0x1 > > #define RDMA1_SOUT_DSI2 0x4 > > #define RDMA1_SOUT_DSI3 0x5 > > +#define RDMA1_SOUT_MASK 0x7 > > #define RDMA2_SOUT_DPI0 0x2 > > #define RDMA2_SOUT_DPI1 0x3 > > #define RDMA2_SOUT_DSI1 0x1 > > #define RDMA2_SOUT_DSI2 0x4 > > #define RDMA2_SOUT_DSI3 0x5 > > +#define RDMA2_SOUT_MASK 0x7 > > #define DPI0_SEL_IN_RDMA1 0x1 > > #define DPI0_SEL_IN_RDMA2 0x3 > > +#define DPI0_SEL_IN_MASK 0x3 > > #define DPI1_SEL_IN_RDMA1 (0x1 << 8) > > #define DPI1_SEL_IN_RDMA2 (0x3 << 8) > > +#define DPI1_SEL_IN_MASK (0x3 << 8) > > #define DSI0_SEL_IN_RDMA1 0x1 > > #define DSI0_SEL_IN_RDMA2 0x4 > > +#define DSI0_SEL_IN_MASK 0x7 > > #define DSI1_SEL_IN_RDMA1 0x1 > > #define DSI1_SEL_IN_RDMA2 0x4 > > +#define DSI1_SEL_IN_MASK 0x7 > > #define DSI2_SEL_IN_RDMA1 (0x1 << 16) > > #define DSI2_SEL_IN_RDMA2 (0x4 << 16) > > +#define DSI2_SEL_IN_MASK (0x7 << 16) > > #define DSI3_SEL_IN_RDMA1 (0x1 << 16) > > #define DSI3_SEL_IN_RDMA2 (0x4 << 16) > > +#define DSI3_SEL_IN_MASK (0x7 << 16) > > #define
Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers
On 08/10/2020 09:49, Enric Balletbo i Serra wrote:
Hi Chun-Kuang,
On 8/10/20 2:01, Chun-Kuang Hu wrote:
Hi, Enric:
Enric Balletbo i Serra 於 2020年10月7日 週三 上午3:33寫道：
From: CK Hu
Actually, setting the registers for routing, use multiple 'if-else' for
different
routes, but this code would be more and more complicated while we
support more and more SoCs. Change that and use a table per SoC so the
code will be more portable and clear.
Signed-off-by: CK Hu
Signed-off-by: Enric Balletbo i Serra
---
drivers/soc/mediatek/mtk-mmsys.c | 393 +--
1 file changed, 210 insertions(+), 183 deletions(-)
[snip]
static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
@@ -93,10 +115,6 @@ static const struct mtk_mmsys_driver_data
mt6797_mmsys_driver_data = {
.clk_driver = "clk-mt6797-mm",
};
-static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
- .clk_driver = "clk-mt8173-mm",
-};
-
static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
.clk_driver = "clk-mt8183-mm",
};
@@ -106,180 +124,192 @@ struct mtk_mmsys {
const struct mtk_mmsys_driver_data *data;
};
[snip]
+static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
+ .clk_driver = "clk-mt8173-mm",
+ .routes = mt8173_mmsys_routing_table,
+ .num_routes = ARRAY_SIZE(mt8173_mmsys_routing_table),
+};
I remove my Reviewed-by tag. You does not set routes for mt2701 and
mt2712, but these two SoC need that. Maybe now they use the same table
as mt8173.
I did that on purpose as explained in the cover letter, and asked for someone
with the hardware to provide me a working routing table. But, if you think the
same routing should work on those devices I'm fine to use the same for all the
current devices. I don't have that hardware, so anyway, will need to test.
But you could deduce the routes needed by having a look into the components in
mtk_drm_drv.c, correct?
Well see my other email, but defining them twice sounds like not a good approach
to me.
Regards,
Matthias
Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers
Hi Enric and CK,
On 06/10/2020 21:33, Enric Balletbo i Serra wrote:
From: CK Hu
Actually, setting the registers for routing, use multiple 'if-else' for
different
routes, but this code would be more and more complicated while we
support more and more SoCs. Change that and use a table per SoC so the
code will be more portable and clear.
I'd like to have some clarifications on this patch. Now we use one big if-else
structure to decide which value and address we need. As this values work for a
large set of SoCs I suppose the hardware block is the same.
When we add new HW like mt8183, mt8192 or mt6779 we will need different values.
But my question is, if the HW between theses new platforms will be different
(read values change for every SoC). Or will it be the same HW block for all
these SoCs so that we could add three new functions ddp_mout_en, ddp_sout_sel
and ddp_sel_in.
Why I'm asking? Because right now it seems like double the work we have to do
for every SoC. We will need to define the components in the DRM driver and then
we need to add the values for the routing.
More comments below.
Signed-off-by: CK Hu
Signed-off-by: Enric Balletbo i Serra
---
drivers/soc/mediatek/mtk-mmsys.c | 393 +--
1 file changed, 210 insertions(+), 183 deletions(-)
diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
index da2de8f6969e..f00d6d08c9c5 100644
--- a/drivers/soc/mediatek/mtk-mmsys.c
+++ b/drivers/soc/mediatek/mtk-mmsys.c
@@ -42,39 +42,61 @@
#define RDMA0_SOUT_DSI1 0x1
#define RDMA0_SOUT_DSI2 0x4
#define RDMA0_SOUT_DSI3 0x5
+#define RDMA0_SOUT_MASK0x7
#define RDMA1_SOUT_DPI0 0x2
#define RDMA1_SOUT_DPI1 0x3
#define RDMA1_SOUT_DSI1 0x1
#define RDMA1_SOUT_DSI2 0x4
#define RDMA1_SOUT_DSI3 0x5
+#define RDMA1_SOUT_MASK0x7
#define RDMA2_SOUT_DPI0 0x2
#define RDMA2_SOUT_DPI1 0x3
#define RDMA2_SOUT_DSI1 0x1
#define RDMA2_SOUT_DSI2 0x4
#define RDMA2_SOUT_DSI3 0x5
+#define RDMA2_SOUT_MASK0x7
#define DPI0_SEL_IN_RDMA1 0x1
#define DPI0_SEL_IN_RDMA2 0x3
+#define DPI0_SEL_IN_MASK 0x3
#define DPI1_SEL_IN_RDMA1 (0x1 << 8)
#define DPI1_SEL_IN_RDMA2 (0x3 << 8)
+#define DPI1_SEL_IN_MASK (0x3 << 8)
#define DSI0_SEL_IN_RDMA1 0x1
#define DSI0_SEL_IN_RDMA2 0x4
+#define DSI0_SEL_IN_MASK 0x7
#define DSI1_SEL_IN_RDMA1 0x1
#define DSI1_SEL_IN_RDMA2 0x4
+#define DSI1_SEL_IN_MASK 0x7
#define DSI2_SEL_IN_RDMA1 (0x1 << 16)
#define DSI2_SEL_IN_RDMA2 (0x4 << 16)
+#define DSI2_SEL_IN_MASK (0x7 << 16)
#define DSI3_SEL_IN_RDMA1 (0x1 << 16)
#define DSI3_SEL_IN_RDMA2 (0x4 << 16)
+#define DSI3_SEL_IN_MASK (0x7 << 16)
#define COLOR1_SEL_IN_OVL10x1
#define OVL_MOUT_EN_RDMA 0x1
#define BLS_TO_DSI_RDMA1_TO_DPI1 0x8
#define BLS_TO_DPI_RDMA1_TO_DSI 0x2
+#define BLS_RDMA1_DSI_DPI_MASK 0xf
#define DSI_SEL_IN_BLS0x0
#define DPI_SEL_IN_BLS0x0
+#define DPI_SEL_IN_MASK0x1
#define DSI_SEL_IN_RDMA 0x1
+#define DSI_SEL_IN_MASK0x1
+
+struct mtk_mmsys_routes {
+ u32 from_comp;
+ u32 to_comp;
+ u32 addr;
+ u32 mask;
We didn't need the mask up to now, if needed in the future, we should add it
when needed. This will make the code cleaner for now.
+ u32 val;
+};
struct mtk_mmsys_driver_data {
const char *clk_driver;
+ const struct mtk_mmsys_routes *routes;
+ const unsigned int num_routes;
};
static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
@@ -93,10 +115,6 @@ static const struct mtk_mmsys_driver_data
mt6797_mmsys_driver_data = {
.clk_driver = "clk-mt6797-mm",
};
-static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
- .clk_driver = "clk-mt8173-mm",
-};
-
static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
.clk_driver = "clk-mt8183-mm",
};
@@ -106,180 +124,192 @@ struct mtk_mmsys {
const
Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers
Hi Chun-Kuang,
On 8/10/20 2:01, Chun-Kuang Hu wrote:
> Hi, Enric:
>
> Enric Balletbo i Serra 於 2020年10月7日 週三
> 上午3:33寫道：
>>
>> From: CK Hu
>>
>> Actually, setting the registers for routing, use multiple 'if-else' for
>> different
>> routes, but this code would be more and more complicated while we
>> support more and more SoCs. Change that and use a table per SoC so the
>> code will be more portable and clear.
>>
>> Signed-off-by: CK Hu
>> Signed-off-by: Enric Balletbo i Serra
>> ---
>>
>> drivers/soc/mediatek/mtk-mmsys.c | 393 +--
>> 1 file changed, 210 insertions(+), 183 deletions(-)
>>
>
> [snip]
>
>>
>> static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
>> @@ -93,10 +115,6 @@ static const struct mtk_mmsys_driver_data
>> mt6797_mmsys_driver_data = {
>> .clk_driver = "clk-mt6797-mm",
>> };
>>
>> -static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
>> - .clk_driver = "clk-mt8173-mm",
>> -};
>> -
>> static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
>> .clk_driver = "clk-mt8183-mm",
>> };
>> @@ -106,180 +124,192 @@ struct mtk_mmsys {
>> const struct mtk_mmsys_driver_data *data;
>> };
>>
>
> [snip]
>
>> +static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
>> + .clk_driver = "clk-mt8173-mm",
>> + .routes = mt8173_mmsys_routing_table,
>> + .num_routes = ARRAY_SIZE(mt8173_mmsys_routing_table),
>> +};
>>
>
> I remove my Reviewed-by tag. You does not set routes for mt2701 and
> mt2712, but these two SoC need that. Maybe now they use the same table
> as mt8173.
>
I did that on purpose as explained in the cover letter, and asked for someone
with the hardware to provide me a working routing table. But, if you think the
same routing should work on those devices I'm fine to use the same for all the
current devices. I don't have that hardware, so anyway, will need to test.
Thanks,
Enric
> Regards,
> Chun-Kuang.
>
Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers
Hi, Enric:
Enric Balletbo i Serra 於 2020年10月7日 週三 上午3:33寫道：
>
> From: CK Hu
>
> Actually, setting the registers for routing, use multiple 'if-else' for
> different
> routes, but this code would be more and more complicated while we
> support more and more SoCs. Change that and use a table per SoC so the
> code will be more portable and clear.
>
> Signed-off-by: CK Hu
> Signed-off-by: Enric Balletbo i Serra
> ---
>
> drivers/soc/mediatek/mtk-mmsys.c | 393 +--
> 1 file changed, 210 insertions(+), 183 deletions(-)
>
[snip]
>
> static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
> @@ -93,10 +115,6 @@ static const struct mtk_mmsys_driver_data
> mt6797_mmsys_driver_data = {
> .clk_driver = "clk-mt6797-mm",
> };
>
> -static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
> - .clk_driver = "clk-mt8173-mm",
> -};
> -
> static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
> .clk_driver = "clk-mt8183-mm",
> };
> @@ -106,180 +124,192 @@ struct mtk_mmsys {
> const struct mtk_mmsys_driver_data *data;
> };
>
[snip]
> +static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
> + .clk_driver = "clk-mt8173-mm",
> + .routes = mt8173_mmsys_routing_table,
> + .num_routes = ARRAY_SIZE(mt8173_mmsys_routing_table),
> +};
>
I remove my Reviewed-by tag. You does not set routes for mt2701 and
mt2712, but these two SoC need that. Maybe now they use the same table
as mt8173.
Regards,
Chun-Kuang.
Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers
Enric Balletbo i Serra 於 2020年10月7日 週三 上午3:33寫道： > > From: CK Hu > > Actually, setting the registers for routing, use multiple 'if-else' for > different > routes, but this code would be more and more complicated while we > support more and more SoCs. Change that and use a table per SoC so the > code will be more portable and clear. Reviewed-by: Chun-Kuang Hu > > Signed-off-by: CK Hu > Signed-off-by: Enric Balletbo i Serra > --- > > drivers/soc/mediatek/mtk-mmsys.c | 393 +-- > 1 file changed, 210 insertions(+), 183 deletions(-) >
