Re: [LinuxBIOS] new target iei juki-511p/rocky-512

[Uwe Hermann]

Hi Nikolay,

thanks a lot for your great work and the patch!

As Stefan said, please resend with a Signed-off-by so we can commit, see
http://linuxbios.org/Development_Guidelines#Sign-off_Procedure

Here are a few more comments on the patch:

On Sun, Jun 03, 2007 at 12:20:10AM +0600, Nikolay Petukhov wrote:
> Problems:
> Filo load bzImage only from ide0.

What does not work? ide1? AFAICS the boards only have one IDE port!?
> JUKI-511P – PCISA half–size board:
> Specification:
> CPU: Geode GX1 300Mhz
> System Chipset: cs5530

CS5530A according to the website/manual for both boards, but it's mostly
the same as CS5530. We should at some point check whether there are
differences we need to take care of in LinuxBIOS, though.

(this will not hold up your patch, just a sidenote for later...)
> diff -Nru LinuxBIOSv2-2700/src/mainboard/iei/juki511p/auto.c LinuxBIOSv2-2700-juki/src/mainboard/iei/juki511p/auto.c
> --- LinuxBIOSv2-2700/src/mainboard/iei/juki511p/auto.c	1970-01-01 05:00:00.000000000 +0500
> +++ LinuxBIOSv2-2700-juki/src/mainboard/iei/juki511p/auto.c	2007-06-02 20:02:10.000000000 +0600

Please rename the directory to 'juki-511p' if possible, as that seems to be
the canonical (lowercase'd) name used by the vendor.
> +#include "superio/winbond/w83977fa/w83977fa_early_serial.c"

Maybe this should be 'w83977f' (without the 'a')? IIRC, we omitted any
'-A' or '-AW' suffix from other Super I/Os, too. Comments anyone?
> +void udelay(int usecs) 
> +{
> int i;
> for(i = 0; i < usecs; i++)
> outb(i&0xff, 0x80);
>
> +}

Is this needed? There's a global implementation in the repository
already.
>        device pnp 3f0.4 on		# RTC
>        end

Is this ok if left empty?

My 'lspnp -v' (on another mainboard) says:

00:04 PNP0b00 AT real-time clock
    state = active
        io 0x70-0x71
        irq 8

So maybe something like

device pnp 3f0.4 on              # RTC
    io 0x60 = 0x70
    irq 0x70 = 8
  end

is needed here? I'm just guessing, though, please correct me if I'm
wrong.
>        device pnp 3f0.5 on		# Keyboard
>           io 0x60 = 0x60
>           io 0x62 = 0x64
>          irq 0x70 = 0x01     # int  1 for PS/2 keyboard
>          irq 0x72 = 0x0c     # int 12 for PS/2 mouse

Why not like this? Easier to read, IMHO:

irq 0x70 = 1     # int  1 for PS/2 keyboard
  irq 0x72 = 12    # int 12 for PS/2 mouse
> +const struct irq_routing_table intel_irq_routing_table = {
> PIRQ_SIGNATURE,  /* u32 signature */
> PIRQ_VERSION,    /* u16 version   */
> 32+16*2,	 /* There can be total 6 devices on the bus */

^
typo? --->                                     2?

Maybe even

32+16*IRQ_SLOT_COUNT,         /* There can be IRQ_SLOT_COUNT devices on the bus */
> diff -Nru LinuxBIOSv2-2700/src/northbridge/amd/gx1/raminit.c LinuxBIOSv2-2700-juki/src/northbridge/amd/gx1/raminit.c
> --- LinuxBIOSv2-2700/src/northbridge/amd/gx1/raminit.c	2005-07-06 23:11:02.000000000 +0600
> +++ LinuxBIOSv2-2700-juki/src/northbridge/amd/gx1/raminit.c	2007-05-31 16:06:40.000000000 +0600
> @@ -324,6 +324,7 @@
>    outb(0x70, 0x80);
>
> setGX1Mem(GX_BASE + MC_MEM_CNTRL2, 0x000007d8); /* Disable all CLKS, Shift = 3 */
> +//	setGX1Mem(GX_BASE + MC_MEM_CNTRL1, 0x92080000); /* MD_DS=2, MA_DS=2, CNTL_DS=2 SDCLKRATE=2.5 */
>    setGX1Mem(GX_BASE + MC_MEM_CNTRL1, 0x92140000); /* MD_DS=2, MA_DS=2, CNTL_DS=2 SDCLKRATE=4 */
>    setGX1Mem(GX_BASE + MC_BANK_CFG,   0x00700070); /* No DIMMS installed */
>    setGX1Mem(GX_BASE + MC_SYNC_TIM1,  0x3a733225); /* LTMODE=3, RC=10, RAS=7, RP=3, RCD=3, RRD=2, DPL=2 */

OK, not sure what we should do with this. Will it break other GX1-based
mainboards? Or is it generic for all GX1s?
> diff -Nru LinuxBIOSv2-2700/src/superio/winbond/w83977fa/chip.h LinuxBIOSv2-2700-juki/src/superio/winbond/w83977fa/chip.h
> --- LinuxBIOSv2-2700/src/superio/winbond/w83977fa/chip.h	1970-01-01 05:00:00.000000000 +0500
> +++ LinuxBIOSv2-2700-juki/src/superio/winbond/w83977fa/chip.h	2007-06-02 18:52:57.000000000 +0600

Please provide an extra patch for the addition of the new Super I/O,
this is independant of the mainboards...
> diff -Nru LinuxBIOSv2-2700/targets/iei/juki511p/Config.lb LinuxBIOSv2-2700-juki/targets/iei/juki511p/Config.lb
> --- LinuxBIOSv2-2700/targets/iei/juki511p/Config.lb	1970-01-01 05:00:00.000000000 +0500
> +++ LinuxBIOSv2-2700-juki/targets/iei/juki511p/Config.lb	2007-06-02 19:07:21.000000000 +0600

Please add a copyright header to this file, too.

Uwe.