Skip to content

Commit

Permalink
sdram: fix cdelay prototype to remove warning in sdram_phy.h
Browse files Browse the repository at this point in the history
enjoy-digital committed Jan 23, 2016
1 parent 0607e92 commit 1330c7e
Showing 3 changed files with 4 additions and 2 deletions.
2 changes: 1 addition & 1 deletion misoclib/mem/sdram/phy/initsequence.py
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@ def get_sdram_phy_header(sdram_phy_settings):
nphases = sdram_phy_settings.nphases
r += "#define DFII_NPHASES "+str(nphases)+"\n\n"

r += "static void cdelay(int i);\n"
r += "extern void cdelay(int i);\n"

This comment has been minimized.

Copy link
@sbourdeauducq

sbourdeauducq Jan 23, 2016

Member

extern is not necessary for function prototype declarations.

This comment has been minimized.

Copy link
@sbourdeauducq

sbourdeauducq Jan 23, 2016

Member

Additionally, that function should be renamed if it is to appear as a global symbol.


# commands_px functions
for n in range(nphases):
2 changes: 1 addition & 1 deletion software/bios/sdram.c
Original file line number Diff line number Diff line change
@@ -11,7 +11,7 @@

#include "sdram.h"

static void cdelay(int i)
void cdelay(int i)
{
while(i > 0) {
#if defined (__lm32__)
2 changes: 2 additions & 0 deletions software/bios/sdram.h
Original file line number Diff line number Diff line change
@@ -3,6 +3,8 @@

#include <generated/csr.h>

void cdelay(int i);

This comment has been minimized.

Copy link
@sbourdeauducq

sbourdeauducq Jan 23, 2016

Member

This conflicts with the declaration done by initsequence.py. Why not have one include the other?


void sdrsw(void);
void sdrhw(void);
void sdrrow(char *_row);

4 comments on commit 1330c7e

@sbourdeauducq
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specification of this function is: it should wait for at least the given number of CPU cycles.

When I wrote this code, there wasn't timer support and only SDRAM needs it, so I made it a function local to the SDRAM code (hence the static forward declaration that you just removed).

If you make it global, it should not live in the SDRAM code anymore. I guess a good place would be libbase/time.c, and it should probably use timers too. Of course, this will need careful testing, especially as there might be some bugs in the SDRAM timing computations (e.g. the margin parameter of the ns function is inappropriate for any PHY other than half-rate, but you used it with others). Writing clean code is hard...

@enjoy-digital
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I tried here to fix a warning, but I'll have a closer look and put a delay function in libbase/time.c. IIRC there is also a delay function in libnet/microudp.c that we can try to remove. Once done I'll apply these change also to master. I'll have a closer look at SDRAM too.

@sbourdeauducq
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@enjoy-digital can I revert this in the meantime?

@enjoy-digital
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes you can.

Please sign in to comment.