Re: [PATCH v9 4/9] soc: mediatek: Add multiple step bus protection control

On Wed, Dec 18, 2019 at 10:25 AM Weiyi Lu [off-list ref] wrote:

On Tue, 2019-12-17 at 13:33 +0800, Nicolas Boichat wrote:

quoted

On Tue, Dec 17, 2019 at 10:51 AM Weiyi Lu [off-list ref] wrote:

quoted

On Mon, 2019-12-16 at 15:21 +0800, Nicolas Boichat wrote:

quoted

On Tue, Dec 10, 2019 at 2:47 PM Weiyi Lu [off-list ref] wrote:

quoted

Both MT8183 & MT6765 have more control steps of bus protection
than previous project. And there add more bus protection registers
reside at infracfg & smi-common. Also add new APIs for multiple
step bus protection control with more customized arguments.

Signed-off-by: Weiyi Lu <redacted>
---
 drivers/soc/mediatek/Makefile           |  2 +-
 drivers/soc/mediatek/mtk-scpsys-ext.c   | 99 +++++++++++++++++++++++++++++++++
 drivers/soc/mediatek/mtk-scpsys.c       | 39 +++++++++----
 include/linux/soc/mediatek/scpsys-ext.h | 39 +++++++++++++
 4 files changed, 168 insertions(+), 11 deletions(-)
 create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
 create mode 100644 include/linux/soc/mediatek/scpsys-ext.h

diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index b017330..b442be9 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile

@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
-obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
+obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
 obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
 obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o

diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
new file mode 100644
index 0000000..4f1adda
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-scpsys-ext.c

@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Owen Chen <Owen.Chen@mediatek.com>
+ */
+#include <linux/ktime.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/soc/mediatek/scpsys-ext.h>
+
+#define MTK_POLL_DELAY_US   10
+#define MTK_POLL_TIMEOUT    USEC_PER_SEC
+
+static int set_bus_protection(struct regmap *map, u32 mask, u32 ack_mask,
+               u32 reg_set, u32 reg_sta, u32 reg_en)
+{
+       u32 val;
+
+       if (reg_set)
+               regmap_write(map, reg_set, mask);
+       else
+               regmap_update_bits(map, reg_en, mask, mask);

At least for 8183, we never seen to use the reg_set case, can we
simplify this function?

Actually 6765 will use it and all the other MediaTek chips at least in
near future.
https://patchwork.kernel.org/patch/11042003/

Ok, that's fine then.

quoted

quoted

quoted

+
+       return regmap_read_poll_timeout(map, reg_sta,
+                       val, (val & ack_mask) == ack_mask,
+                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);

From 8183, I see that you have either:
 1. mask == ack_mask
 2. ack_mask == 0 (essentially this skips this test)

Would it be simpler to just skip this test if reg_sta == 0, and always
assume mask == ack_mask otherwise?

e.g.
if (reg_sta == 0)
   return 0;

return regmap_read_poll_timeout(map, reg_sta,
                       val, (val & mask) == mask,
                       MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);

I'm not sure if you mean ack_mask == 0?
reg_sta would be possible to be 0 because it's a register address
offset.

Right, so maybe "0" is not a good invalid value, or maybe you can have a
#define REG_STA_INVALID U32_MAX

And then test for:
if (reg_sta == REG_STA_INVALID)
   return 0;

My point here is that mask and ack_mask are always the same (unless
you don't care about reading back the status), so maybe you only need
to specify mask?

(but if you need different mask and ack_mask for future chips, feel
free to ignore)

I do need different mask and ack_mask.
For the special case of 8183 here, we still have to check the ack status
when setting bus protection but only ignore the ack status when clearing
bus protection.

case A.
SET: reg_set, reg_sta, mask == ack mask
CLEAR: reg_clr, reg_sta, mask == clear ack mask

case B. (the special case we discussed)
SET: reg_set, reg_sta, mask == ack mask
CLEAR: reg_clr, reg_sta, mask != clear ack mask(which is 0 now)

If I use the REG_STA_INVALID to replace the clear ack mask way.
I might need two reg_sta to satisfy the check of set and clear bus
protection.
One is valid for setting case and another is invalid for clearing case,
e.g. case 2 below

case 1.
SET: reg_set, reg_sta(valid), mask == ack mask
CLEAR: reg_clr, reg_sta(valid), mask == ack mask

case 2.
SET: reg_set, reg_sta(valid), mask == ack mask
CLEAR: reg_clr, reg_sta(invalid then return), mask == ack mask

so I'd like to keep the original proposal. What do you think?

Ooh, I see. This is quite confusing to be honest... I wonder if it
wouldn't be clearer to have a boolean instead in bus_prot (something
like "clk_no_check", can't find a better name right now).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel