Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers

From: Enric Balletbo i Serra
Date: Thu Oct 15 2020 - 09:32:15 EST

Next message: Tom Rix: "Re: [PATCH v10 1/6] fpga: dfl: fix the definitions of type & feature_id for dfl devices"
Previous message: Christophe Leroy: "Re: [PATCH] powerpc: force inlining of csum_partial() to avoid multiple csum_partial() with GCC10"
In reply to: Chun-Kuang Hu: "Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Matthias and Chun-Kuang,

On 11/10/20 4:22, Chun-Kuang Hu wrote:
> Hi, Matthias:
>
> Matthias Brugger <matthias.bgg@xxxxxxxxx> 於 2020年10月8日週四下午6:22寫道：
>>
>> Hi Enric and CK,
>>
>> On 06/10/2020 21:33, Enric Balletbo i Serra wrote:
>>> From: CK Hu <ck.hu@xxxxxxxxxxxx>
>>>
>>> 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 <ck.hu@xxxxxxxxxxxx>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>>> ---
>>>
>>> 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 COLOR1_SEL_IN_OVL1 0x1
>>>
>>> #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_BLS 0x0
>>> #define DPI_SEL_IN_BLS 0x0
>>> +#define DPI_SEL_IN_MASK 0x1
>>> #define DSI_SEL_IN_RDMA 0x1
>>> +#define DSI_SEL_IN_MASK 0x1
>>> +
>>> +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.
>
> Agree.
>
>>
>>> + 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 struct mtk_mmsys_driver_data *data;
>>> };
>>>
>>> -static unsigned int mtk_mmsys_ddp_mout_en(enum mtk_ddp_comp_id cur,
>>> - enum mtk_ddp_comp_id next,
>>> - unsigned int *addr)
>>> -{
>>> - unsigned int value;
>>> -
>>> - if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_COLOR0) {
>>> - *addr = DISP_REG_CONFIG_DISP_OVL0_MOUT_EN;
>>> - value = OVL0_MOUT_EN_COLOR0;
>>> - } else if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_RDMA0) {
>>> - *addr = DISP_REG_CONFIG_DISP_OVL_MOUT_EN;
>>> - value = OVL_MOUT_EN_RDMA;
>>> - } else if (cur == DDP_COMPONENT_OD0 && next == DDP_COMPONENT_RDMA0) {
>>> - *addr = DISP_REG_CONFIG_DISP_OD_MOUT_EN;
>>> - value = OD_MOUT_EN_RDMA0;
>>> - } else if (cur == DDP_COMPONENT_UFOE && next == DDP_COMPONENT_DSI0) {
>>> - *addr = DISP_REG_CONFIG_DISP_UFOE_MOUT_EN;
>>> - value = UFOE_MOUT_EN_DSI0;
>>> - } else if (cur == DDP_COMPONENT_OVL1 && next == DDP_COMPONENT_COLOR1) {
>>> - *addr = DISP_REG_CONFIG_DISP_OVL1_MOUT_EN;
>>> - value = OVL1_MOUT_EN_COLOR1;
>>> - } else if (cur == DDP_COMPONENT_GAMMA && next == DDP_COMPONENT_RDMA1) {
>>> - *addr = DISP_REG_CONFIG_DISP_GAMMA_MOUT_EN;
>>> - value = GAMMA_MOUT_EN_RDMA1;
>>> - } else if (cur == DDP_COMPONENT_OD1 && next == DDP_COMPONENT_RDMA1) {
>>> - *addr = DISP_REG_CONFIG_DISP_OD_MOUT_EN;
>>> - value = OD1_MOUT_EN_RDMA1;
>>> - } else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DPI0) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
>>> - value = RDMA0_SOUT_DPI0;
>>> - } else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DPI1) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
>>> - value = RDMA0_SOUT_DPI1;
>>> - } else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI1) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
>>> - value = RDMA0_SOUT_DSI1;
>>> - } else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI2) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
>>> - value = RDMA0_SOUT_DSI2;
>>> - } else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI3) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
>>> - value = RDMA0_SOUT_DSI3;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI1) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
>>> - value = RDMA1_SOUT_DSI1;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI2) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
>>> - value = RDMA1_SOUT_DSI2;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI3) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
>>> - value = RDMA1_SOUT_DSI3;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
>>> - value = RDMA1_SOUT_DPI0;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI1) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
>>> - value = RDMA1_SOUT_DPI1;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DPI0) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
>>> - value = RDMA2_SOUT_DPI0;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DPI1) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
>>> - value = RDMA2_SOUT_DPI1;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI1) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
>>> - value = RDMA2_SOUT_DSI1;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI2) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
>>> - value = RDMA2_SOUT_DSI2;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI3) {
>>> - *addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
>>> - value = RDMA2_SOUT_DSI3;
>>> - } else {
>>> - value = 0;
>>> - }
>>> -
>>> - return value;
>>> -}
>>> -
>>> -static unsigned int mtk_mmsys_ddp_sel_in(enum mtk_ddp_comp_id cur,
>>> - enum mtk_ddp_comp_id next,
>>> - unsigned int *addr)
>>> -{
>>> - unsigned int value;
>>> -
>>> - if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_COLOR0) {
>>> - *addr = DISP_REG_CONFIG_DISP_COLOR0_SEL_IN;
>>> - value = COLOR0_SEL_IN_OVL0;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) {
>>> - *addr = DISP_REG_CONFIG_DPI_SEL_IN;
>>> - value = DPI0_SEL_IN_RDMA1;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI1) {
>>> - *addr = DISP_REG_CONFIG_DPI_SEL_IN;
>>> - value = DPI1_SEL_IN_RDMA1;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
>>> - *addr = DISP_REG_CONFIG_DSIE_SEL_IN;
>>> - value = DSI0_SEL_IN_RDMA1;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI1) {
>>> - *addr = DISP_REG_CONFIG_DSIO_SEL_IN;
>>> - value = DSI1_SEL_IN_RDMA1;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI2) {
>>> - *addr = DISP_REG_CONFIG_DSIE_SEL_IN;
>>> - value = DSI2_SEL_IN_RDMA1;
>>> - } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI3) {
>>> - *addr = DISP_REG_CONFIG_DSIO_SEL_IN;
>>> - value = DSI3_SEL_IN_RDMA1;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DPI0) {
>>> - *addr = DISP_REG_CONFIG_DPI_SEL_IN;
>>> - value = DPI0_SEL_IN_RDMA2;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DPI1) {
>>> - *addr = DISP_REG_CONFIG_DPI_SEL_IN;
>>> - value = DPI1_SEL_IN_RDMA2;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI0) {
>>> - *addr = DISP_REG_CONFIG_DSIE_SEL_IN;
>>> - value = DSI0_SEL_IN_RDMA2;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI1) {
>>> - *addr = DISP_REG_CONFIG_DSIO_SEL_IN;
>>> - value = DSI1_SEL_IN_RDMA2;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI2) {
>>> - *addr = DISP_REG_CONFIG_DSIE_SEL_IN;
>>> - value = DSI2_SEL_IN_RDMA2;
>>> - } else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI3) {
>>> - *addr = DISP_REG_CONFIG_DSIE_SEL_IN;
>>> - value = DSI3_SEL_IN_RDMA2;
>>> - } else if (cur == DDP_COMPONENT_OVL1 && next == DDP_COMPONENT_COLOR1) {
>>> - *addr = DISP_REG_CONFIG_DISP_COLOR1_SEL_IN;
>>> - value = COLOR1_SEL_IN_OVL1;
>>> - } else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
>>> - *addr = DISP_REG_CONFIG_DSI_SEL;
>>> - value = DSI_SEL_IN_BLS;
>>> - } else {
>>> - value = 0;
>>> +static const struct mtk_mmsys_routes mt8173_mmsys_routing_table[] = {
>>> + {
>>> + DDP_COMPONENT_BLS, DDP_COMPONENT_DSI0,
>>> + DISP_REG_CONFIG_OUT_SEL,
>>> + BLS_RDMA1_DSI_DPI_MASK, BLS_TO_DSI_RDMA1_TO_DPI1
>>> + }, {
>>> + DDP_COMPONENT_BLS, DDP_COMPONENT_DSI0,
>>> + DISP_REG_CONFIG_DSI_SEL,
>>> + DSI_SEL_IN_MASK, DSI_SEL_IN_BLS
>>> + }, {
>>> + DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
>>> + DISP_REG_CONFIG_OUT_SEL,
>>> + BLS_RDMA1_DSI_DPI_MASK, BLS_TO_DPI_RDMA1_TO_DSI
>>> + }, {
>>> + DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
>>> + DISP_REG_CONFIG_DSI_SEL,
>>> + DSI_SEL_IN_MASK, DSI_SEL_IN_RDMA
>>> + }, {
>>> + DDP_COMPONENT_BLS, DDP_COMPONENT_DPI0,
>>> + DISP_REG_CONFIG_DPI_SEL,
>>> + DPI_SEL_IN_MASK, DPI_SEL_IN_BLS
>>
>> I wonder if we could make the data structure easier to read by grouping actions
>> for the same component tuple together.
>
> Maybe if-else is the simple way to group then. Two layer array could
> achieve this but it's more complicated.
>
>>
>>> + }, {
>>> + DDP_COMPONENT_GAMMA, DDP_COMPONENT_RDMA1,
>>> + DISP_REG_CONFIG_DISP_GAMMA_MOUT_EN,
>>> + GAMMA_MOUT_EN_RDMA1, GAMMA_MOUT_EN_RDMA1
>>> + }, {
>>> + DDP_COMPONENT_OD0, DDP_COMPONENT_RDMA0,
>>> + DISP_REG_CONFIG_DISP_OD_MOUT_EN,
>>> + OD_MOUT_EN_RDMA0, OD_MOUT_EN_RDMA0
>>> + }, {
>>> + DDP_COMPONENT_OD1, DDP_COMPONENT_RDMA1,
>>> + DISP_REG_CONFIG_DISP_OD_MOUT_EN,
>>> + OD1_MOUT_EN_RDMA1, OD1_MOUT_EN_RDMA1
>>> + }, {
>>> + DDP_COMPONENT_OVL0, DDP_COMPONENT_COLOR0,
>>> + DISP_REG_CONFIG_DISP_OVL0_MOUT_EN,
>>> + OVL0_MOUT_EN_COLOR0, OVL0_MOUT_EN_COLOR0
>>> + }, {
>>> + DDP_COMPONENT_OVL0, DDP_COMPONENT_COLOR0,
>>> + DISP_REG_CONFIG_DISP_COLOR0_SEL_IN,
>>> + COLOR0_SEL_IN_OVL0, COLOR0_SEL_IN_OVL0
>>> + }, {
>>> + DDP_COMPONENT_OVL0, DDP_COMPONENT_RDMA0,
>>> + DISP_REG_CONFIG_DISP_OVL_MOUT_EN,
>>> + OVL_MOUT_EN_RDMA, OVL_MOUT_EN_RDMA
>>> + }, {
>>> + DDP_COMPONENT_OVL1, DDP_COMPONENT_COLOR1,
>>> + DISP_REG_CONFIG_DISP_OVL1_MOUT_EN,
>>> + OVL1_MOUT_EN_COLOR1, OVL1_MOUT_EN_COLOR1
>>> + }, {
>>> + DDP_COMPONENT_OVL1, DDP_COMPONENT_COLOR1,
>>> + DISP_REG_CONFIG_DISP_COLOR1_SEL_IN,
>>> + COLOR1_SEL_IN_OVL1, COLOR1_SEL_IN_OVL1
>>> + }, {
>>> + DDP_COMPONENT_RDMA0, DDP_COMPONENT_DPI0,
>>> + DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN,
>>> + RDMA0_SOUT_MASK, RDMA0_SOUT_DPI0
>>> + }, {
>>> + DDP_COMPONENT_RDMA0, DDP_COMPONENT_DPI1,
>>> + DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN,
>>> + RDMA0_SOUT_MASK, RDMA0_SOUT_DPI1
>>> + }, {
>>> + DDP_COMPONENT_RDMA0, DDP_COMPONENT_DSI1,
>>> + DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN,
>>> + RDMA0_SOUT_MASK, RDMA0_SOUT_DSI1
>>> + }, {
>>> + DDP_COMPONENT_RDMA0, DDP_COMPONENT_DSI2,
>>> + DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN,
>>> + RDMA0_SOUT_MASK, RDMA0_SOUT_DSI2
>>> + }, {
>>> + DDP_COMPONENT_RDMA0, DDP_COMPONENT_DSI3,
>>> + DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN,
>>> + RDMA0_SOUT_MASK, RDMA0_SOUT_DSI3
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DPI0,
>>> + DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN,
>>> + RDMA1_SOUT_MASK, RDMA1_SOUT_DPI0
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DPI0,
>>> + DISP_REG_CONFIG_DPI_SEL_IN,
>>> + DPI0_SEL_IN_MASK, DPI0_SEL_IN_RDMA1
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DPI1,
>>> + DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN,
>>> + RDMA1_SOUT_MASK, RDMA1_SOUT_DPI1
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DPI1,
>>> + DISP_REG_CONFIG_DPI_SEL_IN,
>>> + DPI1_SEL_IN_MASK, DPI1_SEL_IN_RDMA1
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DSI0,
>>> + DISP_REG_CONFIG_DSIE_SEL_IN,
>>> + DSI0_SEL_IN_MASK, DSI0_SEL_IN_RDMA1
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DSI1,
>>> + DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN,
>>> + RDMA1_SOUT_MASK, RDMA1_SOUT_DSI1
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DSI1,
>>> + DISP_REG_CONFIG_DSIO_SEL_IN,
>>> + DSI1_SEL_IN_MASK, DSI1_SEL_IN_RDMA1
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DSI2,
>>> + DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN,
>>> + RDMA1_SOUT_MASK, RDMA1_SOUT_DSI2
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DSI2,
>>> + DISP_REG_CONFIG_DSIE_SEL_IN,
>>> + DSI2_SEL_IN_MASK, DSI2_SEL_IN_RDMA1
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DSI3,
>>> + DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN,
>>> + RDMA1_SOUT_MASK, RDMA1_SOUT_DSI3
>>> + }, {
>>> + DDP_COMPONENT_RDMA1, DDP_COMPONENT_DSI3,
>>> + DISP_REG_CONFIG_DSIO_SEL_IN,
>>> + DSI3_SEL_IN_MASK, DSI3_SEL_IN_RDMA1
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DPI0,
>>> + DISP_REG_CONFIG_DISP_RDMA2_SOUT,
>>> + RDMA2_SOUT_MASK, RDMA2_SOUT_DPI0
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DPI0,
>>> + DISP_REG_CONFIG_DPI_SEL_IN,
>>> + DPI0_SEL_IN_MASK, DPI0_SEL_IN_RDMA2
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DPI1,
>>> + DISP_REG_CONFIG_DISP_RDMA2_SOUT,
>>> + RDMA2_SOUT_MASK, RDMA2_SOUT_DPI1
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DPI1,
>>> + DISP_REG_CONFIG_DPI_SEL_IN,
>>> + DPI1_SEL_IN_MASK, DPI1_SEL_IN_RDMA2
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DSI0,
>>> + DISP_REG_CONFIG_DSIE_SEL_IN,
>>> + DSI0_SEL_IN_MASK, DSI0_SEL_IN_RDMA2
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DSI1,
>>> + DISP_REG_CONFIG_DISP_RDMA2_SOUT,
>>> + RDMA2_SOUT_MASK, RDMA2_SOUT_DSI1
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DSI1,
>>> + DISP_REG_CONFIG_DSIO_SEL_IN,
>>> + DSI1_SEL_IN_MASK, DSI1_SEL_IN_RDMA2
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DSI2,
>>> + DISP_REG_CONFIG_DISP_RDMA2_SOUT,
>>> + RDMA2_SOUT_MASK, RDMA2_SOUT_DSI2
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DSI2,
>>> + DISP_REG_CONFIG_DSIE_SEL_IN,
>>> + DSI2_SEL_IN_MASK, DSI2_SEL_IN_RDMA2
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DSI3,
>>> + DISP_REG_CONFIG_DISP_RDMA2_SOUT,
>>> + RDMA2_SOUT_MASK, RDMA2_SOUT_DSI3
>>> + }, {
>>> + DDP_COMPONENT_RDMA2, DDP_COMPONENT_DSI3,
>>> + DISP_REG_CONFIG_DSIO_SEL_IN,
>>> + DSI3_SEL_IN_MASK, DSI3_SEL_IN_RDMA2
>>> }
>>> +};
>>>
>>> - return value;
>>> -}
>>> -
>>> -static void mtk_mmsys_ddp_sout_sel(void __iomem *config_regs,
>>> - enum mtk_ddp_comp_id cur,
>>> - enum mtk_ddp_comp_id next)
>>> -{
>>> - if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
>>> - writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
>>> - config_regs + DISP_REG_CONFIG_OUT_SEL);
>>> - } else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
>>> - writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
>>> - config_regs + DISP_REG_CONFIG_OUT_SEL);
>>> - writel_relaxed(DSI_SEL_IN_RDMA,
>>> - config_regs + DISP_REG_CONFIG_DSI_SEL);
>>> - writel_relaxed(DPI_SEL_IN_BLS,
>>> - config_regs + DISP_REG_CONFIG_DPI_SEL);
>>> - }
>>> -}
>>> +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),
>>> +};
>>>
>>> void mtk_mmsys_ddp_connect(struct device *dev,
>>> enum mtk_ddp_comp_id cur,
>>> enum mtk_ddp_comp_id next)
>>> {
>>> struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
>>> - unsigned int addr, value, reg;
>>> -
>>> - value = mtk_mmsys_ddp_mout_en(cur, next, &addr);
>>> - if (value) {
>>> - reg = readl_relaxed(mmsys->regs + addr) | value;
>>> - writel_relaxed(reg, mmsys->regs + addr);
>>> - }
>>> -
>>> - mtk_mmsys_ddp_sout_sel(mmsys->regs, cur, next);
>>> -
>>> - value = mtk_mmsys_ddp_sel_in(cur, next, &addr);
>>> - if (value) {
>>> - reg = readl_relaxed(mmsys->regs + addr) | value;
>>> - writel_relaxed(reg, mmsys->regs + addr);
>>> - }
>>
>> What I don't like of this new approach is the fact, that we loose information
>> about the different grouping we had up to now, mout_en, sout_sel and sel_in.
>
> Agree. Maybe we should keep if-else model in mt8173, mt2701, mt2712.
> And ask mt8183 [1] , mt8192 [2] to use if-else model.
>
> [1] https://patchwork.kernel.org/patch/11706231/
> [2] https://patchwork.kernel.org/patch/11725595/
>

Ok, I'll give a second round of these patches trying to address your comments.
Matthias, patch 1 and 2 are kind of independent/unrelated patches and can be
applied. I just added here to avoid conflicts. So I am wondering if you are fine
with them, if you can pick these two.

Thanks,
Enric

> Regards,
> Chun-Kuang.
>
>>
>> Regards,
>> Matthias

Next message: Tom Rix: "Re: [PATCH v10 1/6] fpga: dfl: fix the definitions of type & feature_id for dfl devices"
Previous message: Christophe Leroy: "Re: [PATCH] powerpc: force inlining of csum_partial() to avoid multiple csum_partial() with GCC10"
In reply to: Chun-Kuang Hu: "Re: [PATCH 4/4] soc: mediatek: mmsys: Use an array for setting the routing registers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]