Re: [PATCH v3 1/2] dt-bindings: clock: mediatek: Add SMI LARBs reset for MT8188

On Fri, 2025-01-24 at 17:31 +0000, Conor Dooley wrote:

On Wed, Jan 22, 2025 at 07:40:12AM +0000, Friday Yang (杨阳) wrote:

quoted

On Tue, 2025-01-21 at 17:30 +0000, Conor Dooley wrote:

quoted

On Tue, Jan 21, 2025 at 02:50:40PM +0800, Friday Yang wrote:

quoted

SMI LARBs require reset functions when applying clamp
operations.
Add '#reset-cells' for the clock controller located in image,
camera
and IPE subsystems.

A new required property is an abi break, please explain why this
is
required.

You never answered this part. From a quick check, looks like the
change
you made will cause probe failures if the resets are not present?
Maybe
I misread the driver code in my quick skim - but that is the
implication
of a new required property, so I didn't dig super far.

Adding new properties that break a driver is not really acceptable,
so I
hope I made a mistake there.

Sorry to reply late.
This is a known bus glitch issue. It worked because MediaTek has
provided patches 1, 2 and 3. In other word, it can not work
without patches 1, 2 and 3.

1.
https://lore.kernel.org/all/20240327055732.28198-1-yu-chang.lee@mediatek.com/ (local)
2.
https://lore.kernel.org/all/20240327055732.28198-2-yu-chang.lee@mediatek.com/ (local)
3.
https://lore.kernel.org/all/20240327055732.28198-3-yu-chang.lee@mediatek.com/ (local)

Patches 1, 2 and 3 have been previously reviewed, and the reviewers
provided the following comments:
4.
https://lore.kernel.org/all/CAFGrd9qZhObQXvm2_abqaX83xMLqxjQETB2=wXpobDWU1CnvkA@mail.gmail.com/ (local)
5.
https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/ (local)
As I mentioned earlier, SMI clamp and reset operations should be
implemented in SMI driver rather than the PM driver. Additionally, the
reset operations have already been implemented in the clock control
driver. There is no need to submit duplicate code. 

To address this, I have provided patches 6, 7 to replace patches 1, 2,
and 3, as I believe this approach aligns more closely with the
reviewers' requirements.
6.
https://lore.kernel.org/lkml/20250121065045.13514-1-friday.yang@mediatek.com/ (local)
7.
https://lore.kernel.org/lkml/20250121064934.13482-1-friday.yang@mediatek.com/ (local)

What's more, I have tested the patch 6, 7 in MediaTek MT8188 SoC.
It could work well. If you have any questions, please feel free to
contact me.

quoted

What are "SMI LARBs"? Why did things previously work

quoted

without
acting as a reset controller?

The background can refer to the discussion in the following link:

https://lore.kernel.org/all/CAFGrd9qZhObQXvm2_abqaX83xMLqxjQETB2=wXpobDWU1CnvkA@mail.gmail.com/ (local)

quoted

https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/ (local)

quoted

SMI clamp and reset operations should be implemented in SMI driver
instead of PM driver.

So the answer to how it worked previously was that nothing actually
used
this multimedia interface?

Your commit message needs to explain why a new required property is
okay
and why it was not there before.

Thanks,
Conor.

quoted

I previously added the SMI reset control driver. However, the
reviewer's comments are correct, these functions have already
been implemented in the clock control driver. There is no need
to submit duplicate code. 

https://lore.kernel.org/lkml/20241120063305.8135-2-friday.yang@mediatek.com/ (local)

quoted

https://lore.kernel.org/lkml/20241120063305.8135-3-friday.yang@mediatek.com/ (local)

quoted

On the MediaTek platform, the SMI block diagram like this:

                DRAM
                 |
            EMI(External Memory Interface)
                 |  |
          MediaTek IOMMU(Input Output Memory Management Unit)
                 |  |
             SMI-Common(Smart Multimedia Interface Common)
                 |
         +----------------+------------------+
         |                |                  |
         |                |                  |
         |                |                  |
         |                |                  |
         |                |                  |
       larb0       SMI-Sub-Common0     SMI-Sub-Common1
                   |      |     |      |             |
                  larb1  larb2 larb3  larb7       larb9

The SMI-Common connects with SMI LARBs and IOMMU. The maximum LARBs
number that connects with a SMI-Common is 8. If the engines number
is
over 8, sometimes we use a SMI-Sub-Common which is nearly same with
SMI-Common. It supports up to 8 input and 1 output(SMI-Common has 2
output).

quoted

quoted

Signed-off-by: Friday Yang <redacted>
---
 .../bindings/clock/mediatek,mt8188-clock.yaml | 21
+++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git
a/Documentation/devicetree/bindings/clock/mediatek,mt8188-
clock.yaml
b/Documentation/devicetree/bindings/clock/mediatek,mt8188-
clock.yaml
index 860570320545..2985c8c717d7 100644

--- a/Documentation/devicetree/bindings/clock/mediatek,mt8188-

clock.yaml

+++ b/Documentation/devicetree/bindings/clock/mediatek,mt8188-

clock.yaml

@@ -57,6 +57,27 @@ required:
   - reg
   - '#clock-cells'

+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt8188-camsys-rawa
+              - mediatek,mt8188-camsys-rawb
+              - mediatek,mt8188-camsys-yuva
+              - mediatek,mt8188-camsys-yuvb
+              - mediatek,mt8188-imgsys-wpe1
+              - mediatek,mt8188-imgsys-wpe2
+              - mediatek,mt8188-imgsys-wpe3
+              - mediatek,mt8188-imgsys1-dip-nr
+              - mediatek,mt8188-imgsys1-dip-top
+              - mediatek,mt8188-ipesys
+
+    then:
+      required:
+        - '#reset-cells'
+
 additionalProperties: false

 examples:
--

2.46.0