| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250124-aide-feisty-821cf9cf1382@spud>
Date: Fri, 24 Jan 2025 17:31:33 +0000
From: Conor Dooley <conor@...nel.org>
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>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
Yong Wu (吴勇) <Yong.Wu@...iatek.com>,
"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>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v3 1/2] dt-bindings: clock: mediatek: Add SMI LARBs reset
for MT8188
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@...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
> > >
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists