diff mbox

[v3,5/7] net: cpsw: Add am33xx MACID readout

Message ID 1408202315-20006-6-git-send-email-mpa@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Markus Pargmann Aug. 16, 2014, 3:18 p.m. UTC
This patch adds a function to get the MACIDs from the am33xx SoC
control module registers which hold unique vendor MACIDs. This is only
used if of_get_mac_address() fails to get a valid mac address.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 Documentation/devicetree/bindings/net/cpsw.txt |  4 +++
 drivers/net/ethernet/ti/Kconfig                |  2 ++
 drivers/net/ethernet/ti/Makefile               |  1 +
 drivers/net/ethernet/ti/cpsw.c                 | 46 ++++++++++++++++++++++++--
 4 files changed, 51 insertions(+), 2 deletions(-)

Comments

Wolfram Sang Aug. 16, 2014, 4:46 p.m. UTC | #1
> diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
> index 9cfaab8152be..5a31c2b322ee 100644
> --- a/drivers/net/ethernet/ti/Makefile
> +++ b/drivers/net/ethernet/ti/Makefile
> @@ -8,5 +8,6 @@ obj-$(CONFIG_TI_DAVINCI_EMAC) += davinci_emac.o
>  obj-$(CONFIG_TI_DAVINCI_MDIO) += davinci_mdio.o
>  obj-$(CONFIG_TI_DAVINCI_CPDMA) += davinci_cpdma.o
>  obj-$(CONFIG_TI_CPSW_PHY_SEL) += cpsw-phy-sel.o
> +obj-$(CONFIG_TI_CPSW_CTRL_MACID) += cpsw-ctrl-macid.o
>  obj-$(CONFIG_TI_CPSW) += ti_cpsw.o
>  ti_cpsw-y := cpsw_ale.o cpsw.o cpts.o

Leftover from your last series?
Wolfram Sang Aug. 16, 2014, 4:53 p.m. UTC | #2
> +	mac_addr[5] = (macid_lo >> 8) & 0xff;
> +	mac_addr[4] = macid_lo & 0xff;
> +	mac_addr[3] = (macid_hi >> 24) & 0xff;
> +	mac_addr[2] = (macid_hi >> 16) & 0xff;
> +	mac_addr[1] = (macid_hi >> 8) & 0xff;
> +	mac_addr[0] = macid_hi & 0xff;

That looks twisted, but I assume that you tested it is correct.
Markus Pargmann Aug. 18, 2014, 6:58 a.m. UTC | #3
Hi,

On Sat, Aug 16, 2014 at 11:46:48AM -0500, Wolfram Sang wrote:
> 
> > diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
> > index 9cfaab8152be..5a31c2b322ee 100644
> > --- a/drivers/net/ethernet/ti/Makefile
> > +++ b/drivers/net/ethernet/ti/Makefile
> > @@ -8,5 +8,6 @@ obj-$(CONFIG_TI_DAVINCI_EMAC) += davinci_emac.o
> >  obj-$(CONFIG_TI_DAVINCI_MDIO) += davinci_mdio.o
> >  obj-$(CONFIG_TI_DAVINCI_CPDMA) += davinci_cpdma.o
> >  obj-$(CONFIG_TI_CPSW_PHY_SEL) += cpsw-phy-sel.o
> > +obj-$(CONFIG_TI_CPSW_CTRL_MACID) += cpsw-ctrl-macid.o
> >  obj-$(CONFIG_TI_CPSW) += ti_cpsw.o
> >  ti_cpsw-y := cpsw_ale.o cpsw.o cpts.o
> 
> Leftover from your last series?

Yes thanks, will remove it for the next version.

Best regards,

Markus
Markus Pargmann Aug. 18, 2014, 7:03 a.m. UTC | #4
On Sat, Aug 16, 2014 at 11:53:18AM -0500, Wolfram Sang wrote:
> 
> > +	mac_addr[5] = (macid_lo >> 8) & 0xff;
> > +	mac_addr[4] = macid_lo & 0xff;
> > +	mac_addr[3] = (macid_hi >> 24) & 0xff;
> > +	mac_addr[2] = (macid_hi >> 16) & 0xff;
> > +	mac_addr[1] = (macid_hi >> 8) & 0xff;
> > +	mac_addr[0] = macid_hi & 0xff;
> 
> That looks twisted, but I assume that you tested it is correct.

The registers are twisted that way.

Best regards,

Markus
Mugunthan V N Aug. 18, 2014, 6:24 p.m. UTC | #5
On Saturday 16 August 2014 08:48 PM, Markus Pargmann wrote:
> +	mac_addr[5] = (macid_lo >> 8) & 0xff;
> +	mac_addr[4] = macid_lo & 0xff;
> +	mac_addr[3] = (macid_hi >> 24) & 0xff;
> +	mac_addr[2] = (macid_hi >> 16) & 0xff;
> +	mac_addr[1] = (macid_hi >> 8) & 0xff;
> +	mac_addr[0] = macid_hi & 0xff;
> +
This will fail incase of DRA74x and DRA72x platforms, please check for
u-boot src for parsing logic as TRM is not out yet. Below is the actual
code for DRA7 platforms for MAC address parsing

mac_addr[0] = (mac_hi & 0xFF0000) >> 16;
mac_addr[1] = (mac_hi & 0xFF00) >> 8;
mac_addr[2] = mac_hi & 0xFF;
mac_addr[3] = (mac_lo & 0xFF0000) >> 16;
mac_addr[4] = (mac_lo & 0xFF00) >> 8;
mac_addr[5] = mac_lo & 0xFF;

Regards
Mugunthan V N
Steven Rostedt Aug. 18, 2014, 7:41 p.m. UTC | #6
On Mon, 18 Aug 2014 23:54:26 +0530
Mugunthan V N <mugunthanvnm@ti.com> wrote:

> On Saturday 16 August 2014 08:48 PM, Markus Pargmann wrote:
> > +	mac_addr[5] = (macid_lo >> 8) & 0xff;
> > +	mac_addr[4] = macid_lo & 0xff;
> > +	mac_addr[3] = (macid_hi >> 24) & 0xff;
> > +	mac_addr[2] = (macid_hi >> 16) & 0xff;
> > +	mac_addr[1] = (macid_hi >> 8) & 0xff;
> > +	mac_addr[0] = macid_hi & 0xff;
> > +
> This will fail incase of DRA74x and DRA72x platforms, please check for
> u-boot src for parsing logic as TRM is not out yet. Below is the actual
> code for DRA7 platforms for MAC address parsing
> 
> mac_addr[0] = (mac_hi & 0xFF0000) >> 16;
> mac_addr[1] = (mac_hi & 0xFF00) >> 8;
> mac_addr[2] = mac_hi & 0xFF;
> mac_addr[3] = (mac_lo & 0xFF0000) >> 16;
> mac_addr[4] = (mac_lo & 0xFF00) >> 8;
> mac_addr[5] = mac_lo & 0xFF;
> 

But this fails with my beaglebone white.

I tested Markus's patches and it came up with the same ethaddr that
U-Boot had.

From U-Boot:

ethaddr=d4:94:a1:8b:ec:78

With Markus's changes:

 eth0      Link encap:Ethernet  HWaddr D4:94:A1:8B:EC:78

But when I changed the code to match what you wrote, I got this:

 eth0      Link encap:Ethernet  HWaddr CE:5A:8B:0E:44:45

but it also gave me:

cpsw 4a100000.ethernet: Random MACID = ce:5a:8b:0e:44:45

which means it failed the valid mac test.

Here's how I implemented your change:

#if 1
        mac_addr[0] = (macid_hi & 0xFF0000) >> 16;
        mac_addr[1] = (macid_hi & 0xFF00) >> 8;
        mac_addr[2] = macid_hi & 0xFF;
        mac_addr[3] = (macid_lo & 0xFF0000) >> 16;
        mac_addr[4] = (macid_lo & 0xFF00) >> 8;
        mac_addr[5] = macid_lo & 0xFF;

#else   
        mac_addr[5] = (macid_lo >> 8) & 0xff;
        mac_addr[4] = macid_lo & 0xff;
        mac_addr[3] = (macid_hi >> 24) & 0xff;
        mac_addr[2] = (macid_hi >> 16) & 0xff;
        mac_addr[1] = (macid_hi >> 8) & 0xff;
        mac_addr[0] = macid_hi & 0xff;
#endif  

Just to be consistent, I updated the code as this too:

        mac_addr[0] = (macid_hi >> 16) & 0xFF;
        mac_addr[1] = (macid_hi >> 8) & 0xFF;
        mac_addr[2] = macid_hi & 0xFF;
        mac_addr[3] = (macid_lo >> 16) & 0xFF;
        mac_addr[4] = (macid_lo >> 8) & 0xFF;
        mac_addr[5] = macid_lo & 0xFF;

With the same affect.

Thus, for this patchset, as is:

Tested-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve
Mugunthan V N Aug. 18, 2014, 7:58 p.m. UTC | #7
On Tuesday 19 August 2014 01:11 AM, Steven Rostedt wrote:
> On Mon, 18 Aug 2014 23:54:26 +0530
> Mugunthan V N <mugunthanvnm@ti.com> wrote:
>
>> On Saturday 16 August 2014 08:48 PM, Markus Pargmann wrote:
>>> +	mac_addr[5] = (macid_lo >> 8) & 0xff;
>>> +	mac_addr[4] = macid_lo & 0xff;
>>> +	mac_addr[3] = (macid_hi >> 24) & 0xff;
>>> +	mac_addr[2] = (macid_hi >> 16) & 0xff;
>>> +	mac_addr[1] = (macid_hi >> 8) & 0xff;
>>> +	mac_addr[0] = macid_hi & 0xff;
>>> +
>> This will fail incase of DRA74x and DRA72x platforms, please check for
>> u-boot src for parsing logic as TRM is not out yet. Below is the actual
>> code for DRA7 platforms for MAC address parsing
>>
>> mac_addr[0] = (mac_hi & 0xFF0000) >> 16;
>> mac_addr[1] = (mac_hi & 0xFF00) >> 8;
>> mac_addr[2] = mac_hi & 0xFF;
>> mac_addr[3] = (mac_lo & 0xFF0000) >> 16;
>> mac_addr[4] = (mac_lo & 0xFF00) >> 8;
>> mac_addr[5] = mac_lo & 0xFF;
>>
> But this fails with my beaglebone white.
>
> I tested Markus's patches and it came up with the same ethaddr that
> U-Boot had.
>
> From U-Boot:
>
> ethaddr=d4:94:a1:8b:ec:78
>
> With Markus's changes:
>
>  eth0      Link encap:Ethernet  HWaddr D4:94:A1:8B:EC:78
>
> But when I changed the code to match what you wrote, I got this:
>
>  eth0      Link encap:Ethernet  HWaddr CE:5A:8B:0E:44:45
>
> but it also gave me:
>
> cpsw 4a100000.ethernet: Random MACID = ce:5a:8b:0e:44:45
>
> which means it failed the valid mac test.
>
> Here's how I implemented your change:
>
> #if 1
>         mac_addr[0] = (macid_hi & 0xFF0000) >> 16;
>         mac_addr[1] = (macid_hi & 0xFF00) >> 8;
>         mac_addr[2] = macid_hi & 0xFF;
>         mac_addr[3] = (macid_lo & 0xFF0000) >> 16;
>         mac_addr[4] = (macid_lo & 0xFF00) >> 8;
>         mac_addr[5] = macid_lo & 0xFF;
>
> #else   
>         mac_addr[5] = (macid_lo >> 8) & 0xff;
>         mac_addr[4] = macid_lo & 0xff;
>         mac_addr[3] = (macid_hi >> 24) & 0xff;
>         mac_addr[2] = (macid_hi >> 16) & 0xff;
>         mac_addr[1] = (macid_hi >> 8) & 0xff;
>         mac_addr[0] = macid_hi & 0xff;
> #endif  
>
> Just to be consistent, I updated the code as this too:
>
>         mac_addr[0] = (macid_hi >> 16) & 0xFF;
>         mac_addr[1] = (macid_hi >> 8) & 0xFF;
>         mac_addr[2] = macid_hi & 0xFF;
>         mac_addr[3] = (macid_lo >> 16) & 0xFF;
>         mac_addr[4] = (macid_lo >> 8) & 0xFF;
>         mac_addr[5] = macid_lo & 0xFF;
>
> With the same affect.
>
> Thus, for this patchset, as is:
>
> Tested-by: Steven Rostedt <rostedt@goodmis.org>

This will fail for DRA7xx not in AM33xx

Regards
Mugunthan V N
Steven Rostedt Aug. 18, 2014, 8:57 p.m. UTC | #8
On Tue, 19 Aug 2014 01:28:09 +0530
Mugunthan V N <mugunthanvnm@ti.com> wrote:


> This will fail for DRA7xx not in AM33xx

OK, is there a way to test the difference?

-- Steve
Javier Martinez Canillas Aug. 18, 2014, 10:50 p.m. UTC | #9
Hello Mugunthan,

On Mon, Aug 18, 2014 at 9:58 PM, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>
>> Thus, for this patchset, as is:
>>
>> Tested-by: Steven Rostedt <rostedt@goodmis.org>
>
> This will fail for DRA7xx not in AM33xx
>

cpsw_am33xx_cm_get_macid() checks for
of_machine_is_compatible("ti,am33xx") and returns 0 if the machine is
not an am33xx. cpsw_probe_dt() only propagates the return value if is
not 0 so this patch does not change the semantics for other SoCs
besides am33xx.

If the driver already fails for DRA7xx that certainly is not this
patch's fault. Of course it would be nice to add support for DRA7xx as
well but I think that could be a follow-up patch and shouldn't be a
blocker to merge this change if is useful for users.

> Regards
> Mugunthan V N
> --

Best regards,
Javier
Markus Pargmann Aug. 19, 2014, 8:50 a.m. UTC | #10
Hi,

On Tue, Aug 19, 2014 at 12:50:59AM +0200, Javier Martinez Canillas wrote:
> Hello Mugunthan,
> 
> On Mon, Aug 18, 2014 at 9:58 PM, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> >>
> >> Thus, for this patchset, as is:
> >>
> >> Tested-by: Steven Rostedt <rostedt@goodmis.org>
> >
> > This will fail for DRA7xx not in AM33xx
> >
> 
> cpsw_am33xx_cm_get_macid() checks for
> of_machine_is_compatible("ti,am33xx") and returns 0 if the machine is
> not an am33xx. cpsw_probe_dt() only propagates the return value if is
> not 0 so this patch does not change the semantics for other SoCs
> besides am33xx.

Yes, this patch is only about the am33xx. I don't have the DRA7xx
hardware so I am not able to test on that hardware. Mugunthan, perhaps
you can supply some followup patches for DRA7xx.

Best regards,

Markus
Mugunthan V N Aug. 19, 2014, 4:37 p.m. UTC | #11
On Tuesday 19 August 2014 02:20 PM, Markus Pargmann wrote:
> Hi,
>
> On Tue, Aug 19, 2014 at 12:50:59AM +0200, Javier Martinez Canillas wrote:
>> Hello Mugunthan,
>>
>> On Mon, Aug 18, 2014 at 9:58 PM, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>>> Thus, for this patchset, as is:
>>>>
>>>> Tested-by: Steven Rostedt <rostedt@goodmis.org>
>>> This will fail for DRA7xx not in AM33xx
>>>
>> cpsw_am33xx_cm_get_macid() checks for
>> of_machine_is_compatible("ti,am33xx") and returns 0 if the machine is
>> not an am33xx. cpsw_probe_dt() only propagates the return value if is
>> not 0 so this patch does not change the semantics for other SoCs
>> besides am33xx.
> Yes, this patch is only about the am33xx. I don't have the DRA7xx
> hardware so I am not able to test on that hardware. Mugunthan, perhaps
> you can supply some followup patches for DRA7xx.
>
>

I will check on this thursday and update.

Regards
Mugunthan V N
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 107caf174a0e..33fe8462edf4 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -24,6 +24,8 @@  Optional properties:
 - ti,hwmods		: Must be "cpgmac0"
 - no_bd_ram		: Must be 0 or 1
 - dual_emac		: Specifies Switch to act as Dual EMAC
+- syscon		: Phandle to the system control device node, which is
+			  the control module device of the am33x
 
 Slave Properties:
 Required properties:
@@ -57,6 +59,7 @@  Examples:
 		active_slave = <0>;
 		cpts_clock_mult = <0x80000000>;
 		cpts_clock_shift = <29>;
+		syscon = <&cm>;
 		cpsw_emac0: slave@0 {
 			phy_id = <&davinci_mdio>, <0>;
 			phy-mode = "rgmii-txid";
@@ -85,6 +88,7 @@  Examples:
 		active_slave = <0>;
 		cpts_clock_mult = <0x80000000>;
 		cpts_clock_shift = <29>;
+		syscon = <&cm>;
 		cpsw_emac0: slave@0 {
 			phy_id = <&davinci_mdio>, <0>;
 			phy-mode = "rgmii-txid";
diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
index 53150c25a96b..afaf0196ffd2 100644
--- a/drivers/net/ethernet/ti/Kconfig
+++ b/drivers/net/ethernet/ti/Kconfig
@@ -62,6 +62,8 @@  config TI_CPSW
 	select TI_DAVINCI_CPDMA
 	select TI_DAVINCI_MDIO
 	select TI_CPSW_PHY_SEL
+	select MFD_SYSCON
+	select REGMAP
 	---help---
 	  This driver supports TI's CPSW Ethernet Switch.
 
diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
index 9cfaab8152be..5a31c2b322ee 100644
--- a/drivers/net/ethernet/ti/Makefile
+++ b/drivers/net/ethernet/ti/Makefile
@@ -8,5 +8,6 @@  obj-$(CONFIG_TI_DAVINCI_EMAC) += davinci_emac.o
 obj-$(CONFIG_TI_DAVINCI_MDIO) += davinci_mdio.o
 obj-$(CONFIG_TI_DAVINCI_CPDMA) += davinci_cpdma.o
 obj-$(CONFIG_TI_CPSW_PHY_SEL) += cpsw-phy-sel.o
+obj-$(CONFIG_TI_CPSW_CTRL_MACID) += cpsw-ctrl-macid.o
 obj-$(CONFIG_TI_CPSW) += ti_cpsw.o
 ti_cpsw-y := cpsw_ale.o cpsw.o cpts.o
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index b52df53441b0..aa13f68a178c 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -33,6 +33,8 @@ 
 #include <linux/of_net.h>
 #include <linux/of_device.h>
 #include <linux/if_vlan.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 #include <linux/pinctrl/consumer.h>
 
@@ -1796,6 +1798,39 @@  static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
 	slave->port_vlan = data->dual_emac_res_vlan;
 }
 
+#define AM33XX_CTRL_MAC_LO_REG(id) (0x630 + 0x8 * id)
+#define AM33XX_CTRL_MAC_HI_REG(id) (0x630 + 0x8 * id + 0x4)
+
+static int cpsw_am33xx_cm_get_macid(struct device *dev, int slave,
+		u8 *mac_addr)
+{
+	u32 macid_lo;
+	u32 macid_hi;
+	struct regmap *syscon;
+
+	if (!of_machine_is_compatible("ti,am33xx"))
+		return 0;
+
+	syscon = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
+	if (IS_ERR(syscon)) {
+		if (PTR_ERR(syscon) == -ENODEV)
+			return 0;
+		return PTR_ERR(syscon);
+	}
+
+	regmap_read(syscon, AM33XX_CTRL_MAC_LO_REG(slave), &macid_lo);
+	regmap_read(syscon, AM33XX_CTRL_MAC_HI_REG(slave), &macid_hi);
+
+	mac_addr[5] = (macid_lo >> 8) & 0xff;
+	mac_addr[4] = macid_lo & 0xff;
+	mac_addr[3] = (macid_hi >> 24) & 0xff;
+	mac_addr[2] = (macid_hi >> 16) & 0xff;
+	mac_addr[1] = (macid_hi >> 8) & 0xff;
+	mac_addr[0] = macid_hi & 0xff;
+
+	return 0;
+}
+
 static int cpsw_probe_dt(struct cpsw_platform_data *data,
 			 struct platform_device *pdev)
 {
@@ -1908,8 +1943,15 @@  static int cpsw_probe_dt(struct cpsw_platform_data *data,
 			 PHY_ID_FMT, mdio->name, phyid);
 
 		mac_addr = of_get_mac_address(slave_node);
-		if (mac_addr)
-			memcpy(slave_data->mac_addr, mac_addr, ETH_ALEN);
+		if (mac_addr) {
+			memcpy(slave_data->mac_addr, mac_addr,
+					ETH_ALEN);
+		} else {
+			ret = cpsw_am33xx_cm_get_macid(&pdev->dev, i,
+					slave_data->mac_addr);
+			if (ret)
+				return ret;
+		}
 
 		slave_data->phy_if = of_get_phy_mode(slave_node);
 		if (slave_data->phy_if < 0) {