From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752662AbbE0HgP (ORCPT ); Wed, 27 May 2015 03:36:15 -0400 Received: from mailgw02.mediatek.com ([218.249.47.111]:50775 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752179AbbE0HgM (ORCPT ); Wed, 27 May 2015 03:36:12 -0400 X-Listener-Flag: 11101 Message-ID: <1432712164.721.135.camel@mhfsdcap03> Subject: Re: [PATCH v2 2/6] dt-bindings: mediatek: Add smi dts binding From: Yong Wu To: Tomasz Figa CC: Rob Herring , Joerg Roedel , Matthias Brugger , Robin Murphy , Will Deacon , Daniel Kurtz , Lucas Stach , Mark Rutland , Catalin Marinas , , Sasha Hauer , , , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "open list:IOMMU DRIVERS" , , , , , Date: Wed, 27 May 2015 15:36:04 +0800 In-Reply-To: References: <1431683009-18158-1-git-send-email-yong.wu@mediatek.com> <1431683009-18158-3-git-send-email-yong.wu@mediatek.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 wrote: > > This patch add smi binding document. > > > > Signed-off-by: Yong Wu > > --- > > .../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" : , > - "smi" : . Thanks. I will follow this. > > + > > +Example: > > + larb0:larb@14021000 { > > + 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