This patch series is to add dual fec support for mx28, which is
a mxs-based soc. Some code changes related to the following commits
are also made in this patch set for some reasons.
e6b043d512fa8d9a3801bf5d72bfa3b8fc3b3cc8
netdev/fec.c: add phylib supporting to enable carrier detection (v2)
e3fe8558c7fc182972c3d947d88744482111f304
net/fec: fix pm to survive to suspend/resume
It's been tested on mx28 evk and mx51 babbage. For mx28, it has
to work against the tree
git://git.pengutronix.de/git/imx/linux-2.6.git imx-for-2.6.38
plus patch
[PATCH v4] ARM: mxs: Change duart device to use amba-pl011
The 3 patches below preceding with * have changes since v3, and
the detailed change log can be found in individual patch.
[PATCH v4 01/10] net/fec: fix MMFR_OP type in fec_enet_mdio_write
[PATCH v4 02/10] net/fec: remove the use of "index" which is legacy
[PATCH v4 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac
[PATCH v4 04/10] net/fec: improve pm for better suspend/resume
*[PATCH v4 05/10] net/fec: add dual fec support for mx28
*[PATCH v4 06/10] ARM: mx28: update clock and device name for dual fec support
[PATCH v4 07/10] ARM: mx28: add the second fec device registration
*[PATCH v4 08/10] ARM: mxs: add ocotp read function
[PATCH v4 09/10] ARM: mx28: read fec mac address from ocotp
[PATCH v4 10/10] ARM: mxs: add initial pm support
Thanks for the review.
Regards,
Shawn
FEC_MMFR_OP_WRITE should be used than FEC_MMFR_OP_READ in
a mdio write operation.
It's probably a typo introduced by commit:
e6b043d512fa8d9a3801bf5d72bfa3b8fc3b3cc8
netdev/fec.c: add phylib supporting to enable carrier detection (v2)
Signed-off-by: Shawn Guo <redacted>
---
drivers/net/fec.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
@@ -651,8 +651,8 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,fep->mii_timeout=0;init_completion(&fep->mdio_done);-/* start a read op */-writel(FEC_MMFR_ST|FEC_MMFR_OP_READ|+/* start a write op */+writel(FEC_MMFR_ST|FEC_MMFR_OP_WRITE|FEC_MMFR_PA(mii_id)|FEC_MMFR_RA(regnum)|FEC_MMFR_TA|FEC_MMFR_DATA(value),fep->hwp+FEC_MII_DATA);
The "index" becomes legacy since fep->pdev->id starts working
to identify the instance.
Moreover, the call of fec_enet_init(ndev, 0) always passes 0
to fep->index. This makes the following code in fec_get_mac buggy.
/* Adjust MAC if using default MAC address */
if (iap == fec_mac_default)
dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->index;
It may be the time to remove "index" and use fep->pdev->id instead.
Signed-off-by: Shawn Guo <redacted>
---
drivers/net/fec.c | 9 +++------
1 files changed, 3 insertions(+), 6 deletions(-)
@@ -566,7 +565,7 @@ static void __inline__ fec_get_mac(struct net_device *dev)/* Adjust MAC if using default MAC address */if(iap==fec_mac_default)-dev->dev_addr[ETH_ALEN-1]=fec_mac_default[ETH_ALEN-1]+fep->index;+dev->dev_addr[ETH_ALEN-1]=fec_mac_default[ETH_ALEN-1]+fep->pdev->id;}#endif
Add mac field into fec_platform_data and consolidate function
fec_get_mac to get mac address in following order.
1) module parameter via kernel command line fec.macaddr=0x00,0x04,...
2) from flash in case of CONFIG_M5272 or fec_platform_data mac
field for others, which typically have mac stored in fuse
3) fec mac address registers set by bootloader
Signed-off-by: Shawn Guo <redacted>
---
Changes for v3:
- Use module parameter than new kernel command line to pass
mac address
- Change variable name and comment to remove confusing word
"default"
- Fix copyright breakage in fec.h
drivers/net/fec.c | 81 ++++++++++++++++++++++++---------------------------
include/linux/fec.h | 3 ++
2 files changed, 41 insertions(+), 43 deletions(-)
@@ -537,37 +533,50 @@ rx_processing_done:}/* ------------------------------------------------------------------------- */-#ifdef CONFIG_M5272staticvoid__inline__fec_get_mac(structnet_device*dev){structfec_enet_private*fep=netdev_priv(dev);+structfec_platform_data*pdata=fep->pdev->dev.platform_data;unsignedchar*iap,tmpaddr[ETH_ALEN];-if(FEC_FLASHMAC){-/*-*GetMACaddressfromFLASH.-*Ifitisall1'sor0's,usethedefault.-*/-iap=(unsignedchar*)FEC_FLASHMAC;-if((iap[0]==0)&&(iap[1]==0)&&(iap[2]==0)&&-(iap[3]==0)&&(iap[4]==0)&&(iap[5]==0))-iap=fec_mac_default;-if((iap[0]==0xff)&&(iap[1]==0xff)&&(iap[2]==0xff)&&-(iap[3]==0xff)&&(iap[4]==0xff)&&(iap[5]==0xff))-iap=fec_mac_default;-}else{-*((unsignedlong*)&tmpaddr[0])=readl(fep->hwp+FEC_ADDR_LOW);-*((unsignedshort*)&tmpaddr[4])=(readl(fep->hwp+FEC_ADDR_HIGH)>>16);+/*+*trytogetmacaddressinfollowingorder:+*+*1)moduleparameterviakernelcommandlineinform+*fec.macaddr=0x00,0x04,0x9f,0x01,0x30,0xe0+*/+iap=macaddr;++/*+*2)fromflashorfuse(viaplatformdata)+*/+if(!is_valid_ether_addr(iap)){+#ifdef CONFIG_M5272+if(FEC_FLASHMAC)+iap=(unsignedchar*)FEC_FLASHMAC;+#else+if(pdata)+memcpy(iap,pdata->mac,ETH_ALEN);+#endif+}++/*+*3)FECmacregisterssetbybootloader+*/+if(!is_valid_ether_addr(iap)){+*((unsignedlong*)&tmpaddr[0])=+be32_to_cpu(readl(fep->hwp+FEC_ADDR_LOW));+*((unsignedshort*)&tmpaddr[4])=+be16_to_cpu(readl(fep->hwp+FEC_ADDR_HIGH)>>16);iap=&tmpaddr[0];}memcpy(dev->dev_addr,iap,ETH_ALEN);-/* Adjust MAC if using default MAC address */-if(iap==fec_mac_default)-dev->dev_addr[ETH_ALEN-1]=fec_mac_default[ETH_ALEN-1]+fep->pdev->id;+/* Adjust MAC if using macaddr */+if(iap==macaddr)+dev->dev_addr[ETH_ALEN-1]=macaddr[ETH_ALEN-1]+fep->pdev->id;}-#endif/* ------------------------------------------------------------------------- */
@@ -1087,22 +1096,8 @@ static int fec_enet_init(struct net_device *dev)fep->hwp=(void__iomem*)dev->base_addr;fep->netdev=dev;-/* Set the Ethernet address */-#ifdef CONFIG_M5272+/* Get the Ethernet address */fec_get_mac(dev);-#else-{-unsignedlongl;-l=readl(fep->hwp+FEC_ADDR_LOW);-dev->dev_addr[0]=(unsignedchar)((l&0xFF000000)>>24);-dev->dev_addr[1]=(unsignedchar)((l&0x00FF0000)>>16);-dev->dev_addr[2]=(unsignedchar)((l&0x0000FF00)>>8);-dev->dev_addr[3]=(unsignedchar)((l&0x000000FF)>>0);-l=readl(fep->hwp+FEC_ADDR_HIGH);-dev->dev_addr[4]=(unsignedchar)((l&0xFF000000)>>24);-dev->dev_addr[5]=(unsignedchar)((l&0x00FF0000)>>16);-}-#endif/* Set receive and transmit descriptor base. */fep->rx_bd_base=cbd_base;
The following commit made a fix to use fec_enet_open/fec_enet_close
over fec_enet_init/fec_stop for suspend/resume, because fec_enet_init
does not allow to have a working network interface at resume.
e3fe8558c7fc182972c3d947d88744482111f304
net/fec: fix pm to survive to suspend/resume
This fix works for i.mx/mxc fec controller, but fails on mx28 fec
which gets a different interrupt logic design. On i.mx fec, interrupt
can be triggered even bit ETHER_EN of ECR register is not set. But
on mx28 fec, ETHER_EN must be set to get interrupt work. Meanwhile,
MII interrupt is mandatory to resume the driver, because MDIO
read/write changed to interrupt mode by commit below.
97b72e4320a9aaa4a7f1592ee7d2da7e2c9bd349
fec: use interrupt for MDIO completion indication
fec_restart/fec_stop comes out as the solution working for both
cases.
Signed-off-by: Shawn Guo <redacted>
---
drivers/net/fec.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
This patch is to add mx28 dual fec support. Here are some key notes
for mx28 fec controller.
- The mx28 fec controller naming ENET-MAC is a different IP from FEC
used on other i.mx variants. But they are basically compatible
on software interface, so it's possible to share the same driver.
- ENET-MAC design on mx28 made an improper assumption that it runs
on a big-endian system. As the result, driver has to swap every
frame going to and coming from the controller.
- The external phys can only be configured by fec0, which means fec1
can not work independently and both phys need to be configured by
mii_bus attached on fec0.
- ENET-MAC reset will get mac address registers reset too.
- ENET-MAC MII/RMII mode and 10M/100M speed are configured
differently FEC.
- ETHER_EN bit must be set to get ENET-MAC interrupt work.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v4:
- Use #ifndef CONFIG_ARM to include ColdFire header files
- Define quirk bits in id_entry.driver_data to handle controller
difference, which is more scalable than using device name
- Define fec0_mii_bus as a static function in fec_enet_mii_init
to fold the mii_bus instance attached on fec0
- Use cpu_to_be32 over __swab32 in function swap_buffer
Changes for v3:
- Move v2 changes into patch #3
- Use device name to check if it's running on ENET-MAC
drivers/net/Kconfig | 7 ++-
drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------
drivers/net/fec.h | 5 +-
3 files changed, 139 insertions(+), 21 deletions(-)
@@ -487,6 +529,9 @@ fec_enet_rx(struct net_device *dev)dma_unmap_single(NULL,bdp->cbd_bufaddr,bdp->cbd_datlen,DMA_FROM_DEVICE);+if(id_entry->driver_data&FEC_QUIRK_SWAP_FRAME)+swap_buffer(data,pkt_len);+/* This does 16 byte alignment, exactly what we need.*ThepacketlengthincludesFCS,butwedon'twantto*includethatwhenpassingupstreamasitmessesup
@@ -689,6 +734,7 @@ static int fec_enet_mii_probe(struct net_device *dev)charmdio_bus_id[MII_BUS_ID_SIZE];charphy_name[MII_BUS_ID_SIZE+3];intphy_id;+intdev_id=fep->pdev->id;fep->phy_dev=NULL;
@@ -700,6 +746,8 @@ static int fec_enet_mii_probe(struct net_device *dev)continue;if(fep->mii_bus->phy_map[phy_id]->phy_id==0)continue;+if(dev_id--)+continue;strncpy(mdio_bus_id,fep->mii_bus->id,MII_BUS_ID_SIZE);break;}
@@ -777,6 +850,10 @@ static int fec_enet_mii_init(struct platform_device *pdev)if(mdiobus_register(fep->mii_bus))gotoerr_out_free_mdio_irq;+/* save fec0 mii_bus */+if(id_entry->driver_data&FEC_QUIRK_ENET_MAC)+fec0_mii_bus=fep->mii_bus;+return0;err_out_free_mdio_irq:
@@ -1148,12 +1225,25 @@ static voidfec_restart(structnet_device*dev,intduplex){structfec_enet_private*fep=netdev_priv(dev);+conststructplatform_device_id*id_entry=+platform_get_device_id(fep->pdev);inti;+u32val,temp_mac[2];/* Whack a reset. We should wait for this. */writel(1,fep->hwp+FEC_ECNTRL);udelay(10);+/*+*enet-macresetwillresetmacaddressregisterstoo,+*soneedtoreconfigureit.+*/+if(id_entry->driver_data&FEC_QUIRK_ENET_MAC){+memcpy(&temp_mac,dev->dev_addr,ETH_ALEN);+writel(cpu_to_be32(temp_mac[0]),fep->hwp+FEC_ADDR_LOW);+writel(cpu_to_be32(temp_mac[1]),fep->hwp+FEC_ADDR_HIGH);+}+/* Clear any outstanding interrupt. */writel(0xffc00000,fep->hwp+FEC_IEVENT);
@@ -1200,20 +1290,45 @@ fec_restart(struct net_device *dev, int duplex)/* Set MII speed */writel(fep->phy_speed,fep->hwp+FEC_MII_SPEED);-#ifdef FEC_MIIGSK_ENR-if(fep->phy_interface==PHY_INTERFACE_MODE_RMII){-/* disable the gasket and wait */-writel(0,fep->hwp+FEC_MIIGSK_ENR);-while(readl(fep->hwp+FEC_MIIGSK_ENR)&4)-udelay(1);+/*+*Thephyinterfaceandspeedneedtogetconfigured+*differentlyonenet-mac.+*/+if(id_entry->driver_data&FEC_QUIRK_ENET_MAC){+val=readl(fep->hwp+FEC_R_CNTRL);-/* configure the gasket: RMII, 50 MHz, no loopback, no echo */-writel(1,fep->hwp+FEC_MIIGSK_CFGR);+/* MII or RMII */+if(fep->phy_interface==PHY_INTERFACE_MODE_RMII)+val|=(1<<8);+else+val&=~(1<<8);-/* re-enable the gasket */-writel(2,fep->hwp+FEC_MIIGSK_ENR);-}+/* 10M or 100M */+if(fep->phy_dev&&fep->phy_dev->speed==SPEED_100)+val&=~(1<<9);+else+val|=(1<<9);++writel(val,fep->hwp+FEC_R_CNTRL);+}else{+#ifdef FEC_MIIGSK_ENR+if(fep->phy_interface==PHY_INTERFACE_MODE_RMII){+/* disable the gasket and wait */+writel(0,fep->hwp+FEC_MIIGSK_ENR);+while(readl(fep->hwp+FEC_MIIGSK_ENR)&4)+udelay(1);++/*+*configurethegasket:+*RMII,50MHz,noloopback,noecho+*/+writel(1,fep->hwp+FEC_MIIGSK_CFGR);++/* re-enable the gasket */+writel(2,fep->hwp+FEC_MIIGSK_ENR);+}#endif+}/* And last, enable the transmit and receive processing */writel(2,fep->hwp+FEC_ECNTRL);
@@ -78,7 +79,7 @@/**Definethebufferdescriptorstructure.*/-#ifdef CONFIG_ARCH_MXC+#if defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28)structbufdesc{unsignedshortcbd_datlen;/* Data length */unsignedshortcbd_sc;/* Control and status info */
Change device name from "fec" to "imx28-fec", so that fec driver
can distinguish mx28.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v4:
- Use "imx28-fec" as fec device name
Changes for v3:
- Change device name to "enet-mac"
arch/arm/mach-mxs/clock-mx28.c | 3 ++-
arch/arm/mach-mxs/devices/platform-fec.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -0,0 +1,79 @@+/*+*Copyright2010FreescaleSemiconductor,Inc.AllRightsReserved.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseaspublishedby+*theFreeSoftwareFoundation;eitherversion2oftheLicense,or+*(atyouroption)anylaterversion.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*/++#include<linux/delay.h>+#include<linux/err.h>+#include<linux/mutex.h>++#include<mach/mxs.h>++#define BM_OCOTP_CTRL_BUSY (1 << 8)+#define BM_OCOTP_CTRL_ERROR (1 << 9)+#define BM_OCOTP_CTRL_RD_BANK_OPEN (1 << 12)++staticDEFINE_MUTEX(ocotp_mutex);++intmxs_read_ocotp(unsignedoffset,size_tcount,u32*values)+{+void__iomem*ocotp_base=MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);+inttimeout=0x400;+size_ti;++mutex_lock(&ocotp_mutex);++/*+*clk_enable(hbus_clk)forocotpcanbeskipped+*asitmustbeonwhensystemisrunning.+*/++/* try to clear ERROR bit */+__mxs_clrl(BM_OCOTP_CTRL_ERROR,ocotp_base);++/* check both BUSY and ERROR cleared */+while((__raw_readl(ocotp_base)&+(BM_OCOTP_CTRL_BUSY|BM_OCOTP_CTRL_ERROR))&&--timeout)+cpu_relax();++if(unlikely(!timeout))+gotoerror_unlock;++/* open OCOTP banks for read */+__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN,ocotp_base);++/* approximately wait 32 hclk cycles */+udelay(1);++/* poll BUSY bit becoming cleared */+timeout=0x400;+while((__raw_readl(ocotp_base)&BM_OCOTP_CTRL_BUSY)&&--timeout)+cpu_relax();++if(unlikely(!timeout))+gotoerror_unlock;++for(i=0;i<count;i++,offset+=4)+*values++=__raw_readl(ocotp_base+offset);++/* close banks for power saving */+__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN,ocotp_base);++mutex_unlock(&ocotp_mutex);++return0;++error_unlock:+mutex_unlock(&ocotp_mutex);+pr_err("%s: timeout in reading OCOTP\n",__func__);+return-ETIMEDOUT;+}
Read fec mac address from ocotp and save it into fec_platform_data
mac field for fec driver to use.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v2:
- It's not necessary to remove "const" for fec_platform_data from
platform-fec.c and devices-common.h, so add it back.
- Hard-coding Freescale OUI (00:04:9f) instead of just the first
two two octets.
- Correct the return of mx28evk_fec_get_mac() and check it
with caller
arch/arm/mach-mxs/mach-mx28evk.c | 32 ++++++++++++++++++++++++++++++++
1 files changed, 32 insertions(+), 0 deletions(-)
@@ -129,12 +129,44 @@ static struct fec_platform_data mx28_fec_pdata[] = {},};+staticint__initmx28evk_fec_get_mac(void)+{+inti,ret;+u32val;++/*+*OCOTPonlystoresthelast4octetsforeachmacaddress,+*sohard-codeFreescaleOUI(00:04:9f)here.+*/+for(i=0;i<2;i++){+ret=mxs_read_ocotp(0x20+i*0x10,1,&val);+if(ret)+gotoerror;++mx28_fec_pdata[i].mac[0]=0x00;+mx28_fec_pdata[i].mac[1]=0x04;+mx28_fec_pdata[i].mac[2]=0x9f;+mx28_fec_pdata[i].mac[3]=(val>>16)&0xff;+mx28_fec_pdata[i].mac[4]=(val>>8)&0xff;+mx28_fec_pdata[i].mac[5]=(val>>0)&0xff;+}++return0;++error:+pr_err("%s: timeout when reading fec mac from OCOTP\n",__func__);+returnret;+}+staticvoid__initmx28evk_init(void){mxs_iomux_setup_multiple_pads(mx28evk_pads,ARRAY_SIZE(mx28evk_pads));mx28_add_duart();+if(mx28evk_fec_get_mac())+pr_warn("%s: failed on fec mac setup\n",__func__);+mx28evk_fec_reset();mx28_add_fec(0,&mx28_fec_pdata[0]);#ifdef CONFIG_FEC2
This is a very initial pm support and basically does nothing.
With this pm support entry, drivers can start testing their own
pm functions.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v2:
- Let build of pm.c depend on CONFIG_PM
- Remove the blank line above device_initcall in pm.c
arch/arm/mach-mxs/Makefile | 2 ++
arch/arm/mach-mxs/pm.c | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-mxs/pm.c
This patch series is to add dual fec support for mx28, which is
a mxs-based soc. Some code changes related to the following commits
are also made in this patch set for some reasons.
e6b043d512fa8d9a3801bf5d72bfa3b8fc3b3cc8
netdev/fec.c: add phylib supporting to enable carrier detection (v2)
e3fe8558c7fc182972c3d947d88744482111f304
net/fec: fix pm to survive to suspend/resume
It's been tested on mx28 evk and mx51 babbage. For mx28, it has
to work against the tree
git://git.pengutronix.de/git/imx/linux-2.6.git imx-for-2.6.38
plus patch
[PATCH v4] ARM: mxs: Change duart device to use amba-pl011
The 3 patches below preceding with * have changes since v3, and
the detailed change log can be found in individual patch.
I've applied all of the "net/fec:" patches (#1 to #5) to net-2.6,
please push the ARM changes via the appropriate ARM tree.
Thanks.
This patch series is to add dual fec support for mx28, which is
a mxs-based soc. Some code changes related to the following commits
are also made in this patch set for some reasons.
e6b043d512fa8d9a3801bf5d72bfa3b8fc3b3cc8
netdev/fec.c: add phylib supporting to enable carrier detection (v2)
e3fe8558c7fc182972c3d947d88744482111f304
net/fec: fix pm to survive to suspend/resume
It's been tested on mx28 evk and mx51 babbage. For mx28, it has
to work against the tree
git://git.pengutronix.de/git/imx/linux-2.6.git imx-for-2.6.38
plus patch
[PATCH v4] ARM: mxs: Change duart device to use amba-pl011
The 3 patches below preceding with * have changes since v3, and
the detailed change log can be found in individual patch.
I've applied all of the "net/fec:" patches (#1 to #5) to net-2.6,
please push the ARM changes via the appropriate ARM tree.
Thanks.
Thanks, David. I will ping Sascha for ARM changes.
--
Regards,
Shawn
This patch series is to add dual fec support for mx28, which is
a mxs-based soc. Some code changes related to the following commits
are also made in this patch set for some reasons.
e6b043d512fa8d9a3801bf5d72bfa3b8fc3b3cc8
netdev/fec.c: add phylib supporting to enable carrier detection (v2)
e3fe8558c7fc182972c3d947d88744482111f304
net/fec: fix pm to survive to suspend/resume
It's been tested on mx28 evk and mx51 babbage. For mx28, it has
to work against the tree
git://git.pengutronix.de/git/imx/linux-2.6.git imx-for-2.6.38
plus patch
[PATCH v4] ARM: mxs: Change duart device to use amba-pl011
The 3 patches below preceding with * have changes since v3, and
the detailed change log can be found in individual patch.
I've applied all of the "net/fec:" patches (#1 to #5) to net-2.6,
please push the ARM changes via the appropriate ARM tree.
Is there anything needs to be done to get the following patches
merged?
[PATCH v4] ARM: mxs: Change duart device to use amba-pl011
[PATCH v4 00/10] net/fec: add dual fec support for i.MX28, #6 to #10.
--
Regards,
Shawn
On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote:
quoted hunk
This patch is to add mx28 dual fec support. Here are some key notes
for mx28 fec controller.
- The mx28 fec controller naming ENET-MAC is a different IP from FEC
used on other i.mx variants. But they are basically compatible
on software interface, so it's possible to share the same driver.
- ENET-MAC design on mx28 made an improper assumption that it runs
on a big-endian system. As the result, driver has to swap every
frame going to and coming from the controller.
- The external phys can only be configured by fec0, which means fec1
can not work independently and both phys need to be configured by
mii_bus attached on fec0.
- ENET-MAC reset will get mac address registers reset too.
- ENET-MAC MII/RMII mode and 10M/100M speed are configured
differently FEC.
- ETHER_EN bit must be set to get ENET-MAC interrupt work.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v4:
- Use #ifndef CONFIG_ARM to include ColdFire header files
- Define quirk bits in id_entry.driver_data to handle controller
difference, which is more scalable than using device name
- Define fec0_mii_bus as a static function in fec_enet_mii_init
to fold the mii_bus instance attached on fec0
- Use cpu_to_be32 over __swab32 in function swap_buffer
Changes for v3:
- Move v2 changes into patch #3
- Use device name to check if it's running on ENET-MAC
drivers/net/Kconfig | 7 ++-
drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------
drivers/net/fec.h | 5 +-
3 files changed, 139 insertions(+), 21 deletions(-)
@@ -1944,18 +1944,19 @@ config 68360_ENETconfigFECbool"FEC ethernet controller (of ColdFire and some i.MX CPUs)"depends onM523x||M527x||M5272||M528x||M520x||M532x||\-MACH_MX27||ARCH_MX35||ARCH_MX25||ARCH_MX5+MACH_MX27||ARCH_MX35||ARCH_MX25||ARCH_MX5||SOC_IMX28selectPHYLIBhelpSayYhereifyouwanttousethebuilt-in10/100FastethernetcontrolleronsomeMotorolaColdFireandFreescalei.MXprocessors.configFEC2-bool"Second FEC ethernet controller (on some ColdFire CPUs)"+bool"Second FEC ethernet controller"depends onFEChelpSayYhereifyouwanttousethesecondbuilt-in10/100Fast-ethernetcontrolleronsomeMotorolaColdFireprocessors.+ethernetcontrolleronsomeMotorolaColdFireandFreescale+i.MXprocessors.
This option is used nowhere and should be removed. Certainly it does not
have the effect of enabling the second ethernet controller.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Please don't do this. If you really want to make this configurable with
kconfig use a board specific option, not a driver specific option. I
think this should be made unconditional though.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Jan 11, 2011 at 11:27:17AM +0100, Sascha Hauer wrote:
On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote:
quoted
This patch is to add mx28 dual fec support. Here are some key notes
for mx28 fec controller.
- The mx28 fec controller naming ENET-MAC is a different IP from FEC
used on other i.mx variants. But they are basically compatible
on software interface, so it's possible to share the same driver.
- ENET-MAC design on mx28 made an improper assumption that it runs
on a big-endian system. As the result, driver has to swap every
frame going to and coming from the controller.
- The external phys can only be configured by fec0, which means fec1
can not work independently and both phys need to be configured by
mii_bus attached on fec0.
- ENET-MAC reset will get mac address registers reset too.
- ENET-MAC MII/RMII mode and 10M/100M speed are configured
differently FEC.
- ETHER_EN bit must be set to get ENET-MAC interrupt work.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v4:
- Use #ifndef CONFIG_ARM to include ColdFire header files
- Define quirk bits in id_entry.driver_data to handle controller
difference, which is more scalable than using device name
- Define fec0_mii_bus as a static function in fec_enet_mii_init
to fold the mii_bus instance attached on fec0
- Use cpu_to_be32 over __swab32 in function swap_buffer
Changes for v3:
- Move v2 changes into patch #3
- Use device name to check if it's running on ENET-MAC
drivers/net/Kconfig | 7 ++-
drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------
drivers/net/fec.h | 5 +-
3 files changed, 139 insertions(+), 21 deletions(-)
Please don't do this. If you really want to make this configurable with
kconfig use a board specific option, not a driver specific option. I
think this should be made unconditional though.
I will resend this patch as v5 than the whole patch set, if you
do not mind.
--
Regards,
Shawn
Please don't do this. If you really want to make this configurable with
kconfig use a board specific option, not a driver specific option. I
think this should be made unconditional though.
I will resend this patch as v5 than the whole patch set, if you
do not mind.
ok
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Signed-off-by: Shawn Guo <redacted>
---
Changes for v5:
- Do not use CONFIG_FEC2 which is a fec driver configration
arch/arm/mach-mxs/mach-mx28evk.c | 26 +++++++++++++++++++++++---
1 files changed, 23 insertions(+), 3 deletions(-)
On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote:
quoted
This patch is to add mx28 dual fec support. Here are some key notes
for mx28 fec controller.
- The mx28 fec controller naming ENET-MAC is a different IP from FEC
used on other i.mx variants. But they are basically compatible
on software interface, so it's possible to share the same driver.
- ENET-MAC design on mx28 made an improper assumption that it runs
on a big-endian system. As the result, driver has to swap every
frame going to and coming from the controller.
- The external phys can only be configured by fec0, which means fec1
can not work independently and both phys need to be configured by
mii_bus attached on fec0.
- ENET-MAC reset will get mac address registers reset too.
- ENET-MAC MII/RMII mode and 10M/100M speed are configured
differently FEC.
- ETHER_EN bit must be set to get ENET-MAC interrupt work.
Signed-off-by: Shawn Guo<redacted>
---
Changes for v4:
- Use #ifndef CONFIG_ARM to include ColdFire header files
- Define quirk bits in id_entry.driver_data to handle controller
difference, which is more scalable than using device name
- Define fec0_mii_bus as a static function in fec_enet_mii_init
to fold the mii_bus instance attached on fec0
- Use cpu_to_be32 over __swab32 in function swap_buffer
Changes for v3:
- Move v2 changes into patch #3
- Use device name to check if it's running on ENET-MAC
drivers/net/Kconfig | 7 ++-
drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------
drivers/net/fec.h | 5 +-
3 files changed, 139 insertions(+), 21 deletions(-)
Hi Greg,
On Tue, Jan 11, 2011 at 10:24:12PM +1000, Greg Ungerer wrote:
On 11/01/11 20:27, Sascha Hauer wrote:
quoted
On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote:
This option is used nowhere and should be removed. Certainly it does not
have the effect of enabling the second ethernet controller.
It does for a ColdFire platform...
grep -r CONFIG_FEC2 *
arch/m68knommu/configs/m5275evb_defconfig:CONFIG_FEC2=y
arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2
arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2
Hi Uwe,
On 11/01/11 23:07, Uwe Kleine-K?nig wrote:
Hi Greg,
On Tue, Jan 11, 2011 at 10:24:12PM +1000, Greg Ungerer wrote:
quoted
On 11/01/11 20:27, Sascha Hauer wrote:
quoted
On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote:
This option is used nowhere and should be removed. Certainly it does not
have the effect of enabling the second ethernet controller.
It does for a ColdFire platform...
grep -r CONFIG_FEC2 *
arch/m68knommu/configs/m5275evb_defconfig:CONFIG_FEC2=y
arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2
arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2
IMHO Sascha's comment[1] applies here, too.
I am not arguing that it doesn't :-)
Simply that removing that config option and doing nothing
else would not be the right thing to do.
Regards
Greg
This operation does not try to clear the error bit but actually clears
it...
+
+ /* check both BUSY and ERROR cleared */
+ while ((__raw_readl(ocotp_base) &
+ (BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
+ cpu_relax();
...which means you do not have to poll the error bit here...
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ /* open OCOTP banks for read */
+ __mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
+
+ /* approximately wait 32 hclk cycles */
+ udelay(1);
+
+ /* poll BUSY bit becoming cleared */
+ timeout = 0x400;
+ while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
+ cpu_relax();
...which means you can factor out a ocotp_wait_busy function and let the
code speak instead of the comments.
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ for (i = 0; i < count; i++, offset += 4)
+ *values++ = __raw_readl(ocotp_base + offset);
The registers in the ocotp are 16 byte aligned. Does it really make
sense to provide a function allowing to read the gaps between the
registers?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
This operation does not try to clear the error bit but actually clears
it...
quoted
+
+ /* check both BUSY and ERROR cleared */
+ while ((__raw_readl(ocotp_base) &
+ (BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
+ cpu_relax();
...which means you do not have to poll the error bit here...
well, I don't know how the hardware works here, but in general the
argument is broken. Registers are not memory, so just because you set a
bit in register space it doesn't mean it is really set when you read
from the same address.
If there is something wrong with the ocotp I'd even expect that clearing
the error bit doesn't work because it doesn't change the general
condition.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
This operation does not try to clear the error bit but actually clears
it...
quoted
+
+ /* check both BUSY and ERROR cleared */
+ while ((__raw_readl(ocotp_base) &
+ (BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
+ cpu_relax();
...which means you do not have to poll the error bit here...
quoted
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ /* open OCOTP banks for read */
+ __mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
+
+ /* approximately wait 32 hclk cycles */
+ udelay(1);
+
+ /* poll BUSY bit becoming cleared */
+ timeout = 0x400;
+ while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
+ cpu_relax();
...which means you can factor out a ocotp_wait_busy function and let the
code speak instead of the comments.
quoted
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ for (i = 0; i < count; i++, offset += 4)
+ *values++ = __raw_readl(ocotp_base + offset);
The registers in the ocotp are 16 byte aligned. Does it really make
sense to provide a function allowing to read the gaps between the
registers?
Good catch. The count was added to ease the consecutive otp word
reading, as there is bank open/close cost for otp read. What about
the following changes?
int mxs_read_ocotp(unsigned offset, size_t otp_word_cnt, u32 *values)
{
......
for (i = 0; i < otp_word_cnt; i++, offset += 0x10)
*values++ = __raw_readl(ocotp_base + offset);
......
}
--
Regards,
Shawn
Hi Greg,
On Tue, Jan 11, 2011 at 11:25:41PM +1000, Greg Ungerer wrote:
On 11/01/11 23:07, Uwe Kleine-K?nig wrote:
quoted
On Tue, Jan 11, 2011 at 10:24:12PM +1000, Greg Ungerer wrote:
quoted
On 11/01/11 20:27, Sascha Hauer wrote:
quoted
On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote:
This option is used nowhere and should be removed. Certainly it does not
have the effect of enabling the second ethernet controller.
It does for a ColdFire platform...
grep -r CONFIG_FEC2 *
arch/m68knommu/configs/m5275evb_defconfig:CONFIG_FEC2=y
arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2
arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2
IMHO Sascha's comment[1] applies here, too.
I am not arguing that it doesn't :-)
Simply that removing that config option and doing nothing
else would not be the right thing to do.
Regards
Greg
quoted
And someone might want to do what he suggested soon or the patch
removing CONFIG_FEC2[2] needs to be commented accordingly.
note that davem took the patch removing CONFIG_FEC2 now.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
This operation does not try to clear the error bit but actually clears
it...
quoted
+
+ /* check both BUSY and ERROR cleared */
+ while ((__raw_readl(ocotp_base) &
+ (BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
+ cpu_relax();
...which means you do not have to poll the error bit here...
quoted
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ /* open OCOTP banks for read */
+ __mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
+
+ /* approximately wait 32 hclk cycles */
+ udelay(1);
+
+ /* poll BUSY bit becoming cleared */
+ timeout = 0x400;
+ while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
+ cpu_relax();
...which means you can factor out a ocotp_wait_busy function and let the
code speak instead of the comments.
quoted
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ for (i = 0; i < count; i++, offset += 4)
+ *values++ = __raw_readl(ocotp_base + offset);
The registers in the ocotp are 16 byte aligned. Does it really make
sense to provide a function allowing to read the gaps between the
registers?
Good catch. The count was added to ease the consecutive otp word
reading, as there is bank open/close cost for otp read. What about
the following changes?
int mxs_read_ocotp(unsigned offset, size_t otp_word_cnt, u32 *values)
{
......
for (i = 0; i < otp_word_cnt; i++, offset += 0x10)
*values++ = __raw_readl(ocotp_base + offset);
......
}
I would rather make a function like this:
static u32 ocotp[0x27];
const u32 *mxs_get_ocotp(void)
{
static int once = 0;
if (once)
return ocotp
/* bank open */
for (i = 0; i < 0x27; i++)
ocotp[i] = readl(ocotp_base + 0x20 + i * 0x10)
/* bank_close */
once = 1;
return ocotp;
}
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
This operation does not try to clear the error bit but actually clears
it...
quoted
+
+ /* check both BUSY and ERROR cleared */
+ while ((__raw_readl(ocotp_base) &
+ (BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
+ cpu_relax();
...which means you do not have to poll the error bit here...
quoted
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ /* open OCOTP banks for read */
+ __mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
+
+ /* approximately wait 32 hclk cycles */
+ udelay(1);
+
+ /* poll BUSY bit becoming cleared */
+ timeout = 0x400;
+ while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
+ cpu_relax();
...which means you can factor out a ocotp_wait_busy function and let the
code speak instead of the comments.
quoted
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ for (i = 0; i < count; i++, offset += 4)
+ *values++ = __raw_readl(ocotp_base + offset);
The registers in the ocotp are 16 byte aligned. Does it really make
sense to provide a function allowing to read the gaps between the
registers?
Good catch. The count was added to ease the consecutive otp word
reading, as there is bank open/close cost for otp read. What about
the following changes?
int mxs_read_ocotp(unsigned offset, size_t otp_word_cnt, u32 *values)
{
......
for (i = 0; i < otp_word_cnt; i++, offset += 0x10)
*values++ = __raw_readl(ocotp_base + offset);
......
}
I would rather make a function like this:
static u32 ocotp[0x27];
const u32 *mxs_get_ocotp(void)
{
static int once = 0;
if (once)
return ocotp
/* bank open */
for (i = 0; i < 0x27; i++)
ocotp[i] = readl(ocotp_base + 0x20 + i * 0x10)
/* bank_close */
once = 1;
return ocotp;
which is save on UP when it's not called from irq context.
Additionally I suggest a #define for 0x27 and 0x20.
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
This operation does not try to clear the error bit but actually clears
it...
quoted
+
+ /* check both BUSY and ERROR cleared */
+ while ((__raw_readl(ocotp_base) &
+ (BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
+ cpu_relax();
...which means you do not have to poll the error bit here...
quoted
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ /* open OCOTP banks for read */
+ __mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
+
+ /* approximately wait 32 hclk cycles */
+ udelay(1);
+
+ /* poll BUSY bit becoming cleared */
+ timeout = 0x400;
+ while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
+ cpu_relax();
...which means you can factor out a ocotp_wait_busy function and let the
code speak instead of the comments.
quoted
+
+ if (unlikely(!timeout))
+ goto error_unlock;
+
+ for (i = 0; i < count; i++, offset += 4)
+ *values++ = __raw_readl(ocotp_base + offset);
The registers in the ocotp are 16 byte aligned. Does it really make
sense to provide a function allowing to read the gaps between the
registers?
Good catch. The count was added to ease the consecutive otp word
reading, as there is bank open/close cost for otp read. What about
the following changes?
int mxs_read_ocotp(unsigned offset, size_t otp_word_cnt, u32 *values)
{
......
for (i = 0; i < otp_word_cnt; i++, offset += 0x10)
*values++ = __raw_readl(ocotp_base + offset);
......
}
I would rather make a function like this:
static u32 ocotp[0x27];
const u32 *mxs_get_ocotp(void)
{
static int once = 0;
if (once)
return ocotp
/* bank open */
for (i = 0; i < 0x27; i++)
ocotp[i] = readl(ocotp_base + 0x20 + i * 0x10)
/* bank_close */
once = 1;
return ocotp;
which is save on UP when it's not called from irq context.
Additionally I suggest a #define for 0x27 and 0x20.
So I will keep the mutex and not read SRK bits.
Thanks for the comments.
--
Regards,
Shawn
Hello,
hmm, this review comes to late, probably I will post a follow-up patch
for the low-hanging fruits at least.
On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote:
This patch is to add mx28 dual fec support. Here are some key notes
for mx28 fec controller.
- The mx28 fec controller naming ENET-MAC is a different IP from FEC
used on other i.mx variants. But they are basically compatible
on software interface, so it's possible to share the same driver.
- ENET-MAC design on mx28 made an improper assumption that it runs
on a big-endian system. As the result, driver has to swap every
frame going to and coming from the controller.
- The external phys can only be configured by fec0, which means fec1
can not work independently and both phys need to be configured by
mii_bus attached on fec0.
- ENET-MAC reset will get mac address registers reset too.
- ENET-MAC MII/RMII mode and 10M/100M speed are configured
differently FEC.
- ETHER_EN bit must be set to get ENET-MAC interrupt work.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v4:
- Use #ifndef CONFIG_ARM to include ColdFire header files
I intended you to not use CONFIG_ARCH_MXS at all, at least up to now
CONFIG_ARM works quite well.
- Define quirk bits in id_entry.driver_data to handle controller
difference, which is more scalable than using device name
- Define fec0_mii_bus as a static function in fec_enet_mii_init
to fold the mii_bus instance attached on fec0
IMHO not very good. At least the current code doesn't allow to have two
dual-fecs, because the 2nd dual-fec's slave would be attached to the 1st
dual-fec's mii_bus. Don't know a nice solution though. Probably you
either need a slave pointer in platform_data or have to treat a dual-fec
as only a single device.
quoted hunk
- Use cpu_to_be32 over __swab32 in function swap_buffer
Changes for v3:
- Move v2 changes into patch #3
- Use device name to check if it's running on ENET-MAC
drivers/net/Kconfig | 7 ++-
drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------
drivers/net/fec.h | 5 +-
3 files changed, 139 insertions(+), 21 deletions(-)
@@ -1944,18 +1944,19 @@ config 68360_ENETconfigFECbool"FEC ethernet controller (of ColdFire and some i.MX CPUs)"depends onM523x||M527x||M5272||M528x||M520x||M532x||\-MACH_MX27||ARCH_MX35||ARCH_MX25||ARCH_MX5+MACH_MX27||ARCH_MX35||ARCH_MX25||ARCH_MX5||SOC_IMX28
IMX_HAVE_PLATFORM_FEC || MXS_HAVE_PLATFORM_FEC ? Again this calls for a
more global approach for these registration facilities.
quoted hunk
select PHYLIB
help
Say Y here if you want to use the built-in 10/100 Fast ethernet
controller on some Motorola ColdFire and Freescale i.MX processors.
config FEC2
- bool "Second FEC ethernet controller (on some ColdFire CPUs)"
+ bool "Second FEC ethernet controller"
depends on FEC
help
Say Y here if you want to use the second built-in 10/100 Fast
- ethernet controller on some Motorola ColdFire processors.
+ ethernet controller on some Motorola ColdFire and Freescale
+ i.MX processors.
config FEC_MPC52xx
tristate "MPC52xx FEC driver"
I wonder what is excluded here. FEC depends on
M523x || M527x || M5272 || M528x || M520x || M532x || \
MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28
so the only difference is that the latter lists M5272 which seems a bit
redundant in the presence of M527x.
@@ -208,10 +227,23 @@ static void fec_stop(struct net_device *dev); /* Transmitter timeout */ #define TX_TIMEOUT (2 * HZ)+static void *swap_buffer(void *bufaddr, int len)+{+ int i;+ unsigned int *buf = bufaddr;++ for (i = 0; i < (len + 3) / 4; i++, buf++)+ *buf = cpu_to_be32(*buf);
if len isn't a multiple of 4 this accesses bytes behind len. Is this
generally OK here? (E.g. because skbs always have a length that is a
multiple of 4?)
@@ -256,6 +288,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev) bufaddr = fep->tx_bounce[index]; }+ /*+ * Some design made an incorrect assumption on endian mode of+ * the system that it's running on. As the result, driver has to+ * swap every frame going to and coming from the controller.+ */+ if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)+ swap_buffer(bufaddr, skb->len);+ /* Save skb pointer */ fep->tx_skbuff[fep->skb_cur] = skb;
@@ -487,6 +529,9 @@ fec_enet_rx(struct net_device *dev) dma_unmap_single(NULL, bdp->cbd_bufaddr, bdp->cbd_datlen, DMA_FROM_DEVICE);+ if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)+ swap_buffer(data, pkt_len);+ /* This does 16 byte alignment, exactly what we need. * The packet length includes FCS, but we don't want to * include that when passing upstream as it messes up
@@ -689,6 +734,7 @@ static int fec_enet_mii_probe(struct net_device *dev) char mdio_bus_id[MII_BUS_ID_SIZE]; char phy_name[MII_BUS_ID_SIZE + 3]; int phy_id;+ int dev_id = fep->pdev->id; fep->phy_dev = NULL;
@@ -700,6 +746,8 @@ static int fec_enet_mii_probe(struct net_device *dev) continue; if (fep->mii_bus->phy_map[phy_id]->phy_id == 0) continue;+ if (dev_id--)+ continue; strncpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE); break; }
@@ -737,10 +785,35 @@ static int fec_enet_mii_probe(struct net_device *dev) static int fec_enet_mii_init(struct platform_device *pdev) {+ static struct mii_bus *fec0_mii_bus; struct net_device *dev = platform_get_drvdata(pdev); struct fec_enet_private *fep = netdev_priv(dev);+ const struct platform_device_id *id_entry =+ platform_get_device_id(fep->pdev); int err = -ENXIO, i;+ /*+ * The dual fec interfaces are not equivalent with enet-mac.+ * Here are the differences:+ *+ * - fec0 supports MII & RMII modes while fec1 only supports RMII+ * - fec0 acts as the 1588 time master while fec1 is slave+ * - external phys can only be configured by fec0+ *+ * That is to say fec1 can not work independently. It only works+ * when fec0 is working. The reason behind this design is that the+ * second interface is added primarily for Switch mode.+ *+ * Because of the last point above, both phys are attached on fec0+ * mdio interface in board design, and need to be configured by+ * fec0 mii_bus.+ */+ if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id) {+ /* fec1 uses fec0 mii_bus */+ fep->mii_bus = fec0_mii_bus;+ return 0;
What happens if imx28-fec.1 is probed before imx28-fec.0?
quoted hunk
+ }
+
fep->mii_timeout = 0;
/*
@@ -777,6 +850,10 @@ static int fec_enet_mii_init(struct platform_device *pdev) if (mdiobus_register(fep->mii_bus)) goto err_out_free_mdio_irq;+ /* save fec0 mii_bus */+ if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)+ fec0_mii_bus = fep->mii_bus;+ return 0; err_out_free_mdio_irq:
@@ -1148,12 +1225,25 @@ static void fec_restart(struct net_device *dev, int duplex) { struct fec_enet_private *fep = netdev_priv(dev);+ const struct platform_device_id *id_entry =+ platform_get_device_id(fep->pdev); int i;+ u32 val, temp_mac[2]; /* Whack a reset. We should wait for this. */ writel(1, fep->hwp + FEC_ECNTRL); udelay(10);+ /*+ * enet-mac reset will reset mac address registers too,+ * so need to reconfigure it.+ */+ if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {+ memcpy(&temp_mac, dev->dev_addr, ETH_ALEN);+ writel(cpu_to_be32(temp_mac[0]), fep->hwp + FEC_ADDR_LOW);+ writel(cpu_to_be32(temp_mac[1]), fep->hwp + FEC_ADDR_HIGH);+ }+ /* Clear any outstanding interrupt. */ writel(0xffc00000, fep->hwp + FEC_IEVENT);
@@ -1200,20 +1290,45 @@ fec_restart(struct net_device *dev, int duplex) /* Set MII speed */ writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);-#ifdef FEC_MIIGSK_ENR- if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) {- /* disable the gasket and wait */- writel(0, fep->hwp + FEC_MIIGSK_ENR);- while (readl(fep->hwp + FEC_MIIGSK_ENR) & 4)- udelay(1);+ /*+ * The phy interface and speed need to get configured+ * differently on enet-mac.+ */+ if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {+ val = readl(fep->hwp + FEC_R_CNTRL);- /* configure the gasket: RMII, 50 MHz, no loopback, no echo */- writel(1, fep->hwp + FEC_MIIGSK_CFGR);+ /* MII or RMII */+ if (fep->phy_interface == PHY_INTERFACE_MODE_RMII)+ val |= (1 << 8);
Can we have a #define for 1 << 8 please?
+ else
+ val &= ~(1 << 8);
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Tue, Jan 11, 2011 at 08:09:24PM +0800, Shawn Guo wrote:
quoted hunk
Signed-off-by: Shawn Guo <redacted>
---
Changes for v5:
- Do not use CONFIG_FEC2 which is a fec driver configration
arch/arm/mach-mxs/mach-mx28evk.c | 26 +++++++++++++++++++++++---
1 files changed, 23 insertions(+), 3 deletions(-)
Hello Shawn,
$SUBJECT ~= s,mx28,mxs/mx28evk, please
On Thu, Jan 06, 2011 at 03:13:17PM +0800, Shawn Guo wrote:
quoted hunk
Read fec mac address from ocotp and save it into fec_platform_data
mac field for fec driver to use.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v2:
- It's not necessary to remove "const" for fec_platform_data from
platform-fec.c and devices-common.h, so add it back.
- Hard-coding Freescale OUI (00:04:9f) instead of just the first
two two octets.
- Correct the return of mx28evk_fec_get_mac() and check it
with caller
arch/arm/mach-mxs/mach-mx28evk.c | 32 ++++++++++++++++++++++++++++++++
1 files changed, 32 insertions(+), 0 deletions(-)
@@ -129,12 +129,44 @@ static struct fec_platform_data mx28_fec_pdata[] = {},};+staticint__initmx28evk_fec_get_mac(void)+{+inti,ret;+u32val;++/*+*OCOTPonlystoresthelast4octetsforeachmacaddress,+*sohard-codeFreescaleOUI(00:04:9f)here.+*/+for(i=0;i<2;i++){+ret=mxs_read_ocotp(0x20+i*0x10,1,&val);+if(ret)+gotoerror;++mx28_fec_pdata[i].mac[0]=0x00;+mx28_fec_pdata[i].mac[1]=0x04;+mx28_fec_pdata[i].mac[2]=0x9f;+mx28_fec_pdata[i].mac[3]=(val>>16)&0xff;+mx28_fec_pdata[i].mac[4]=(val>>8)&0xff;+mx28_fec_pdata[i].mac[5]=(val>>0)&0xff;+}++return0;++error:+pr_err("%s: timeout when reading fec mac from OCOTP\n",__func__);+returnret;+}+staticvoid__initmx28evk_init(void){mxs_iomux_setup_multiple_pads(mx28evk_pads,ARRAY_SIZE(mx28evk_pads));mx28_add_duart();+if(mx28evk_fec_get_mac())+pr_warn("%s: failed on fec mac setup\n",__func__);+mx28evk_fec_reset();mx28_add_fec(0,&mx28_fec_pdata[0]);#ifdef CONFIG_FEC2
Hi Shawn,
On Thu, Jan 06, 2011 at 03:13:14PM +0800, Shawn Guo wrote:
quoted hunk
Change device name from "fec" to "imx28-fec", so that fec driver
can distinguish mx28.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v4:
- Use "imx28-fec" as fec device name
Changes for v3:
- Change device name to "enet-mac"
arch/arm/mach-mxs/clock-mx28.c | 3 ++-
arch/arm/mach-mxs/devices/platform-fec.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -0,0 +1,79 @@+/*+*Copyright2010FreescaleSemiconductor,Inc.AllRightsReserved.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseaspublishedby+*theFreeSoftwareFoundation;eitherversion2oftheLicense,or+*(atyouroption)anylaterversion.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*/++#include<linux/delay.h>+#include<linux/err.h>+#include<linux/mutex.h>++#include<mach/mxs.h>++#define BM_OCOTP_CTRL_BUSY (1 << 8)+#define BM_OCOTP_CTRL_ERROR (1 << 9)+#define BM_OCOTP_CTRL_RD_BANK_OPEN (1 << 12)++staticDEFINE_MUTEX(ocotp_mutex);++intmxs_read_ocotp(unsignedoffset,size_tcount,u32*values)+{+void__iomem*ocotp_base=MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);+inttimeout=0x400;+size_ti;++mutex_lock(&ocotp_mutex);++/*+*clk_enable(hbus_clk)forocotpcanbeskipped+*asitmustbeonwhensystemisrunning.+*/++/* try to clear ERROR bit */+__mxs_clrl(BM_OCOTP_CTRL_ERROR,ocotp_base);++/* check both BUSY and ERROR cleared */+while((__raw_readl(ocotp_base)&+(BM_OCOTP_CTRL_BUSY|BM_OCOTP_CTRL_ERROR))&&--timeout)+cpu_relax();++if(unlikely(!timeout))+gotoerror_unlock;++/* open OCOTP banks for read */+__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN,ocotp_base);++/* approximately wait 32 hclk cycles */+udelay(1);++/* poll BUSY bit becoming cleared */+timeout=0x400;+while((__raw_readl(ocotp_base)&BM_OCOTP_CTRL_BUSY)&&--timeout)+cpu_relax();++if(unlikely(!timeout))+gotoerror_unlock;++for(i=0;i<count;i++,offset+=4)+*values++=__raw_readl(ocotp_base+offset);++/* close banks for power saving */+__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN,ocotp_base);++mutex_unlock(&ocotp_mutex);++return0;++error_unlock:+mutex_unlock(&ocotp_mutex);+pr_err("%s: timeout in reading OCOTP\n",__func__);+return-ETIMEDOUT;+}
@@ -256,6 +288,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev) bufaddr = fep->tx_bounce[index]; }+ /*+ * Some design made an incorrect assumption on endian mode of+ * the system that it's running on. As the result, driver has to+ * swap every frame going to and coming from the controller.+ */+ if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)+ swap_buffer(bufaddr, skb->len);+
Is that save here? bufaddr either points to a bounce buffer (which
should be OK definitely) or skb->data. Or asked differently: Is the
skb here owned by the driver such that it is allowed to write to it?
Does the driver eventually need to restore the original data?
Just before this if, there is some bounce buffer handling. If it is not
OK to modify skb->data, the call to swap_buffer can easily be moved in
there.
quoted hunk
/* Save skb pointer */
fep->tx_skbuff[fep->skb_cur] = skb;
Here I guess it's OK, the hardware just wrote to the buffer, so the skb
cannot be shared to anything else and the write is all right.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would
be more accurate.
When your make this change, you may want to pick a better name for
function swap_buffer too.
[...]
quoted
+static void *swap_buffer(void *bufaddr, int len)
+{
+ int i;
+ unsigned int *buf = bufaddr;
+
+ for (i = 0; i < (len + 3) / 4; i++, buf++)
+ *buf = cpu_to_be32(*buf);
if len isn't a multiple of 4 this accesses bytes behind len. Is this
generally OK here? (E.g. because skbs always have a length that is a
multiple of 4?)
The len may not be a multiple of 4. But I believe bufaddr is always
a buffer allocated in a length that is a multiple of 4, and the 1~3
bytes exceeding the len very likely has no data that matters. But
yes, it deserves a safer implementation.
[...]
quoted
+ /*
+ * The dual fec interfaces are not equivalent with enet-mac.
+ * Here are the differences:
+ *
+ * - fec0 supports MII & RMII modes while fec1 only supports RMII
+ * - fec0 acts as the 1588 time master while fec1 is slave
+ * - external phys can only be configured by fec0
+ *
+ * That is to say fec1 can not work independently. It only works
+ * when fec0 is working. The reason behind this design is that the
+ * second interface is added primarily for Switch mode.
+ *
+ * Because of the last point above, both phys are attached on fec0
+ * mdio interface in board design, and need to be configured by
+ * fec0 mii_bus.
+ */
+ if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id) {
+ /* fec1 uses fec0 mii_bus */
+ fep->mii_bus = fec0_mii_bus;
+ return 0;
What happens if imx28-fec.1 is probed before imx28-fec.0?
It's something that generally should not happen, as these two fec are
not equivalent, and fec.1 should always be added after fec.0 if you
intend to get dual interfaces. But yes, we should add error checking
for this case in the driver.
--
Regards,
Shawn
Hi Uwe,
On Thu, Jan 13, 2011 at 04:06:22PM +0100, Uwe Kleine-K?nig wrote:
Hi Shawn,
On Thu, Jan 06, 2011 at 03:13:14PM +0800, Shawn Guo wrote:
quoted
Change device name from "fec" to "imx28-fec", so that fec driver
can distinguish mx28.
Signed-off-by: Shawn Guo <redacted>
---
Changes for v4:
- Use "imx28-fec" as fec device name
Changes for v3:
- Change device name to "enet-mac"
arch/arm/mach-mxs/clock-mx28.c | 3 ++-
arch/arm/mach-mxs/devices/platform-fec.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
Hi Uwe,
On Thu, Jan 13, 2011 at 03:49:00PM +0100, Uwe Kleine-K?nig wrote:
On Tue, Jan 11, 2011 at 08:09:24PM +0800, Shawn Guo wrote:
quoted
Signed-off-by: Shawn Guo <redacted>
---
Changes for v5:
- Do not use CONFIG_FEC2 which is a fec driver configration
arch/arm/mach-mxs/mach-mx28evk.c | 26 +++++++++++++++++++++++---
1 files changed, 23 insertions(+), 3 deletions(-)
Sascha has merged the patch. This one line change may not deserve
a separate patch, so I will take care of it when updating the file
with new version of ocotp patch.
--
Regards,
Shawn
Read fec mac address from ocotp and save it into fec_platform_data
mac field for fec driver to use.
Signed-off-by: Shawn Guo <redacted>
---
arch/arm/mach-mxs/mach-mx28evk.c | 35 ++++++++++++++++++++++++++++++++++-
1 files changed, 34 insertions(+), 1 deletions(-)
@@ -129,12 +129,45 @@ static struct fec_platform_data mx28_fec_pdata[] = {},};+staticint__initmx28evk_fec_get_mac(void)+{+inti;+u32val;+constu32*ocotp=mxs_get_ocotp();++if(!ocotp)+gotoerror;++/*+*OCOTPonlystoresthelast4octetsforeachmacaddress,+*sohard-codeFreescaleOUI(00:04:9f)here.+*/+for(i=0;i<2;i++){+val=ocotp[i*4];+mx28_fec_pdata[i].mac[0]=0x00;+mx28_fec_pdata[i].mac[1]=0x04;+mx28_fec_pdata[i].mac[2]=0x9f;+mx28_fec_pdata[i].mac[3]=(val>>16)&0xff;+mx28_fec_pdata[i].mac[4]=(val>>8)&0xff;+mx28_fec_pdata[i].mac[5]=(val>>0)&0xff;+}++return0;++error:+pr_err("%s: timeout when reading fec mac from OCOTP\n",__func__);+return-ETIMEDOUT;+}+staticvoid__initmx28evk_init(void){mxs_iomux_setup_multiple_pads(mx28evk_pads,ARRAY_SIZE(mx28evk_pads));mx28_add_duart();+if(mx28evk_fec_get_mac())+pr_warn("%s: failed on fec mac setup\n",__func__);+mx28evk_fec_reset();mx28_add_fec(0,&mx28_fec_pdata[0]);mx28_add_fec(1,&mx28_fec_pdata[1]);
IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would
be more accurate.
When your make this change, you may want to pick a better name for
function swap_buffer too.
[...]
quoted
quoted
+static void *swap_buffer(void *bufaddr, int len)
+{
+ int i;
+ unsigned int *buf = bufaddr;
+
+ for (i = 0; i < (len + 3) / 4; i++, buf++)
+ *buf = cpu_to_be32(*buf);
if len isn't a multiple of 4 this accesses bytes behind len. Is this
generally OK here? (E.g. because skbs always have a length that is a
multiple of 4?)
The len may not be a multiple of 4. But I believe bufaddr is always
a buffer allocated in a length that is a multiple of 4, and the 1~3
bytes exceeding the len very likely has no data that matters. But
yes, it deserves a safer implementation.
Did you test what happens if bufaddr isn't aligned? Does it work at all
then?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
1.7.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello Sascha,
On Fri, Jan 14, 2011 at 09:40:09AM +0100, Sascha Hauer wrote:
On Fri, Jan 14, 2011 at 03:24:54PM +0800, Shawn Guo wrote:
quoted
+const u32 *mxs_get_ocotp(void)
+{
+ [...]
+}
EXPORT_SYMBOL?
I don't think this should be necessary. mxs_get_ocotp should only be
called from platform code that cannot (ot at least should not) be
modular. I suggest to skip it for now and if we really need it later
only add it then.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would
be more accurate.
When your make this change, you may want to pick a better name for
function swap_buffer too.
[...]
quoted
quoted
+static void *swap_buffer(void *bufaddr, int len)
+{
+ int i;
+ unsigned int *buf = bufaddr;
+
+ for (i = 0; i < (len + 3) / 4; i++, buf++)
+ *buf = cpu_to_be32(*buf);
if len isn't a multiple of 4 this accesses bytes behind len. Is this
generally OK here? (E.g. because skbs always have a length that is a
multiple of 4?)
The len may not be a multiple of 4. But I believe bufaddr is always
a buffer allocated in a length that is a multiple of 4, and the 1~3
bytes exceeding the len very likely has no data that matters. But
yes, it deserves a safer implementation.
Did you test what happens if bufaddr isn't aligned? Does it work at all
then?
I see many calls passing a len that is not a multiple of 4, but it
works good.
--
Regards,
Shawn
@@ -1,5 +1,5 @@# Common support-obj-y:=clock.odevices.ogpio.oicoll.oiomux.osystem.otimer.o+obj-y:=clock.odevices.ogpio.oicoll.oiomux.oocotp.osystem.otimer.o
is it worth to make ocotp optional? (and let evk select
CONFIG_MXS_OCOTP)
I think not.
I thought it depends on how we think about it. If we think that
ocotp is a block of soc and should be on every system based on
the soc, we should build it unconditional. However, if we think
ocotp is a function that could either be used on a machine or not,
it should be built conditional, so that the machine does not use
ocotp function could save the ocotp build and the memory of
u32 ocotp_words[0x20].
--
Regards,
Shawn
IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would
be more accurate.
When your make this change, you may want to pick a better name for
function swap_buffer too.
[...]
quoted
quoted
+static void *swap_buffer(void *bufaddr, int len)
+{
+ int i;
+ unsigned int *buf = bufaddr;
+
+ for (i = 0; i < (len + 3) / 4; i++, buf++)
+ *buf = cpu_to_be32(*buf);
if len isn't a multiple of 4 this accesses bytes behind len. Is this
generally OK here? (E.g. because skbs always have a length that is a
multiple of 4?)
The len may not be a multiple of 4. But I believe bufaddr is always
a buffer allocated in a length that is a multiple of 4, and the 1~3
bytes exceeding the len very likely has no data that matters. But
yes, it deserves a safer implementation.
Did you test what happens if bufaddr isn't aligned? Does it work at all
then?
I see many calls passing a len that is not a multiple of 4, but it
works good.
That does not prove anything, actually.
Anyway "bufaddr isn't aligned" != "len is not a multiple of 4".
Is there any guarantee that the function cannot be called with a
non-aligned buffer address?
Lothar Wa?mann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would
be more accurate.
When your make this change, you may want to pick a better name for
function swap_buffer too.
[...]
quoted
quoted
+static void *swap_buffer(void *bufaddr, int len)
+{
+ int i;
+ unsigned int *buf = bufaddr;
+
+ for (i = 0; i < (len + 3) / 4; i++, buf++)
+ *buf = cpu_to_be32(*buf);
if len isn't a multiple of 4 this accesses bytes behind len. Is this
generally OK here? (E.g. because skbs always have a length that is a
multiple of 4?)
The len may not be a multiple of 4. But I believe bufaddr is always
a buffer allocated in a length that is a multiple of 4, and the 1~3
bytes exceeding the len very likely has no data that matters. But
yes, it deserves a safer implementation.
Did you test what happens if bufaddr isn't aligned? Does it work at all
then?
I see many calls passing a len that is not a multiple of 4, but it
works good.
That does not prove anything, actually.
Anyway "bufaddr isn't aligned" != "len is not a multiple of 4".
Is there any guarantee that the function cannot be called with a
non-aligned buffer address?
Over the weekend I wondered if we could reach alignment via a dma-mask
setting. Didn't check yet how this is configured.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
I wonder what is excluded here. FEC depends on
M523x || M527x || M5272 || M528x || M520x || M532x || \
MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28
so the only difference is that the latter lists M5272 which seems a bit
redundant in the presence of M527x.
M527x = {M5271, M5275}, so it seems to me that only M5272 is excluded
here. I don't know if it's possible to have a kernel supporting M5272
and (e.g.) M527x. If yes, does the driver work correct on M5272 then?
Greg, it seems to me that M5272 is the exception here, not all the
others. Would it make sense to make the above read:
#if !defined(CONFIG_M5272)
?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would
be more accurate.
When your make this change, you may want to pick a better name for
function swap_buffer too.
[...]
quoted
quoted
+static void *swap_buffer(void *bufaddr, int len)
+{
+ int i;
+ unsigned int *buf = bufaddr;
+
+ for (i = 0; i < (len + 3) / 4; i++, buf++)
+ *buf = cpu_to_be32(*buf);
if len isn't a multiple of 4 this accesses bytes behind len. Is this
generally OK here? (E.g. because skbs always have a length that is a
multiple of 4?)
The len may not be a multiple of 4. But I believe bufaddr is always
a buffer allocated in a length that is a multiple of 4, and the 1~3
bytes exceeding the len very likely has no data that matters. But
yes, it deserves a safer implementation.
Did you test what happens if bufaddr isn't aligned? Does it work at all
then?
I see many calls passing a len that is not a multiple of 4, but it
works good.
That does not prove anything, actually.
Anyway "bufaddr isn't aligned" != "len is not a multiple of 4".
Is there any guarantee that the function cannot be called with a
non-aligned buffer address?
Oops, I misunderstood the comment. With bounce buffer alignment
handling removed, the driver stops working. But at least, mx28
fec driver can work with FEC_ALIGNMENT 0x3 and not necessarily with
0xf.
I hope this is what you intended to know.
--
Regards,
Shawn
@@ -1,5 +1,5 @@# Common support-obj-y:=clock.odevices.ogpio.oicoll.oiomux.osystem.otimer.o+obj-y:=clock.odevices.ogpio.oicoll.oiomux.oocotp.osystem.otimer.o
is it worth to make ocotp optional? (and let evk select
CONFIG_MXS_OCOTP)
I think not.
I thought it depends on how we think about it. If we think that
ocotp is a block of soc and should be on every system based on
the soc, we should build it unconditional. However, if we think
ocotp is a function that could either be used on a machine or not,
it should be built conditional, so that the machine does not use
ocotp function could save the ocotp build and the memory of
u32 ocotp_words[0x20].
You've not merged patch #8, #9, #10, so you still think
CONFIG_MXS_OCOTP should not be added?
--
Regards,
Shawn
This is a very initial pm support and basically does nothing.
With this pm support entry, drivers can start testing their own
pm functions.
Signed-off-by: Shawn Guo <redacted>
---
Hi Sascha,
The merging conflict was fixed in this version, and you can pick it
up on imx-for-2.6.39 branch now.
arch/arm/mach-mxs/Makefile | 1 +
arch/arm/mach-mxs/pm.c | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-mxs/pm.c