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 #17 from Ilija Stanislevik <ilijas@siva.com.mk> 2012-04-05 07:21:40 BST ---
(In reply to comment #16)
Hi Jonathan,

> - 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.

I think that RedBoot is a case where package CYGPKG_DEVS_ETH_ENC424J600 is
included and CYGINT_IO_ETH_INT_SUPPORT_REQUIRED is not active.

> Instead you want something like:
>
> requires { CYGINT_IO_ETH_INT_SUPPORT_REQUIRED implies
> (CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_VECTOR != "\"\"") }
>
> (and the same for _PRIORITY)
>

Anyway, this above is better.

> - 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.

This parameter is upper limit for both receive and transmit packet size. The
description is wrong. I will change the description. According to what I've
been able to find out, the lower limit for an Ethernet packet is 60 bytes, so I
will set the legal_values accordingly.

>
> - 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.
>

The chip will truncate both Rx and Tx packets to
CYGNUM_DEVS_ETH_ENC424J600_ACCEPTABLE_PACKET_SIZE bytes.
CYGNUM_DEVS_ETH_ENC424J600_TXBUF_SIZE serves to reserve space for transmit
buffer in on-chip RAM. I will set its lowest legal value to reflect
CYGNUM_DEVS_ETH_ENC424J600_ACCEPTABLE_PACKET_SIZE.

> - Purely for consistency, please change
> CYGNUM_ETH_ENC424J600_USER_INTERRUPT_VECTOR ->
> CYGNUM_DEVS_ETH_ENC424J600_USER_INTERRUPT_VECTOR
>
> - "treshold" should be "threshold"
>

Of course.

> - 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.
>

I agree.

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

I'll do my best.

>
> 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 != "" } 
>

OK

> - 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"
>

Your suggestion isn't optimal because this component is parented in
CYGPKG_DEVS_ETH_ENC424J600, so in configtool it appears there. The title should
depict the component as part of HAL. You are right about the length, it should
be shorter. I'll try "STM32 HAL resources" and some better description.

> - 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 }
>

Yes, this is much better.

> - CYGHWR_HAL_SPIETH_INTERRUPT_PIN can have just "legal_values 0 to 15"
>

I agree.

> - 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 initialization 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().

I see now. Looks quite elegant.

I will update with recent changes to stm3210e then I will submit the changes.


Ilija

-- 
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]