Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head

2008-06-13 Thread David Jander 
On Thursday 12 June 2008 14:12:15 you wrote:
 On Jun 12, 2008, at 6:45 AM, David Jander wrote:

 Your commit message isn't exactly helpful as most people dont know
 what LTIB is and its not terribly relevant.  It just seems like you
 are adding support for the FEC on MPC5121 and this point.

[...]
  --- a/drivers/net/fec.h
  +++ b/drivers/net/fec.h
  @@ -59,6 +59,7 @@ typedef struct fec {
  } fec_t;
 
  #else
  +#if !defined(CONFIG_FS_ENET_MPC5121_FEC)
 
  /*
   *  Define device register set address map.
  @@ -97,6 +98,48 @@ typedef struct fec {
  unsigned long   fec_fifo_ram[112];  /* FIFO RAM buffer */
  } fec_t;
 
  +#else /* CONFIG_FS_ENET_MPC5121_FEC */
  +
  +typedef struct fec {
  [...]
  +} fec_t;
  +
  +#endif /* CONFIG_FS_ENET_MPC5121_FEC */
  #endif /* CONFIG_M5272 */

 I'm not exactly clear as to why this was done this way but this not
 acceptable as it means we can't build a multiplatform kernel that
 needs this driver.

Well, it wouldn't be possible either, since CONFIG_M5272 is a Cold-fire 
processor, and CONFIG_FS_ENET_MPC5121_FEC is for a PowerPC processor.
In this case.
Otherwise you are right, the driver breaks MPC83xx/MPC5121 multiplatform 
builds.

 I'm also not clear to me if the MPC5121 FEC is really the same device
 or close to it that it should be sharing this driver or have its own.

I am coming to the conclusion that it should have its own driver. 
Altough a lot of code could be shared, there are still enough differences, so 
that writing just ONE driver without some #ifdef's that would break 
multiplatform builds, would instead end up with a much bigger amount of if's, 
that would make it unreadable, unmaintainable and inefficient.
Here's why: The above struct fet_t for instance is mapped to a set of 
registers in the FEC. For processors with a CPM1, a CPM2 or without CPM (i.e. 
MPC5121) the register mapping seems to be significantly different, 
nevertheless the structs are all called struct fec_t. How can one fix this 
at runtime without changing the name of the structs and then just use a lot 
of if's or a combination of macro's and if's everywhere a register of the 
FEC is accessed? I fear it will be a mess.

So I think it's either a separate driver, or break multiplatform builds.

Since I am learning from you that breaking multiplatform builds is a no-go, 
I'll settle for splitting up the driver.

Any suggestion on where to put that split-off? How to name it?
I would suggest drivers/net/fec_mpc512x/*

I just resubmitted PATCH 1/2 again without part 2 (which hasn't much to do 
with it anyway), so Grant may have a final look at it (hopefully I did it 
right this time). Part 2 (MPC5121_FEC) will have to wait until monday or so, 
since it will take me a while, and I have to do other things in between.

Any suggestions on how to solve the puzzle are of course welcome...

Thanks a lot for reviewing.

Best regards,

-- 
David Jander
Protonic Holland.
___
Linuxppc-embedded mailing list
Linuxppc-embedded@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-embedded

[PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head

2008-06-12 Thread David Jander 

Signed-off-by: David Jander [EMAIL PROTECTED]
---
 arch/powerpc/platforms/Kconfig |2 +-
 drivers/net/fec.h  |   43 
 drivers/net/fs_enet/Kconfig|   22 +-
 drivers/net/fs_enet/fs_enet-main.c |   76 ++--
 drivers/net/fs_enet/fs_enet.h  |   17 +---
 drivers/net/fs_enet/mac-fec.c  |   22 +-
 drivers/net/fs_enet/mii-fec.c  |   10 -
 7 files changed, 167 insertions(+), 25 deletions(-)

diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index 87454c5..a96937f 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -288,7 +288,7 @@ config CPM2
 
 config PPC_CPM_NEW_BINDING
bool
-   depends on CPM1 || CPM2
+   depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
default y
 
 config AXON_RAM
diff --git a/drivers/net/fec.h b/drivers/net/fec.h
index 292719d..5c9fe34 100644
--- a/drivers/net/fec.h
+++ b/drivers/net/fec.h
@@ -59,6 +59,7 @@ typedef struct fec {
 } fec_t;
 
 #else
+#if !defined(CONFIG_FS_ENET_MPC5121_FEC)
 
 /*
  * Define device register set address map.
@@ -97,6 +98,48 @@ typedef struct fec {
unsigned long   fec_fifo_ram[112];  /* FIFO RAM buffer */
 } fec_t;
 
+#else /* CONFIG_FS_ENET_MPC5121_FEC */
+
+typedef struct fec {
+   u32 fec_reserved0;
+   u32 fec_ievent; /* Interrupt event reg */
+   u32 fec_imask;  /* Interrupt mask reg */
+   u32 fec_reserved1;
+   u32 fec_r_des_active;   /* Receive descriptor reg */
+   u32 fec_x_des_active;   /* Transmit descriptor reg */
+   u32 fec_reserved2[3];
+   u32 fec_ecntrl; /* Ethernet control reg */
+   u32 fec_reserved3[6];
+   u32 fec_mii_data;   /* MII manage frame reg */
+   u32 fec_mii_speed;  /* MII speed control reg */
+   u32 fec_reserved4[7];
+   u32 fec_mib_ctrlstat;   /* MIB control/status reg */
+   u32 fec_reserved5[7];
+   u32 fec_r_cntrl;/* Receive control reg */
+   u32 fec_reserved6[15];
+   u32 fec_x_cntrl;/* Transmit Control reg */
+   u32 fec_reserved7[7];
+   u32 fec_addr_low;   /* Low 32bits MAC address */
+   u32 fec_addr_high;  /* High 16bits MAC address */
+   u32 fec_opd;/* Opcode + Pause duration */
+   u32 fec_reserved8[10];
+   u32 fec_hash_table_high;/* High 32bits hash table */
+   u32 fec_hash_table_low; /* Low 32bits hash table */
+   u32 fec_grp_hash_table_high;/* High 32bits hash table */
+   u32 fec_grp_hash_table_low; /* Low 32bits hash table */
+   u32 fec_reserved9[7];
+   u32 fec_x_wmrk; /* FIFO transmit water mark */
+   u32 fec_reserved10;
+   u32 fec_r_bound;/* FIFO receive bound reg */
+   u32 fec_r_fstart;   /* FIFO receive start reg */
+   u32 fec_reserved11[11];
+   u32 fec_r_des_start;/* Receive descriptor ring */
+   u32 fec_x_des_start;/* Transmit descriptor ring */
+   u32 fec_r_buff_size;/* Maximum receive buff size */
+   u32 fec_dma_control;/* DMA Endian and other ctrl */
+} fec_t;
+
+#endif /* CONFIG_FS_ENET_MPC5121_FEC */
 #endif /* CONFIG_M5272 */
 
 
diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig
index 562ea68..5e2520b 100644
--- a/drivers/net/fs_enet/Kconfig
+++ b/drivers/net/fs_enet/Kconfig
@@ -1,9 +1,23 @@
 config FS_ENET
tristate Freescale Ethernet Driver
-   depends on CPM1 || CPM2
+   depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
select MII
select PHYLIB
 
+config FS_ENET_MPC5121_FEC
+   bool Freescale MPC512x FEC driver
+   depends on PPC_MPC512x
+   select FS_ENET
+   select FS_ENET_HAS_FEC
+
+config FS_ENET_TX_ALIGN_WORKAROUND
+   bool MPC5121 FEC driver TX alignment workaround
+   depends on FS_ENET_MPC5121_FEC
+   help
+ Workaround for a problem with early Freescale MPC5121 chips.
+ If unsure say 'y'
+   default y
+
 config FS_ENET_HAS_SCC
bool Chip has an SCC usable for ethernet
depends on FS_ENET  (CPM1 || CPM2)
@@ -16,13 +30,15 @@ config FS_ENET_HAS_FCC
 
 config FS_ENET_HAS_FEC
bool Chip has an FEC usable for ethernet
-   depends on FS_ENET  CPM1
+   depends on FS_ENET  (CPM1 || FS_ENET_MPC5121_FEC)
select FS_ENET_MDIO_FEC
default y
 
+
 config FS_ENET_MDIO_FEC
tristate MDIO driver for FEC
-   depends on FS_ENET  CPM1
+   depends on FS_ENET  (CPM1 || FS_ENET_MPC5121_FEC)
+
 
 config FS_ENET_MDIO_FCC
tristate MDIO driver for FCC
diff --git a/drivers/net/fs_enet/fs_enet-main.c 
b/drivers/net/fs_enet/fs_enet-main.c
index 31c9693..54f0079 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -592,6 +592,31 @@ void fs_cleanup_bds(struct net_device *dev)

Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head

2008-06-12 Thread Kumar Gala 


On Jun 12, 2008, at 6:45 AM, David Jander wrote:

Your commit message isn't exactly helpful as most people dont know  
what LTIB is and its not terribly relevant.  It just seems like you  
are adding support for the FEC on MPC5121 and this point.




Signed-off-by: David Jander [EMAIL PROTECTED]
---
arch/powerpc/platforms/Kconfig |2 +-
drivers/net/fec.h  |   43 
drivers/net/fs_enet/Kconfig|   22 +-
drivers/net/fs_enet/fs_enet-main.c |   76 +++ 
+++--

drivers/net/fs_enet/fs_enet.h  |   17 +---
drivers/net/fs_enet/mac-fec.c  |   22 +-
drivers/net/fs_enet/mii-fec.c  |   10 -
7 files changed, 167 insertions(+), 25 deletions(-)

diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/ 
Kconfig

index 87454c5..a96937f 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -288,7 +288,7 @@ config CPM2

config PPC_CPM_NEW_BINDING
bool
-   depends on CPM1 || CPM2
+   depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
default y

config AXON_RAM
diff --git a/drivers/net/fec.h b/drivers/net/fec.h
index 292719d..5c9fe34 100644
--- a/drivers/net/fec.h
+++ b/drivers/net/fec.h
@@ -59,6 +59,7 @@ typedef struct fec {
} fec_t;

#else
+#if !defined(CONFIG_FS_ENET_MPC5121_FEC)

/*
 *  Define device register set address map.
@@ -97,6 +98,48 @@ typedef struct fec {
unsigned long   fec_fifo_ram[112];  /* FIFO RAM buffer */
} fec_t;

+#else /* CONFIG_FS_ENET_MPC5121_FEC */
+
+typedef struct fec {
+   u32 fec_reserved0;
+   u32 fec_ievent; /* Interrupt event reg */
+   u32 fec_imask;  /* Interrupt mask reg */
+   u32 fec_reserved1;
+   u32 fec_r_des_active;   /* Receive descriptor reg */
+   u32 fec_x_des_active;   /* Transmit descriptor reg */
+   u32 fec_reserved2[3];
+   u32 fec_ecntrl; /* Ethernet control reg */
+   u32 fec_reserved3[6];
+   u32 fec_mii_data;   /* MII manage frame reg */
+   u32 fec_mii_speed;  /* MII speed control reg */
+   u32 fec_reserved4[7];
+   u32 fec_mib_ctrlstat;   /* MIB control/status reg */
+   u32 fec_reserved5[7];
+   u32 fec_r_cntrl;/* Receive control reg */
+   u32 fec_reserved6[15];
+   u32 fec_x_cntrl;/* Transmit Control reg */
+   u32 fec_reserved7[7];
+   u32 fec_addr_low;   /* Low 32bits MAC address */
+   u32 fec_addr_high;  /* High 16bits MAC address */
+   u32 fec_opd;/* Opcode + Pause duration */
+   u32 fec_reserved8[10];
+   u32 fec_hash_table_high;/* High 32bits hash table */
+   u32 fec_hash_table_low; /* Low 32bits hash table */
+   u32 fec_grp_hash_table_high;/* High 32bits hash table */
+   u32 fec_grp_hash_table_low; /* Low 32bits hash table */
+   u32 fec_reserved9[7];
+   u32 fec_x_wmrk; /* FIFO transmit water mark */
+   u32 fec_reserved10;
+   u32 fec_r_bound;/* FIFO receive bound reg */
+   u32 fec_r_fstart;   /* FIFO receive start reg */
+   u32 fec_reserved11[11];
+   u32 fec_r_des_start;/* Receive descriptor ring */
+   u32 fec_x_des_start;/* Transmit descriptor ring */
+   u32 fec_r_buff_size;/* Maximum receive buff size */
+   u32 fec_dma_control;/* DMA Endian and other ctrl */
+} fec_t;
+
+#endif /* CONFIG_FS_ENET_MPC5121_FEC */
#endif /* CONFIG_M5272 */


I'm not exactly clear as to why this was done this way but this not  
acceptable as it means we can't build a multiplatform kernel that  
needs this driver.


I'm also not clear to me if the MPC5121 FEC is really the same device  
or close to it that it should be sharing this driver or have its own.


- k
___
Linuxppc-embedded mailing list
Linuxppc-embedded@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head

2008-06-12 Thread Grant Likely 
On Thu, Jun 12, 2008 at 6:12 AM, Kumar Gala [EMAIL PROTECTED] wrote:

 On Jun 12, 2008, at 6:45 AM, David Jander wrote:

 Your commit message isn't exactly helpful as most people dont know what LTIB
 is and its not terribly relevant.  It just seems like you are adding support
 for the FEC on MPC5121 and this point.

Agreed.  Also, you need to CC: the netdev mailing list for network drivers.

 Signed-off-by: David Jander [EMAIL PROTECTED]
 ---
 arch/powerpc/platforms/Kconfig |2 +-
 drivers/net/fec.h  |   43 
 drivers/net/fs_enet/Kconfig|   22 +-
 drivers/net/fs_enet/fs_enet-main.c |   76
 ++--
 drivers/net/fs_enet/fs_enet.h  |   17 +---
 drivers/net/fs_enet/mac-fec.c  |   22 +-
 drivers/net/fs_enet/mii-fec.c  |   10 -
 7 files changed, 167 insertions(+), 25 deletions(-)

 diff --git a/arch/powerpc/platforms/Kconfig
 b/arch/powerpc/platforms/Kconfig
 index 87454c5..a96937f 100644
 --- a/arch/powerpc/platforms/Kconfig
 +++ b/arch/powerpc/platforms/Kconfig
 @@ -288,7 +288,7 @@ config CPM2

 config PPC_CPM_NEW_BINDING
bool
 -   depends on CPM1 || CPM2
 +   depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
default y

 config AXON_RAM
 diff --git a/drivers/net/fec.h b/drivers/net/fec.h
 index 292719d..5c9fe34 100644
 --- a/drivers/net/fec.h
 +++ b/drivers/net/fec.h
 @@ -59,6 +59,7 @@ typedef struct fec {
 } fec_t;

 #else
 +#if !defined(CONFIG_FS_ENET_MPC5121_FEC)

 /*
  *  Define device register set address map.
 @@ -97,6 +98,48 @@ typedef struct fec {
unsigned long   fec_fifo_ram[112];  /* FIFO RAM buffer */
 } fec_t;

 +#else /* CONFIG_FS_ENET_MPC5121_FEC */
 +
 +typedef struct fec {
 +   u32 fec_reserved0;
 +   u32 fec_ievent; /* Interrupt event reg */
 +   u32 fec_imask;  /* Interrupt mask reg */
 +   u32 fec_reserved1;
 +   u32 fec_r_des_active;   /* Receive descriptor reg */
 +   u32 fec_x_des_active;   /* Transmit descriptor reg */
 +   u32 fec_reserved2[3];
 +   u32 fec_ecntrl; /* Ethernet control reg */
 +   u32 fec_reserved3[6];
 +   u32 fec_mii_data;   /* MII manage frame reg */
 +   u32 fec_mii_speed;  /* MII speed control reg */
 +   u32 fec_reserved4[7];
 +   u32 fec_mib_ctrlstat;   /* MIB control/status reg */
 +   u32 fec_reserved5[7];
 +   u32 fec_r_cntrl;/* Receive control reg */
 +   u32 fec_reserved6[15];
 +   u32 fec_x_cntrl;/* Transmit Control reg */
 +   u32 fec_reserved7[7];
 +   u32 fec_addr_low;   /* Low 32bits MAC address */
 +   u32 fec_addr_high;  /* High 16bits MAC address */
 +   u32 fec_opd;/* Opcode + Pause duration */
 +   u32 fec_reserved8[10];
 +   u32 fec_hash_table_high;/* High 32bits hash table */
 +   u32 fec_hash_table_low; /* Low 32bits hash table */
 +   u32 fec_grp_hash_table_high;/* High 32bits hash table */
 +   u32 fec_grp_hash_table_low; /* Low 32bits hash table */
 +   u32 fec_reserved9[7];
 +   u32 fec_x_wmrk; /* FIFO transmit water mark */
 +   u32 fec_reserved10;
 +   u32 fec_r_bound;/* FIFO receive bound reg */
 +   u32 fec_r_fstart;   /* FIFO receive start reg */
 +   u32 fec_reserved11[11];
 +   u32 fec_r_des_start;/* Receive descriptor ring */
 +   u32 fec_x_des_start;/* Transmit descriptor ring */
 +   u32 fec_r_buff_size;/* Maximum receive buff size */
 +   u32 fec_dma_control;/* DMA Endian and other ctrl */
 +} fec_t;
 +
 +#endif /* CONFIG_FS_ENET_MPC5121_FEC */
 #endif /* CONFIG_M5272 */

 I'm not exactly clear as to why this was done this way but this not
 acceptable as it means we can't build a multiplatform kernel that needs this
 driver.

 I'm also not clear to me if the MPC5121 FEC is really the same device or
 close to it that it should be sharing this driver or have its own.

Indeed.  If it is not easy to isolate the differences for runtime
binding then it may be best to just clone the driver as a starting
point and make the 5121 specific changes.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-embedded mailing list
Linuxppc-embedded@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head

2008-06-12 Thread Grant Likely 
On Thu, Jun 12, 2008 at 5:45 AM, David Jander [EMAIL PROTECTED] wrote:


  /* write */
 -#define CBDW_SC(_cbd, _sc) __cbd_out16((_cbd)-cbd_sc, (_sc))
 -#define CBDW_DATLEN(_cbd, _datlen) __cbd_out16((_cbd)-cbd_datlen, 
 (_datlen))
 -#define CBDW_BUFADDR(_cbd, _bufaddr)   __cbd_out32((_cbd)-cbd_bufaddr, 
 (_bufaddr))
 +#define CBDW_SC(_cbd, _sc) __cbd_out16((volatile void __iomem 
 *)(_cbd)-cbd_sc, (_sc))
 +#define CBDW_DATLEN(_cbd, _datlen) __cbd_out16((volatile void __iomem 
 *)(_cbd)-cbd_datlen, (_datlen))
 +#define CBDW_BUFADDR(_cbd, _bufaddr)   __cbd_out32((volatile void __iomem 
 *)(_cbd)-cbd_bufaddr, (_bufaddr))

  /* read */
 -#define CBDR_SC(_cbd)  __cbd_in16((_cbd)-cbd_sc)
 -#define CBDR_DATLEN(_cbd)  __cbd_in16((_cbd)-cbd_datlen)
 -#define CBDR_BUFADDR(_cbd) __cbd_in32((_cbd)-cbd_bufaddr)
 +#define CBDR_SC(_cbd)  __cbd_in16((volatile void __iomem 
 *)(_cbd)-cbd_sc)
 +#define CBDR_DATLEN(_cbd)  __cbd_in16((volatile void __iomem 
 *)(_cbd)-cbd_datlen)
 +#define CBDR_BUFADDR(_cbd) __cbd_in32((volatile void __iomem 
 *)(_cbd)-cbd_bufaddr)

Another comment: This really doesn't look right.  The _cbd pointer
passed in should already be tagged with __iomem.  Trying to fix it
here is a band-aid and unsafe.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-embedded mailing list
Linuxppc-embedded@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-embedded