Thanks, Stefan.<br><br>Please see my answer bellow.<br><br>Regards,<br>Duc Dang.<br><br><div class="gmail_quote">On Mon, Mar 5, 2012 at 2:41 PM, Stefan Roese <span dir="ltr"><<a href="mailto:sr@denx.de">sr@denx.de</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Monday 05 March 2012 05:03:44 Duc Dang wrote:<br>
> This patch includes:<br>
><br>
> Configure EMAC PHY clock source (clock from PHY or internal clock).<br>
><br>
> Do not advertise PHY half duplex capability as APM821XX EMAC does not<br>
> support half duplex mode.<br>
><br>
> Add changes to support configuring jumbo frame for APM821XX EMAC.<br>
><br>
> Signed-off-by: Duc Dang <<a href="mailto:dhdang@apm.com">dhdang@apm.com</a>><br>
> ---<br>
> v2:<br>
> Fix coding style problems.<br>
><br>
> drivers/net/ethernet/ibm/emac/core.c | 26 +++++++++++++++++++++++++-<br>
> drivers/net/ethernet/ibm/emac/core.h | 13 +++++++++++--<br>
> drivers/net/ethernet/ibm/emac/emac.h | 5 ++++-<br>
> 3 files changed, 40 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/drivers/net/ethernet/ibm/emac/core.c<br>
> b/drivers/net/ethernet/ibm/emac/core.c index 2abce96..ad671c5 100644<br>
> --- a/drivers/net/ethernet/ibm/emac/core.c<br>
> +++ b/drivers/net/ethernet/ibm/emac/core.c<br>
> @@ -434,6 +434,11 @@ static inline u32 emac_iff2rmr(struct net_device<br>
> *ndev) else if (!netdev_mc_empty(ndev))<br>
> r |= EMAC_RMR_MAE;<br>
><br>
> + if (emac_has_feature(dev, EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE)) {<br>
> + r &= ~EMAC4_RMR_MJS_MASK;<br>
> + r |= EMAC4_RMR_MJS(ndev->mtu);<br>
> + }<br>
> +<br>
> return r;<br>
> }<br>
><br>
> @@ -965,6 +970,7 @@ static int emac_resize_rx_ring(struct emac_instance<br>
> *dev, int new_mtu) int rx_sync_size = emac_rx_sync_size(new_mtu);<br>
> int rx_skb_size = emac_rx_skb_size(new_mtu);<br>
> int i, ret = 0;<br>
> + int mr1_jumbo_bit_change = 0;<br>
><br>
> mutex_lock(&dev->link_lock);<br>
> emac_netif_stop(dev);<br>
> @@ -1013,7 +1019,14 @@ static int emac_resize_rx_ring(struct emac_instance<br>
> *dev, int new_mtu) }<br>
> skip:<br>
> /* Check if we need to change "Jumbo" bit in MR1 */<br>
> - if ((new_mtu > ETH_DATA_LEN) ^ (dev->ndev->mtu > ETH_DATA_LEN)) {<br>
> + if (emac_has_feature(dev, EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE))<br>
> + mr1_jumbo_bit_change = (new_mtu > ETH_DATA_LEN) ||<br>
> + (dev->ndev->mtu > ETH_DATA_LEN);<br>
> + else<br>
> + mr1_jumbo_bit_change = (new_mtu > ETH_DATA_LEN) ^<br>
> + (dev->ndev->mtu > ETH_DATA_LEN);<br>
> +<br>
> + if (mr1_jumbo_bit_change) {<br>
> /* This is to prevent starting RX channel in emac_rx_enable()<br>
*/<br>
> set_bit(MAL_COMMAC_RX_STOPPED, &dev->commac.flags);<br>
><br>
> @@ -2471,6 +2484,7 @@ static int __devinit emac_init_phy(struct<br>
> emac_instance *dev)<br>
><br>
> /* Disable any PHY features not supported by the platform */<br>
> dev->phy.def->features &= ~dev->phy_feat_exc;<br>
> + dev->phy.features &= ~dev->phy_feat_exc;<br>
><br>
> /* Setup initial link parameters */<br>
> if (dev->phy.features & SUPPORTED_Autoneg) {<br>
> @@ -2568,6 +2582,10 @@ static int __devinit emac_init_config(struct<br>
> emac_instance *dev) if (of_device_is_compatible(np, "ibm,emac-405ex") ||<br>
> of_device_is_compatible(np, "ibm,emac-405exr"))<br>
> dev->features |= EMAC_FTR_440EP_PHY_CLK_FIX;<br>
> + if (of_device_is_compatible(np, "ibm,emac-apm821xx"))<br>
> + dev->features |= (EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE<br>
|<br>
> + EMAC_FTR_APM821XX_NO_HALF_DUPLEX |<br>
> + EMAC_FTR_460EX_PHY_CLK_FIX);<br>
<br>
Nitpick: Parentheses for multi line statement please.<br></blockquote><div>[Duc Dang] I wil add parentheses as you suggested.<br><br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> } else if (of_device_is_compatible(np, "ibm,emac4")) {<br>
> dev->features |= EMAC_FTR_EMAC4;<br>
> if (of_device_is_compatible(np, "ibm,emac-440gx"))<br>
> @@ -2818,6 +2836,12 @@ static int __devinit emac_probe(struct<br>
> platform_device *ofdev) dev->stop_timeout = STOP_TIMEOUT_100;<br>
> INIT_DELAYED_WORK(&dev->link_work, emac_link_timer);<br>
><br>
> + /* Some SoCs like APM821xx does not support Half Duplex mode. */<br>
> + if (emac_has_feature(dev, EMAC_FTR_APM821XX_NO_HALF_DUPLEX))<br>
> + dev->phy_feat_exc = (SUPPORTED_1000baseT_Half |<br>
> + SUPPORTED_100baseT_Half |<br>
> + SUPPORTED_10baseT_Half);<br>
> +<br>
<br>
Nitpick: Parentheses for multi line statement please.<br>
<br>
> /* Find PHY if any */<br>
> err = emac_init_phy(dev);<br>
> if (err != 0)<br>
> diff --git a/drivers/net/ethernet/ibm/emac/core.h<br>
> b/drivers/net/ethernet/ibm/emac/core.h index fa3ec57..9dea85a 100644<br>
> --- a/drivers/net/ethernet/ibm/emac/core.h<br>
> +++ b/drivers/net/ethernet/ibm/emac/core.h<br>
> @@ -325,7 +325,14 @@ struct emac_instance {<br>
> * Set if we need phy clock workaround for 460ex or 460gt<br>
> */<br>
> #define EMAC_FTR_460EX_PHY_CLK_FIX 0x00000400<br>
> -<br>
> +/*<br>
> + * APM821xx requires Jumbo frame size set explicitly<br>
> + */<br>
> +#define EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE 0x00000800<br>
> +/*<br>
> + * APM821xx does not support Half Duplex mode<br>
> + */<br>
> +#define EMAC_FTR_APM821XX_NO_HALF_DUPLEX 0x00001000<br>
><br>
> /* Right now, we don't quite handle the always/possible masks on the<br>
> * most optimal way as we don't have a way to say something like<br>
> @@ -353,7 +360,9 @@ enum {<br>
> EMAC_FTR_NO_FLOW_CONTROL_40x |<br>
> #endif<br>
> EMAC_FTR_460EX_PHY_CLK_FIX |<br>
> - EMAC_FTR_440EP_PHY_CLK_FIX,<br>
> + EMAC_FTR_440EP_PHY_CLK_FIX |<br>
> + EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE |<br>
> + EMAC_FTR_APM821XX_NO_HALF_DUPLEX,<br>
> };<br>
><br>
> static inline int emac_has_feature(struct emac_instance *dev,<br>
> diff --git a/drivers/net/ethernet/ibm/emac/emac.h<br>
> b/drivers/net/ethernet/ibm/emac/emac.h index 1568278..36bcd69 100644<br>
> --- a/drivers/net/ethernet/ibm/emac/emac.h<br>
> +++ b/drivers/net/ethernet/ibm/emac/emac.h<br>
> @@ -212,7 +212,10 @@ struct emac_regs {<br>
> #define EMAC4_RMR_RFAF_64_1024 0x00000006<br>
> #define EMAC4_RMR_RFAF_128_2048 0x00000007<br>
> #define EMAC4_RMR_BASE EMAC4_RMR_RFAF_128_2048<br>
> -<br>
> +#if defined(CONFIG_APM821xx)<br>
> +#define EMAC4_RMR_MJS_MASK 0x0001fff8<br>
> +#define EMAC4_RMR_MJS(s) (((s) << 3) & EMAC4_RMR_MJS_MASK)<br>
> +#endif<br>
<br>
Is this "#if defined" really needed? If not, I suggest you drop it.<br>
</blockquote><div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">[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.<br>
</blockquote><div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks,<br>
Stefan<br>
</blockquote></div><br>