From: "yangbo.lu@freescale.com" <yangbo.lu@freescale.com>
To: Scott Wood <scottwood@freescale.com>
CC: "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] powerpc/dts: Add 1588 timer node for eTSEC
Thread-Topic: [PATCH] powerpc/dts: Add 1588 timer node for eTSEC
Thread-Index: AQHQh+FEkuLqim+yCkex5viVy9b+uJ1vawAAgABekfA=
Date: Thu, 7 May 2015 02:26:08 +0000
Message-ID: <BY1PR0301MB119235AB83BDACB911B5B93AF2DF0@BY1PR0301MB1192.namprd03.prod.outlook.com>
References: <1430905256-4930-1-git-send-email-yangbo.lu@freescale.com>
 <1430945040.16357.293.camel@freescale.com>
In-Reply-To: <1430945040.16357.293.camel@freescale.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 May 2015 02:26:08.5944
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR03MB1477
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id t47302ZI017316
Content-Length: 3347
Lines: 89

Thanks.
Pls see my comments below.

-----Original Message-----
From: Wood Scott-B07421 
Sent: Thursday, May 07, 2015 4:44 AM
To: Lu Yangbo-B47093
Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] powerpc/dts: Add 1588 timer node for eTSEC

On Wed, 2015-05-06 at 17:40 +0800, Yangbo Lu wrote:
> Add 1588 timer node in files:
> arch/powerpc/boot/dts/bsc9131rdb.dtsi
> arch/powerpc/boot/dts/bsc9132qds.dtsi
> arch/powerpc/boot/dts/p1010rdb.dtsi
> arch/powerpc/boot/dts/p1020rdb-pd.dts
> arch/powerpc/boot/dts/p1021rdb-pc.dtsi
> arch/powerpc/boot/dts/p1022ds.dtsi
> arch/powerpc/boot/dts/p1025twr.dtsi
> arch/powerpc/boot/dts/p2020rdb-pc.dtsi
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
>  arch/powerpc/boot/dts/bsc9131rdb.dtsi  | 12 ++++++++++++  
> arch/powerpc/boot/dts/bsc9132qds.dtsi  | 12 ++++++++++++
>  arch/powerpc/boot/dts/p1010rdb.dtsi    | 12 ++++++++++++
>  arch/powerpc/boot/dts/p1020rdb-pd.dts  | 12 ++++++++++++  
> arch/powerpc/boot/dts/p1021rdb-pc.dtsi | 12 ++++++++++++
>  arch/powerpc/boot/dts/p1022ds.dtsi     | 12 ++++++++++++
>  arch/powerpc/boot/dts/p1025twr.dtsi    | 12 ++++++++++++
>  arch/powerpc/boot/dts/p2020rdb-pc.dtsi | 15 +++++++++------
>  8 files changed, 93 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/boot/dts/bsc9131rdb.dtsi 
> b/arch/powerpc/boot/dts/bsc9131rdb.dtsi
> index 45efcba..629cc03 100644
> --- a/arch/powerpc/boot/dts/bsc9131rdb.dtsi
> +++ b/arch/powerpc/boot/dts/bsc9131rdb.dtsi
> @@ -80,6 +80,18 @@
>  		status = "disabled";
>  	};
>  
> +	ptp_clock@b0e00 {
> +		compatible = "fsl,etsec-ptp";
> +		reg = <0xb0e00 0xb0>;
> +		interrupts = <68 2 0 0 69 2 0 0>;
> +		fsl,tclk-period	= <5>;
> +		fsl,tmr-prsc	= <2>;
> +		fsl,tmr-add	= <0xcccccccd>;
> +		fsl,tmr-fiper1	= <0x3b9ac9fb>;
> +		fsl,tmr-fiper2	= <0x00018696>;
> +		fsl,max-adj	= <249999999>;

Please don't use hex for numbers that make more sense as decimal.
[Lu Yangbo-B47093] The hex value is register value, I think it's better to use hex.

> --- a/arch/powerpc/boot/dts/p2020rdb-pc.dtsi
> +++ b/arch/powerpc/boot/dts/p2020rdb-pc.dtsi
> @@ -215,12 +215,15 @@
>  	};
>  
>  	ptp_clock@24e00 {
> -		fsl,tclk-period = <5>;
> -		fsl,tmr-prsc = <200>;
> -		fsl,tmr-add = <0xCCCCCCCD>;
> -		fsl,tmr-fiper1 = <0x3B9AC9FB>;
> -		fsl,tmr-fiper2 = <0x0001869B>;
> -		fsl,max-adj = <249999999>;
> +		compatible = "fsl,etsec-ptp";
> +		reg = <0x24e00 0xb0>;
> +		interrupts = <68 2 0 0 69 2 0 0>;
> +		fsl,tclk-period	= <5>;
> +		fsl,tmr-prsc	= <2>;
> +		fsl,tmr-add	= <0xaaaaaaab>;
> +		fsl,tmr-fiper1	= <0x3b9ac9fb>;
> +		fsl,tmr-fiper2	= <0x00018696>;
> +		fsl,max-adj	= <299999999>;
>  	};

This isn't adding a node -- it's changing values.  If the old ones were wrong, explain that in the changelog.

Also, p2020si-post.dtsi already adds interrupts to this node (and it contains one more interrupt than the above), and it includes pq3-etsec1-timer-0.dtsi which contains the compatible and reg (and interrupts with two specifiers).  Probably all of these should be using pq3-etsec1-timer-0.dtsi and only specifying the board-specific values.

[Lu Yangbo-B47093] I will modify according your comments here.

-Scott


????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?