RE: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree

2013-02-12 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: Linuxppc-dev [mailto:linuxppc-dev-
 bounces+bharat.bhushan=freescale@lists.ozlabs.org] On Behalf Of Stefan 
 Roese
 Sent: Tuesday, February 12, 2013 2:38 PM
 To: net...@vger.kernel.org
 Cc: linuxppc-...@ozlabs.org; Anatolij Gustschin
 Subject: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree
 
 Until now, the MPC5200 FEC ethernet driver relied upon the bootloader
 (U-Boot) to write the MAC address into the ethernet controller registers. The
 Linux driver should not rely on such a thing. So lets read the MAC address 
 from
 the DT as it should be done here.
 
 The following priority is now used to read the MAC address:
 
 1) First, try OF node MAC address, if not present or invalid, then:
 
 2) Read from MAC address registers, if invalid, then:

Why we read from MAC registers if Linux should not rely on bootloader?

-Bharat


 
 3) Log a warning message, and choose a random MAC address.
 
 This fixes a problem with a MPC5200 board that uses the SPL U-Boot version
 without FEC initialization before Linux booting for boot speedup.
 
 Additionally a status line is now be printed upon successful driver probing,
 also displaying this MAC address.
 
 Signed-off-by: Stefan Roese s...@denx.de
 Cc: Anatolij Gustschin ag...@denx.de
 ---
 v2:
 - Remove module parameter mpc52xx_fec_mac_addr
 - Priority for MAC address probing now is DT, controller regs
   If the resulting MAC address is invalid, a random address will
   be generated and used with a warning message
 - Use np variable to simplify the code
 
  drivers/net/ethernet/freescale/fec_mpc52xx.c | 61 
 +---
  1 file changed, 37 insertions(+), 24 deletions(-)
 
 diff --git a/drivers/net/ethernet/freescale/fec_mpc52xx.c
 b/drivers/net/ethernet/freescale/fec_mpc52xx.c
 index 817d081..8b725f4 100644
 --- a/drivers/net/ethernet/freescale/fec_mpc52xx.c
 +++ b/drivers/net/ethernet/freescale/fec_mpc52xx.c
 @@ -76,10 +76,6 @@ static void mpc52xx_fec_stop(struct net_device *dev);  
 static
 void mpc52xx_fec_start(struct net_device *dev);  static void
 mpc52xx_fec_reset(struct net_device *dev);
 
 -static u8 mpc52xx_fec_mac_addr[6];
 -module_param_array_named(mac, mpc52xx_fec_mac_addr, byte, NULL, 0); -
 MODULE_PARM_DESC(mac, six hex digits, ie. 0x1,0x2,0xc0,0x01,0xba,0xbe);
 -
  #define MPC52xx_MESSAGES_DEFAULT ( NETIF_MSG_DRV | NETIF_MSG_PROBE | \
   NETIF_MSG_LINK | NETIF_MSG_IFDOWN | NETIF_MSG_IFUP)
  static int debug = -1;   /* the above default */
 @@ -110,15 +106,6 @@ static void mpc52xx_fec_set_paddr(struct net_device *dev,
 u8 *mac)
   out_be32(fec-paddr2, (*(u16 *)(mac[4])  16) | FEC_PADDR2_TYPE);  }
 
 -static void mpc52xx_fec_get_paddr(struct net_device *dev, u8 *mac) -{
 - struct mpc52xx_fec_priv *priv = netdev_priv(dev);
 - struct mpc52xx_fec __iomem *fec = priv-fec;
 -
 - *(u32 *)(mac[0]) = in_be32(fec-paddr1);
 - *(u16 *)(mac[4]) = in_be32(fec-paddr2)  16;
 -}
 -
  static int mpc52xx_fec_set_mac_address(struct net_device *dev, void *addr)  {
   struct sockaddr *sock = addr;
 @@ -853,6 +840,8 @@ static int mpc52xx_fec_probe(struct platform_device *op)
   struct resource mem;
   const u32 *prop;
   int prop_size;
 + struct device_node *np = op-dev.of_node;
 + const void *p;
 
   phys_addr_t rx_fifo;
   phys_addr_t tx_fifo;
 @@ -866,7 +855,7 @@ static int mpc52xx_fec_probe(struct platform_device *op)
   priv-ndev = ndev;
 
   /* Reserve FEC control zone */
 - rv = of_address_to_resource(op-dev.of_node, 0, mem);
 + rv = of_address_to_resource(np, 0, mem);
   if (rv) {
   printk(KERN_ERR DRIVER_NAME : 
   Error while parsing device node resource\n ); 
 @@ -
 919,7 +908,7 @@ static int mpc52xx_fec_probe(struct platform_device *op)
 
   /* Get the IRQ we need one by one */
   /* Control */
 - ndev-irq = irq_of_parse_and_map(op-dev.of_node, 0);
 + ndev-irq = irq_of_parse_and_map(np, 0);
 
   /* RX */
   priv-r_irq = bcom_get_task_irq(priv-rx_dmatsk);
 @@ -927,11 +916,33 @@ static int mpc52xx_fec_probe(struct platform_device *op)
   /* TX */
   priv-t_irq = bcom_get_task_irq(priv-tx_dmatsk);
 
 - /* MAC address init */
 - if (!is_zero_ether_addr(mpc52xx_fec_mac_addr))
 - memcpy(ndev-dev_addr, mpc52xx_fec_mac_addr, 6);
 - else
 - mpc52xx_fec_get_paddr(ndev, ndev-dev_addr);
 + /*
 +  * MAC address init:
 +  *
 +  * First try to read MAC address from DT
 +  */
 + p = of_get_property(np, local-mac-address, NULL);
 + if (p != NULL) {
 + memcpy(ndev-dev_addr, p, 6);
 + } else {
 + struct mpc52xx_fec __iomem *fec = priv-fec;
 +
 + /*
 +  * If the MAC addresse is not provided via DT then read
 +  * it back from the controller regs
 +  */
 + *(u32 *)(ndev-dev_addr[0])

Re: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree

2013-02-12 Thread Stefan Roese 
On 12.02.2013 11:56, Bhushan Bharat-R65777 wrote:
 Until now, the MPC5200 FEC ethernet driver relied upon the bootloader
 (U-Boot) to write the MAC address into the ethernet controller registers. The
 Linux driver should not rely on such a thing. So lets read the MAC address 
 from
 the DT as it should be done here.

 The following priority is now used to read the MAC address:

 1) First, try OF node MAC address, if not present or invalid, then:

 2) Read from MAC address registers, if invalid, then:
 
 Why we read from MAC registers if Linux should not rely on bootloader?

It was suggested by David. Backwards compatibility. Here Davids comment
to my original patch which removed this register reading completely:


I don't think this is a conservative enough change.

You have to keep the MAC register reading code around, as a backup
code path in case the OF device node lacks a MAC address


Thanks,
Stefan
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

RE: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree

2013-02-12 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: Stefan Roese [mailto:s...@denx.de]
 Sent: Tuesday, February 12, 2013 4:34 PM
 To: Bhushan Bharat-R65777
 Cc: net...@vger.kernel.org; linuxppc-...@ozlabs.org; Anatolij Gustschin; David
 S. Miller
 Subject: Re: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree
 
 On 12.02.2013 11:56, Bhushan Bharat-R65777 wrote:
  Until now, the MPC5200 FEC ethernet driver relied upon the bootloader
  (U-Boot) to write the MAC address into the ethernet controller
  registers. The Linux driver should not rely on such a thing. So lets
  read the MAC address from the DT as it should be done here.
 
  The following priority is now used to read the MAC address:
 
  1) First, try OF node MAC address, if not present or invalid, then:
 
  2) Read from MAC address registers, if invalid, then:
 
  Why we read from MAC registers if Linux should not rely on bootloader?
 
 It was suggested by David. Backwards compatibility. Here Davids comment to my
 original patch which removed this register reading completely:
 
 
 I don't think this is a conservative enough change.
 
 You have to keep the MAC register reading code around, as a backup code path 
 in
 case the OF device node lacks a MAC address 

Ok,

But this is really a backward compatibility or hiding some bug? My thought is 
that if DT does not have a valid MAC address then it is a BUG and should be 
fixed. Is not it?

-Bharat

 
 Thanks,
 Stefan


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree

2013-02-12 Thread Stefan Roese 
On 12.02.2013 12:23, Bhushan Bharat-R65777 wrote:
 Why we read from MAC registers if Linux should not rely on bootloader?

 It was suggested by David. Backwards compatibility. Here Davids comment to my
 original patch which removed this register reading completely:

 
 I don't think this is a conservative enough change.

 You have to keep the MAC register reading code around, as a backup code path 
 in
 case the OF device node lacks a MAC address 
 
 Ok,
 
 But this is really a backward compatibility or hiding some bug? My
 thought is that if DT does not have a valid MAC address then it is
 a BUG and should be fixed. Is not it?

Yes. But it can only be fixed in the bootloader then. And some people
might not want to do this or might be unable to do this. So lets keep
this feature available for such cases.

BTW: U-Boot traditionally always wrote the MAC address into the FEC
registers. Even if FEC was not used in U-Boot at all. I only noticed
this problem with the new SPL fastbooting U-Boot version, which is
basically a very stripped down U-Boot version directly booting into
Linux (or U-Boot if selected). Here the FEC registers are not touched at
all. And the Linux FEC driver then woke up with an invalid MAC address.

Thanks,
Stefan



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree

2013-02-12 Thread Timur Tabi 
On Tue, Feb 12, 2013 at 3:08 AM, Stefan Roese s...@denx.de wrote:

 +* First try to read MAC address from DT
 +*/
 +   p = of_get_property(np, local-mac-address, NULL);

of_get_mac_address()
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree

2013-02-12 Thread David Miller 
From: Bhushan Bharat-R65777 r65...@freescale.com
Date: Tue, 12 Feb 2013 10:56:05 +

 Why we read from MAC registers if Linux should not rely on bootloader?

Because it used to and if you just remove that code then you break
existing setups, and I explicitly told him to code things this way.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree

2013-02-12 Thread David Miller 
From: Stefan Roese s...@denx.de
Date: Tue, 12 Feb 2013 10:08:08 +0100

 Until now, the MPC5200 FEC ethernet driver relied upon the bootloader
 (U-Boot) to write the MAC address into the ethernet controller
 registers. The Linux driver should not rely on such a thing. So
 lets read the MAC address from the DT as it should be done here.
 
 The following priority is now used to read the MAC address:
 
 1) First, try OF node MAC address, if not present or invalid, then:
 
 2) Read from MAC address registers, if invalid, then:
 
 3) Log a warning message, and choose a random MAC address.
 
 This fixes a problem with a MPC5200 board that uses the SPL U-Boot
 version without FEC initialization before Linux booting for
 boot speedup.
 
 Additionally a status line is now be printed upon successful
 driver probing, also displaying this MAC address.
 
 Signed-off-by: Stefan Roese s...@denx.de

Applied.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev