[PATCH 0/4] MT8186 SMI SUPPORT

[PATCH 0/4] MT8186 SMI SUPPORT

[Yong Wu]

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


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs

[Yong Wu]

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs

[Rob Herring]

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs

[Yong Wu]

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
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] dt-bindings: memory: mediatek: Correct the minItems of clk for larbs

[Rob Herring]

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/4] dt-bindings: memory: mediatek: Add mt8186 support

[Yong Wu]

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] dt-bindings: memory: mediatek: Add mt8186 support

[Rob Herring]

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>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[Yong Wu]

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[Krzysztof Kozlowski]

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[Yong Wu]

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[AngeloGioacchino Del Regno]

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 = {
> 

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[Yong Wu]

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 = {
> > 
> 
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[AngeloGioacchino Del Regno]

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 = {
>>>
>>
>>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[Yong Wu]

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 = {
> > > > 
> > > 
> > > 
> 
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[AngeloGioacchino Del Regno]

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 = {
>>>>>
>>>>
>>>>
>>
>>


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[Yong Wu]

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 = {
> > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[AngeloGioacchino Del Regno]

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 4/4] memory: mtk-smi: mt8186: Add smi support

[Yong Wu]

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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] memory: mtk-smi: mt8186: Add smi support

[AngeloGioacchino Del Regno]

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>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu