Hi, Matthias
On Mon, 2021-03-29 at 13:16 +0200, Matthias Brugger wrote:
> As a general comment:
>
> Please split your patch in several, one introducing changes to the existing
> code
> base which are needed for newer SoCs (depending on the changes more then one)
> and one which actually adds support for the new SoC.
>
> More comments below.
>
Thanks for the advice.
I will try to split it in the next version.
>
> On 26/03/2021 08:31, Nina Wu wrote:
> > From: Nina Wu <nina-cm...@mediatek.com>
> >
> > The hardware architecture of mt8192 devapc is slightly
> > different from that of the previous IC.
> > We add necessary extensions to support mt8192 and be
> > back-compatible with other ICs.
> >
> > Signed-off-by: Nina Wu <nina-cm...@mediatek.com>
> > ---
> > drivers/soc/mediatek/mtk-devapc.c | 213
> > ++++++++++++++++++++++++++++----------
> > 1 file changed, 156 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/soc/mediatek/mtk-devapc.c
> > b/drivers/soc/mediatek/mtk-devapc.c
> > index f1cea04..1e40a52 100644
> > --- a/drivers/soc/mediatek/mtk-devapc.c
> > +++ b/drivers/soc/mediatek/mtk-devapc.c
> > @@ -15,6 +15,11 @@
> > #define VIO_MOD_TO_REG_IND(m) ((m) / 32)
> > #define VIO_MOD_TO_REG_OFF(m) ((m) % 32)
> >
> > +#define FOR_EACH_SLAVE_TYPE(ctx, idx) \
> > + for ((idx) = 0; (idx) < (ctx)->slave_type_num; (idx)++)
>
> Not really needed, please drop.
>
> > +#define BASE(i) (ctx->base_list[i])
>
> same here.
>
> > +#define VIO_IDX_NUM(i) (ctx->vio_idx_num[i])
>
> same here.
>
OK, will fixed in the next version.
> > +
> > struct mtk_devapc_vio_dbgs {
> > union {
> > u32 vio_dbg0;
> > @@ -26,20 +31,28 @@ struct mtk_devapc_vio_dbgs {
> > u32 addr_h:4;
> > u32 resv:4;
> > } dbg0_bits;
> > +
> > + /* Not used, reference only */
> > + struct {
> > + u32 dmnid:6;
> > + u32 vio_w:1;
> > + u32 vio_r:1;
> > + u32 addr_h:4;
> > + u32 resv:20;
> > + } dbg0_bits_ver2;
> > };
> >
> > u32 vio_dbg1;
> > + u32 vio_dbg2;
> > };
> >
> > struct mtk_devapc_data {
> > - /* numbers of violation index */
> > - u32 vio_idx_num;
> > -
> > /* reg offset */
> > u32 vio_mask_offset;
> > u32 vio_sta_offset;
> > u32 vio_dbg0_offset;
> > u32 vio_dbg1_offset;
> > + u32 vio_dbg2_offset;
> > u32 apc_con_offset;
> > u32 vio_shift_sta_offset;
> > u32 vio_shift_sel_offset;
> > @@ -48,7 +61,10 @@ struct mtk_devapc_data {
> >
> > struct mtk_devapc_context {
> > struct device *dev;
> > - void __iomem *infra_base;
> > + u32 arch_ver;
> > + u32 slave_type_num;
> > + void __iomem **base_list;
> > + u32 *vio_idx_num;
> > struct clk *infra_clk;
> > const struct mtk_devapc_data *data;
> > };
> > @@ -56,39 +72,39 @@ struct mtk_devapc_context {
> > static void clear_vio_status(struct mtk_devapc_context *ctx)
> > {
> > void __iomem *reg;
> > - int i;
> > + int i, j;
> >
> > - reg = ctx->infra_base + ctx->data->vio_sta_offset;
> > + FOR_EACH_SLAVE_TYPE(ctx, i) {
> > + reg = BASE(i) + ctx->data->vio_sta_offset;
> >
> > - for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1; i++)
> > - writel(GENMASK(31, 0), reg + 4 * i);
> > + for (j = 0; j < VIO_MOD_TO_REG_IND(VIO_IDX_NUM(i) - 1); j++)
> > + writel(GENMASK(31, 0), reg + 4 * j);
> > +
> > + writel(GENMASK(VIO_MOD_TO_REG_OFF(VIO_IDX_NUM(i) - 1), 0),
> > + reg + 4 * j);
> > + }
> >
> > - writel(GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1, 0),
> > - reg + 4 * i);
> > }
> >
> > -static void mask_module_irq(struct mtk_devapc_context *ctx, bool mask)
> > +static void mask_module_irq(void __iomem *reg, int vio_idx_num, bool mask)
> > {
> > - void __iomem *reg;
> > u32 val;
> > int i;
> >
> > - reg = ctx->infra_base + ctx->data->vio_mask_offset;
> > -
> > if (mask)
> > val = GENMASK(31, 0);
> > else
> > val = 0;
> >
> > - for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1; i++)
> > + for (i = 0; i < VIO_MOD_TO_REG_IND(vio_idx_num - 1); i++)
> > writel(val, reg + 4 * i);
> >
> > val = readl(reg + 4 * i);
> > if (mask)
> > - val |= GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1,
> > + val |= GENMASK(VIO_MOD_TO_REG_OFF(vio_idx_num - 1),
> > 0);
> > else
> > - val &= ~GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1,
> > + val &= ~GENMASK(VIO_MOD_TO_REG_OFF(vio_idx_num - 1),
> > 0);
> >
> > writel(val, reg + 4 * i);
> > @@ -108,6 +124,8 @@ static void mask_module_irq(struct mtk_devapc_context
> > *ctx, bool mask)
> > */
> > static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
> > {
> > + int i;
> > + void __iomem *reg_base;
>
> Not needed.
will be removed.
>
> > void __iomem *pd_vio_shift_sta_reg;
> > void __iomem *pd_vio_shift_sel_reg;
> > void __iomem *pd_vio_shift_con_reg;
> > @@ -115,57 +133,87 @@ static int devapc_sync_vio_dbg(struct
> > mtk_devapc_context *ctx)
> > int ret;
> > u32 val;
> >
> > - pd_vio_shift_sta_reg = ctx->infra_base +
> > - ctx->data->vio_shift_sta_offset;
> > - pd_vio_shift_sel_reg = ctx->infra_base +
> > - ctx->data->vio_shift_sel_offset;
> > - pd_vio_shift_con_reg = ctx->infra_base +
> > - ctx->data->vio_shift_con_offset;
> > + FOR_EACH_SLAVE_TYPE(ctx, i) {
> > + reg_base = BASE(i);
> >
> > - /* Find the minimum shift group which has violation */
> > - val = readl(pd_vio_shift_sta_reg);
> > - if (!val)
> > - return false;
> > + pd_vio_shift_sta_reg = reg_base +
> > + ctx->data->vio_shift_sta_offset;
> > + pd_vio_shift_sel_reg = reg_base +
> > + ctx->data->vio_shift_sel_offset;
> > + pd_vio_shift_con_reg = reg_base +
> > + ctx->data->vio_shift_con_offset;
> >
> > - min_shift_group = __ffs(val);
> > + /* Find the minimum shift group which has violation */
> > + val = readl(pd_vio_shift_sta_reg);
> > + if (!val)
> > + continue;
> >
> > - /* Assign the group to sync */
> > - writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
> > + min_shift_group = __ffs(val);
> >
> > - /* Start syncing */
> > - writel(0x1, pd_vio_shift_con_reg);
> > + /* Assign the group to sync */
> > + writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
> >
> > - ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> > - PHY_DEVAPC_TIMEOUT);
> > - if (ret) {
> > - dev_err(ctx->dev, "%s: Shift violation info failed\n",
> > __func__);
> > - return false;
> > - }
> > + /* Start syncing */
> > + writel(0x1, pd_vio_shift_con_reg);
> > +
> > + ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3,
> > + 0, PHY_DEVAPC_TIMEOUT);
> > + if (ret) {
> > + dev_err(ctx->dev, "%s: Shift violation info failed\n",
> > + __func__);
> > + return -ETIMEDOUT;
> > + }
> >
> > - /* Stop syncing */
> > - writel(0x0, pd_vio_shift_con_reg);
> > + /* Stop syncing */
> > + writel(0x0, pd_vio_shift_con_reg);
> >
> > - /* Write clear */
> > - writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
> > + /* Write clear */
> > + writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
> >
> > - return true;
> > + return i;
>
> Not sure why you change that.
There is multiple devapc HW here sharing the same interrupt.
When interrupts come, all devapc HW should be checked.
I just got some advice from other reviewer.
I will drop this modification and try to have multiple nodes in DT
instead.
>
> > + }
> > +
> > + /* No violation found */
> > + return -ENODATA;
> > }
> >
> > /*
> > * devapc_extract_vio_dbg - extract full violation information after doing
> > * shift mechanism.
> > */
> > -static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx,
> > + int vio_slave_type)
> > {
> > struct mtk_devapc_vio_dbgs vio_dbgs;
> > void __iomem *vio_dbg0_reg;
> > void __iomem *vio_dbg1_reg;
> > + void __iomem *vio_dbg2_reg;
> > + u32 vio_addr, bus_id;
> >
> > - vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
> > - vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
> > + vio_dbg0_reg = BASE(vio_slave_type) + ctx->data->vio_dbg0_offset;
> > + vio_dbg1_reg = BASE(vio_slave_type) + ctx->data->vio_dbg1_offset;
> > + vio_dbg2_reg = BASE(vio_slave_type) + ctx->data->vio_dbg2_offset;
> >
> > vio_dbgs.vio_dbg0 = readl(vio_dbg0_reg);
> > vio_dbgs.vio_dbg1 = readl(vio_dbg1_reg);
> > + vio_dbgs.vio_dbg2 = readl(vio_dbg2_reg);
> > +
> > + switch (ctx->arch_ver) {
> > + case 1:
> > + bus_id = vio_dbgs.dbg0_bits.mstid;
> > + vio_addr = vio_dbgs.vio_dbg1;
> > + break;
> > + case 2:
> > + bus_id = vio_dbgs.vio_dbg1;
> > + vio_addr = vio_dbgs.vio_dbg2;
> > +
> > + /* To align with the bit definition of arch_ver 1 */
> > + vio_dbgs.vio_dbg0 = (vio_dbgs.vio_dbg0 << 16);
> > + break;
> > + default:
> > + /* Not Supported */
> > + return;
> > + }
> >
> > /* Print violation information */
> > if (vio_dbgs.dbg0_bits.vio_w)
> > @@ -174,8 +222,7 @@ static void devapc_extract_vio_dbg(struct
> > mtk_devapc_context *ctx)
> > dev_info(ctx->dev, "Read Violation\n");
> >
> > dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
> > - vio_dbgs.dbg0_bits.mstid, vio_dbgs.dbg0_bits.dmnid,
> > - vio_dbgs.vio_dbg1);
> > + bus_id, vio_dbgs.dbg0_bits.dmnid, vio_addr);
> > }
> >
> > /*
> > @@ -186,9 +233,10 @@ static void devapc_extract_vio_dbg(struct
> > mtk_devapc_context *ctx)
> > static irqreturn_t devapc_violation_irq(int irq_number, void *data)
> > {
> > struct mtk_devapc_context *ctx = data;
> > + int vio_slave_type;
> >
> > - while (devapc_sync_vio_dbg(ctx))
> > - devapc_extract_vio_dbg(ctx);
> > + while ((vio_slave_type = devapc_sync_vio_dbg(ctx)) >= 0)
> > + devapc_extract_vio_dbg(ctx, vio_slave_type);
> >
> > clear_vio_status(ctx);
> >
> > @@ -200,9 +248,15 @@ static irqreturn_t devapc_violation_irq(int
> > irq_number, void *data)
> > */
> > static void start_devapc(struct mtk_devapc_context *ctx)
> > {
> > - writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);
> > + int i;
> > + void __iomem *reg_base;
> >
> > - mask_module_irq(ctx, false);
> > + FOR_EACH_SLAVE_TYPE(ctx, i) {
> > + writel(BIT(31), BASE(i) + ctx->data->apc_con_offset);
> > +
> > + reg_base = BASE(i) + ctx->data->vio_mask_offset;
> > + mask_module_irq(reg_base, VIO_IDX_NUM(i), false);
> > + }
> > }
> >
> > /*
> > @@ -210,13 +264,18 @@ static void start_devapc(struct mtk_devapc_context
> > *ctx)
> > */
> > static void stop_devapc(struct mtk_devapc_context *ctx)
> > {
> > - mask_module_irq(ctx, true);
> > + int i;
> > + void __iomem *reg_base;
> > +
> > + FOR_EACH_SLAVE_TYPE(ctx, i) {
> > + reg_base = BASE(i) + ctx->data->vio_mask_offset;
> > + mask_module_irq(reg_base, VIO_IDX_NUM(i), true);
> >
> > - writel(BIT(2), ctx->infra_base + ctx->data->apc_con_offset);
> > + writel(BIT(2), BASE(i) + ctx->data->apc_con_offset);
> > + }
> > }
> >
> > static const struct mtk_devapc_data devapc_mt6779 = {
> > - .vio_idx_num = 511,
> > .vio_mask_offset = 0x0,
> > .vio_sta_offset = 0x400,
> > .vio_dbg0_offset = 0x900,
> > @@ -227,11 +286,26 @@ static const struct mtk_devapc_data devapc_mt6779 = {
> > .vio_shift_con_offset = 0xF20,
> > };
> >
> > +static const struct mtk_devapc_data devapc_mt8192 = {
> > + .vio_mask_offset = 0x0,
> > + .vio_sta_offset = 0x400,
> > + .vio_dbg0_offset = 0x900,
> > + .vio_dbg1_offset = 0x904,
> > + .vio_dbg2_offset = 0x908,
> > + .apc_con_offset = 0xF00,
> > + .vio_shift_sta_offset = 0xF20,
> > + .vio_shift_sel_offset = 0xF30,
> > + .vio_shift_con_offset = 0xF10,
> > +};
> > +
> > static const struct of_device_id mtk_devapc_dt_match[] = {
> > {
> > .compatible = "mediatek,mt6779-devapc",
> > .data = &devapc_mt6779,
> > }, {
> > + .compatible = "mediatek,mt8192-devapc",
> > + .data = &devapc_mt8192,
> > + }, {
> > },
> > };
> >
> > @@ -239,6 +313,7 @@ static int mtk_devapc_probe(struct platform_device
> > *pdev)
> > {
> > struct device_node *node = pdev->dev.of_node;
> > struct mtk_devapc_context *ctx;
> > + int i;
> > u32 devapc_irq;
> > int ret;
> >
> > @@ -252,8 +327,32 @@ static int mtk_devapc_probe(struct platform_device
> > *pdev)
> > ctx->data = of_device_get_match_data(&pdev->dev);
> > ctx->dev = &pdev->dev;
> >
> > - ctx->infra_base = of_iomap(node, 0);
> > - if (!ctx->infra_base)
> > + if (of_property_read_u32(node, "version", &ctx->arch_ver))
> > + return -EINVAL;
> > +
> > + if (of_property_read_u32(node, "slave_type_num", &ctx->slave_type_num))
>
> arch_ver and slave_type_num can be ddriver internal parameters set through the
> DT data instead of through a new property.
>
> > + return -EINVAL;
> > +
> > + ctx->base_list = devm_kzalloc(&pdev->dev,
> > + sizeof(void *) * ctx->slave_type_num,
> > + GFP_KERNEL);
> > + if (!ctx->base_list)
> > + return -ENOMEM;
> > +
> > + FOR_EACH_SLAVE_TYPE(ctx, i) {
> > + BASE(i) = of_iomap(node, i);
> > + if (!BASE(i))
> > + return -EINVAL;
> > + }
> > +
> > + ctx->vio_idx_num = devm_kzalloc(&pdev->dev,
> > + sizeof(u32) * ctx->slave_type_num,
> > + GFP_KERNEL);
> > + if (!ctx->vio_idx_num)
> > + return -ENOMEM;
> > +
> > + if (of_property_read_u32_array(node, "vio_idx_num",
> > + ctx->vio_idx_num, ctx->slave_type_num))
> > return -EINVAL;
> >
> > devapc_irq = irq_of_parse_and_map(node, 0);
> >
