Thanks, Stefan.

Please see my answer bellow.

Regards,
Duc Dang.

On Mon, Mar 5, 2012 at 2:41 PM, Stefan Roese [off-list ref] wrote:

On Monday 05 March 2012 05:03:44 Duc Dang wrote:

quoted

This patch includes:

  Configure EMAC PHY clock source (clock from PHY or internal clock).

  Do not advertise PHY half duplex capability as APM821XX EMAC does not
support half duplex mode.

  Add changes to support configuring jumbo frame for APM821XX EMAC.

Signed-off-by: Duc Dang <redacted>
---
v2:
 Fix coding style problems.

 drivers/net/ethernet/ibm/emac/core.c |   26 +++++++++++++++++++++++++-
 drivers/net/ethernet/ibm/emac/core.h |   13 +++++++++++--
 drivers/net/ethernet/ibm/emac/emac.h |    5 ++++-
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c

b/drivers/net/ethernet/ibm/emac/core.c index 2abce96..ad671c5 100644

--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c

@@ -434,6 +434,11 @@ static inline u32 emac_iff2rmr(struct net_device

*ndev) else if (!netdev_mc_empty(ndev))
              r |= EMAC_RMR_MAE;

+     if (emac_has_feature(dev, EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE)) {
+             r &= ~EMAC4_RMR_MJS_MASK;
+             r |= EMAC4_RMR_MJS(ndev->mtu);
+     }
+
      return r;
 }

@@ -965,6 +970,7 @@ static int emac_resize_rx_ring(struct emac_instance

*dev, int new_mtu) int rx_sync_size = emac_rx_sync_size(new_mtu);
      int rx_skb_size = emac_rx_skb_size(new_mtu);
      int i, ret = 0;
+     int mr1_jumbo_bit_change = 0;

      mutex_lock(&dev->link_lock);
      emac_netif_stop(dev);

@@ -1013,7 +1019,14 @@ static int emac_resize_rx_ring(struct

emac_instance

quoted

*dev, int new_mtu) }
  skip:
      /* Check if we need to change "Jumbo" bit in MR1 */
-     if ((new_mtu > ETH_DATA_LEN) ^ (dev->ndev->mtu > ETH_DATA_LEN)) {
+     if (emac_has_feature(dev, EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE))
+             mr1_jumbo_bit_change = (new_mtu > ETH_DATA_LEN) ||
+                             (dev->ndev->mtu > ETH_DATA_LEN);
+     else
+             mr1_jumbo_bit_change = (new_mtu > ETH_DATA_LEN) ^
+                             (dev->ndev->mtu > ETH_DATA_LEN);
+
+     if (mr1_jumbo_bit_change) {
              /* This is to prevent starting RX channel in

emac_rx_enable()
*/

quoted

              set_bit(MAL_COMMAC_RX_STOPPED, &dev->commac.flags);

@@ -2471,6 +2484,7 @@ static int __devinit emac_init_phy(struct

emac_instance *dev)

      /* Disable any PHY features not supported by the platform */
      dev->phy.def->features &= ~dev->phy_feat_exc;
+     dev->phy.features &= ~dev->phy_feat_exc;

      /* Setup initial link parameters */
      if (dev->phy.features & SUPPORTED_Autoneg) {

@@ -2568,6 +2582,10 @@ static int __devinit emac_init_config(struct

emac_instance *dev) if (of_device_is_compatible(np, "ibm,emac-405ex") ||
                  of_device_is_compatible(np, "ibm,emac-405exr"))
                      dev->features |= EMAC_FTR_440EP_PHY_CLK_FIX;
+             if (of_device_is_compatible(np, "ibm,emac-apm821xx"))
+                     dev->features |=

(EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE
|

quoted

+                                     EMAC_FTR_APM821XX_NO_HALF_DUPLEX |
+                                     EMAC_FTR_460EX_PHY_CLK_FIX);

Nitpick: Parentheses for multi line statement please.

[Duc Dang] I wil add parentheses as you suggested.

      } else if (of_device_is_compatible(np, "ibm,emac4")) {

quoted

              dev->features |= EMAC_FTR_EMAC4;
              if (of_device_is_compatible(np, "ibm,emac-440gx"))

@@ -2818,6 +2836,12 @@ static int __devinit emac_probe(struct

platform_device *ofdev) dev->stop_timeout = STOP_TIMEOUT_100;
      INIT_DELAYED_WORK(&dev->link_work, emac_link_timer);

+     /* Some SoCs like APM821xx does not support Half Duplex mode. */
+     if (emac_has_feature(dev, EMAC_FTR_APM821XX_NO_HALF_DUPLEX))
+             dev->phy_feat_exc = (SUPPORTED_1000baseT_Half |
+                                     SUPPORTED_100baseT_Half |
+                                     SUPPORTED_10baseT_Half);
+

Nitpick: Parentheses for multi line statement please.

quoted

      /* Find PHY if any */
      err = emac_init_phy(dev);
      if (err != 0)

diff --git a/drivers/net/ethernet/ibm/emac/core.h

b/drivers/net/ethernet/ibm/emac/core.h index fa3ec57..9dea85a 100644

--- a/drivers/net/ethernet/ibm/emac/core.h
+++ b/drivers/net/ethernet/ibm/emac/core.h

@@ -325,7 +325,14 @@ struct emac_instance {
  * Set if we need phy clock workaround for 460ex or 460gt
  */
 #define EMAC_FTR_460EX_PHY_CLK_FIX   0x00000400
-
+/*
+ * APM821xx requires Jumbo frame size set explicitly
+ */
+#define EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE   0x00000800
+/*
+ * APM821xx does not support Half Duplex mode
+ */
+#define EMAC_FTR_APM821XX_NO_HALF_DUPLEX     0x00001000

 /* Right now, we don't quite handle the always/possible masks on the
  * most optimal way as we don't have a way to say something like

@@ -353,7 +360,9 @@ enum {
          EMAC_FTR_NO_FLOW_CONTROL_40x |
 #endif
      EMAC_FTR_460EX_PHY_CLK_FIX |
-     EMAC_FTR_440EP_PHY_CLK_FIX,
+     EMAC_FTR_440EP_PHY_CLK_FIX |
+     EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE |
+     EMAC_FTR_APM821XX_NO_HALF_DUPLEX,
 };

 static inline int emac_has_feature(struct emac_instance *dev,

diff --git a/drivers/net/ethernet/ibm/emac/emac.h

b/drivers/net/ethernet/ibm/emac/emac.h index 1568278..36bcd69 100644

--- a/drivers/net/ethernet/ibm/emac/emac.h
+++ b/drivers/net/ethernet/ibm/emac/emac.h

@@ -212,7 +212,10 @@ struct emac_regs {
 #define EMAC4_RMR_RFAF_64_1024               0x00000006
 #define EMAC4_RMR_RFAF_128_2048              0x00000007
 #define EMAC4_RMR_BASE                       EMAC4_RMR_RFAF_128_2048
-
+#if defined(CONFIG_APM821xx)
+#define EMAC4_RMR_MJS_MASK              0x0001fff8
+#define EMAC4_RMR_MJS(s)                (((s) << 3) &

EMAC4_RMR_MJS_MASK)

quoted

+#endif

Is this "#if defined" really needed? If not, I suggest you drop it.

[Duc Dang] Only APM821XX and later SoCs have this register field, but it
is no harm to drop the #if defined as 460EX/460GT/405EX have this register
field as reserved. So I will drop it.

Thanks,
Stefan