| Message ID | 20250121065045.13514-2-friday.yang@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add SMI LARBs reset for MediaTek MT8188 SoC | expand |
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 >
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 > >
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 > > >
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 > > > >
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 >>>>>
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.
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:
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(+) -- 2.46.0