This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug 1000910] New port: Ethernet over SPI driver for ENC424J600


Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000910

--- Comment #16 from Jonathan Larmour <jifl@ecoscentric.com> 2012-04-02 21:21:22 BST ---
Hi Ilija,

Let's try and get this committed. I've gone through the current version of the
patch right from the beginning again.

Driver comments:

- In the CDL you don't want to use
!is_active(CYGINT_IO_ETH_INT_SUPPORT_REQUIRED), is it will always be active if
the package is included which it always would be. Instead you want something
like:

requires { CYGINT_IO_ETH_INT_SUPPORT_REQUIRED implies
(CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_VECTOR != "\"\"") }

(and the same for _PRIORITY)

- I think I'd prefer the option
CYGNUM_DEVS_ETH_ENC424J600_ACCEPTABLE_PACKET_SIZE to be
CYGNUM_DEVS_ETH_ENC424J600_MAX_RX_SIZE since that's what it is really.

- In CYGNUM_DEVS_ETH_ENC424J600_TXBUF_SIZE, I think the lowest transmit buffer
size could be lower than 1518. lwIP for example allows you to limit the packet
size, so it would be possible for a user to guarantee that larger packets
weren't sent by the stack. But I'd leave the default as it is.

- Purely for consistency, please change
CYGNUM_ETH_ENC424J600_USER_INTERRUPT_VECTOR ->
CYGNUM_DEVS_ETH_ENC424J600_USER_INTERRUPT_VECTOR

- "treshold" should be "threshold"

- For CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_PRIORITY and
CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_VECTOR, you don't need to have a separate
"user" option. You just need to have the default_value of
CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_PRIORITY to be: 

is_active(CYGNUM_HAL_SPIETH_INTERRUPT_PRIORITY) ?
CYGNUM_HAL_SPIETH_INTERRUPT_PRIORITY : "\"\""

That way it also allows users to override the HAL if they've hooked it up
differently.

- On committing I will probably just tidy up a few of the CDL descriptions for
English clarity, please don't be offended!


STM3210e changes comments:

- It may need to be updated due to recent commits in the stm3210e HAL.

- You can remove these from CYGNUM_ETH_SPIETH_HAL_INTERRUPT_VECTOR:
            requires { CYGHWR_HAL_SPIETH_INTERRUPT_PIN != "" } 
            requires { CYGHWR_HAL_CORTEXM_STM32_SPIETH_INTERRUPT_PORT != "" } 

- Think of how the CDL will look in the graphical config tool -   "STM32 HAL
resources assigned to SPI Ethernet controller" is a very long string to
display. It would be better just as "Optional ENC424J600 SPI Ethernet"

- In CYGHWR_HAL_CORTEXM_STM32_SPIETH_SPI_BUS, rather than setting the legal
values from the enabled buses, it should be the other way round - require the
buses to be enabled based on the setting. In other words:
  legal_values { 1 2 3 }
  requires { (CYGHWR_HAL_CORTEXM_STM32_SPIETH_SPI_BUS == 1) implies
CYGHWR_DEVS_SPI_CORTEXM_STM32_BUS1 }
  requires { (CYGHWR_HAL_CORTEXM_STM32_SPIETH_SPI_BUS == 2) implies
CYGHWR_DEVS_SPI_CORTEXM_STM32_BUS2 }
  requires { (CYGHWR_HAL_CORTEXM_STM32_SPIETH_SPI_BUS == 3) implies
CYGHWR_DEVS_SPI_CORTEXM_STM32_BUS3 }

- CYGHWR_HAL_SPIETH_INTERRUPT_PIN can have just "legal_values 0 to 15"

- I don't think you should be putting anything in stm3210e_eval_misc here. That
should be kept for HAL stuff, and this is a very optional peripheral add-on. It
should be a separate file. But even then, what you do doesn't seem quite right.
Any port specific initialisation should be performed via the
enc424j600_spi_init() function. The eth driver should use a HAL provided hook.
It can get this by including <cyg/hal/hal_io.h> but in practice it will usually
be defined in the platform's plf_io.h. For example for stm3210e's plf_io.h:

struct cyg_netdevtab_entry;
__externC cyg_devs_cortexm_stm3210e_enc424j600_init( struct cyg_netdevtab_entry
* );
#define CYG_DEVS_ETH_ENC424J600_PLF_INIT( _tab_ ) \
  cyg_devs_cortexm_stm3210e_enc424j600_init( _tab_ )

where "_tab_" is the cyg_netdevtab_entry passed to enc424j600_spi_init().

This would also get rid of the very yucky duplication of netdev_lookup().

Comments for both: The ChangeLog format still isn't quite right I'm afraid. It
doesn't mention the files affected. It's probably not worth worrying about this
for the eth driver since it's a new package (but please do it for new changes
in that package), but you should follow the format for the existing ChangeLogs.

Sorry that there's rather more comments than I'd have expected at this point,
but I've been through it in more detail this time than before.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]