[PATCH v3 0/2] Add SMI LARBs reset for MediaTek MT8188 SoC

[PATCH v3 0/2] Add SMI LARBs reset for MediaTek MT8188 SoC

[Friday Yang]

Based on tag: next-20250120, linux-next/master

When we enable/disable power domain, the SMI LARBs linked to this power
domain could be affected by the bus glitch. To avoid this issue, SMI
need to apply clamp and reset opereations.

This patch mainly add these functions:
1) Add reset platform data for SMI LARBs to implement reset opereations
   in current clock control driver.
2) Add bindings to support the reset controller driver.

Changes v3:
- Drop the v2 smi reset binding
- Add '#reset-cells' for the clock controller in the image, camera
  and IPE subsystems.
- Drop the v2 smi reset driver and use the existed clock control driver
- Add reset platform data for SMI LARBs in the image, camera and IPE
  subsystems.

v2:
https://lore.kernel.org/lkml/20241120063305.8135-2-friday.yang@mediatek.com/
https://lore.kernel.org/lkml/20241120063305.8135-3-friday.yang@mediatek.com/

Friday Yang (2):
  dt-bindings: clock: mediatek: Add support for SMI LARBs reset
  clk: mediatek: Add support for SMI LARBs reset

 .../bindings/clock/mediatek,mt8188-clock.yaml | 21 +++++++++++++++++++
 drivers/clk/mediatek/clk-mt8188-cam.c         | 17 +++++++++++++++
 drivers/clk/mediatek/clk-mt8188-img.c         | 18 ++++++++++++++++
 drivers/clk/mediatek/clk-mt8188-ipe.c         | 14 +++++++++++++
 4 files changed, 70 insertions(+)

--
2.46.0

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

[Friday Yang]

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

Signed-off-by: Friday Yang <friday.yang@mediatek.com>
---
 .../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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]

On Tue, Jan 21, 2025 at 02:50:40PM +0800, Friday Yang wrote:
> 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. What are "SMI LARBs"? Why did things previously work without
acting as a reset controller?

> 
> Signed-off-by: Friday Yang <friday.yang@mediatek.com>
> ---
>  .../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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Friday Yang (杨阳)]

On Tue, 2025-01-21 at 17:30 +0000, Conor Dooley wrote:
> On Tue, Jan 21, 2025 at 02:50:40PM +0800, Friday Yang wrote:
> > 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. What are "SMI LARBs"? Why did things previously work
> 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/

https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
SMI clamp and reset operations should be implemented in SMI driver
instead of PM driver.

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/

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


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).

> > 
> > Signed-off-by: Friday Yang <friday.yang@mediatek.com>
> > ---
> >  .../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
> > 

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4680 bytes --]

On Wed, Jan 22, 2025 at 07:40:12AM +0000, Friday Yang (杨阳) wrote:
> On Tue, 2025-01-21 at 17:30 +0000, Conor Dooley wrote:
> > On Tue, Jan 21, 2025 at 02:50:40PM +0800, Friday Yang wrote:
> > > 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.

> What are "SMI LARBs"? Why did things previously work
> > 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/
> 
> https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
> 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.

> 
> 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/
> 
> https://lore.kernel.org/lkml/20241120063305.8135-3-friday.yang@mediatek.com/
> 
> 
> 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).
> 
> > > 
> > > Signed-off-by: Friday Yang <friday.yang@mediatek.com>
> > > ---
> > >  .../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
> > > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Friday Yang (杨阳)]

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:
> > On Tue, 2025-01-21 at 17:30 +0000, Conor Dooley wrote:
> > > On Tue, Jan 21, 2025 at 02:50:40PM +0800, Friday Yang wrote:
> > > > 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/
2.
https://lore.kernel.org/all/20240327055732.28198-2-yu-chang.lee@mediatek.com/
3.
https://lore.kernel.org/all/20240327055732.28198-3-yu-chang.lee@mediatek.com/

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/
5.
https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
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/
7.
https://lore.kernel.org/lkml/20250121064934.13482-1-friday.yang@mediatek.com/

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.

> > What are "SMI LARBs"? Why did things previously work
> > > 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/
> > 
> > 
https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
> > 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.
> 
> > 
> > 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/
> > 
> > 
https://lore.kernel.org/lkml/20241120063305.8135-3-friday.yang@mediatek.com/
> > 
> > 
> > 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).
> > 
> > > > 
> > > > Signed-off-by: Friday Yang <friday.yang@mediatek.com>
> > > > ---
> > > >  .../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
> > > > 

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

[AngeloGioacchino Del Regno]

Il 18/02/25 13:44, Friday Yang (杨阳) ha scritto:
> 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:
>>> On Tue, 2025-01-21 at 17:30 +0000, Conor Dooley wrote:
>>>> On Tue, Jan 21, 2025 at 02:50:40PM +0800, Friday Yang wrote:
>>>>> 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/
> 2.
> https://lore.kernel.org/all/20240327055732.28198-2-yu-chang.lee@mediatek.com/
> 3.
> https://lore.kernel.org/all/20240327055732.28198-3-yu-chang.lee@mediatek.com/
> 
> 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/
> 5.
> https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
> 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/
> 7.
> https://lore.kernel.org/lkml/20250121064934.13482-1-friday.yang@mediatek.com/
> 
> 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.
> 
>>> What are "SMI LARBs"? Why did things previously work
>>>> 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/
>>>
>>>
> https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
>>> 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.
>>

This conversation slipped through the cracks - wanted to reply to this quite a bit
of time ago but then for whatever reason .... eh here we are :-)

Anyway.

The cleanest option to get the glitching situation to get resolved is probably
exactly the one that Friday proposed with this series...

I agree that the commit needs a proper description, though, and even though the
drivers were never actually used (so it's not a huge problem - as in - no device
gets broken when this is merged), it's still an ABI breakage, and that has to be
justified with a good reason.

The good reason is that there's a hardware bug that you're trying to resolve here
and that emerged only after the initial upstreaming of this binding (do *not*
mention drivers in DT bindings, those describe the hardware, not software!), and
the only way to resolve this situation is by resetting the Local Arbiter (LARB)
of the cam/img/ipe macro-blocks.

Failing to do this, the hardware is going to be unstable during high/dynamic load
conditions.

So, just describe the problem and how you're solving it in the commit description:
that's going to be okay and justifying everything that you're doing here.

I'm sorry for chiming in that late, btw.

Cheers,
Angelo

>> Thanks,
>> Conor.
>>
>>>
>>> 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/
>>>
>>>
> https://lore.kernel.org/lkml/20241120063305.8135-3-friday.yang@mediatek.com/
>>>
>>>
>>> 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).
>>>
>>>>>
>>>>> Signed-off-by: Friday Yang <friday.yang@mediatek.com>
>>>>> ---
>>>>>   .../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
>>>>>

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5128 bytes --]

On Tue, Feb 18, 2025 at 03:24:58PM +0100, AngeloGioacchino Del Regno wrote:
> Il 18/02/25 13:44, Friday Yang (杨阳) ha scritto:
> > 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:
> > > > On Tue, 2025-01-21 at 17:30 +0000, Conor Dooley wrote:
> > > > > On Tue, Jan 21, 2025 at 02:50:40PM +0800, Friday Yang wrote:
> > > > > > 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/
> > 2.
> > https://lore.kernel.org/all/20240327055732.28198-2-yu-chang.lee@mediatek.com/
> > 3.
> > https://lore.kernel.org/all/20240327055732.28198-3-yu-chang.lee@mediatek.com/
> > 
> > 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/
> > 5.
> > https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
> > 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/
> > 7.
> > https://lore.kernel.org/lkml/20250121064934.13482-1-friday.yang@mediatek.com/
> > 
> > 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.
> > 
> > > > What are "SMI LARBs"? Why did things previously work
> > > > > 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/
> > > > 
> > > > 
> > https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
> > > > 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.
> > > 
> 
> This conversation slipped through the cracks - wanted to reply to this quite a bit
> of time ago but then for whatever reason .... eh here we are :-)
> 
> Anyway.
> 
> The cleanest option to get the glitching situation to get resolved is probably
> exactly the one that Friday proposed with this series...
> 
> I agree that the commit needs a proper description, though, and even though the
> drivers were never actually used (so it's not a huge problem - as in - no device
> gets broken when this is merged), it's still an ABI breakage, and that has to be
> justified with a good reason.
> 
> The good reason is that there's a hardware bug that you're trying to resolve here
> and that emerged only after the initial upstreaming of this binding (do *not*
> mention drivers in DT bindings, those describe the hardware, not software!), and
> the only way to resolve this situation is by resetting the Local Arbiter (LARB)
> of the cam/img/ipe macro-blocks.
> 
> Failing to do this, the hardware is going to be unstable during high/dynamic load
> conditions.
> 
> So, just describe the problem and how you're solving it in the commit description:
> that's going to be okay and justifying everything that you're doing here.

Aye, seems fair enough to me to make the change if nothings ever used it
as it currently is, provided it's justified in the commit message :+1:

> 
> I'm sorry for chiming in that late, btw.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 2/2] clk: mediatek: Add SMI LARBs reset for MT8188

[Friday Yang]

SMI LARBs require reset functions when applying clamp operations.
Add reset platform data for SMI LARBs in the image, camera and IPE
subsystems.

Signed-off-by: Friday Yang <friday.yang@mediatek.com>
---
 drivers/clk/mediatek/clk-mt8188-cam.c | 17 +++++++++++++++++
 drivers/clk/mediatek/clk-mt8188-img.c | 18 ++++++++++++++++++
 drivers/clk/mediatek/clk-mt8188-ipe.c | 14 ++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mt8188-cam.c b/drivers/clk/mediatek/clk-mt8188-cam.c
index 7500bd25387f..9b029fdd584e 100644
--- a/drivers/clk/mediatek/clk-mt8188-cam.c
+++ b/drivers/clk/mediatek/clk-mt8188-cam.c
@@ -20,6 +20,8 @@ static const struct mtk_gate_regs cam_cg_regs = {
 #define GATE_CAM(_id, _name, _parent, _shift)			\
 	GATE_MTK(_id, _name, _parent, &cam_cg_regs, _shift, &mtk_clk_gate_ops_setclr)

+#define CAM_SYS_SMI_LARB_RST_OFF	(0xA0)
+
 static const struct mtk_gate cam_main_clks[] = {
 	GATE_CAM(CLK_CAM_MAIN_LARB13, "cam_main_larb13", "top_cam", 0),
 	GATE_CAM(CLK_CAM_MAIN_LARB14, "cam_main_larb14", "top_cam", 1),
@@ -72,6 +74,17 @@ static const struct mtk_gate cam_yuvb_clks[] = {
 	GATE_CAM(CLK_CAM_YUVB_CAMTG, "cam_yuvb_camtg", "top_cam", 2),
 };

+/* Reset for SMI larb 16a/16b/17a/17b */
+static u16 cam_sys_rst_ofs[] = {
+	CAM_SYS_SMI_LARB_RST_OFF,
+};
+
+static const struct mtk_clk_rst_desc cam_sys_rst_desc = {
+	.version = MTK_RST_SIMPLE,
+	.rst_bank_ofs = cam_sys_rst_ofs,
+	.rst_bank_nr = ARRAY_SIZE(cam_sys_rst_ofs),
+};
+
 static const struct mtk_clk_desc cam_main_desc = {
 	.clks = cam_main_clks,
 	.num_clks = ARRAY_SIZE(cam_main_clks),
@@ -80,21 +93,25 @@ static const struct mtk_clk_desc cam_main_desc = {
 static const struct mtk_clk_desc cam_rawa_desc = {
 	.clks = cam_rawa_clks,
 	.num_clks = ARRAY_SIZE(cam_rawa_clks),
+	.rst_desc = &cam_sys_rst_desc,
 };

 static const struct mtk_clk_desc cam_rawb_desc = {
 	.clks = cam_rawb_clks,
 	.num_clks = ARRAY_SIZE(cam_rawb_clks),
+	.rst_desc = &cam_sys_rst_desc,
 };

 static const struct mtk_clk_desc cam_yuva_desc = {
 	.clks = cam_yuva_clks,
 	.num_clks = ARRAY_SIZE(cam_yuva_clks),
+	.rst_desc = &cam_sys_rst_desc,
 };

 static const struct mtk_clk_desc cam_yuvb_desc = {
 	.clks = cam_yuvb_clks,
 	.num_clks = ARRAY_SIZE(cam_yuvb_clks),
+	.rst_desc = &cam_sys_rst_desc,
 };

 static const struct of_device_id of_match_clk_mt8188_cam[] = {
diff --git a/drivers/clk/mediatek/clk-mt8188-img.c b/drivers/clk/mediatek/clk-mt8188-img.c
index cb2fbd4136b9..d44bfbd8308a 100644
--- a/drivers/clk/mediatek/clk-mt8188-img.c
+++ b/drivers/clk/mediatek/clk-mt8188-img.c
@@ -20,6 +20,8 @@ static const struct mtk_gate_regs imgsys_cg_regs = {
 #define GATE_IMGSYS(_id, _name, _parent, _shift)			\
 	GATE_MTK(_id, _name, _parent, &imgsys_cg_regs, _shift, &mtk_clk_gate_ops_setclr)

+#define IMG_SYS_SMI_LARB_RST_OFF	(0xC)
+
 static const struct mtk_gate imgsys_main_clks[] = {
 	GATE_IMGSYS(CLK_IMGSYS_MAIN_LARB9, "imgsys_main_larb9", "top_img", 0),
 	GATE_IMGSYS(CLK_IMGSYS_MAIN_TRAW0, "imgsys_main_traw0", "top_img", 1),
@@ -58,6 +60,17 @@ static const struct mtk_gate imgsys1_dip_nr_clks[] = {
 	GATE_IMGSYS(CLK_IMGSYS1_DIP_NR_DIP_NR, "imgsys1_dip_nr_dip_nr", "top_img", 1),
 };

+/* Reset for SMI larb 10/11a/11b/11c/15 */
+static u16 img_sys_rst_ofs[] = {
+	IMG_SYS_SMI_LARB_RST_OFF,
+};
+
+static const struct mtk_clk_rst_desc img_sys_rst_desc = {
+	.version = MTK_RST_SIMPLE,
+	.rst_bank_ofs = img_sys_rst_ofs,
+	.rst_bank_nr = ARRAY_SIZE(img_sys_rst_ofs),
+};
+
 static const struct mtk_clk_desc imgsys_main_desc = {
 	.clks = imgsys_main_clks,
 	.num_clks = ARRAY_SIZE(imgsys_main_clks),
@@ -66,26 +79,31 @@ static const struct mtk_clk_desc imgsys_main_desc = {
 static const struct mtk_clk_desc imgsys_wpe1_desc = {
 	.clks = imgsys_wpe1_clks,
 	.num_clks = ARRAY_SIZE(imgsys_wpe1_clks),
+	.rst_desc = &img_sys_rst_desc,
 };

 static const struct mtk_clk_desc imgsys_wpe2_desc = {
 	.clks = imgsys_wpe2_clks,
 	.num_clks = ARRAY_SIZE(imgsys_wpe2_clks),
+	.rst_desc = &img_sys_rst_desc,
 };

 static const struct mtk_clk_desc imgsys_wpe3_desc = {
 	.clks = imgsys_wpe3_clks,
 	.num_clks = ARRAY_SIZE(imgsys_wpe3_clks),
+	.rst_desc = &img_sys_rst_desc,
 };

 static const struct mtk_clk_desc imgsys1_dip_top_desc = {
 	.clks = imgsys1_dip_top_clks,
 	.num_clks = ARRAY_SIZE(imgsys1_dip_top_clks),
+	.rst_desc = &img_sys_rst_desc,
 };

 static const struct mtk_clk_desc imgsys1_dip_nr_desc = {
 	.clks = imgsys1_dip_nr_clks,
 	.num_clks = ARRAY_SIZE(imgsys1_dip_nr_clks),
+	.rst_desc = &img_sys_rst_desc,
 };

 static const struct of_device_id of_match_clk_mt8188_imgsys_main[] = {
diff --git a/drivers/clk/mediatek/clk-mt8188-ipe.c b/drivers/clk/mediatek/clk-mt8188-ipe.c
index 8f1933b71e28..70a011c1f9ce 100644
--- a/drivers/clk/mediatek/clk-mt8188-ipe.c
+++ b/drivers/clk/mediatek/clk-mt8188-ipe.c
@@ -20,6 +20,8 @@ static const struct mtk_gate_regs ipe_cg_regs = {
 #define GATE_IPE(_id, _name, _parent, _shift)			\
 	GATE_MTK(_id, _name, _parent, &ipe_cg_regs, _shift, &mtk_clk_gate_ops_setclr)

+#define IPE_SYS_SMI_LARB_RST_OFF	(0xC)
+
 static const struct mtk_gate ipe_clks[] = {
 	GATE_IPE(CLK_IPE_DPE, "ipe_dpe", "top_ipe", 0),
 	GATE_IPE(CLK_IPE_FDVT, "ipe_fdvt", "top_ipe", 1),
@@ -28,9 +30,21 @@ static const struct mtk_gate ipe_clks[] = {
 	GATE_IPE(CLK_IPE_SMI_LARB12, "ipe_smi_larb12", "top_ipe", 4),
 };

+/* Reset for SMI larb 12 */
+static u16 ipe_sys_rst_ofs[] = {
+	IPE_SYS_SMI_LARB_RST_OFF,
+};
+
+static const struct mtk_clk_rst_desc ipe_sys_rst_desc = {
+	.version = MTK_RST_SIMPLE,
+	.rst_bank_ofs = ipe_sys_rst_ofs,
+	.rst_bank_nr = ARRAY_SIZE(ipe_sys_rst_ofs),
+};
+
 static const struct mtk_clk_desc ipe_desc = {
 	.clks = ipe_clks,
 	.num_clks = ARRAY_SIZE(ipe_clks),
+	.rst_desc = &ipe_sys_rst_desc,
 };

 static const struct of_device_id of_match_clk_mt8188_ipe[] = {
--
2.46.0