tsec: Allow Ten Bit Interface to be configurable

PATCH In review Joe Hamman 4 participants 12 messages Aug 13, 2007 – Aug 14, 2007 Network drivers

lore

Share & tools

lore

Joe Hamman

author

2007-08-13 22:57 UTC #0 lore raw

PATCH 1/1 tsec: Allow Ten Bit Interface to be configurable

Allow the address of the Ten Bit Interface (TBI) to be changed in the event of a conflict with another device.

Signed-off by: Joe Hamman <[email protected]>
---

Please ignore the last patch - I missed a cut & paste error on the range that my testing didn't catch.
---

drivers/net/Kconfig +8 -0

index 81ef81c..b4813d9 100644
@@ -2276,6 +2276,14 @@ config GFAR_NAPI
 	bool "NAPI Support"
 	depends on GIANFAR
 
+config GFAR_TBIPA_VALUE
+	hex "Ten Bit Interface Port Address Value"
+	depends on GIANFAR
+	range 0 0x1f
+	default "0x1f"
+	help
+	  Select an address that does not conflict with other addresses on the board.
+
 config UCC_GETH
 	tristate "Freescale QE Gigabit Ethernet"
 	depends on QUICC_ENGINE

drivers/net/gianfar.c +4 -4

index f926905..91ae0d3 100644
@@ -490,15 +490,15 @@ static void gfar_configure_serdes(struct net_device *dev)
 	/* Initialise TBI i/f to communicate with serdes (lynx phy) */
 
 	/* Single clk mode, mii mode off(for aerdes communication) */
-	gfar_local_mdio_write(regs, TBIPA_VALUE, MII_TBICON, TBICON_CLK_SELECT);
+	gfar_local_mdio_write(regs, CONFIG_GFAR_TBIPA_VALUE, MII_TBICON, TBICON_CLK_SELECT);
 
 	/* Supported pause and full-duplex, no half-duplex */
-	gfar_local_mdio_write(regs, TBIPA_VALUE, MII_ADVERTISE,
+	gfar_local_mdio_write(regs, CONFIG_GFAR_TBIPA_VALUE, MII_ADVERTISE,
 			ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
 			ADVERTISE_1000XPSE_ASYM);
 
 	/* ANEG enable, restart ANEG, full duplex mode, speed[1] set */
-	gfar_local_mdio_write(regs, TBIPA_VALUE, MII_BMCR, BMCR_ANENABLE |
+	gfar_local_mdio_write(regs, CONFIG_GFAR_TBIPA_VALUE, MII_BMCR, BMCR_ANENABLE |
 			BMCR_ANRESTART | BMCR_FULLDPLX | BMCR_SPEED1000);
 }

@@ -547,7 +547,7 @@ static void init_registers(struct net_device *dev)
 	gfar_write(&priv->regs->minflr, MINFLR_INIT_SETTINGS);
 
 	/* Assign the TBI an address which won't conflict with the PHYs */
-	gfar_write(&priv->regs->tbipa, TBIPA_VALUE);
+	gfar_write(&priv->regs->tbipa, CONFIG_GFAR_TBIPA_VALUE);
 }

drivers/net/gianfar.h +0 -1

index d8e779c..0fd1c02 100644
@@ -131,7 +131,6 @@ extern const char gfar_driver_version[];
 #define DEFAULT_RXCOUNT	16
 #define DEFAULT_RXTIME	4
 
-#define TBIPA_VALUE		0x1f
 #define MIIMCFG_INIT_VALUE	0x00000007
 #define MIIMCFG_RESET           0x80000000
 #define MIIMIND_BUSY            0x00000001

Kumar Gala

2007-08-14 03:07 UTC #1 lore raw

Quoted 8 lines · Joe Hamman

Allow the address of the Ten Bit Interface (TBI) to be changed in the event of a conflict with another device.
Signed-off by: Joe Hamman <[email protected]>
---
Please ignore the last patch - I missed a cut & paste error on the range that my testing didn't catch.

I think we'd rather this came from the device tree.

- k

Joe Hamman

author

2007-08-14 12:47 UTC #3 lore raw

Hi Kumar,

Or do you think it should be added to gianfar_mdio_data? Easier, but does it belong there?

Thanks,
Joe

Quoted 83 lines

-----Original Message-----
From: linuxppc-embedded- [email protected] [mailto:linuxppc- [email protected]] On Behalf Of Joe Hamman
Sent: Monday, August 13, 2007 10:30 PM
To: 'Kumar Gala'
Cc: [email protected]
Subject: RE: [PATCH] [UPDATED] tsec: Allow Ten Bit Interface to be configurable
Quoted 14 lines
-----Original Message-----
From: Kumar Gala [mailto:[email protected]]
Sent: Monday, August 13, 2007 10:10 PM
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH] [UPDATED] tsec: Allow Ten Bit Interface to be configurable
Quoted 3 lines · Joe Hamman
Allow the address of the Ten Bit Interface (TBI) to be changed in the event of a conflict with another device.
Quoted 9 lines
Quoted 6 lines
Signed-off by: Joe Hamman <[email protected]>
---
Please ignore the last patch - I missed a cut & paste error on the range
that my testing didn't catch.
I think we'd rather this came from the device tree.
Duh, that makes sense ;-)
Has there been any discussion yet? Maybe something like this:
		mdio@24520 {
			#address-cells = <1>;
			#size-cells = <0>;
			device_type = "mdio";
			compatible = "gianfar";
			reg = <24520 20>;
			phy0: ethernet-phy@0 {
				interrupt-parent = <&mpic>;
				interrupts = <a 1>;
				reg = <0>;
				device_type = "ethernet-phy";
			};
			phy1: ethernet-phy@1 {
				interrupt-parent = <&mpic>;
				interrupts = <a 1>;
				reg = <1>;
				device_type = "ethernet-phy";
			};
			phy2: ethernet-phy@2 {
				interrupt-parent = <&mpic>;
				interrupts = <a 1>;
				reg = <2>;
				device_type = "ethernet-phy";
			};
			phy3: ethernet-phy@3 {
				interrupt-parent = <&mpic>;
				interrupts = <a 1>;
				reg = <3>;
				device_type = "ethernet-phy";
			};
			tbi:  ethernet-tbi@1f {
				reg = <1f>;
				device_type = "ethernet-tbi";
			};
		};
Thanks,
Joe
_______________________________________________
Linuxppc-embedded mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Andy Fleming

2007-08-14 16:12 UTC #4 lore raw

In reply to Joe Hamman

Quoted 22 lines · Joe Hamman

Quoted 7 lines · Joe Hamman
Quoted 4 lines
Allow the address of the Ten Bit Interface (TBI) to be changed in the event of a conflict with another device.
Quoted 9 lines
Quoted 6 lines
Signed-off by: Joe Hamman <[email protected]>
---
Please ignore the last patch - I missed a cut & paste error on the range
that my testing didn't catch.
I think we'd rather this came from the device tree.
Duh, that makes sense ;-)
Has there been any discussion yet? Maybe something like this:
		mdio@24520 {

...

Quoted 10 lines

			phy3: ethernet-phy@3 {
				interrupt-parent = <&mpic>;
				interrupts = <a 1>;
				reg = <3>;
				device_type = "ethernet-phy";
			};
			tbi:  ethernet-tbi@1f {
				reg = <1f>;
				device_type = "ethernet-tbi";
			};

It's actually a per-tsec property. There's not one tbi, there's one per TSEC. The one on TSEC 0 is special in that it can interfere with PHYs on the MDIO bus.

So I would suggest making it a property of the ethernet node:

ethernet@24000 {
...
tbipa = <1f>;
...
}

etc

Andy

Scott Wood

2007-08-14 16:16 UTC #5 lore raw

Quoted 9 lines · Andy Fleming

It's actually a per-tsec property. There's not one tbi, there's one per TSEC. The one on TSEC 0 is special in that it can interfere with PHYs on the MDIO bus.
So I would suggest making it a property of the ethernet node:
ethernet@24000 {
...
tbipa = <1f>;
...
}

"tbipa" isn't likely to pass the Segher test. :-)

If the TBI address is in PHY-space, then it should go in the MDIO bus. For the second TSEC, create a second MDIO bus node.

-Scott

Scott Wood

2007-08-14 18:00 UTC #7 lore raw

Quoted 5 lines · Joe Hamman

		ethernet@24000 {
			#address-cells = <1>;
			#size-cells = <0>;
			device_type = "network";
			model = "eTSEC";

Quoted 8 lines

			compatible = "gianfar";
			reg = <24000 1000>;
			mac-address = [ 00 E0 0C 00 73 00 ];
			interrupts = <1d 2 1e 2 22 2>;
			interrupt-parent = <&mpic>;
			phy-handle = <&phy1f>;
			tbi-handle = <&tbi1e>;
		};

Is any given board going to have at runtime (i.e. not jumper selectable)
both a phy and a tbi (I'm not very familiar with the latter, so I
apologize if this is a dumb question). If not, I'd stick with phy-handle and have something in the phy node to indicate that it's tbi.

-Scott

Andy Fleming

2007-08-14 20:37 UTC #8 lore raw

Quoted 15 lines · Scott Wood

Quoted 5 lines · Joe Hamman

		ethernet@24000 {
			#address-cells = <1>;
			#size-cells = <0>;
			device_type = "network";
			model = "eTSEC";

Quoted 8 lines

			compatible = "gianfar";
			reg = <24000 1000>;
			mac-address = [ 00 E0 0C 00 73 00 ];
			interrupts = <1d 2 1e 2 22 2>;
			interrupt-parent = <&mpic>;
			phy-handle = <&phy1f>;
			tbi-handle = <&tbi1e>;
		};

Quoted 1 line

Is any given board going to have at runtime (i.e. not jumper selectable) both a phy and a tbi (I'm not very familiar with the latter, so I apologize if this is a dumb question). If not, I'd stick with phy-handle and have something in the phy node to indicate that it's tbi.

Yes. That will be the more common case. The TBI PHYs are connected to the MDIO pins of each TSEC. The TBIPA register defines what address it sits on. It is used to configure non MII-style data connections, usually to another PHY. For instance, to configure a TSEC for SGMII, you first configure the attached TBI to communicate with the on-chip SERDES.

While the TBI devices technically all sit on MDIO busses, only the TBI PHY connected to the first TSEC will interfere with MDIO transactions. I don't think we need to create nodes for each of the TBIs. They exist as a secondary part of the ethernet controller, and their address is only really important to that controller.

I still think it should just be a property of the ethernet node. We aren't describing the TBI, we're describing a setting for the ethernet controller's register.

Andy

Joe Hamman

author

2007-08-14 21:01 UTC #9 lore raw

Hi Andy,

Quoted 21 lines

Quoted 6 lines
Is any given board going to have at runtime (i.e. not jumper selectable) both a phy and a tbi (I'm not very familiar with the latter, so I apologize if this is a dumb question). If not, I'd stick with phy-handle and have something in the phy node to indicate that it's tbi.
Yes. That will be the more common case. The TBI PHYs are connected to the MDIO pins of each TSEC. The TBIPA register defines what address it sits on. It is used to configure non MII-style data connections, usually to another PHY. For instance, to configure a TSEC for SGMII, you first configure the attached TBI to communicate with the on-chip SERDES.
While the TBI devices technically all sit on MDIO busses, only the TBI PHY connected to the first TSEC will interfere with MDIO transactions. I don't think we need to create nodes for each of the TBIs. They exist as a secondary part of the ethernet controller, and their address is only really important to that controller.

Quoted 4 lines

I still think it should just be a property of the ethernet node. We aren't describing the TBI, we're describing a setting for the ethernet controller's register.

Like this? Do we need to have one for each ethernet@xxxxxx?

	ethernet@24000 {
		#address-cells = <1>;
		#size-cells = <0>;
		device_type = "network";
		model = "eTSEC";
		compatible = "gianfar";
		reg = <24000 1000>;
		mac-address = [ 00 E0 0C 00 73 00 ];
		interrupts = <1d 2 1e 2 22 2>;
		interrupt-parent = <&mpic>;
		phy-handle = <&phy1f>;
		tbi-address = <0x1e>;
	};

Joe

Andy Fleming

2007-08-14 21:30 UTC #10 lore raw

Quoted 8 lines · Joe Hamman

Hi Andy,
Quoted 5 lines
I still think it should just be a property of the ethernet node. We aren't describing the TBI, we're describing a setting for the ethernet controller's register.
Like this? Do we need to have one for each ethernet@xxxxxx?

Scott and I have been talking, and right now it seems like a better idea might be to have the TBI address get assigned dynamically, based on where PHYs aren't.

It's non-trivial to do (though not difficult), but this way we won't have to regret creating a bad interface. You just need to look at the mdio bus, and find out which addresses responded. Then you create a variable in the gfar_private structure to hold what it gets set to, and make sure that gfar_configure_serdes uses that value instead of the TBIPA constant.

Andy

Joe Hamman

author

2007-08-14 21:41 UTC #11 lore raw

Quoted 27 lines

-----Original Message-----
From: Andy Fleming [mailto:[email protected]]
Sent: Tuesday, August 14, 2007 4:30 PM
To: [email protected]
Cc: 'Scott Wood'; [email protected]
Subject: Re: [PATCH] [UPDATED] tsec: Allow Ten Bit Interface to be configurable
Quoted 8 lines · Joe Hamman
Hi Andy,
Quoted 5 lines
I still think it should just be a property of the ethernet node. We aren't describing the TBI, we're describing a setting for the ethernet controller's register.
Like this? Do we need to have one for each ethernet@xxxxxx?
Scott and I have been talking, and right now it seems like a better idea might be to have the TBI address get assigned dynamically, based on where PHYs aren't.

Quoted 6 lines

It's non-trivial to do (though not difficult), but this way we won't have to regret creating a bad interface. You just need to look at the mdio bus, and find out which addresses responded. Then you create a variable in the gfar_private structure to hold what it gets set to, and make sure that gfar_configure_serdes uses that value instead of the TBIPA constant.

Sounds like a good plan. I'll try to put something together and post for comments.

Thanks,
Joe