* [PATCH 0/4] MT8186 SMI SUPPORT
@ 2021-12-03 6:40 Yong Wu
2021-12-03 6:40 ` [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs Yong Wu
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Yong Wu @ 2021-12-03 6:40 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Matthias Brugger
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, yong.wu, youlin.pei,
anan.sun, lc.kan, yi.kuo, anthony.huang
This patchset add mt8186 smi support.
mainly add a sleep control function.
Base on v5.16-rc1.
Yong Wu (4):
dt-bindings: memory: mediatek: Correct the minItems of clk for larbs
dt-bindings: memory: mediatek: Add mt8186 support
memory: mtk-smi: Add sleep ctrl function
memory: mtk-smi: mt8186: Add smi support
.../mediatek,smi-common.yaml | 4 +-
.../memory-controllers/mediatek,smi-larb.yaml | 5 +-
drivers/memory/mtk-smi.c | 52 +++++++++++++++++--
3 files changed, 55 insertions(+), 6 deletions(-)
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs
2021-12-03 6:40 [PATCH 0/4] MT8186 SMI SUPPORT Yong Wu
@ 2021-12-03 6:40 ` Yong Wu
2021-12-03 23:34 ` Rob Herring
2021-12-03 6:40 ` [PATCH 2/4] dt-bindings: memory: mediatek: Add mt8186 support Yong Wu
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Yong Wu @ 2021-12-03 6:40 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Matthias Brugger
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, yong.wu, youlin.pei,
anan.sun, lc.kan, yi.kuo, anthony.huang
If a platform's larb support gals, there will be some larbs have a one
more "gals" clock while the others still only need "apb"/"smi" clocks.
then the minItems is 2 and the maxItems is 3.
Fixes: 27bb0e42855a ("dt-bindings: memory: mediatek: Convert SMI to DT schema")
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
.../bindings/memory-controllers/mediatek,smi-larb.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
index eaeff1ada7f8..a1402f3b8344 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
@@ -81,7 +81,7 @@ allOf:
properties:
clock:
items:
- minItems: 3
+ minItems: 2
maxItems: 3
clock-names:
items:
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/4] dt-bindings: memory: mediatek: Add mt8186 support
2021-12-03 6:40 [PATCH 0/4] MT8186 SMI SUPPORT Yong Wu
2021-12-03 6:40 ` [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs Yong Wu
@ 2021-12-03 6:40 ` Yong Wu
2021-12-13 20:31 ` Rob Herring
2021-12-03 6:40 ` [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function Yong Wu
2021-12-03 6:40 ` [PATCH 4/4] memory: mtk-smi: mt8186: Add smi support Yong Wu
3 siblings, 1 reply; 19+ messages in thread
From: Yong Wu @ 2021-12-03 6:40 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Matthias Brugger
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, yong.wu, youlin.pei,
anan.sun, lc.kan, yi.kuo, anthony.huang
Add mt8186 smi support in the bindings.
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
.../bindings/memory-controllers/mediatek,smi-common.yaml | 4 +++-
.../bindings/memory-controllers/mediatek,smi-larb.yaml | 3 +++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
index 3a82b0b27fa0..fbe5077408d8 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
@@ -16,7 +16,7 @@ description: |
MediaTek SMI have two generations of HW architecture, here is the list
which generation the SoCs use:
generation 1: mt2701 and mt7623.
- generation 2: mt2712, mt6779, mt8167, mt8173, mt8183, mt8192 and mt8195.
+ generation 2: mt2712, mt6779, mt8167, mt8173, mt8183, mt8186, mt8192 and mt8195.
There's slight differences between the two SMI, for generation 2, the
register which control the iommu port is at each larb's register base. But
@@ -35,6 +35,7 @@ properties:
- mediatek,mt8167-smi-common
- mediatek,mt8173-smi-common
- mediatek,mt8183-smi-common
+ - mediatek,mt8186-smi-common
- mediatek,mt8192-smi-common
- mediatek,mt8195-smi-common-vdo
- mediatek,mt8195-smi-common-vpp
@@ -127,6 +128,7 @@ allOf:
enum:
- mediatek,mt6779-smi-common
- mediatek,mt8183-smi-common
+ - mediatek,mt8186-smi-common
- mediatek,mt8192-smi-common
- mediatek,mt8195-smi-common-vdo
- mediatek,mt8195-smi-common-vpp
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
index a1402f3b8344..e019990f892a 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
@@ -23,6 +23,7 @@ properties:
- mediatek,mt8167-smi-larb
- mediatek,mt8173-smi-larb
- mediatek,mt8183-smi-larb
+ - mediatek,mt8186-smi-larb
- mediatek,mt8192-smi-larb
- mediatek,mt8195-smi-larb
@@ -75,6 +76,7 @@ allOf:
compatible:
enum:
- mediatek,mt8183-smi-larb
+ - mediatek,mt8186-smi-larb
- mediatek,mt8195-smi-larb
then:
@@ -109,6 +111,7 @@ allOf:
- mediatek,mt2712-smi-larb
- mediatek,mt6779-smi-larb
- mediatek,mt8167-smi-larb
+ - mediatek,mt8186-smi-larb
- mediatek,mt8192-smi-larb
- mediatek,mt8195-smi-larb
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-03 6:40 [PATCH 0/4] MT8186 SMI SUPPORT Yong Wu
2021-12-03 6:40 ` [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs Yong Wu
2021-12-03 6:40 ` [PATCH 2/4] dt-bindings: memory: mediatek: Add mt8186 support Yong Wu
@ 2021-12-03 6:40 ` Yong Wu
2021-12-04 11:48 ` Krzysztof Kozlowski
2021-12-06 15:08 ` AngeloGioacchino Del Regno
2021-12-03 6:40 ` [PATCH 4/4] memory: mtk-smi: mt8186: Add smi support Yong Wu
3 siblings, 2 replies; 19+ messages in thread
From: Yong Wu @ 2021-12-03 6:40 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Matthias Brugger
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, yong.wu, youlin.pei,
anan.sun, lc.kan, yi.kuo, anthony.huang
sleep control means that when the larb go to sleep, we should wait a bit
until all the current commands are finished. thus, when the larb runtime
suspend, we need enable this function to wait until all the existed
command are finished. when the larb resume, just disable this function.
This function only improve the safe of bus. Add a new flag for this
function. Prepare for mt8186.
Signed-off-by: Anan Sun <anan.sun@mediatek.com>
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
drivers/memory/mtk-smi.c | 39 +++++++++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index b883dcc0bbfa..4b59b28e4d73 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -8,6 +8,7 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/io.h>
+#include <linux/iopoll.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_platform.h>
@@ -32,6 +33,10 @@
#define SMI_DUMMY 0x444
/* SMI LARB */
+#define SMI_LARB_SLP_CON 0x00c
+#define SLP_PROT_EN BIT(0)
+#define SLP_PROT_RDY BIT(16)
+
#define SMI_LARB_CMD_THRT_CON 0x24
#define SMI_LARB_THRT_RD_NU_LMT_MSK GENMASK(7, 4)
#define SMI_LARB_THRT_RD_NU_LMT (5 << 4)
@@ -81,6 +86,7 @@
#define MTK_SMI_FLAG_THRT_UPDATE BIT(0)
#define MTK_SMI_FLAG_SW_FLAG BIT(1)
+#define MTK_SMI_FLAG_SLEEP_CTL BIT(2)
#define MTK_SMI_CAPS(flags, _x) (!!((flags) & (_x)))
struct mtk_smi_reg_pair {
@@ -371,6 +377,24 @@ static const struct of_device_id mtk_smi_larb_of_ids[] = {
{}
};
+static int mtk_smi_larb_sleep_ctrl(struct device *dev, bool to_sleep)
+{
+ struct mtk_smi_larb *larb = dev_get_drvdata(dev);
+ int ret = 0;
+ u32 tmp;
+
+ if (to_sleep) {
+ writel_relaxed(SLP_PROT_EN, larb->base + SMI_LARB_SLP_CON);
+ ret = readl_poll_timeout_atomic(larb->base + SMI_LARB_SLP_CON,
+ tmp, !!(tmp & SLP_PROT_RDY), 10, 1000);
+ if (ret)
+ dev_warn(dev, "sleep ctrl is not ready(0x%x).\n", tmp);
+ } else {
+ writel_relaxed(0, larb->base + SMI_LARB_SLP_CON);
+ }
+ return ret;
+}
+
static int mtk_smi_device_link_common(struct device *dev, struct device **com_dev)
{
struct platform_device *smi_com_pdev;
@@ -477,24 +501,31 @@ static int __maybe_unused mtk_smi_larb_resume(struct device *dev)
{
struct mtk_smi_larb *larb = dev_get_drvdata(dev);
const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
- int ret;
+ int ret = 0;
ret = clk_bulk_prepare_enable(larb->smi.clk_num, larb->smi.clks);
- if (ret < 0)
+ if (ret)
return ret;
+ if (MTK_SMI_CAPS(larb->larb_gen->flags_general, MTK_SMI_FLAG_SLEEP_CTL))
+ ret = mtk_smi_larb_sleep_ctrl(dev, false);
+
/* Configure the basic setting for this larb */
larb_gen->config_port(dev);
- return 0;
+ return ret;
}
static int __maybe_unused mtk_smi_larb_suspend(struct device *dev)
{
struct mtk_smi_larb *larb = dev_get_drvdata(dev);
+ int ret = 0;
+
+ if (MTK_SMI_CAPS(larb->larb_gen->flags_general, MTK_SMI_FLAG_SLEEP_CTL))
+ ret = mtk_smi_larb_sleep_ctrl(dev, true);
clk_bulk_disable_unprepare(larb->smi.clk_num, larb->smi.clks);
- return 0;
+ return ret;
}
static const struct dev_pm_ops smi_larb_pm_ops = {
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/4] memory: mtk-smi: mt8186: Add smi support
2021-12-03 6:40 [PATCH 0/4] MT8186 SMI SUPPORT Yong Wu
` (2 preceding siblings ...)
2021-12-03 6:40 ` [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function Yong Wu
@ 2021-12-03 6:40 ` Yong Wu
2021-12-06 15:00 ` AngeloGioacchino Del Regno
3 siblings, 1 reply; 19+ messages in thread
From: Yong Wu @ 2021-12-03 6:40 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Matthias Brugger
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, yong.wu, youlin.pei,
anan.sun, lc.kan, yi.kuo, anthony.huang
Add mt8186 SMI support.
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
drivers/memory/mtk-smi.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index 4b59b28e4d73..29d7cd1cc8f8 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -355,6 +355,11 @@ static const struct mtk_smi_larb_gen mtk_smi_larb_mt8183 = {
/* IPU0 | IPU1 | CCU */
};
+static const struct mtk_smi_larb_gen mtk_smi_larb_mt8186 = {
+ .config_port = mtk_smi_larb_config_port_gen2_general,
+ .flags_general = MTK_SMI_FLAG_SLEEP_CTL,
+};
+
static const struct mtk_smi_larb_gen mtk_smi_larb_mt8192 = {
.config_port = mtk_smi_larb_config_port_gen2_general,
};
@@ -372,6 +377,7 @@ static const struct of_device_id mtk_smi_larb_of_ids[] = {
{.compatible = "mediatek,mt8167-smi-larb", .data = &mtk_smi_larb_mt8167},
{.compatible = "mediatek,mt8173-smi-larb", .data = &mtk_smi_larb_mt8173},
{.compatible = "mediatek,mt8183-smi-larb", .data = &mtk_smi_larb_mt8183},
+ {.compatible = "mediatek,mt8186-smi-larb", .data = &mtk_smi_larb_mt8186},
{.compatible = "mediatek,mt8192-smi-larb", .data = &mtk_smi_larb_mt8192},
{.compatible = "mediatek,mt8195-smi-larb", .data = &mtk_smi_larb_mt8195},
{}
@@ -575,6 +581,12 @@ static const struct mtk_smi_common_plat mtk_smi_common_mt8183 = {
F_MMU1_LARB(7),
};
+static const struct mtk_smi_common_plat mtk_smi_common_mt8186 = {
+ .type = MTK_SMI_GEN2,
+ .has_gals = true,
+ .bus_sel = F_MMU1_LARB(1) | F_MMU1_LARB(4) | F_MMU1_LARB(7),
+};
+
static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = {
.type = MTK_SMI_GEN2,
.has_gals = true,
@@ -609,6 +621,7 @@ static const struct of_device_id mtk_smi_common_of_ids[] = {
{.compatible = "mediatek,mt8167-smi-common", .data = &mtk_smi_common_gen2},
{.compatible = "mediatek,mt8173-smi-common", .data = &mtk_smi_common_gen2},
{.compatible = "mediatek,mt8183-smi-common", .data = &mtk_smi_common_mt8183},
+ {.compatible = "mediatek,mt8186-smi-common", .data = &mtk_smi_common_mt8186},
{.compatible = "mediatek,mt8192-smi-common", .data = &mtk_smi_common_mt8192},
{.compatible = "mediatek,mt8195-smi-common-vdo", .data = &mtk_smi_common_mt8195_vdo},
{.compatible = "mediatek,mt8195-smi-common-vpp", .data = &mtk_smi_common_mt8195_vpp},
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs
2021-12-03 6:40 ` [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs Yong Wu
@ 2021-12-03 23:34 ` Rob Herring
2021-12-13 6:48 ` Yong Wu
0 siblings, 1 reply; 19+ messages in thread
From: Rob Herring @ 2021-12-03 23:34 UTC (permalink / raw)
To: Yong Wu
Cc: linux-arm-kernel, iommu, Tomasz Figa, Krzysztof Kozlowski,
Joerg Roedel, yi.kuo, anan.sun, Will Deacon, devicetree,
Robin Murphy, Rob Herring, linux-kernel, Krzysztof Kozlowski,
anthony.huang, lc.kan, srv_heupstream, Matthias Brugger,
linux-mediatek, youlin.pei
On Fri, 03 Dec 2021 14:40:24 +0800, Yong Wu wrote:
> If a platform's larb support gals, there will be some larbs have a one
> more "gals" clock while the others still only need "apb"/"smi" clocks.
> then the minItems is 2 and the maxItems is 3.
>
> Fixes: 27bb0e42855a ("dt-bindings: memory: mediatek: Convert SMI to DT schema")
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> .../bindings/memory-controllers/mediatek,smi-larb.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.
Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.
Full log is available here: https://patchwork.ozlabs.org/patch/1563127
larb@14016000: 'mediatek,larb-id' is a required property
arch/arm64/boot/dts/mediatek/mt8167-pumpkin.dt.yaml
larb@14017000: clock-names: ['apb', 'smi'] is too short
arch/arm64/boot/dts/mediatek/mt8183-evb.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-burnet.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-damu.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel14.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-sku1.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-sku6.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-juniper-sku16.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kappa.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kenzo.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-sku0.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-sku1.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku16.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku272.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku288.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku32.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dt.yaml
larb@15001000: 'mediatek,larb-id' is a required property
arch/arm64/boot/dts/mediatek/mt8167-pumpkin.dt.yaml
larb@16010000: clock-names: ['apb', 'smi'] is too short
arch/arm64/boot/dts/mediatek/mt8183-evb.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-burnet.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-damu.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel14.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-sku1.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-sku6.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-juniper-sku16.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kappa.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kenzo.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-sku0.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-sku1.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku16.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku272.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku288.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku32.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dt.yaml
larb@16010000: 'mediatek,larb-id' is a required property
arch/arm64/boot/dts/mediatek/mt8167-pumpkin.dt.yaml
larb@17010000: clock-names: ['apb', 'smi'] is too short
arch/arm64/boot/dts/mediatek/mt8183-evb.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-burnet.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-damu.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel14.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-sku1.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-sku6.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-juniper-sku16.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kappa.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kenzo.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-sku0.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-sku1.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku16.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku272.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku288.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku32.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dt.yaml
arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dt.yaml
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-03 6:40 ` [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function Yong Wu
@ 2021-12-04 11:48 ` Krzysztof Kozlowski
2021-12-06 8:15 ` Yong Wu
2021-12-06 15:08 ` AngeloGioacchino Del Regno
1 sibling, 1 reply; 19+ messages in thread
From: Krzysztof Kozlowski @ 2021-12-04 11:48 UTC (permalink / raw)
To: Yong Wu, Rob Herring, Matthias Brugger
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang
On 03/12/2021 07:40, Yong Wu wrote:
> sleep control means that when the larb go to sleep, we should wait a bit
s/go/goes/
> until all the current commands are finished. thus, when the larb runtime
Please start every sentence with a capital letter.
> suspend, we need enable this function to wait until all the existed
s/suspend/suspends/
s/we need enable/we need to enable/
> command are finished. when the larb resume, just disable this function.
s/command/commands/
s/resume/resumes/
> This function only improve the safe of bus. Add a new flag for this
s/improve/improves/
s/the safe/the safety/
> function. Prepare for mt8186.
In total it is hard to parse, really.
>
> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> drivers/memory/mtk-smi.c | 39 +++++++++++++++++++++++++++++++++++----
> 1 file changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index b883dcc0bbfa..4b59b28e4d73 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -8,6 +8,7 @@
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_platform.h>
> @@ -32,6 +33,10 @@
> #define SMI_DUMMY 0x444
>
> /* SMI LARB */
> +#define SMI_LARB_SLP_CON 0x00c
> +#define SLP_PROT_EN BIT(0)
> +#define SLP_PROT_RDY BIT(16)
> +
> #define SMI_LARB_CMD_THRT_CON 0x24
> #define SMI_LARB_THRT_RD_NU_LMT_MSK GENMASK(7, 4)
> #define SMI_LARB_THRT_RD_NU_LMT (5 << 4)
> @@ -81,6 +86,7 @@
>
> #define MTK_SMI_FLAG_THRT_UPDATE BIT(0)
> #define MTK_SMI_FLAG_SW_FLAG BIT(1)
> +#define MTK_SMI_FLAG_SLEEP_CTL BIT(2)
> #define MTK_SMI_CAPS(flags, _x) (!!((flags) & (_x)))
>
> struct mtk_smi_reg_pair {
> @@ -371,6 +377,24 @@ static const struct of_device_id mtk_smi_larb_of_ids[] = {
> {}
> };
>
> +static int mtk_smi_larb_sleep_ctrl(struct device *dev, bool to_sleep)
> +{
Make two functions instead. There is no single code reuse (shared)
between sleep and resume. In the same time bool arguments are confusing
when looking at caller and one never knows whether true means to resume
or to sleep. Having two functions is obvious. Obvious code is easier to
read and maintain.
> + struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> + int ret = 0;
> + u32 tmp;
> +
> + if (to_sleep) {
> + writel_relaxed(SLP_PROT_EN, larb->base + SMI_LARB_SLP_CON);
> + ret = readl_poll_timeout_atomic(larb->base + SMI_LARB_SLP_CON,
> + tmp, !!(tmp & SLP_PROT_RDY), 10, 1000);
> + if (ret)
> + dev_warn(dev, "sleep ctrl is not ready(0x%x).\n", tmp);
> + } else {
> + writel_relaxed(0, larb->base + SMI_LARB_SLP_CON);
> + }
> + return ret;
> +}
> +
> static int mtk_smi_device_link_common(struct device *dev, struct device **com_dev)
> {
> struct platform_device *smi_com_pdev;
> @@ -477,24 +501,31 @@ static int __maybe_unused mtk_smi_larb_resume(struct device *dev)
> {
> struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> - int ret;
> + int ret = 0;
This line does not have a sense.
>
> ret = clk_bulk_prepare_enable(larb->smi.clk_num, larb->smi.clks);
> - if (ret < 0)
> + if (ret)
Why changing this?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-04 11:48 ` Krzysztof Kozlowski
@ 2021-12-06 8:15 ` Yong Wu
0 siblings, 0 replies; 19+ messages in thread
From: Yong Wu @ 2021-12-06 8:15 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang, Rob Herring, Matthias Brugger
On Sat, 2021-12-04 at 12:48 +0100, Krzysztof Kozlowski wrote:
> On 03/12/2021 07:40, Yong Wu wrote:
> > sleep control means that when the larb go to sleep, we should wait
> > a bit
>
> s/go/goes/
>
> > until all the current commands are finished. thus, when the larb
> > runtime
>
> Please start every sentence with a capital letter.
>
> > suspend, we need enable this function to wait until all the existed
>
> s/suspend/suspends/
> s/we need enable/we need to enable/
>
> > command are finished. when the larb resume, just disable this
> > function.
>
> s/command/commands/
> s/resume/resumes/
>
> > This function only improve the safe of bus. Add a new flag for this
>
> s/improve/improves/
> s/the safe/the safety/
>
> > function. Prepare for mt8186.
>
> In total it is hard to parse, really.
Will fix them in next version.
Thanks for reviewing so detailedly. Sorry. I didn't pay attention to
the grammar before.
>
> >
> > Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > drivers/memory/mtk-smi.c | 39 +++++++++++++++++++++++++++++++++++-
> > ---
> > 1 file changed, 35 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > index b883dcc0bbfa..4b59b28e4d73 100644
> > --- a/drivers/memory/mtk-smi.c
> > +++ b/drivers/memory/mtk-smi.c
> > @@ -8,6 +8,7 @@
> > #include <linux/device.h>
> > #include <linux/err.h>
> > #include <linux/io.h>
> > +#include <linux/iopoll.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_platform.h>
> > @@ -32,6 +33,10 @@
> > #define SMI_DUMMY 0x444
> >
> > /* SMI LARB */
> > +#define SMI_LARB_SLP_CON 0x00c
> > +#define SLP_PROT_EN BIT(0)
> > +#define SLP_PROT_RDY BIT(16)
> > +
> > #define SMI_LARB_CMD_THRT_CON 0x24
> > #define SMI_LARB_THRT_RD_NU_LMT_MSK GENMASK(7, 4)
> > #define SMI_LARB_THRT_RD_NU_LMT (5 << 4)
> > @@ -81,6 +86,7 @@
> >
> > #define MTK_SMI_FLAG_THRT_UPDATE BIT(0)
> > #define MTK_SMI_FLAG_SW_FLAG BIT(1)
> > +#define MTK_SMI_FLAG_SLEEP_CTL BIT(2)
> > #define MTK_SMI_CAPS(flags, _x) (!!((flags) & (_x)))
> >
> > struct mtk_smi_reg_pair {
> > @@ -371,6 +377,24 @@ static const struct of_device_id
> > mtk_smi_larb_of_ids[] = {
> > {}
> > };
> >
> > +static int mtk_smi_larb_sleep_ctrl(struct device *dev, bool
> > to_sleep)
> > +{
>
> Make two functions instead. There is no single code reuse (shared)
> between sleep and resume. In the same time bool arguments are
> confusing
> when looking at caller and one never knows whether true means to
> resume
> or to sleep. Having two functions is obvious. Obvious code is easier
> to
> read and maintain.
Make sense. Thanks for this suggestion.
>
> > + struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > + int ret = 0;
> > + u32 tmp;
> > +
> > + if (to_sleep) {
> > + writel_relaxed(SLP_PROT_EN, larb->base +
> > SMI_LARB_SLP_CON);
> > + ret = readl_poll_timeout_atomic(larb->base +
> > SMI_LARB_SLP_CON,
> > + tmp, !!(tmp &
> > SLP_PROT_RDY), 10, 1000);
> > + if (ret)
> > + dev_warn(dev, "sleep ctrl is not
> > ready(0x%x).\n", tmp);
> > + } else {
> > + writel_relaxed(0, larb->base + SMI_LARB_SLP_CON);
> > + }
> > + return ret;
> > +}
> > +
> > static int mtk_smi_device_link_common(struct device *dev, struct
> > device **com_dev)
> > {
> > struct platform_device *smi_com_pdev;
> > @@ -477,24 +501,31 @@ static int __maybe_unused
> > mtk_smi_larb_resume(struct device *dev)
> > {
> > struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> > - int ret;
> > + int ret = 0;
>
> This line does not have a sense.
Yes. This is unhelpful. Will remove this.
>
> >
> > ret = clk_bulk_prepare_enable(larb->smi.clk_num, larb-
> > >smi.clks);
> > - if (ret < 0)
> > + if (ret)
>
> Why changing this?
The successful return value should be 0. I will use a independent patch
for this.
>
> Best regards,
> Krzysztof
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] memory: mtk-smi: mt8186: Add smi support
2021-12-03 6:40 ` [PATCH 4/4] memory: mtk-smi: mt8186: Add smi support Yong Wu
@ 2021-12-06 15:00 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-06 15:00 UTC (permalink / raw)
To: Yong Wu, Krzysztof Kozlowski, Rob Herring, Matthias Brugger
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang
Il 03/12/21 07:40, Yong Wu ha scritto:
> Add mt8186 SMI support.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
Acked-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-03 6:40 ` [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function Yong Wu
2021-12-04 11:48 ` Krzysztof Kozlowski
@ 2021-12-06 15:08 ` AngeloGioacchino Del Regno
2021-12-07 6:24 ` Yong Wu
1 sibling, 1 reply; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-06 15:08 UTC (permalink / raw)
To: Yong Wu, Krzysztof Kozlowski, Rob Herring, Matthias Brugger
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang
Il 03/12/21 07:40, Yong Wu ha scritto:
> sleep control means that when the larb go to sleep, we should wait a bit
> until all the current commands are finished. thus, when the larb runtime
> suspend, we need enable this function to wait until all the existed
> command are finished. when the larb resume, just disable this function.
> This function only improve the safe of bus. Add a new flag for this
> function. Prepare for mt8186.
>
> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> drivers/memory/mtk-smi.c | 39 +++++++++++++++++++++++++++++++++++----
> 1 file changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index b883dcc0bbfa..4b59b28e4d73 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -8,6 +8,7 @@
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_platform.h>
> @@ -32,6 +33,10 @@
> #define SMI_DUMMY 0x444
>
> /* SMI LARB */
> +#define SMI_LARB_SLP_CON 0x00c
> +#define SLP_PROT_EN BIT(0)
> +#define SLP_PROT_RDY BIT(16)
> +
> #define SMI_LARB_CMD_THRT_CON 0x24
> #define SMI_LARB_THRT_RD_NU_LMT_MSK GENMASK(7, 4)
> #define SMI_LARB_THRT_RD_NU_LMT (5 << 4)
> @@ -81,6 +86,7 @@
>
> #define MTK_SMI_FLAG_THRT_UPDATE BIT(0)
> #define MTK_SMI_FLAG_SW_FLAG BIT(1)
> +#define MTK_SMI_FLAG_SLEEP_CTL BIT(2)
> #define MTK_SMI_CAPS(flags, _x) (!!((flags) & (_x)))
>
> struct mtk_smi_reg_pair {
> @@ -371,6 +377,24 @@ static const struct of_device_id mtk_smi_larb_of_ids[] = {
> {}
> };
>
> +static int mtk_smi_larb_sleep_ctrl(struct device *dev, bool to_sleep)
> +{
> + struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> + int ret = 0;
> + u32 tmp;
> +
> + if (to_sleep) {
> + writel_relaxed(SLP_PROT_EN, larb->base + SMI_LARB_SLP_CON);
> + ret = readl_poll_timeout_atomic(larb->base + SMI_LARB_SLP_CON,
> + tmp, !!(tmp & SLP_PROT_RDY), 10, 1000);
> + if (ret)
> + dev_warn(dev, "sleep ctrl is not ready(0x%x).\n", tmp);
> + } else {
> + writel_relaxed(0, larb->base + SMI_LARB_SLP_CON);
> + }
> + return ret;
> +}
> +
> static int mtk_smi_device_link_common(struct device *dev, struct device **com_dev)
> {
> struct platform_device *smi_com_pdev;
> @@ -477,24 +501,31 @@ static int __maybe_unused mtk_smi_larb_resume(struct device *dev)
> {
> struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> - int ret;
> + int ret = 0;
>
> ret = clk_bulk_prepare_enable(larb->smi.clk_num, larb->smi.clks);
> - if (ret < 0)
> + if (ret)
> return ret;
>
> + if (MTK_SMI_CAPS(larb->larb_gen->flags_general, MTK_SMI_FLAG_SLEEP_CTL))
> + ret = mtk_smi_larb_sleep_ctrl(dev, false);
> +
> /* Configure the basic setting for this larb */
> larb_gen->config_port(dev);
>
> - return 0;
> + return ret;
> }
>
> static int __maybe_unused mtk_smi_larb_suspend(struct device *dev)
> {
> struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + if (MTK_SMI_CAPS(larb->larb_gen->flags_general, MTK_SMI_FLAG_SLEEP_CTL))
> + ret = mtk_smi_larb_sleep_ctrl(dev, true);
Sorry but what happens if SLP_PROT_RDY is not getting set properly?
From what I can understand in the commit description that you wrote, if we reach
the timeout, then the LARB transactions are not over....
I see that you are indeed returning a failure here, but you are also turning off
the clocks regardless of whether we get a failure or a success; I'm not sure that
this is right, as this may leave the hardware in an unpredictable state (since
there were some more LARB transactions that didn't go through), leading to crashes
at system resume (or when retyring to suspend).
>
> clk_bulk_disable_unprepare(larb->smi.clk_num, larb->smi.clks);
> - return 0;
> + return ret;
> }
>
> static const struct dev_pm_ops smi_larb_pm_ops = {
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-06 15:08 ` AngeloGioacchino Del Regno
@ 2021-12-07 6:24 ` Yong Wu
2021-12-07 8:56 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 19+ messages in thread
From: Yong Wu @ 2021-12-07 6:24 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang, Rob Herring, Matthias Brugger
Hi AngeloGioacchino,
Thanks for your review.
On Mon, 2021-12-06 at 16:08 +0100, AngeloGioacchino Del Regno wrote:
> Il 03/12/21 07:40, Yong Wu ha scritto:
> > sleep control means that when the larb go to sleep, we should wait
> > a bit
> > until all the current commands are finished. thus, when the larb
> > runtime
> > suspend, we need enable this function to wait until all the existed
> > command are finished. when the larb resume, just disable this
> > function.
> > This function only improve the safe of bus. Add a new flag for this
> > function. Prepare for mt8186.
> >
> > Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > drivers/memory/mtk-smi.c | 39
> > +++++++++++++++++++++++++++++++++++----
> > 1 file changed, 35 insertions(+), 4 deletions(-)
[snip]
> > static int __maybe_unused mtk_smi_larb_suspend(struct device
> > *dev)
> > {
> > struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > + int ret = 0;
> > +
> > + if (MTK_SMI_CAPS(larb->larb_gen->flags_general,
> > MTK_SMI_FLAG_SLEEP_CTL))
> > + ret = mtk_smi_larb_sleep_ctrl(dev, true);
>
> Sorry but what happens if SLP_PROT_RDY is not getting set properly?
> From what I can understand in the commit description that you wrote,
> if we reach
> the timeout, then the LARB transactions are not over....
>
> I see that you are indeed returning a failure here, but you are also
> turning off
> the clocks regardless of whether we get a failure or a success; I'm
> not sure that
> this is right, as this may leave the hardware in an unpredictable
> state (since
> there were some more LARB transactions that didn't go through),
> leading to crashes
> at system resume (or when retyring to suspend).
Thanks for this question. In theory you are right. In this case, the
bus already hang.
We only printed a fail log in this patch. If this fail happens, we
should request the master to check which case cause the larb hang.
If the master has a good reason or limitation, the hang is expected, I
think we have to add larb reset in this fail case: Reset the larb when
the larb runtime resume.
Fortunately, we have never got this issue. We could add this reset when
necessary. Is this OK for you?
Thanks.
>
> >
> > clk_bulk_disable_unprepare(larb->smi.clk_num, larb->smi.clks);
> > - return 0;
> > + return ret;
> > }
> >
> > static const struct dev_pm_ops smi_larb_pm_ops = {
> >
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-07 6:24 ` Yong Wu
@ 2021-12-07 8:56 ` AngeloGioacchino Del Regno
2021-12-07 12:10 ` Yong Wu
0 siblings, 1 reply; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-07 8:56 UTC (permalink / raw)
To: Yong Wu, Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang, Rob Herring, Matthias Brugger
Il 07/12/21 07:24, Yong Wu ha scritto:
> Hi AngeloGioacchino,
>
> Thanks for your review.
>
> On Mon, 2021-12-06 at 16:08 +0100, AngeloGioacchino Del Regno wrote:
>> Il 03/12/21 07:40, Yong Wu ha scritto:
>>> sleep control means that when the larb go to sleep, we should wait
>>> a bit
>>> until all the current commands are finished. thus, when the larb
>>> runtime
>>> suspend, we need enable this function to wait until all the existed
>>> command are finished. when the larb resume, just disable this
>>> function.
>>> This function only improve the safe of bus. Add a new flag for this
>>> function. Prepare for mt8186.
>>>
>>> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>> ---
>>> drivers/memory/mtk-smi.c | 39
>>> +++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 35 insertions(+), 4 deletions(-)
>
> [snip]
>
>>> static int __maybe_unused mtk_smi_larb_suspend(struct device
>>> *dev)
>>> {
>>> struct mtk_smi_larb *larb = dev_get_drvdata(dev);
>>> + int ret = 0;
>>> +
>>> + if (MTK_SMI_CAPS(larb->larb_gen->flags_general,
>>> MTK_SMI_FLAG_SLEEP_CTL))
>>> + ret = mtk_smi_larb_sleep_ctrl(dev, true);
>>
>> Sorry but what happens if SLP_PROT_RDY is not getting set properly?
>> From what I can understand in the commit description that you wrote,
>> if we reach
>> the timeout, then the LARB transactions are not over....
>>
>> I see that you are indeed returning a failure here, but you are also
>> turning off
>> the clocks regardless of whether we get a failure or a success; I'm
>> not sure that
>> this is right, as this may leave the hardware in an unpredictable
>> state (since
>> there were some more LARB transactions that didn't go through),
>> leading to crashes
>> at system resume (or when retyring to suspend).
>
> Thanks for this question. In theory you are right. In this case, the
> bus already hang.
>
> We only printed a fail log in this patch. If this fail happens, we
> should request the master to check which case cause the larb hang.
>
> If the master has a good reason or limitation, the hang is expected, I
> think we have to add larb reset in this fail case: Reset the larb when
> the larb runtime resume.
>
Think about the case in which the system gets resumed only partially due to a
failure during resume of some driver, or due to a RTC or arch timer resume and
suspend right after... or perhaps during runtime suspend/resume of some devices.
In that case, we definitely want to avoid any kind of failure point that would
lead to a system crash, or any kind of user noticeable (or UX disrupting) "strange
behavior".
I think that we should make sure that the system suspends cleanly, instead of
patching up any possible leftover issue at resume time: if this is doable with
a LARB reset in suspend error case, that looks like being a good option indeed.
As a side note, thinking about UX, losing a little more time during suspend is
nothing really noticeable for the user... on the other hand, spending more time
during resume may be something noticeable to the user.
For this reason, I think that guaranteeing that the system resumes as fast as
possible is very important, which adds up to the need of suspending cleanly.
> Fortunately, we have never got this issue. We could add this reset when
> necessary. Is this OK for you?
>
> Thanks.
>
>>
>>>
>>> clk_bulk_disable_unprepare(larb->smi.clk_num, larb->smi.clks);
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> static const struct dev_pm_ops smi_larb_pm_ops = {
>>>
>>
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-07 8:56 ` AngeloGioacchino Del Regno
@ 2021-12-07 12:10 ` Yong Wu
2021-12-07 12:16 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 19+ messages in thread
From: Yong Wu @ 2021-12-07 12:10 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang, Rob Herring, Matthias Brugger
On Tue, 2021-12-07 at 09:56 +0100, AngeloGioacchino Del Regno wrote:
> Il 07/12/21 07:24, Yong Wu ha scritto:
> > Hi AngeloGioacchino,
> >
> > Thanks for your review.
> >
> > On Mon, 2021-12-06 at 16:08 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > Il 03/12/21 07:40, Yong Wu ha scritto:
> > > > sleep control means that when the larb go to sleep, we should
> > > > wait
> > > > a bit
> > > > until all the current commands are finished. thus, when the
> > > > larb
> > > > runtime
> > > > suspend, we need enable this function to wait until all the
> > > > existed
> > > > command are finished. when the larb resume, just disable this
> > > > function.
> > > > This function only improve the safe of bus. Add a new flag for
> > > > this
> > > > function. Prepare for mt8186.
> > > >
> > > > Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > > ---
> > > > drivers/memory/mtk-smi.c | 39
> > > > +++++++++++++++++++++++++++++++++++----
> > > > 1 file changed, 35 insertions(+), 4 deletions(-)
> >
> > [snip]
> >
> > > > static int __maybe_unused mtk_smi_larb_suspend(struct device
> > > > *dev)
> > > > {
> > > > struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > > > + int ret = 0;
> > > > +
> > > > + if (MTK_SMI_CAPS(larb->larb_gen->flags_general,
> > > > MTK_SMI_FLAG_SLEEP_CTL))
> > > > + ret = mtk_smi_larb_sleep_ctrl(dev, true);
> > >
> > > Sorry but what happens if SLP_PROT_RDY is not getting set
> > > properly?
> > > From what I can understand in the commit description that you
> > > wrote,
> > > if we reach
> > > the timeout, then the LARB transactions are not over....
> > >
> > > I see that you are indeed returning a failure here, but you are
> > > also
> > > turning off
> > > the clocks regardless of whether we get a failure or a success;
> > > I'm
> > > not sure that
> > > this is right, as this may leave the hardware in an unpredictable
> > > state (since
> > > there were some more LARB transactions that didn't go through),
> > > leading to crashes
> > > at system resume (or when retyring to suspend).
> >
> > Thanks for this question. In theory you are right. In this case,
> > the
> > bus already hang.
> >
> > We only printed a fail log in this patch. If this fail happens, we
> > should request the master to check which case cause the larb hang.
> >
> > If the master has a good reason or limitation, the hang is
> > expected, I
> > think we have to add larb reset in this fail case: Reset the larb
> > when
> > the larb runtime resume.
> >
>
> Think about the case in which the system gets resumed only partially
> due to a
>
> failure during resume of some driver, or due to a RTC or arch timer
> resume and
> suspend right after... or perhaps during runtime suspend/resume of
> some devices.
> In that case, we definitely want to avoid any kind of failure point
> that would
> lead to a system crash, or any kind of user noticeable (or UX
> disrupting) "strange
> behavior".
>
> I think that we should make sure that the system suspends cleanly,
> instead of
> patching up any possible leftover issue at resume time: if this is
> doable with
> a LARB reset in suspend error case, that looks like being a good
> option indeed.
>
> As a side note, thinking about UX, losing a little more time during
> suspend is
> nothing really noticeable for the user... on the other hand, spending
> more time
> during resume may be something noticeable to the user.
> For this reason, I think that guaranteeing that the system resumes as
> fast as
> possible is very important, which adds up to the need of suspending
> cleanly.
Thanks for this comment. I will put it in the suspend when adding the
reset. But I have no plan to add it in this version since I don't see
the need for this right now. Maybe I should add a comment in the code
for this.
>
> > Fortunately, we have never got this issue. We could add this reset
> > when
> > necessary. Is this OK for you?
> >
> > Thanks.
> >
> > >
> > > >
> > > > clk_bulk_disable_unprepare(larb->smi.clk_num, larb-
> > > > >smi.clks);
> > > > - return 0;
> > > > + return ret;
> > > > }
> > > >
> > > > static const struct dev_pm_ops smi_larb_pm_ops = {
> > > >
> > >
> > >
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-07 12:10 ` Yong Wu
@ 2021-12-07 12:16 ` AngeloGioacchino Del Regno
2021-12-08 2:42 ` Yong Wu
0 siblings, 1 reply; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-07 12:16 UTC (permalink / raw)
To: Yong Wu, Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang, Rob Herring, Matthias Brugger
Il 07/12/21 13:10, Yong Wu ha scritto:
> On Tue, 2021-12-07 at 09:56 +0100, AngeloGioacchino Del Regno wrote:
>> Il 07/12/21 07:24, Yong Wu ha scritto:
>>> Hi AngeloGioacchino,
>>>
>>> Thanks for your review.
>>>
>>> On Mon, 2021-12-06 at 16:08 +0100, AngeloGioacchino Del Regno
>>> wrote:
>>>> Il 03/12/21 07:40, Yong Wu ha scritto:
>>>>> sleep control means that when the larb go to sleep, we should
>>>>> wait
>>>>> a bit
>>>>> until all the current commands are finished. thus, when the
>>>>> larb
>>>>> runtime
>>>>> suspend, we need enable this function to wait until all the
>>>>> existed
>>>>> command are finished. when the larb resume, just disable this
>>>>> function.
>>>>> This function only improve the safe of bus. Add a new flag for
>>>>> this
>>>>> function. Prepare for mt8186.
>>>>>
>>>>> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>>> ---
>>>>> drivers/memory/mtk-smi.c | 39
>>>>> +++++++++++++++++++++++++++++++++++----
>>>>> 1 file changed, 35 insertions(+), 4 deletions(-)
>>>
>>> [snip]
>>>
>>>>> static int __maybe_unused mtk_smi_larb_suspend(struct device
>>>>> *dev)
>>>>> {
>>>>> struct mtk_smi_larb *larb = dev_get_drvdata(dev);
>>>>> + int ret = 0;
>>>>> +
>>>>> + if (MTK_SMI_CAPS(larb->larb_gen->flags_general,
>>>>> MTK_SMI_FLAG_SLEEP_CTL))
>>>>> + ret = mtk_smi_larb_sleep_ctrl(dev, true);
>>>>
>>>> Sorry but what happens if SLP_PROT_RDY is not getting set
>>>> properly?
>>>> From what I can understand in the commit description that you
>>>> wrote,
>>>> if we reach
>>>> the timeout, then the LARB transactions are not over....
>>>>
>>>> I see that you are indeed returning a failure here, but you are
>>>> also
>>>> turning off
>>>> the clocks regardless of whether we get a failure or a success;
>>>> I'm
>>>> not sure that
>>>> this is right, as this may leave the hardware in an unpredictable
>>>> state (since
>>>> there were some more LARB transactions that didn't go through),
>>>> leading to crashes
>>>> at system resume (or when retyring to suspend).
>>>
>>> Thanks for this question. In theory you are right. In this case,
>>> the
>>> bus already hang.
>>>
>>> We only printed a fail log in this patch. If this fail happens, we
>>> should request the master to check which case cause the larb hang.
>>>
>>> If the master has a good reason or limitation, the hang is
>>> expected, I
>>> think we have to add larb reset in this fail case: Reset the larb
>>> when
>>> the larb runtime resume.
>>>
>>
>> Think about the case in which the system gets resumed only partially
>> due to a
>>
>> failure during resume of some driver, or due to a RTC or arch timer
>> resume and
>> suspend right after... or perhaps during runtime suspend/resume of
>> some devices.
>> In that case, we definitely want to avoid any kind of failure point
>> that would
>> lead to a system crash, or any kind of user noticeable (or UX
>> disrupting) "strange
>> behavior".
>>
>> I think that we should make sure that the system suspends cleanly,
>> instead of
>> patching up any possible leftover issue at resume time: if this is
>> doable with
>> a LARB reset in suspend error case, that looks like being a good
>> option indeed.
>>
>> As a side note, thinking about UX, losing a little more time during
>> suspend is
>> nothing really noticeable for the user... on the other hand, spending
>> more time
>> during resume may be something noticeable to the user.
>> For this reason, I think that guaranteeing that the system resumes as
>> fast as
>> possible is very important, which adds up to the need of suspending
>> cleanly.
>
> Thanks for this comment. I will put it in the suspend when adding the
> reset. But I have no plan to add it in this version since I don't see
> the need for this right now. Maybe I should add a comment in the code
> for this.
>
What I understand from your reply is that the reset is not trivial work
and needs quite some time to be done properly; in that case: yes, please
add a TODO comment that explains the situation and the discussed solution.
Also, since this SLP_PROT_RDY flag seems to be very nice, just a simple
question: is this a new feature in the SMI IP of MT8186, or is there anything
similar that we may use on other SoCs, like 8183, 8192, 8195, as a follow-up
of this series?
>>
>>> Fortunately, we have never got this issue. We could add this reset
>>> when
>>> necessary. Is this OK for you?
>>>
>>> Thanks.
>>>
>>>>
>>>>>
>>>>> clk_bulk_disable_unprepare(larb->smi.clk_num, larb-
>>>>>> smi.clks);
>>>>> - return 0;
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> static const struct dev_pm_ops smi_larb_pm_ops = {
>>>>>
>>>>
>>>>
>>
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-07 12:16 ` AngeloGioacchino Del Regno
@ 2021-12-08 2:42 ` Yong Wu
2021-12-09 9:12 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 19+ messages in thread
From: Yong Wu @ 2021-12-08 2:42 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang, Rob Herring, Matthias Brugger
On Tue, 2021-12-07 at 13:16 +0100, AngeloGioacchino Del Regno wrote:
> Il 07/12/21 13:10, Yong Wu ha scritto:
> > On Tue, 2021-12-07 at 09:56 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > Il 07/12/21 07:24, Yong Wu ha scritto:
> > > > Hi AngeloGioacchino,
> > > >
> > > > Thanks for your review.
> > > >
> > > > On Mon, 2021-12-06 at 16:08 +0100, AngeloGioacchino Del Regno
> > > > wrote:
> > > > > Il 03/12/21 07:40, Yong Wu ha scritto:
> > > > > > sleep control means that when the larb go to sleep, we
> > > > > > should
> > > > > > wait
> > > > > > a bit
> > > > > > until all the current commands are finished. thus, when the
> > > > > > larb
> > > > > > runtime
> > > > > > suspend, we need enable this function to wait until all the
> > > > > > existed
> > > > > > command are finished. when the larb resume, just disable
> > > > > > this
> > > > > > function.
> > > > > > This function only improve the safe of bus. Add a new flag
> > > > > > for
> > > > > > this
> > > > > > function. Prepare for mt8186.
> > > > > >
> > > > > > Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > > > > ---
> > > > > > drivers/memory/mtk-smi.c | 39
> > > > > > +++++++++++++++++++++++++++++++++++----
> > > > > > 1 file changed, 35 insertions(+), 4 deletions(-)
> > > >
> > > > [snip]
> > > >
> > > > > > static int __maybe_unused mtk_smi_larb_suspend(struct
> > > > > > device
> > > > > > *dev)
> > > > > > {
> > > > > > struct mtk_smi_larb *larb =
> > > > > > dev_get_drvdata(dev);
> > > > > > + int ret = 0;
> > > > > > +
> > > > > > + if (MTK_SMI_CAPS(larb->larb_gen->flags_general,
> > > > > > MTK_SMI_FLAG_SLEEP_CTL))
> > > > > > + ret = mtk_smi_larb_sleep_ctrl(dev, true);
> > > > >
> > > > > Sorry but what happens if SLP_PROT_RDY is not getting set
> > > > > properly?
> > > > > From what I can understand in the commit description that
> > > > > you
> > > > > wrote,
> > > > > if we reach
> > > > > the timeout, then the LARB transactions are not over....
> > > > >
> > > > > I see that you are indeed returning a failure here, but you
> > > > > are
> > > > > also
> > > > > turning off
> > > > > the clocks regardless of whether we get a failure or a
> > > > > success;
> > > > > I'm
> > > > > not sure that
> > > > > this is right, as this may leave the hardware in an
> > > > > unpredictable
> > > > > state (since
> > > > > there were some more LARB transactions that didn't go
> > > > > through),
> > > > > leading to crashes
> > > > > at system resume (or when retyring to suspend).
> > > >
> > > > Thanks for this question. In theory you are right. In this
> > > > case,
> > > > the
> > > > bus already hang.
> > > >
> > > > We only printed a fail log in this patch. If this fail happens,
> > > > we
> > > > should request the master to check which case cause the larb
> > > > hang.
> > > >
> > > > If the master has a good reason or limitation, the hang is
> > > > expected, I
> > > > think we have to add larb reset in this fail case: Reset the
> > > > larb
> > > > when
> > > > the larb runtime resume.
> > > >
> > >
> > > Think about the case in which the system gets resumed only
> > > partially
> > > due to a
> > >
> > > failure during resume of some driver, or due to a RTC or arch
> > > timer
> > > resume and
> > > suspend right after... or perhaps during runtime suspend/resume
> > > of
> > > some devices.
> > > In that case, we definitely want to avoid any kind of failure
> > > point
> > > that would
> > > lead to a system crash, or any kind of user noticeable (or UX
> > > disrupting) "strange
> > > behavior".
> > >
> > > I think that we should make sure that the system suspends
> > > cleanly,
> > > instead of
> > > patching up any possible leftover issue at resume time: if this
> > > is
> > > doable with
> > > a LARB reset in suspend error case, that looks like being a good
> > > option indeed.
> > >
> > > As a side note, thinking about UX, losing a little more time
> > > during
> > > suspend is
> > > nothing really noticeable for the user... on the other hand,
> > > spending
> > > more time
> > > during resume may be something noticeable to the user.
> > > For this reason, I think that guaranteeing that the system
> > > resumes as
> > > fast as
> > > possible is very important, which adds up to the need of
> > > suspending
> > > cleanly.
> >
> > Thanks for this comment. I will put it in the suspend when adding
> > the
> > reset. But I have no plan to add it in this version since I don't
> > see
> > the need for this right now. Maybe I should add a comment in the
> > code
> > for this.
> >
>
> What I understand from your reply is that the reset is not trivial
> work
Yes. the reset bit is in different register regions, like mmsys,
vdecsys. But the main problem is that I don't see why we need that. We
never that problem.
The sleep ctrl function is just for the safety of the bus. If we have
not it, It also should be ok...If not, the question is: why does the
larb master device call pm_runtime_put before his HW finish the job?
> and needs quite some time to be done properly; in that case: yes,
> please
> add a TODO comment that explains the situation and the discussed
> solution.
>
> Also, since this SLP_PROT_RDY flag seems to be very nice, just a
> simple question: is this a new feature in the SMI IP of MT8186, or is
> there anything similar that we may use on other SoCs, like 8183,
> 8192, 8195, as a follow-up of this series?
All the three SoC support this function. I expect the later SoC will
support this. but the previous SoC has already MP... If someone has
already tested ok after adding it for the previous SoC, I'm ok of
course.
>
> > >
> > > > Fortunately, we have never got this issue. We could add this
> > > > reset
> > > > when
> > > > necessary. Is this OK for you?
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > >
> > > > > > clk_bulk_disable_unprepare(larb->smi.clk_num,
> > > > > > larb-
> > > > > > > smi.clks);
> > > > > >
> > > > > > - return 0;
> > > > > > + return ret;
> > > > > > }
> > > > > >
> > > > > > static const struct dev_pm_ops smi_larb_pm_ops = {
> > > > > >
> > > > >
> > > > >
> > >
> > >
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function
2021-12-08 2:42 ` Yong Wu
@ 2021-12-09 9:12 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-12-09 9:12 UTC (permalink / raw)
To: Yong Wu, Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, Joerg Roedel, Will Deacon, Robin Murphy,
Tomasz Figa, linux-mediatek, srv_heupstream, linux-kernel,
devicetree, linux-arm-kernel, iommu, youlin.pei, anan.sun,
lc.kan, yi.kuo, anthony.huang, Rob Herring, Matthias Brugger
Il 08/12/21 03:42, Yong Wu ha scritto:
> On Tue, 2021-12-07 at 13:16 +0100, AngeloGioacchino Del Regno wrote:
>> Il 07/12/21 13:10, Yong Wu ha scritto:
>>> On Tue, 2021-12-07 at 09:56 +0100, AngeloGioacchino Del Regno
>>> wrote:
>>>> Il 07/12/21 07:24, Yong Wu ha scritto:
>>>>> Hi AngeloGioacchino,
>>>>>
>>>>> Thanks for your review.
>>>>>
>>>>> On Mon, 2021-12-06 at 16:08 +0100, AngeloGioacchino Del Regno
>>>>> wrote:
>>>>>> Il 03/12/21 07:40, Yong Wu ha scritto:
>>>>>>> sleep control means that when the larb go to sleep, we
>>>>>>> should
>>>>>>> wait
>>>>>>> a bit
>>>>>>> until all the current commands are finished. thus, when the
>>>>>>> larb
>>>>>>> runtime
>>>>>>> suspend, we need enable this function to wait until all the
>>>>>>> existed
>>>>>>> command are finished. when the larb resume, just disable
>>>>>>> this
>>>>>>> function.
>>>>>>> This function only improve the safe of bus. Add a new flag
>>>>>>> for
>>>>>>> this
>>>>>>> function. Prepare for mt8186.
>>>>>>>
>>>>>>> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
>>>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>>>>> ---
>>>>>>> drivers/memory/mtk-smi.c | 39
>>>>>>> +++++++++++++++++++++++++++++++++++----
>>>>>>> 1 file changed, 35 insertions(+), 4 deletions(-)
>>>>>
>>>>> [snip]
>>>>>
>>>>>>> static int __maybe_unused mtk_smi_larb_suspend(struct
>>>>>>> device
>>>>>>> *dev)
>>>>>>> {
>>>>>>> struct mtk_smi_larb *larb =
>>>>>>> dev_get_drvdata(dev);
>>>>>>> + int ret = 0;
>>>>>>> +
>>>>>>> + if (MTK_SMI_CAPS(larb->larb_gen->flags_general,
>>>>>>> MTK_SMI_FLAG_SLEEP_CTL))
>>>>>>> + ret = mtk_smi_larb_sleep_ctrl(dev, true);
>>>>>>
>>>>>> Sorry but what happens if SLP_PROT_RDY is not getting set
>>>>>> properly?
>>>>>> From what I can understand in the commit description that
>>>>>> you
>>>>>> wrote,
>>>>>> if we reach
>>>>>> the timeout, then the LARB transactions are not over....
>>>>>>
>>>>>> I see that you are indeed returning a failure here, but you
>>>>>> are
>>>>>> also
>>>>>> turning off
>>>>>> the clocks regardless of whether we get a failure or a
>>>>>> success;
>>>>>> I'm
>>>>>> not sure that
>>>>>> this is right, as this may leave the hardware in an
>>>>>> unpredictable
>>>>>> state (since
>>>>>> there were some more LARB transactions that didn't go
>>>>>> through),
>>>>>> leading to crashes
>>>>>> at system resume (or when retyring to suspend).
>>>>>
>>>>> Thanks for this question. In theory you are right. In this
>>>>> case,
>>>>> the
>>>>> bus already hang.
>>>>>
>>>>> We only printed a fail log in this patch. If this fail happens,
>>>>> we
>>>>> should request the master to check which case cause the larb
>>>>> hang.
>>>>>
>>>>> If the master has a good reason or limitation, the hang is
>>>>> expected, I
>>>>> think we have to add larb reset in this fail case: Reset the
>>>>> larb
>>>>> when
>>>>> the larb runtime resume.
>>>>>
>>>>
>>>> Think about the case in which the system gets resumed only
>>>> partially
>>>> due to a
>>>>
>>>> failure during resume of some driver, or due to a RTC or arch
>>>> timer
>>>> resume and
>>>> suspend right after... or perhaps during runtime suspend/resume
>>>> of
>>>> some devices.
>>>> In that case, we definitely want to avoid any kind of failure
>>>> point
>>>> that would
>>>> lead to a system crash, or any kind of user noticeable (or UX
>>>> disrupting) "strange
>>>> behavior".
>>>>
>>>> I think that we should make sure that the system suspends
>>>> cleanly,
>>>> instead of
>>>> patching up any possible leftover issue at resume time: if this
>>>> is
>>>> doable with
>>>> a LARB reset in suspend error case, that looks like being a good
>>>> option indeed.
>>>>
>>>> As a side note, thinking about UX, losing a little more time
>>>> during
>>>> suspend is
>>>> nothing really noticeable for the user... on the other hand,
>>>> spending
>>>> more time
>>>> during resume may be something noticeable to the user.
>>>> For this reason, I think that guaranteeing that the system
>>>> resumes as
>>>> fast as
>>>> possible is very important, which adds up to the need of
>>>> suspending
>>>> cleanly.
>>>
>>> Thanks for this comment. I will put it in the suspend when adding
>>> the
>>> reset. But I have no plan to add it in this version since I don't
>>> see
>>> the need for this right now. Maybe I should add a comment in the
>>> code
>>> for this.
>>>
>>
>> What I understand from your reply is that the reset is not trivial
>> work
>
> Yes. the reset bit is in different register regions, like mmsys,
> vdecsys. But the main problem is that I don't see why we need that. We
> never that problem.
>
The fact that we didn't get any "visible" problem with that is very good,
indeed, but having a recovery mechanism in place in the event that something
like that happens is going to be helpful in the future, as driver updates (either
to support new SoCs or Linux API changes, or new APIs) may produce unexpected
results sometimes and this will make sure that, despite there may be a problem,
the hardware will still work even before solving the producer of the issue.
Sometimes it may happen that solving an issue is nothing trivial, hence requires
a lot of time, and that's the main usefulness of that - and it's as useful as
your *great* idea of enabling SLP_PROT_RDY to check on the bus.
> The sleep ctrl function is just for the safety of the bus. If we have
> not it, It also should be ok...If not, the question is: why does the
> larb master device call pm_runtime_put before his HW finish the job?
>
I agree on the fact that calling pm_runtime_put before the HW finishes the
job is something that should *never* happen.
>> and needs quite some time to be done properly; in that case: yes,
>> please
>> add a TODO comment that explains the situation and the discussed
>> solution.
>>
>> Also, since this SLP_PROT_RDY flag seems to be very nice, just a
>> simple question: is this a new feature in the SMI IP of MT8186, or is
>> there anything similar that we may use on other SoCs, like 8183,
>> 8192, 8195, as a follow-up of this series?
>
> All the three SoC support this function. I expect the later SoC will
> support this. but the previous SoC has already MP... If someone has
> already tested ok after adding it for the previous SoC, I'm ok of
> course.
>
Thanks for the information. Again, this feature is very nice, so if it can
be used on any other SoC, it's going to be helpful in the future.
I will do some research.
Regards,
- Angelo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs
2021-12-03 23:34 ` Rob Herring
@ 2021-12-13 6:48 ` Yong Wu
2021-12-13 20:30 ` Rob Herring
0 siblings, 1 reply; 19+ messages in thread
From: Yong Wu @ 2021-12-13 6:48 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-kernel, iommu, Tomasz Figa, Krzysztof Kozlowski,
Joerg Roedel, yi.kuo, anan.sun, Will Deacon, devicetree,
Robin Murphy, Rob Herring, linux-kernel, Krzysztof Kozlowski,
anthony.huang, lc.kan, srv_heupstream, Matthias Brugger,
linux-mediatek, youlin.pei
On Fri, 2021-12-03 at 17:34 -0600, Rob Herring wrote:
> On Fri, 03 Dec 2021 14:40:24 +0800, Yong Wu wrote:
> > If a platform's larb support gals, there will be some larbs have a
> > one
> > more "gals" clock while the others still only need "apb"/"smi"
> > clocks.
> > then the minItems is 2 and the maxItems is 3.
> >
> > Fixes: 27bb0e42855a ("dt-bindings: memory: mediatek: Convert SMI to
> > DT schema")
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > .../bindings/memory-controllers/mediatek,smi-larb.yaml |
> > 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
>
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
>
> Note that it is not yet a requirement to have 0 warnings for
> dtbs_check.
> This will change in the future.
>
> Full log is available here:
> https://patchwork.ozlabs.org/patch/1563127
>
>
> larb@14016000: 'mediatek,larb-id' is a required property
> arch/arm64/boot/dts/mediatek/mt8167-pumpkin.dt.yaml
I will fix this in next version. This property is not needed in mt8167.
>
> larb@14017000: clock-names: ['apb', 'smi'] is too short
> arch/arm64/boot/dts/mediatek/mt8183-evb.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-
> burnet.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-damu.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-
> fennel14.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-
> sku1.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-
> sku6.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-juniper-
> sku16.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kappa.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kenzo.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-
> sku0.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-
> sku1.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku16.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku272.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku288.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku32.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dt.yaml
Some larbs only have two clocks(apb/smi) in mt8183. thus it is
reasonable for me. I won't fix this in next version.
Please tell me if I miss something.
Thanks.
>
> larb@15001000: 'mediatek,larb-id' is a required property
> arch/arm64/boot/dts/mediatek/mt8167-pumpkin.dt.yaml
>
> larb@16010000: clock-names: ['apb', 'smi'] is too short
> arch/arm64/boot/dts/mediatek/mt8183-evb.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-
> burnet.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-damu.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-
> fennel14.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-
> sku1.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-
> sku6.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-juniper-
> sku16.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kappa.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kenzo.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-
> sku0.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-
> sku1.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku16.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku272.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku288.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku32.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dt.yaml
>
> larb@16010000: 'mediatek,larb-id' is a required property
> arch/arm64/boot/dts/mediatek/mt8167-pumpkin.dt.yaml
>
> larb@17010000: clock-names: ['apb', 'smi'] is too short
> arch/arm64/boot/dts/mediatek/mt8183-evb.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-
> burnet.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-damu.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-
> fennel14.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-
> sku1.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-
> sku6.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-juniper-
> sku16.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kappa.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kenzo.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-
> sku0.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-
> sku1.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku16.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku272.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku288.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku32.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dt.yaml
> arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dt.yaml
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs
2021-12-13 6:48 ` Yong Wu
@ 2021-12-13 20:30 ` Rob Herring
0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2021-12-13 20:30 UTC (permalink / raw)
To: Yong Wu
Cc: linux-arm-kernel, iommu, Tomasz Figa, Krzysztof Kozlowski,
Joerg Roedel, yi.kuo, anan.sun, Will Deacon, devicetree,
Robin Murphy, linux-kernel, Krzysztof Kozlowski, anthony.huang,
lc.kan, srv_heupstream, Matthias Brugger, linux-mediatek,
youlin.pei
On Mon, Dec 13, 2021 at 02:48:52PM +0800, Yong Wu wrote:
> On Fri, 2021-12-03 at 17:34 -0600, Rob Herring wrote:
> > On Fri, 03 Dec 2021 14:40:24 +0800, Yong Wu wrote:
> > > If a platform's larb support gals, there will be some larbs have a
> > > one
> > > more "gals" clock while the others still only need "apb"/"smi"
> > > clocks.
> > > then the minItems is 2 and the maxItems is 3.
> > >
> > > Fixes: 27bb0e42855a ("dt-bindings: memory: mediatek: Convert SMI to
> > > DT schema")
> > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > ---
> > > .../bindings/memory-controllers/mediatek,smi-larb.yaml |
> > > 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> >
> > Running 'make dtbs_check' with the schema in this patch gives the
> > following warnings. Consider if they are expected or the schema is
> > incorrect. These may not be new warnings.
> >
> > Note that it is not yet a requirement to have 0 warnings for
> > dtbs_check.
> > This will change in the future.
> >
> > Full log is available here:
> > https://patchwork.ozlabs.org/patch/1563127
> >
> >
> > larb@14016000: 'mediatek,larb-id' is a required property
> > arch/arm64/boot/dts/mediatek/mt8167-pumpkin.dt.yaml
>
> I will fix this in next version. This property is not needed in mt8167.
>
> >
> > larb@14017000: clock-names: ['apb', 'smi'] is too short
> > arch/arm64/boot/dts/mediatek/mt8183-evb.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-
> > burnet.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-damu.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-
> > fennel14.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-
> > sku1.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-fennel-
> > sku6.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-juniper-
> > sku16.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kappa.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-kenzo.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-
> > sku0.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-willow-
> > sku1.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku16.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku272.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku288.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-kodama-sku32.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku0.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-kukui-krane-sku176.dt.yaml
> > arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dt.yaml
>
> Some larbs only have two clocks(apb/smi) in mt8183. thus it is
> reasonable for me. I won't fix this in next version.
You also need to adjust 'clock-names' to allow for 2 items.
Rob
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] dt-bindings: memory: mediatek: Add mt8186 support
2021-12-03 6:40 ` [PATCH 2/4] dt-bindings: memory: mediatek: Add mt8186 support Yong Wu
@ 2021-12-13 20:31 ` Rob Herring
0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2021-12-13 20:31 UTC (permalink / raw)
To: Yong Wu
Cc: srv_heupstream, Joerg Roedel, Matthias Brugger, anan.sun, yi.kuo,
Krzysztof Kozlowski, Robin Murphy, iommu, Will Deacon,
Tomasz Figa, Krzysztof Kozlowski, linux-arm-kernel, lc.kan,
linux-kernel, youlin.pei, devicetree, Rob Herring, anthony.huang,
linux-mediatek
On Fri, 03 Dec 2021 14:40:25 +0800, Yong Wu wrote:
> Add mt8186 smi support in the bindings.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> .../bindings/memory-controllers/mediatek,smi-common.yaml | 4 +++-
> .../bindings/memory-controllers/mediatek,smi-larb.yaml | 3 +++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
Acked-by: Rob Herring <robh@kernel.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-12-13 20:32 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-03 6:40 [PATCH 0/4] MT8186 SMI SUPPORT Yong Wu
2021-12-03 6:40 ` [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs Yong Wu
2021-12-03 23:34 ` Rob Herring
2021-12-13 6:48 ` Yong Wu
2021-12-13 20:30 ` Rob Herring
2021-12-03 6:40 ` [PATCH 2/4] dt-bindings: memory: mediatek: Add mt8186 support Yong Wu
2021-12-13 20:31 ` Rob Herring
2021-12-03 6:40 ` [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function Yong Wu
2021-12-04 11:48 ` Krzysztof Kozlowski
2021-12-06 8:15 ` Yong Wu
2021-12-06 15:08 ` AngeloGioacchino Del Regno
2021-12-07 6:24 ` Yong Wu
2021-12-07 8:56 ` AngeloGioacchino Del Regno
2021-12-07 12:10 ` Yong Wu
2021-12-07 12:16 ` AngeloGioacchino Del Regno
2021-12-08 2:42 ` Yong Wu
2021-12-09 9:12 ` AngeloGioacchino Del Regno
2021-12-03 6:40 ` [PATCH 4/4] memory: mtk-smi: mt8186: Add smi support Yong Wu
2021-12-06 15:00 ` AngeloGioacchino Del Regno
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).