linux-kernel - Re: [PATCH v2 1/2] dt-bindings: memory: mediatek: Add SMI reset and clamp related property

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2b9b7a52536bfa07155a24feecd03a3b4268b07b.camel@mediatek.com>
Date: Fri, 22 Nov 2024 09:40:22 +0000
From: Friday Yang (杨阳) <Friday.Yang@...iatek.com>
To: "krzk@...nel.org" <krzk@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	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>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: memory: mediatek: Add SMI reset and
 clamp related property

On Wed, 2024-11-20 at 08:45 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Nov 20, 2024 at 02:36:38PM +0800, Friday Yang wrote:
> > diff --git a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > index 2381660b324c..302c0f93b49d 100644
> > --- a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > +++ b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > @@ -69,6 +69,18 @@ properties:
> >      description: the hardware id of this larb. It's only required
> > when this
> >        hardware id is not consecutive from its M4U point of view.
> > 
> > +  resets:
> > +    maxItems: 1
> > +    description: This contains a phandle to the reset controller
> > node and an index
> > +      to a reset signal. SMI larbs need to get the reset
> > controller by the node.
> 
> First sentence is 100% redundant. Arguments depend on the reset-
> cells,
> not this binding.
> 
> > +      SMI could get the reset signal by the index number defined
> > in the header
> > +      include/dt-bindings/reset/mt8188-resets.h.
> 
> What? How this can depend on consumer? Drop entire description, it is
> useless.
> 

Thanks for your comments, I will remove the entire description.

> > +
> > +  reset-names:
> > +    const: larb
> > +    description: The name of reset controller. SMI driver need to
> > obtain the
> > +      reset controller based on this.
> 
> Drop description, useless.

OK, I will remove the entire description.

> 
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -125,19 +137,38 @@ allOf:
> >        required:
> >          - mediatek,larb-id
> > 
> > +  - if:  # only for camera and image subsys
> > +      properties:
> > +        mediatek,smi:
> > +            contains:
> 
> Never tested.

OK, I will fix the indentation.

> 
> > +              enum:
> > +                - smi_sub_common_img0_4x1
> > +                - smi_sub_common_img1_4x1
> > +                - smi_sub_common_cam_5x1
> > +                - smi_sub_common_cam_8x1
> 
> Does not work. Test your code before you send it. No clue what you
> want
> to achieve, so not sure how to help.
> 

As I mentioned in SMI driver, only SMI larbs located in
camera and image subsys need to apply clamp and reset operation,
others can be skipped.
So I want to know if this description method meets your expectations
here(smi_sub_common_img0_4x1, smi_sub_common_img1_4x1 ...).

> 
> > +
> > +    then:
> > +      required:
> > +        - resets
> > +        - reset-names
> > +
> >  additionalProperties: false
> > 
> >  examples:
> >    - |+
> > -    #include <dt-bindings/clock/mt8173-clk.h>
> > -    #include <dt-bindings/power/mt8173-power.h>
> > -
> > -    larb1: larb@...10000 {
> > -      compatible = "mediatek,mt8173-smi-larb";
> > -      reg = <0x16010000 0x1000>;
> > -      mediatek,smi = <&smi_common>;
> > -      power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
> > -      clocks = <&vdecsys CLK_VDEC_CKEN>,
> > -               <&vdecsys CLK_VDEC_LARB_CKEN>;
> > -      clock-names = "apb", "smi";
> > +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
> > +    #include <dt-bindings/power/mediatek,mt8188-power.h>
> > +    #include <dt-bindings/reset/mt8188-resets.h>
> 
> This is some total mess. Never tested, not correct. Sorry, run it
> internally through some sort of review or internal checklist which
> will
> ask you to test the code before sending.
> 
> Best regards,
> Krzysztof
>