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

[Conor Dooley <conor@kernel.org>]

[-- 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 --]