On 23.08.2007 00:33, Uwe Hermann wrote:
> On Wed, Aug 22, 2007 at 07:14:09PM +0200, Carl-Daniel Hailfinger wrote:
>>> Index: include/device/device.h
>>> ===================================================================
>>> --- include/device/device.h (Revision 476)
>>> +++ include/device/device.h (Arbeitskopie)
>>> @@ -202,7 +202,7 @@
>>> struct resource resource[MAX_RESOURCES];
>>> unsigned int resources;
>>>
>>> - /* link are (down sream) buses attached to the device, usually a leaf
>>> + /* link are (downstream) buses attached to the device, usually a leaf
>
> ACK.
>
>
>>> * device with no children have 0 buses attached and a bridge has 1 bus
>>> */
>>> struct bus link[MAX_LINKS];
>>> Index: mainboard/adl/msm800sev/stage1.c
>>> ===================================================================
>>> --- mainboard/adl/msm800sev/stage1.c (Revision 476)
>>> +++ mainboard/adl/msm800sev/stage1.c (Arbeitskopie)
>>> @@ -33,7 +33,9 @@
>>> #include <southbridge/amd/cs5536/cs5536.h>
>>> #include <superio/winbond/w83627hf/w83627hf.h>
>>>
>>> +/* This is wrong! SERIAL_DEV can't be >=0x10 because it's a PNP device
>>> number */
>>> #define SERIAL_DEV 0x30
>>> +#define SERIAL_IOBASE 0x3f8
>
> This is probably the wrong location for this information anyway. This is
> hardware property and should thus be in the dts, IMHO.
> This includes the 0x2e, the 0x30 (I think), and the 0x3f8.
Is the dts already used in stage 1? Otherwise I would just get some of
the values from the w83627hf.h
>>>
>>> void hardware_stage1(void)
>>> {
>>> @@ -49,6 +51,6 @@
>>> * for cs5536
>>> */
>>> cs5536_disable_internal_uart();
>>> - w83627hf_enable_serial(0x2e, 0x30, 0x3f8);
>>> + w83627hf_enable_serial(0x2e, SERIAL_DEV, SERIAL_IOBASE);
>>> printk(BIOS_DEBUG, "Done %s\n", __FUNCTION__);
>>> }
>>> Index: device/device.c
>>> ===================================================================
>>> --- device/device.c (Revision 476)
>>> +++ device/device.c (Arbeitskopie)
>>> @@ -260,8 +260,10 @@
>>> for (curdev = bus->children; curdev; curdev = curdev->sibling) {
>>> unsigned int links;
>>> int i;
>>> - printk(BIOS_SPEW, "%s: %s(%s) have_resources %d enabled %d\n",
>>> + printk(BIOS_SPEW,
>>> + "%s: %s(%s) dtsname %s have_resources %d enabled %d\n",
>>> __func__, bus->dev->dtsname, dev_path(bus->dev),
>>> + curdev->dtsname,
>>> curdev->have_resources, curdev->enabled);
>>> if (curdev->have_resources) {
>>> continue;
>>> @@ -402,7 +404,11 @@
>>> min_align = 0;
>>> base = bridge->base;
>>>
>>> - printk(BIOS_SPEW, "%s compute_allocate_%s: base: %08Lx size: %08Lx
>>> align: %d gran: %d\n", dev_path(bus->dev), (bridge->flags & IORESOURCE_IO)
>>> ? "io" : (bridge->flags & IORESOURCE_PREFETCH) ? "prefmem" : "mem", base,
>>> bridge->size, bridge->align, bridge->gran);
>>> + printk(BIOS_SPEW,
>>> + "%s compute_allocate_%s: base: %08llx size: %08llx align: %d
>>> gran: %d\n",
>>> + dev_path(bus->dev),
>>> + (bridge->flags & IORESOURCE_IO) ? "io" : (bridge->flags &
>>> IORESOURCE_PREFETCH) ? "prefmem" : "mem",
>>> + base, bridge->size, bridge->align, bridge->gran);
>>>
>>> /* We want different minimum alignments for different kinds of
>>> * resources. These minimums are not device type specific but
>>> @@ -485,7 +491,7 @@
>>> base += size;
>>>
>>> printk(BIOS_SPEW,
>>> - "%s %02lx * [0x%08Lx - 0x%08Lx] %s\n",
>>> + "%s %02lx * [0x%08llx - 0x%08llx] %s\n",
>
> Yep, I guess so. If this works (didn't follow the last printk-fixes
> thread too closely), then please make one patch for all of these and
> related printk-fixes.
Will do.
>>> dev_path(dev),
>>> resource->index,
>>> resource->base,
>>> @@ -503,7 +509,11 @@
>>> */
>>> bridge->size = align_up(base, bridge->gran) - bridge->base;
>>>
>>> - printk(BIOS_SPEW, "%s compute_allocate_%s: base: %08Lx size: %08Lx
>>> align: %d gran: %d done\n", dev_path(bus->dev), (bridge->flags &
>>> IORESOURCE_IO) ? "io" : (bridge->flags & IORESOURCE_PREFETCH) ? "prefmem" :
>>> "mem", base, bridge->size, bridge->align, bridge->gran);
>>> + printk(BIOS_SPEW,
>>> + "%s compute_allocate_%s: base: %08llx size: %08llx align: %d
>>> gran: %d done\n",
>>> + dev_path(bus->dev),
>>> + (bridge->flags & IORESOURCE_IO) ? "io" : (bridge->flags &
>>> IORESOURCE_PREFETCH) ? "prefmem" : "mem",
>>> + base, bridge->size, bridge->align, bridge->gran);
>>> }
>>>
>>> #if defined(CONFIG_PCI_OPTION_ROM_RUN) && CONFIG_PCI_OPTION_ROM_RUN == 1
>>> Index: device/pnp_device.c
>>> ===================================================================
>>> --- device/pnp_device.c (Revision 476)
>>> +++ device/pnp_device.c (Arbeitskopie)
>>> @@ -87,7 +87,7 @@
>>> {
>>> if (!(resource->flags & IORESOURCE_ASSIGNED)) {
>>> printk(BIOS_ERR,
>>> - "ERROR: %s %02lx %s size: 0x%010Lx not assigned\n",
>>> + "ERROR: %s %02lx %s size: 0x%010llx not assigned\n",
>>> dev_path(dev), resource->index, resource_type(resource),
>>> resource->size);
>>> return;
>>> Index: device/pci_device.c
>>> ===================================================================
>>> --- device/pci_device.c (Revision 476)
>>> +++ device/pci_device.c (Arbeitskopie)
>>> @@ -252,10 +252,10 @@
>>> printk(BIOS_DEBUG, "%s %02x ->",
>>> dev_path(dev), resource->index);
>>> printk(BIOS_DEBUG,
>>> - " value: 0x%08Lx zeroes: 0x%08Lx ones: 0x%08Lx attr:
>>> %08lx\n",
>>> + " value: 0x%08llx zeroes: 0x%08llx ones: 0x%08llx attr:
>>> %08lx\n",
>
> Are we sure %ll will never produce more than 8 digits? On all
> architectures?
No, it can produce up to 16 digits even on 32bit. However, the digits
will not be truncated, the "08" is a minimum length of 8 with leading
zeroes.
>>> value, zeroes, ones, attr);
>>> printk(BIOS_DEBUG,
>>> - "%s %02x -> size: 0x%08Lx max: 0x%08Lx %s\n ",
>>> + "%s %02x -> size: 0x%08llx max: 0x%08llx %s\n ",
>>> dev_path(dev), resource->index, resource->size,
>>> resource->limit, resource_type(resource));
>>> }
>>> @@ -456,7 +456,7 @@
>>> /* Make certain the resource has actually been set. */
>>> if (!(resource->flags & IORESOURCE_ASSIGNED)) {
>>> printk(BIOS_ERR,
>>> - "ERROR: %s %02lx %s size: 0x%010Lx not assigned\n",
>>> + "ERROR: %s %02lx %s size: 0x%010llx not assigned\n",
>>> dev_path(dev), resource->index, resource_type(resource),
>>> resource->size);
>>> return;
>>> @@ -863,7 +863,7 @@
>>> (*list)->path.type);
>>> continue;
>>> }
>>> - printk(BIOS_SPEW, "%s: check dev %s it has devfn 0x%x\n",
>>> + printk(BIOS_SPEW, "%s: check dev %s it has devfn 0x%02x\n",
>
> ACK, same for all other %x -> %02x (etc.) changes.
>
>
>>> __func__, (*list)->dtsname, (*list)->path.u.pci.devfn);
>>> if ((*list)->path.u.pci.devfn == devfn) {
>>> /* Unlink from the list. */
>>> Index: device/device_util.c
>>> ===================================================================
>>> --- device/device_util.c (Revision 476)
>>> +++ device/device_util.c (Arbeitskopie)
>>> @@ -229,7 +229,7 @@
>>> memcpy(buffer, "Root Device", 12);
>>> break;
>>> case DEVICE_ID_PCI:
>>> - sprintf(buffer, "PCI: %02x:%02x", id->u.pci.vendor,
>>> + sprintf(buffer, "PCI: %04x:%04x", id->u.pci.vendor,
>>> id->u.pci.device);
>>> break;
>>> case DEVICE_ID_PNP:
>>> @@ -243,7 +243,7 @@
>>> id->u.apic.device);
>>> break;
>>> case DEVICE_ID_PCI_DOMAIN:
>>> - sprintf(buffer, "PCI_DOMAIN: %02x:%02x",
>>> + sprintf(buffer, "PCI_DOMAIN: %04x:%04x",
>>> id->u.pci_domain.vendor,
>>> id->u.pci_domain.device);
>>> break;
>>> @@ -602,7 +602,7 @@
>>> #endif
>>> }
>>> printk(BIOS_DEBUG,
>>> - "%s %02lx <- [0x%010Lx - 0x%010Lx] %s%s%s\n",
>>> + "%s %02lx <- [0x%010llx - 0x%010llx] %s%s%s\n",
>>> dev_path(dev),
>>> resource->index,
>>> base, end, buf, resource_type(resource), comment);
>>> @@ -656,7 +656,7 @@
>>> for (i = 0; i < curdev->resources; i++) {
>>> struct resource *resource = &curdev->resource[i];
>>> printk(BIOS_SPEW,
>>> - "%s: dev %s, resource %d, flags %lx base 0x%Lx
>>> size 0x%Lx\n",
>>> + "%s: dev %s, resource %d, flags %lx base 0x%llx
>>> size 0x%llx\n",
>>> __func__, curdev->dtsname, i, resource->flags,
>>> resource->base, resource->size);
>>> /* If it isn't the right kind of resource ignore it. */
>>> Index: lib/stage2.c
>>> ===================================================================
>>> --- lib/stage2.c (Revision 476)
>>> +++ lib/stage2.c (Arbeitskopie)
>>> @@ -31,8 +31,9 @@
>>> /**
>>> * Main function of the DRAM part of LinuxBIOS.
>>> *
>>> - * LinuxBIOS is divided into pre-DRAM part and DRAM part. The phases before
>>> - * this part are phase 0 and phase 1. This part contains phases x through
>>> y.
>>> + * LinuxBIOS is divided into pre-DRAM part and DRAM part. The stages before
>>> + * this part are stage 0 and stage 1. This part contains stage 2, which
>>> + * consists of phases 1 through 6.
>
> Looks correct, but using "stages" _and_ "phases" will confuse the hell
> out of everyone. I vote for only using stage 0-2, just drop the phases
> terminology completely. Or maybe something like "which consists of
> multiple steps"?
I was just making the code comments conform to the design document.
>>> *
>>> * Device Enumeration: in the dev_enumerate() phase.
>>> *
>>> @@ -53,6 +54,7 @@
>>>
>>> post_code(0x20);
>>>
>>> + /* TODO: Explain why we use printk here although it is impossible */
>>> printk(BIOS_NOTICE, console_test);
>
> Hm, good point. It works in QEMU, but that's not a good indicator in
> this case.
>
>
>>>
>>> dev_init();
>>> Index: lib/elfboot.c
>>> ===================================================================
>>> --- lib/elfboot.c (Revision 476)
>>> +++ lib/elfboot.c (Arbeitskopie)
>>> @@ -61,7 +61,7 @@
>>> }
>>> if (i == mem_entries) {
>>> printk(BIOS_ERR, "No matching RAM area found for range:\n");
>>> - printk(BIOS_ERR, " [0x%016Lx, 0x%016Lx)\n", start, end);
>>> + printk(BIOS_ERR, " [0x%016llx, 0x%016llx)\n", start, end);
>>> printk(BIOS_ERR, "RAM areas\n");
>>> for(i = 0; i < mem_entries; i++) {
>>> u64 mstart, mend;
>>> @@ -69,7 +69,7 @@
>>> mtype = mem->map[i].type;
>>> mstart = unpack_lb64(mem->map[i].start);
>>> mend = mstart + unpack_lb64(mem->map[i].size);
>>> - printk(BIOS_ERR, " [0x%016Lx, 0x%016Lx) %s\n",
>>> + printk(BIOS_ERR, " [0x%016llx, 0x%016llx) %s\n",
>>> mstart, mend, (mtype ==
>>> LB_MEM_RAM)?"RAM":"Reserved");
>>>
>>> }
>>> Index: northbridge/intel/i440bxemulation/i440bx.c
>>> ===================================================================
>>> --- northbridge/intel/i440bxemulation/i440bx.c (Revision 476)
>>> +++ northbridge/intel/i440bxemulation/i440bx.c (Arbeitskopie)
>>> @@ -79,7 +79,7 @@
>>> resource->size = ((resource_t) sizek) << 10;
>>> resource->flags = IORESOURCE_MEM | IORESOURCE_CACHEABLE |
>>> IORESOURCE_FIXED | IORESOURCE_STORED | IORESOURCE_ASSIGNED;
>>> - printk(BIOS_DEBUG, "%s: add ram resoource %Ld bytes\n", __func__,
>>> + printk(BIOS_DEBUG, "%s: add ram resource %lld bytes\n", __func__,
>>> resource->size);
>>> }
>>>
>>> Index: arch/x86/linuxbios_table.c
>>> ===================================================================
>>> --- arch/x86/linuxbios_table.c (Revision 476)
>>> +++ arch/x86/linuxbios_table.c (Arbeitskopie)
>>> @@ -177,7 +177,7 @@
>>> {
>>> int entries;
>>>
>>> - printk(BIOS_DEBUG, "%s: start 0x%Lx size 0x%Lx\n",
>>> + printk(BIOS_DEBUG, "%s: start 0x%llx size 0x%llx\n",
>>> __func__, start, size);
>>>
>>> entries = (mem->size - sizeof(*mem))/sizeof(mem->map[0]);
>>> Index: arch/x86/pci_ops_mmconf.c
>>> ===================================================================
>>> --- arch/x86/pci_ops_mmconf.c (Revision 476)
>>> +++ arch/x86/pci_ops_mmconf.c (Arbeitskopie)
>>> @@ -18,32 +18,32 @@
>>>
>>> #include <mmio_conf.h>
>>>
>>> -static uint8_t pci_mmconf_read_config8(struct bus *pbus, int bus, int
>>> devfn, int where)
>>> +static u8 pci_mmconf_read_config8(struct bus *pbus, int bus, int devfn,
>>> int where)
>
> ACK (same for the other occurences).
>
>
>>> {
>>> return (read8x(PCI_MMIO_ADDR(bus, devfn, where)));
>>> }
>>>
>>> -static uint16_t pci_mmconf_read_config16(struct bus *pbus, int bus, int
>>> devfn, int where)
>>> +static u16 pci_mmconf_read_config16(struct bus *pbus, int bus, int devfn,
>>> int where)
>>> {
>>> return (read16x(PCI_MMIO_ADDR(bus, devfn, where)));
>>> }
>>>
>>> -static uint32_t pci_mmconf_read_config32(struct bus *pbus, int bus, int
>>> devfn, int where)
>>> +static u32 pci_mmconf_read_config32(struct bus *pbus, int bus, int devfn,
>>> int where)
>>> {
>>> return (read32x(PCI_MMIO_ADDR(bus, devfn, where)));
>>> }
>>>
>>> -static void pci_mmconf_write_config8(struct bus *pbus, int bus, int
>>> devfn, int where, uint8_t value)
>>> +static void pci_mmconf_write_config8(struct bus *pbus, int bus, int
>>> devfn, int where, u8 value)
>>> {
>>> write8x(PCI_MMIO_ADDR(bus, devfn, where), value);
>>> }
>>>
>>> -static void pci_mmconf_write_config16(struct bus *pbus, int bus, int
>>> devfn, int where, uint16_t value)
>>> +static void pci_mmconf_write_config16(struct bus *pbus, int bus, int
>>> devfn, int where, u16 value)
>>> {
>>> write8x(PCI_MMIO_ADDR(bus, devfn, where), value);
>>> }
>>>
>>> -static void pci_mmconf_write_config32(struct bus *pbus, int bus, int
>>> devfn, int where, uint32_t value)
>>> +static void pci_mmconf_write_config32(struct bus *pbus, int bus, int
>>> devfn, int where, u32 value)
>>> {
>>> write8x(PCI_MMIO_ADDR(bus, devfn, where), value);
>>> }
>
>
> Please repost all of this as two or three separate patches, each fixing
> a single issue repository-wide. I think we can apply this soon.
Will do. Thanks for the review.
Regards,
Carl-Daniel
--
linuxbios mailing list
[email protected]
http://www.linuxbios.org/mailman/listinfo/linuxbios
