| Message ID | 20220411072403.24016-2-moudy.ho@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add mutex support for MDP | expand |
Il 11/04/22 09:23, Moudy Ho ha scritto: > In order to allow multiple modules to operate MUTEX hardware through > a common interfrace, a flexible index "mtk_mutex_table_index" needs to > be added to replace original component ID so that like DDP and MDP > can add their own MUTEX table settings independently. > > In addition, 4 generic interface "mtk_mutex_set_mod", "mtk_mutex_set_sof", > "mtk_mutex_clear_mod" and "mtk_mutex_clear_sof" have been added, which is > expected to replace the "mtk_mutex_add_comp" and "mtk_mutex_remove_comp" > pair originally dedicated to DDP in the future. > > Signed-off-by: Moudy Ho <moudy.ho@mediatek.com> > Change-Id: I6a2ab74fccf36248165ce4a6b268d82a1177afc9 > --- > drivers/soc/mediatek/mtk-mutex.c | 89 ++++++++++++++++++++++++++ > include/linux/soc/mediatek/mtk-mutex.h | 21 ++++++ > 2 files changed, 110 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-mutex.c b/drivers/soc/mediatek/mtk-mutex.c > index aaf8fc1abb43..48a04dce50d5 100644 > --- a/drivers/soc/mediatek/mtk-mutex.c > +++ b/drivers/soc/mediatek/mtk-mutex.c > @@ -156,6 +156,8 @@ struct mtk_mutex_data { > const unsigned int *mutex_sof; > const unsigned int mutex_mod_reg; > const unsigned int mutex_sof_reg; > + const unsigned int *mutex_table_mod; > + const unsigned int *mutex_table_sof; > const bool no_clk; > }; > > @@ -445,6 +447,54 @@ void mtk_mutex_add_comp(struct mtk_mutex *mutex, > } > EXPORT_SYMBOL_GPL(mtk_mutex_add_comp); > Hello Moudy, Some critical things, and one cleanup. First of all, the commit title is very long, and it also contains a typo. I would go for something like "soc: mediatek: mutex: Add common interface for modules setting". Also, please remove your internal "Change-Id" tag, this is meaningless on upstream, hence not applicable here. Now for the cleanup: I have an idea to make this a bit shorter (and please feel free to change function names with something more appropriate, if needed): static int mtk_mutex_write_mod(struct mtk_mutex *mutex, enum mtk_mutex_table_index idx, bool clear) { > +{ > + struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, > + mutex[mutex->id]); > + unsigned int reg; > + unsigned int offset; > + > + WARN_ON(&mtx->mutex[mutex->id] != mutex); > + > + if (idx < MUTEX_TABLE_IDX_MDP_RDMA0 || > + idx >= MUTEX_TABLE_IDX_MAX) { > + dev_err(mtx->dev, "Not supported MOD table index : %d", idx); > + return; return -EINVAL; > + } > + > + offset = DISP_REG_MUTEX_MOD(mtx->data->mutex_mod_reg, > + mutex->id); > + > + reg = readl_relaxed(mtx->regs + offset); if (clear) reg &= ~BIT(mtx->data->mutex_table_mod[idx]) else reg |= BIT(mtx->data->mutex_table_mod[idx]) > + reg |= 1 << mtx->data->mutex_table_mod[idx]; > + writel_relaxed(reg, mtx->regs + offset); > +} int mtk_mutex_set_mod(struct mtk_mutex *mutex, enum mtk_mutex_table_index idx) { return mtk_mutex_write_mod(mutex, idx, false); } int mtk_mutex_clear_mod(struct mtk_mutex *mutex, enum mtk_mutex_table_index idx) { return mtk_mutex_clear_mod(mutex, idx, true); } > +EXPORT_SYMBOL_GPL(mtk_mutex_set_mod); > + > +void mtk_mutex_set_sof(struct mtk_mutex *mutex, > + enum mtk_mutex_table_index idx) > +{ > + struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, > + mutex[mutex->id]); > + unsigned int sof_id; > + > + WARN_ON(&mtx->mutex[mutex->id] != mutex); > + > + if (idx < MUTEX_TABLE_IDX_MDP_RDMA0 || > + idx >= MUTEX_TABLE_IDX_MAX) { > + dev_err(mtx->dev, "Not supported SOF table index : %d", idx); > + return; > + } > + > + sof_id = mtx->data->mutex_table_sof[idx]; ... same changes here, except we'd have something like if (clear) val = MUTEX_SOF_SINGLE_MODE; else val = mtx->data->mutex_sof[sof_id]; writel_relaxed(val, ...etc) but feel free to give me valid reasons to not use this approach. In any case, the code looks ok to me. Regards, Angelo
On Wed, 2022-04-13 at 10:27 +0200, AngeloGioacchino Del Regno wrote: > Il 11/04/22 09:23, Moudy Ho ha scritto: > > In order to allow multiple modules to operate MUTEX hardware > > through > > a common interfrace, a flexible index "mtk_mutex_table_index" needs > > to > > be added to replace original component ID so that like DDP and MDP > > can add their own MUTEX table settings independently. > > > > In addition, 4 generic interface "mtk_mutex_set_mod", > > "mtk_mutex_set_sof", > > "mtk_mutex_clear_mod" and "mtk_mutex_clear_sof" have been added, > > which is > > expected to replace the "mtk_mutex_add_comp" and > > "mtk_mutex_remove_comp" > > pair originally dedicated to DDP in the future. > > > > Signed-off-by: Moudy Ho <moudy.ho@mediatek.com> > > Change-Id: I6a2ab74fccf36248165ce4a6b268d82a1177afc9 > > --- > > drivers/soc/mediatek/mtk-mutex.c | 89 > > ++++++++++++++++++++++++++ > > include/linux/soc/mediatek/mtk-mutex.h | 21 ++++++ > > 2 files changed, 110 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-mutex.c > > b/drivers/soc/mediatek/mtk-mutex.c > > index aaf8fc1abb43..48a04dce50d5 100644 > > --- a/drivers/soc/mediatek/mtk-mutex.c > > +++ b/drivers/soc/mediatek/mtk-mutex.c > > @@ -156,6 +156,8 @@ struct mtk_mutex_data { > > const unsigned int *mutex_sof; > > const unsigned int mutex_mod_reg; > > const unsigned int mutex_sof_reg; > > + const unsigned int *mutex_table_mod; > > + const unsigned int *mutex_table_sof; > > const bool no_clk; > > }; > > > > @@ -445,6 +447,54 @@ void mtk_mutex_add_comp(struct mtk_mutex > > *mutex, > > } > > EXPORT_SYMBOL_GPL(mtk_mutex_add_comp); > > > Hi Angelo, Thanks you for helping to point out the deficiencies. > Hello Moudy, > > Some critical things, and one cleanup. > > First of all, the commit title is very long, and it also contains a > typo. > I would go for something like > "soc: mediatek: mutex: Add common interface for modules setting". > As you said, this modification will make the description more explicit. > Also, please remove your internal "Change-Id" tag, this is > meaningless on > upstream, hence not applicable here. > Thanks for the correction, I also found this error and resent using the link below: Message ID = 20220411074925.25539-2-moudy.ho@mediatek.com/ It will be additionally confirmed when the next version is sent out. > Now for the cleanup: I have an idea to make this a bit shorter (and > please > feel free to change function names with something more appropriate, > if needed): > Appreciate for the good changes. As suggested, I'll add return values for error handling and simplify the set and clear MOD/SOF functions. Thanks, Moudy > static int mtk_mutex_write_mod(struct mtk_mutex *mutex, > enum mtk_mutex_table_index idx, > bool clear) > { > > > > +{ > > + struct mtk_mutex_ctx *mtx = container_of(mutex, struct > > mtk_mutex_ctx, > > + mutex[mutex->id]); > > + unsigned int reg; > > + unsigned int offset; > > + > > + WARN_ON(&mtx->mutex[mutex->id] != mutex); > > + > > + if (idx < MUTEX_TABLE_IDX_MDP_RDMA0 || > > + idx >= MUTEX_TABLE_IDX_MAX) { > > + dev_err(mtx->dev, "Not supported MOD table index : %d", > > idx); > > + return; > > return -EINVAL; > > > + } > > + > > + offset = DISP_REG_MUTEX_MOD(mtx->data->mutex_mod_reg, > > + mutex->id); > > + > > + reg = readl_relaxed(mtx->regs + offset); > > if (clear) > reg &= ~BIT(mtx->data->mutex_table_mod[idx]) > else > reg |= BIT(mtx->data->mutex_table_mod[idx]) > > > + reg |= 1 << mtx->data->mutex_table_mod[idx]; > > + writel_relaxed(reg, mtx->regs + offset); > > +} > > int mtk_mutex_set_mod(struct mtk_mutex *mutex, > enum mtk_mutex_table_index idx) > { > return mtk_mutex_write_mod(mutex, idx, false); > } > > int mtk_mutex_clear_mod(struct mtk_mutex *mutex, > enum mtk_mutex_table_index idx) > { > return mtk_mutex_clear_mod(mutex, idx, true); > } > > > +EXPORT_SYMBOL_GPL(mtk_mutex_set_mod); > > + > > +void mtk_mutex_set_sof(struct mtk_mutex *mutex, > > + enum mtk_mutex_table_index idx) > > +{ > > + struct mtk_mutex_ctx *mtx = container_of(mutex, struct > > mtk_mutex_ctx, > > + mutex[mutex->id]); > > + unsigned int sof_id; > > + > > + WARN_ON(&mtx->mutex[mutex->id] != mutex); > > + > > + if (idx < MUTEX_TABLE_IDX_MDP_RDMA0 || > > + idx >= MUTEX_TABLE_IDX_MAX) { > > + dev_err(mtx->dev, "Not supported SOF table index : %d", > > idx); > > + return; > > + } > > + > > + sof_id = mtx->data->mutex_table_sof[idx]; > > ... same changes here, except we'd have something like > > if (clear) > val = MUTEX_SOF_SINGLE_MODE; > else > val = mtx->data->mutex_sof[sof_id]; > > writel_relaxed(val, ...etc) > > but feel free to give me valid reasons to not use this approach. > > In any case, the code looks ok to me. > > > Regards, > Angelo
diff --git a/drivers/soc/mediatek/mtk-mutex.c b/drivers/soc/mediatek/mtk-mutex.c index aaf8fc1abb43..48a04dce50d5 100644 --- a/drivers/soc/mediatek/mtk-mutex.c +++ b/drivers/soc/mediatek/mtk-mutex.c @@ -156,6 +156,8 @@ struct mtk_mutex_data { const unsigned int *mutex_sof; const unsigned int mutex_mod_reg; const unsigned int mutex_sof_reg; + const unsigned int *mutex_table_mod; + const unsigned int *mutex_table_sof; const bool no_clk; }; @@ -445,6 +447,54 @@ void mtk_mutex_add_comp(struct mtk_mutex *mutex, } EXPORT_SYMBOL_GPL(mtk_mutex_add_comp); +void mtk_mutex_set_mod(struct mtk_mutex *mutex, + enum mtk_mutex_table_index idx) +{ + struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, + mutex[mutex->id]); + unsigned int reg; + unsigned int offset; + + WARN_ON(&mtx->mutex[mutex->id] != mutex); + + if (idx < MUTEX_TABLE_IDX_MDP_RDMA0 || + idx >= MUTEX_TABLE_IDX_MAX) { + dev_err(mtx->dev, "Not supported MOD table index : %d", idx); + return; + } + + offset = DISP_REG_MUTEX_MOD(mtx->data->mutex_mod_reg, + mutex->id); + + reg = readl_relaxed(mtx->regs + offset); + reg |= 1 << mtx->data->mutex_table_mod[idx]; + writel_relaxed(reg, mtx->regs + offset); +} +EXPORT_SYMBOL_GPL(mtk_mutex_set_mod); + +void mtk_mutex_set_sof(struct mtk_mutex *mutex, + enum mtk_mutex_table_index idx) +{ + struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, + mutex[mutex->id]); + unsigned int sof_id; + + WARN_ON(&mtx->mutex[mutex->id] != mutex); + + if (idx < MUTEX_TABLE_IDX_MDP_RDMA0 || + idx >= MUTEX_TABLE_IDX_MAX) { + dev_err(mtx->dev, "Not supported SOF table index : %d", idx); + return; + } + + sof_id = mtx->data->mutex_table_sof[idx]; + + writel_relaxed(mtx->data->mutex_sof[sof_id], + mtx->regs + + DISP_REG_MUTEX_SOF(mtx->data->mutex_sof_reg, mutex->id)); +} +EXPORT_SYMBOL_GPL(mtk_mutex_set_sof); + void mtk_mutex_remove_comp(struct mtk_mutex *mutex, enum mtk_ddp_comp_id id) { @@ -485,6 +535,45 @@ void mtk_mutex_remove_comp(struct mtk_mutex *mutex, } EXPORT_SYMBOL_GPL(mtk_mutex_remove_comp); +void mtk_mutex_clear_mod(struct mtk_mutex *mutex, + enum mtk_mutex_table_index idx) +{ + struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, + mutex[mutex->id]); + unsigned int reg; + unsigned int offset; + + WARN_ON(&mtx->mutex[mutex->id] != mutex); + + if (idx < MUTEX_TABLE_IDX_MDP_RDMA0 || + idx >= MUTEX_TABLE_IDX_MAX) { + dev_err(mtx->dev, "Not supported MOD table index : %d", idx); + return; + } + + offset = DISP_REG_MUTEX_MOD(mtx->data->mutex_mod_reg, + mutex->id); + + reg = readl_relaxed(mtx->regs + offset); + reg &= ~(1 << mtx->data->mutex_table_mod[idx]); + writel_relaxed(reg, mtx->regs + offset); +} +EXPORT_SYMBOL_GPL(mtk_mutex_clear_mod); + +void mtk_mutex_clear_sof(struct mtk_mutex *mutex) +{ + struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, + mutex[mutex->id]); + + WARN_ON(&mtx->mutex[mutex->id] != mutex); + + writel_relaxed(MUTEX_SOF_SINGLE_MODE, + mtx->regs + + DISP_REG_MUTEX_SOF(mtx->data->mutex_sof_reg, + mutex->id)); +} +EXPORT_SYMBOL_GPL(mtk_mutex_clear_sof); + void mtk_mutex_enable(struct mtk_mutex *mutex) { struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, diff --git a/include/linux/soc/mediatek/mtk-mutex.h b/include/linux/soc/mediatek/mtk-mutex.h index 6fe4ffbde290..200f4365c950 100644 --- a/include/linux/soc/mediatek/mtk-mutex.h +++ b/include/linux/soc/mediatek/mtk-mutex.h @@ -10,14 +10,35 @@ struct regmap; struct device; struct mtk_mutex; +enum mtk_mutex_table_index { + /* MDP table index */ + MUTEX_TABLE_IDX_MDP_RDMA0, + MUTEX_TABLE_IDX_MDP_RSZ0, + MUTEX_TABLE_IDX_MDP_RSZ1, + MUTEX_TABLE_IDX_MDP_TDSHP0, + MUTEX_TABLE_IDX_MDP_WROT0, + MUTEX_TABLE_IDX_MDP_WDMA, + MUTEX_TABLE_IDX_MDP_AAL0, + MUTEX_TABLE_IDX_MDP_CCORR0, + + MUTEX_TABLE_IDX_MAX /* ALWAYS keep at the end */ +}; + struct mtk_mutex *mtk_mutex_get(struct device *dev); int mtk_mutex_prepare(struct mtk_mutex *mutex); void mtk_mutex_add_comp(struct mtk_mutex *mutex, enum mtk_ddp_comp_id id); +void mtk_mutex_set_mod(struct mtk_mutex *mutex, + enum mtk_mutex_table_index idx); +void mtk_mutex_set_sof(struct mtk_mutex *mutex, + enum mtk_mutex_table_index idx); void mtk_mutex_enable(struct mtk_mutex *mutex); void mtk_mutex_disable(struct mtk_mutex *mutex); void mtk_mutex_remove_comp(struct mtk_mutex *mutex, enum mtk_ddp_comp_id id); +void mtk_mutex_clear_mod(struct mtk_mutex *mutex, + enum mtk_mutex_table_index idx); +void mtk_mutex_clear_sof(struct mtk_mutex *mutex); void mtk_mutex_unprepare(struct mtk_mutex *mutex); void mtk_mutex_put(struct mtk_mutex *mutex); void mtk_mutex_acquire(struct mtk_mutex *mutex);
In order to allow multiple modules to operate MUTEX hardware through a common interfrace, a flexible index "mtk_mutex_table_index" needs to be added to replace original component ID so that like DDP and MDP can add their own MUTEX table settings independently. In addition, 4 generic interface "mtk_mutex_set_mod", "mtk_mutex_set_sof", "mtk_mutex_clear_mod" and "mtk_mutex_clear_sof" have been added, which is expected to replace the "mtk_mutex_add_comp" and "mtk_mutex_remove_comp" pair originally dedicated to DDP in the future. Signed-off-by: Moudy Ho <moudy.ho@mediatek.com> Change-Id: I6a2ab74fccf36248165ce4a6b268d82a1177afc9 --- drivers/soc/mediatek/mtk-mutex.c | 89 ++++++++++++++++++++++++++ include/linux/soc/mediatek/mtk-mutex.h | 21 ++++++ 2 files changed, 110 insertions(+)