linux-kernel - Re: [PATCH v2 2/6] dt-bindings: mediatek: Add smi dts binding

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1432712164.721.135.camel@mhfsdcap03>
Date:	Wed, 27 May 2015 15:36:04 +0800
From:	Yong Wu <yong.wu@...iatek.com>
To:	Tomasz Figa <tfiga@...gle.com>
CC:	Rob Herring <robh+dt@...nel.org>, Joerg Roedel <joro@...tes.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Robin Murphy <robin.murphy@....com>,
	Will Deacon <will.deacon@....com>,
	Daniel Kurtz <djkurtz@...gle.com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Mark Rutland <mark.rutland@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	<linux-mediatek@...ts.infradead.org>,
	Sasha Hauer <kernel@...gutronix.de>,
	<srv_heupstream@...iatek.com>, <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"open list:IOMMU DRIVERS" <iommu@...ts.linux-foundation.org>,
	<pebolle@...cali.nl>, <arnd@...db.de>, <mitchelh@...eaurora.org>,
	<k.zhang@...iatek.com>, <youhua.li@...iatek.com>
Subject: Re: [PATCH v2 2/6] dt-bindings: mediatek: Add smi dts binding

Hi Tomasz,
	
On Mon, 2015-05-25 at 15:48 +0900, Tomasz Figa wrote:
> Hi,
> 
> Please see my comments inline.
> 
> On Fri, May 15, 2015 at 6:43 PM, Yong Wu <yong.wu@...iatek.com> wrote:
> > This patch add smi binding document.
> >
> > Signed-off-by: Yong Wu <yong.wu@...iatek.com>
> > ---
> >  .../bindings/soc/mediatek/mediatek,smi-larb.txt    | 24 ++++++++++++++++++++++
> >  .../bindings/soc/mediatek/mediatek,smi.txt         | 22 ++++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/mediatek,smi-larb.txt
> >  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
> >
> > diff --git a/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi-larb.txt b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi-larb.txt
> > new file mode 100644
> > index 0000000..c06c5b6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi-larb.txt
> > @@ -0,0 +1,24 @@
> > +SMI(Smart Multimedia Interface) Local Arbiter
> > +
> > +The hardware block diagram please check bindings/iommu/mediatek,iommu.txt
> > +
> > +Required properties:
> > +- compatible : must be "mediatek,mt8173-smi-larb"
> > +- reg : the register and size of each local arbiter.
> 
> Shouldn't it be "of this local arbiter"?
> 
> > +- smi : must be "&smi_common". Refer to bindings/soc/mediatek,smi.txt.
> 
> This is incorrect. Device tree source authors are free to use any
> labels for their nodes and documentation should not rely on the fact
> that there is some node with some particular label. This should be
> something like:
> 
> - smi : a phandle to node of XYZ
> 
> with proper description of that node in place of XYZ.
Thanks. how about this?
smi : a phandle to the smi_common node 
> 
> > +- clocks : must contain one entry for each clock-names.
> > +       There are 2 clockes:
> > +       APB clock : Advanced Peripheral Bus clock, It's the clock for setting
> > +               the register.
> > +       SMI clock : It's the clock for transfer data and command.
> 
> The description of each clock should go to "clock-names" property.
> Please use some of already existing bindings as an example.
> 
> > +- clock-name: must be "apb" and "smi".
> 
> typo: Should be "clock-names".
> 
> I'd recommend describing this property as below:
> 
> - clock-names: must contain 2 entries, as follows:
>   - "apb" : <here comes the description of APB clock>,
>   - "smi" : <here comes the description of SMI clock>.
Thanks. I will follow this.

> > +
> > +Example:
> > +       larb0:larb@...21000 {
> > +               compatible = "mediatek,mt8173-smi-larb";
> > +               reg = <0 0x14021000 0 0x1000>;
> > +               smi = <&smi_common>;
> > +               clocks = <&mmsys MM_SMI_LARB0>,
> > +                        <&mmsys MM_SMI_LARB0>;
> > +               clock-names = "apb", "smi";
And another question,
Could I add a item(port-count) in larb node. like:
	          port-count = M4U_PORT_LARB0_NR;
This item will only check the portid in client dsti is right or not?
                   iommu = <&iommu larbid portid>

Then, the port-count is necessary to add? or we could assume all the
input is correct.

> > +       };
> > diff --git a/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
> > new file mode 100644
> > index 0000000..c2389b4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
> > @@ -0,0 +1,22 @@
> > +SMI(Smart Multimedia Interface)
> > +
> > +The hardware block diagram please check bindings/iommu/mediatek,iommu.txt
> > +
> > +Required properties:
> > +- compatible : must be "mediatek,mt8173-smi"
> > +- reg : the register and size of the smi.
> 
> nit: s/smi/SMI block/
> 
> > +- clocks : must contain one entry for each clock-names.
> > +       There are 2 clockes:
> > +       APB clock : Advanced Peripheral Bus clock, It's the clock for setting
> > +               the register.
> > +       SMI clock : It's the clock for transfer data and command.
> > +- clock-name: must be "apb" and "smi".
> 
> See my comment for those properties in the first binding.
> 
> Best regards,
> Tomasz


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/