Message-ID: <ddd1d62e-b1d2-4271-99a3-74bb0a48fb48@collabora.com>
Date: Tue, 18 Feb 2025 15:24:58 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Friday Yang (杨阳) <Friday.Yang@...iatek.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
 "mturquette@...libre.com" <mturquette@...libre.com>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 Garmin Chang (張家銘) <Garmin.Chang@...iatek.com>,
 "sboyd@...nel.org" <sboyd@...nel.org>, Yong Wu (吴勇)
 <Yong.Wu@...iatek.com>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "robh@...nel.org" <robh@...nel.org>,
 Project_Global_Chrome_Upstream_Group
 <Project_Global_Chrome_Upstream_Group@...iatek.com>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
 "conor@...nel.org" <conor@...nel.org>
Subject: Re: [PATCH v3 1/2] dt-bindings: clock: mediatek: Add SMI LARBs reset
 for MT8188

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@...iatek.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
>>>>>

Powered by blists - more mailing lists