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

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

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

--- Comment #8 from Ilija Stanislevik <ilijas@siva.com.mk> 2010-11-30 15:10:11 GMT ---
(In reply to comment #4)
Hi Jifl,

Sorry for the long delay :-/.

I agree with most of your comment, so I modified the driver accordingly:

> - And starting with the most trivial first of all :-), proper ChangeLog format
...

Agree. Now I use the correct ChangeLog format, hopefully.


> - The board in question is effectively a plug-in board - an optional component
> to the normal stm3210e. I think it may be better _not_ to mark the package as a
> 'hardware' package...

Agree. The enc424j600 package is not 'hardware' anymore.

> - Some of your copyright years can't be right as they go back too far and also
> don't include 2009/2010 (depending on when you wrote this). Please go through
> and check them.

Checked and corrected.

> - In the CDL file, if you don't have descriptions, I'd expect you can just
> leave them out entirely.

Agree. Corrected accordingly.

> - I think CYGNUM_DEVS_ETH_ENC424J600_AUTO_SPDPX,
> CYGNUM_DEVS_ETH_ENC424J600_FULL_DUPLEX and CYGNUM_DEVS_ETH_ENC424J600_SPEED
> would be better expressed as CYGNUM_DEVS_ETH_ENC424J600_AUTO_SPDPX being
> changed to CYGSEM_DEVS_ETH_ENC424J600_NO_AUTO_NEGOTIATION and those other two
> being child options of it. The duplex option should have the CYGSEM_ prefix
> rather than CYGNUM_

Agree. Corrected accordingly.

> - How is a sensible value for CYGNUM_DEVS_ETH_ENC424J600_INTERRUPT_VECTOR and
> _PRIORITY to be set automatically? Normally we arrange things so that hardware
> should work by default, but especially we arrange things so that if we know it
> shouldn't work then it should give an error preferably when configuring, or
> otherwise when building. At present it would build with the values set to 0.

Interrupt vector and priority can hardly be set automatically. I decided to
have default values which cause configuration tool to complain, so the user is
forced to supply meaningful values.

> In this case, we could apply the necessary change to the stm3210e HAL so that
> it does set the values. Similarly we can make the stm3210e do the relevant
> platform-specific initialisation you mentioned if the
> CYGPKG_DEVS_ETH_ENC424J600 is active in the configuration.

I made changes to stm3210e HAL. Also added here cdl options to select SPI bus
and interrupt port. 

> - A trivial thing that would be nice: CYGNUM_DEVS_ETH_ENC424J600_FLOWC has
> values which could be friendlier when e.g. presented in the configuration tool.
> Any chance they could actually be just None and Auto? If you wanted, some
> #ifdef magic could still translate them if you preferred.

There is a possibility for third option, besides no-flow-control and
on-chip-flow-control, that is in-driver-flow-control. I did not implement it,
but I think that it might be implemented in future. So, I decided to keep
CYGNUM_DEVS_ETH_ENC424J600_FLOWC unchanged.

> CYGNUM_DEVS_ETH_ENC424J600_CLOCKOUT_FREQUENCY values could be presented in a
> more readable way.

I tried to make it more readable. Also extended the description a little bit. I
would welcome a suggestion.

> - Can you explain more about CYGNUM_DEVS_ETH_ENC424J600_CLOCKOUT_FREQUENCY to
> me please? Particularly given the fixed dependency on the 25MHz clock. Can the
> clock be easily changed in this? Is it possible to have the clock freq as its
> own option and have things change appropriately?

Clockuot is enc424j600 facility which can be used to supply clock to external
parts. This facility is not used by the Ethernet driver. It is included in
configuration just as a convenience.

After double checking the enc424j600 manual, I decided that reference to 25MHz
clock is redundant, since this is the only possibility. 25MHz is not mentioned
any more.

> - The test is pretty stm3210e specific and should really therefore live in its
> HAL (but still as a standalone file - not run automatically by including it in
> the CDL). Again it's better to make this driver package independent of the
> stm3210e.

Agree. The initialization is now moved to HAL, so the test is made independent
of platform. Being like that, the test can stay within driver package.

> - Minor nitpick... There are various mentions of CYGPKG_REDBOOT throughout. I
> believe these should ideally be CYGPKG_IO_ETH_DRIVERS_STAND_ALONE in fact.

Agree. I decided to use CYGINT_IO_ETH_INT_SUPPORT_REQUIRED instead, as a more
versatile distinction. What do you think about it? 

> - Ideally the device name should be configurable, not hard-coded as "eth0".

Agree. It is configurable now.

> - None of the priv data seems to be used as variables - they are all in fact
> configuration constants. Perhaps they can be eliminated thus saving a little
> space - given the cortex is tight for space anyway, every little bit helps.

Agree. Changed.

> - Where does the 25.6 usec wait delay come from? Is it fixed for the enc424j600
> or could it vary on other SPI bus speeds? (Hopefully it's based on the 25MHz
> clock mentioned before).

According to enc424j600 manual, this delay is fixed. Probably depends on 25MHz
clock, which is invariable.

Ilija

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