Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Anton Vorontsov]

On Fri, Apr 11, 2008 at 09:13:36AM -0500, Kumar Gala wrote:
>
> On Mar 11, 2008, at 12:10 PM, Anton Vorontsov wrote:
>> - get rid of `model = "UCC"' in the ucc nodes
>>  It isn't used anywhere, so remove it. If we'll ever need something
>>  like this, we'll use compatible property instead.
>> - replace cell-index and device-id properties by fsl,ucc.
>>
>> Drivers are modified for backward compatibility's sake.
>
> I'd prefer we use cell-index and not introduce "fsl,ucc".  I'm ok with  
> dropping device-id and model (its implied in the compatiable).

Ok. Here it is. netdev and linux-serial Cc'ed.

- - - -
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Subject: [POWERPC] UCC nodes cleanup

- get rid of `model = "UCC"' in the ucc nodes
  It isn't used anywhere, so remove it. If we'll ever need something
  like this, we'll use compatible property instead.
- replace last occurrences of device-id with cell-index.
  Drivers are modified for backward compatibility's sake.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Documentation/powerpc/booting-without-of.txt |    6 ++----
 arch/powerpc/boot/dts/mpc832x_mds.dts        |    7 +------
 arch/powerpc/boot/dts/mpc832x_rdb.dts        |    4 ----
 arch/powerpc/boot/dts/mpc836x_mds.dts        |    4 ----
 arch/powerpc/boot/dts/mpc8568mds.dts         |    4 ----
 drivers/net/ucc_geth.c                       |    8 +++++++-
 drivers/net/ucc_geth_mii.c                   |   11 ++++++++---
 drivers/serial/ucc_uart.c                    |   16 ++++++++++++----
 8 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index be41a5c..f0a99aa 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -1619,8 +1619,7 @@ platforms are moved over to use the flattened-device-tree model.
    - device_type : should be "network", "hldc", "uart", "transparent"
      "bisync", "atm", or "serial".
    - compatible : could be "ucc_geth" or "fsl_atm" and so on.
-   - model : should be "UCC".
-   - device-id : the ucc number(1-8), corresponding to UCCx in UM.
+   - cell-index : the ucc number(1-8), corresponding to UCCx in UM.
    - reg : Offset and length of the register set for the device
    - interrupts : <a b> where a is the interrupt number and b is a
      field that represents an encoding of the sense and level
@@ -1677,8 +1676,7 @@ platforms are moved over to use the flattened-device-tree model.
 	ucc@2000 {
 		device_type = "network";
 		compatible = "ucc_geth";
-		model = "UCC";
-		device-id = <1>;
+		cell-index = <1>;
 		reg = <2000 200>;
 		interrupts = <a0 0>;
 		interrupt-parent = <700>;
diff --git a/arch/powerpc/boot/dts/mpc832x_mds.dts b/arch/powerpc/boot/dts/mpc832x_mds.dts
index 9bb4083..539e02f 100644
--- a/arch/powerpc/boot/dts/mpc832x_mds.dts
+++ b/arch/powerpc/boot/dts/mpc832x_mds.dts
@@ -255,9 +255,7 @@
 		enet0: ucc@2200 {
 			device_type = "network";
 			compatible = "ucc_geth";
-			model = "UCC";
 			cell-index = <3>;
-			device-id = <3>;
 			reg = <0x2200 0x200>;
 			interrupts = <34>;
 			interrupt-parent = <&qeic>;
@@ -271,9 +269,7 @@
 		enet1: ucc@3200 {
 			device_type = "network";
 			compatible = "ucc_geth";
-			model = "UCC";
 			cell-index = <4>;
-			device-id = <4>;
 			reg = <0x3200 0x200>;
 			interrupts = <35>;
 			interrupt-parent = <&qeic>;
@@ -287,8 +283,7 @@
 		ucc@2400 {
 			device_type = "serial";
 			compatible = "ucc_uart";
-			model = "UCC";
-			device-id = <5>;	/* The UCC number, 1-7*/
+			cell-index = <5>;	/* The UCC number, 1-7*/
 			port-number = <0>;	/* Which ttyQEx device */
 			soft-uart;		/* We need Soft-UART */
 			reg = <0x2400 0x200>;
diff --git a/arch/powerpc/boot/dts/mpc832x_rdb.dts b/arch/powerpc/boot/dts/mpc832x_rdb.dts
index 94f93d2..179c81c 100644
--- a/arch/powerpc/boot/dts/mpc832x_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc832x_rdb.dts
@@ -208,9 +208,7 @@
 		enet0: ucc@3000 {
 			device_type = "network";
 			compatible = "ucc_geth";
-			model = "UCC";
 			cell-index = <2>;
-			device-id = <2>;
 			reg = <0x3000 0x200>;
 			interrupts = <33>;
 			interrupt-parent = <&qeic>;
@@ -224,9 +222,7 @@
 		enet1: ucc@2200 {
 			device_type = "network";
 			compatible = "ucc_geth";
-			model = "UCC";
 			cell-index = <3>;
-			device-id = <3>;
 			reg = <0x2200 0x200>;
 			interrupts = <34>;
 			interrupt-parent = <&qeic>;
diff --git a/arch/powerpc/boot/dts/mpc836x_mds.dts b/arch/powerpc/boot/dts/mpc836x_mds.dts
index 55f03e8..8160ff2 100644
--- a/arch/powerpc/boot/dts/mpc836x_mds.dts
+++ b/arch/powerpc/boot/dts/mpc836x_mds.dts
@@ -257,9 +257,7 @@
 		enet0: ucc@2000 {
 			device_type = "network";
 			compatible = "ucc_geth";
-			model = "UCC";
 			cell-index = <1>;
-			device-id = <1>;
 			reg = <0x2000 0x200>;
 			interrupts = <32>;
 			interrupt-parent = <&qeic>;
@@ -274,9 +272,7 @@
 		enet1: ucc@3000 {
 			device_type = "network";
 			compatible = "ucc_geth";
-			model = "UCC";
 			cell-index = <2>;
-			device-id = <2>;
 			reg = <0x3000 0x200>;
 			interrupts = <33>;
 			interrupt-parent = <&qeic>;
diff --git a/arch/powerpc/boot/dts/mpc8568mds.dts b/arch/powerpc/boot/dts/mpc8568mds.dts
index 97bc048..df4b5e8 100644
--- a/arch/powerpc/boot/dts/mpc8568mds.dts
+++ b/arch/powerpc/boot/dts/mpc8568mds.dts
@@ -324,9 +324,7 @@
 		enet2: ucc@2000 {
 			device_type = "network";
 			compatible = "ucc_geth";
-			model = "UCC";
 			cell-index = <1>;
-			device-id = <1>;
 			reg = <2000 200>;
 			interrupts = <20>;
 			interrupt-parent = <&qeic>;
@@ -341,9 +339,7 @@
 		enet3: ucc@3000 {
 			device_type = "network";
 			compatible = "ucc_geth";
-			model = "UCC";
 			cell-index = <2>;
-			device-id = <2>;
 			reg = <3000 200>;
 			interrupts = <21>;
 			interrupt-parent = <&qeic>;
diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index fba0811..3a68b94 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -3852,7 +3852,13 @@ static int ucc_geth_probe(struct of_device* ofdev, const struct of_device_id *ma
 
 	ugeth_vdbg("%s: IN", __FUNCTION__);
 
-	prop = of_get_property(np, "device-id", NULL);
+	prop = of_get_property(np, "cell-index", NULL);
+	if (!prop) {
+		prop = of_get_property(np, "device-id", NULL);
+		if (!prop)
+			return -ENODEV;
+	}
+
 	ucc_num = *prop - 1;
 	if ((ucc_num < 0) || (ucc_num > 7))
 		return -ENODEV;
diff --git a/drivers/net/ucc_geth_mii.c b/drivers/net/ucc_geth_mii.c
index c69e654..8a48ddb 100644
--- a/drivers/net/ucc_geth_mii.c
+++ b/drivers/net/ucc_geth_mii.c
@@ -203,9 +203,14 @@ static int uec_mdio_probe(struct of_device *ofdev, const struct of_device_id *ma
 		if ((res.start >= tempres.start) &&
 		    (res.end <= tempres.end)) {
 			/* set this UCC to be the MII master */
-			const u32 *id = of_get_property(tempnp, "device-id", NULL);
-			if (id == NULL)
-				goto bus_register_fail;
+			const u32 *id;
+
+			id = of_get_property(tempnp, "cell-index", NULL);
+			if (!id) {
+				id = of_get_property(tempnp, "device-id", NULL);
+				if (!id)
+					goto bus_register_fail;
+			}
 
 			ucc_set_qe_mux_mii_mng(*id - 1);
 
diff --git a/drivers/serial/ucc_uart.c b/drivers/serial/ucc_uart.c
index e0994f0..5e4310c 100644
--- a/drivers/serial/ucc_uart.c
+++ b/drivers/serial/ucc_uart.c
@@ -1270,10 +1270,18 @@ static int ucc_uart_probe(struct of_device *ofdev,
 
 	/* Get the UCC number (device ID) */
 	/* UCCs are numbered 1-7 */
-	iprop = of_get_property(np, "device-id", NULL);
-	if (!iprop || (*iprop < 1) || (*iprop > UCC_MAX_NUM)) {
-		dev_err(&ofdev->dev,
-			"missing or invalid UCC specified in device tree\n");
+	iprop = of_get_property(np, "cell-index", NULL);
+	if (!iprop) {
+		iprop = of_get_property(np, "device-id", NULL);
+		if (!iprop) {
+			dev_err(&ofdev->dev, "UCC is unspecified in "
+				"device tree\n");
+			return -EINVAL;
+		}
+	}
+
+	if ((*iprop < 1) || (*iprop > UCC_MAX_NUM)) {
+		dev_err(&ofdev->dev, "no support for UCC%u\n", *iprop);
 		kfree(qe_port);
 		return -ENODEV;
 	}
-- 
1.5.4.5

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Timur Tabi]

Anton Vorontsov wrote:
> On Fri, Apr 11, 2008 at 09:13:36AM -0500, Kumar Gala wrote:
>> On Mar 11, 2008, at 12:10 PM, Anton Vorontsov wrote:
>>> - get rid of `model = "UCC"' in the ucc nodes
>>>  It isn't used anywhere, so remove it. If we'll ever need something
>>>  like this, we'll use compatible property instead.
>>> - replace cell-index and device-id properties by fsl,ucc.
>>>
>>> Drivers are modified for backward compatibility's sake.
>> I'd prefer we use cell-index and not introduce "fsl,ucc".  I'm ok with  
>> dropping device-id and model (its implied in the compatiable).
> 
> Ok. Here it is. netdev and linux-serial Cc'ed.

Do we want the first UCC to have a cell-index of 1?  Maybe we should fix this
off-by-one error once and for all, and number all UCCs from 0?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Anton Vorontsov]

On Fri, Apr 11, 2008 at 11:48:37AM -0500, Timur Tabi wrote:
> Anton Vorontsov wrote:
> > On Fri, Apr 11, 2008 at 09:13:36AM -0500, Kumar Gala wrote:
> >> On Mar 11, 2008, at 12:10 PM, Anton Vorontsov wrote:
> >>> - get rid of `model = "UCC"' in the ucc nodes
> >>>  It isn't used anywhere, so remove it. If we'll ever need something
> >>>  like this, we'll use compatible property instead.
> >>> - replace cell-index and device-id properties by fsl,ucc.
> >>>
> >>> Drivers are modified for backward compatibility's sake.
> >> I'd prefer we use cell-index and not introduce "fsl,ucc".  I'm ok with  
> >> dropping device-id and model (its implied in the compatiable).
> > 
> > Ok. Here it is. netdev and linux-serial Cc'ed.
> 
> Do we want the first UCC to have a cell-index of 1?  Maybe we should fix this
> off-by-one error once and for all, and number all UCCs from 0?

Isn't documentation numbers UCC from 1? Then I believe we should stick
with it for device tree, since off by one is Linux implementation details.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Timur Tabi]

Anton Vorontsov wrote:

>> Do we want the first UCC to have a cell-index of 1?  Maybe we should fix this
>> off-by-one error once and for all, and number all UCCs from 0?
> 
> Isn't documentation numbers UCC from 1? 

Yes.

> Then I believe we should stick
> with it for device tree, since off by one is Linux implementation details.

Fair enough.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Anton Vorontsov]

On Fri, Apr 11, 2008 at 09:06:57PM +0400, Anton Vorontsov wrote:
> On Fri, Apr 11, 2008 at 11:48:37AM -0500, Timur Tabi wrote:
> > Anton Vorontsov wrote:
> > > On Fri, Apr 11, 2008 at 09:13:36AM -0500, Kumar Gala wrote:
> > >> On Mar 11, 2008, at 12:10 PM, Anton Vorontsov wrote:
> > >>> - get rid of `model = "UCC"' in the ucc nodes
> > >>>  It isn't used anywhere, so remove it. If we'll ever need something
> > >>>  like this, we'll use compatible property instead.
> > >>> - replace cell-index and device-id properties by fsl,ucc.
> > >>>
> > >>> Drivers are modified for backward compatibility's sake.
> > >> I'd prefer we use cell-index and not introduce "fsl,ucc".  I'm ok with  
> > >> dropping device-id and model (its implied in the compatiable).
> > > 
> > > Ok. Here it is. netdev and linux-serial Cc'ed.
> > 
> > Do we want the first UCC to have a cell-index of 1?  Maybe we should fix this
> > off-by-one error once and for all, and number all UCCs from 0?
> 
> Isn't documentation numbers UCC from 1? Then I believe we should stick
> with it for device tree, since off by one is Linux implementation details.

Or maybe I'm thinking here in terms of "fsl,ucc"... and cell-index is
indeed should be -1... don't know. Please decide. ;-)

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Timur Tabi]

Anton Vorontsov wrote:

> Or maybe I'm thinking here in terms of "fsl,ucc"... and cell-index is
> indeed should be -1... don't know. Please decide. ;-)

Well, that's what I was thinking.  cell-index is zero-based, so UCC1 should have
cell-index = <0>.

Of course, this means all the code needs to change, since I think device-id is
one-based.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Anton Vorontsov]

On Fri, Apr 11, 2008 at 12:12:30PM -0500, Timur Tabi wrote:
> Anton Vorontsov wrote:
> 
> > Or maybe I'm thinking here in terms of "fsl,ucc"... and cell-index is
> > indeed should be -1... don't know. Please decide. ;-)
> 
> Well, that's what I was thinking.  cell-index is zero-based, so UCC1 should have
> cell-index = <0>.
> 
> Of course, this means all the code needs to change, since I think device-id is
> one-based.

Yup. You raised a really good question, because we're _introducing_
cell-index for UCC nodes, and if we'll choice wrong numbering scheme
now, then there will be no way back w/o breaking backward compatibility.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Anton Vorontsov]

On Fri, Apr 11, 2008 at 09:21:06PM +0400, Anton Vorontsov wrote:
> On Fri, Apr 11, 2008 at 12:12:30PM -0500, Timur Tabi wrote:
> > Anton Vorontsov wrote:
> > 
> > > Or maybe I'm thinking here in terms of "fsl,ucc"... and cell-index is
> > > indeed should be -1... don't know. Please decide. ;-)
> > 
> > Well, that's what I was thinking.  cell-index is zero-based, so UCC1 should have
> > cell-index = <0>.
> > 
> > Of course, this means all the code needs to change, since I think device-id is
> > one-based.
> 
> Yup. You raised a really good question, because we're _introducing_
> cell-index for UCC nodes, and if we'll choice wrong numbering scheme
> now, then there will be no way back w/o breaking backward compatibility.

Hm... thinking about it more, we're introducing implementation for the
cell-index, but device tree was "infected" already.

So, too late. :-D

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Kumar Gala]

On Apr 11, 2008, at 12:31 PM, Anton Vorontsov wrote:
> On Fri, Apr 11, 2008 at 09:21:06PM +0400, Anton Vorontsov wrote:
>> On Fri, Apr 11, 2008 at 12:12:30PM -0500, Timur Tabi wrote:
>>> Anton Vorontsov wrote:
>>>
>>>> Or maybe I'm thinking here in terms of "fsl,ucc"... and cell- 
>>>> index is
>>>> indeed should be -1... don't know. Please decide. ;-)
>>>
>>> Well, that's what I was thinking.  cell-index is zero-based, so  
>>> UCC1 should have
>>> cell-index = <0>.
>>>
>>> Of course, this means all the code needs to change, since I think  
>>> device-id is
>>> one-based.
>>
>> Yup. You raised a really good question, because we're _introducing_
>> cell-index for UCC nodes, and if we'll choice wrong numbering scheme
>> now, then there will be no way back w/o breaking backward  
>> compatibility.
>
> Hm... thinking about it more, we're introducing implementation for the
> cell-index, but device tree was "infected" already.
>
> So, too late. :-D

I say leave as you have it (UCC1 == cell-index = <1>).

Changing it so cell-index = <0> is just more confusing w/regards to  
the docs.

- k

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Timur Tabi]

Kumar Gala wrote:

> I say leave as you have it (UCC1 == cell-index = <1>).
> 
> Changing it so cell-index = <0> is just more confusing w/regards to  
> the docs.

I don't see anything in booting-without-of.txt that requires cell-index to start
at 0, however, the only other device that starts at 1 is SATA.  Even the SSI
starts at 0:

       - cell-index	  : the SSI, <0> = SSI1, <1> = SSI2, and so on

Although I don't want to make more work for Anton, my vote is to have cell-index
start at 0.  That is the convention.  It's too late to fix SATA, but at least we
can avoid making it worse.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Kumar Gala]

On Apr 14, 2008, at 9:56 AM, Timur Tabi wrote:
> Kumar Gala wrote:
>
>> I say leave as you have it (UCC1 == cell-index = <1>).
>>
>> Changing it so cell-index = <0> is just more confusing w/regards to
>> the docs.
>
> I don't see anything in booting-without-of.txt that requires cell- 
> index to start
> at 0, however, the only other device that starts at 1 is SATA.  Even  
> the SSI
> starts at 0:
>
>       - cell-index	  : the SSI, <0> = SSI1, <1> = SSI2, and so on
>
> Although I don't want to make more work for Anton, my vote is to  
> have cell-index
> start at 0.  That is the convention.  It's too late to fix SATA, but  
> at least we
> can avoid making it worse.

I use cell-index starting at 1 for PCI to match our docs.

I'll take Anton's last patch.

- k

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Grant Likely]

On Fri, Apr 11, 2008 at 11:06 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Fri, Apr 11, 2008 at 11:48:37AM -0500, Timur Tabi wrote:
>  > Anton Vorontsov wrote:
>  > > On Fri, Apr 11, 2008 at 09:13:36AM -0500, Kumar Gala wrote:
>  > >> On Mar 11, 2008, at 12:10 PM, Anton Vorontsov wrote:
>  > >>> - get rid of `model = "UCC"' in the ucc nodes
>  > >>>  It isn't used anywhere, so remove it. If we'll ever need something
>  > >>>  like this, we'll use compatible property instead.
>  > >>> - replace cell-index and device-id properties by fsl,ucc.
>  > >>>
>  > >>> Drivers are modified for backward compatibility's sake.
>  > >> I'd prefer we use cell-index and not introduce "fsl,ucc".  I'm ok with
>  > >> dropping device-id and model (its implied in the compatiable).
>  > >
>  > > Ok. Here it is. netdev and linux-serial Cc'ed.
>  >
>  > Do we want the first UCC to have a cell-index of 1?  Maybe we should fix this
>  > off-by-one error once and for all, and number all UCCs from 0?
>
>  Isn't documentation numbers UCC from 1? Then I believe we should stick
>  with it for device tree, since off by one is Linux implementation details.

Plus making such a change will break deployed device trees.

g.


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Timur Tabi]

Anton Vorontsov wrote:

> From: Anton Vorontsov <avorontsov@ru.mvista.com>
> Subject: [POWERPC] UCC nodes cleanup
> 
> - get rid of `model = "UCC"' in the ucc nodes
>   It isn't used anywhere, so remove it. If we'll ever need something
>   like this, we'll use compatible property instead.
> - replace last occurrences of device-id with cell-index.
>   Drivers are modified for backward compatibility's sake.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: Timur Tabi <timur@freescale.com>

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] [POWERPC] UCC nodes cleanup

[Kumar Gala]

> From: Anton Vorontsov <avorontsov@ru.mvista.com>
> Subject: [POWERPC] UCC nodes cleanup
>
> - get rid of `model = "UCC"' in the ucc nodes
>  It isn't used anywhere, so remove it. If we'll ever need something
>  like this, we'll use compatible property instead.
> - replace last occurrences of device-id with cell-index.
>  Drivers are modified for backward compatibility's sake.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> Documentation/powerpc/booting-without-of.txt |    6 ++----
> arch/powerpc/boot/dts/mpc832x_mds.dts        |    7 +------
> arch/powerpc/boot/dts/mpc832x_rdb.dts        |    4 ----
> arch/powerpc/boot/dts/mpc836x_mds.dts        |    4 ----
> arch/powerpc/boot/dts/mpc8568mds.dts         |    4 ----
> drivers/net/ucc_geth.c                       |    8 +++++++-
> drivers/net/ucc_geth_mii.c                   |   11 ++++++++---
> drivers/serial/ucc_uart.c                    |   16 ++++++++++++----
> 8 files changed, 30 insertions(+), 30 deletions(-)

applied.

- k