v3 Kontron warnings

[Myles Watson]

This patch fixes some warnings in the Kontron build and a little bit of
whitespace.

Signed-off-by: Myles Watson

Thanks,
Myles

[Myles Watson]

On Wed, Dec 10, 2008 at 9:57 AM, Myles Watson  wrote:
> This patch fixes some warnings in the Kontron build and a little bit of
> whitespace.
>
> Signed-off-by: Myles Watson

Ron pointed out that I fixed the type warnings backward.  After the
unsigned->u16 patch, this one doesn't need that anymore.

Signed-off-by: Myles Watson

Thanks,
Myles

[Uwe Hermann]

On Wed, Dec 10, 2008 at 09:57:21AM -0700, Myles Watson wrote:
> This patch fixes some warnings in the Kontron build and a little bit of
> whitespace.
>
> Signed-off-by: Myles Watson
>
> Thanks,
> Myles
> Index: svn/northbridge/intel/i945/raminit.c
>
> --- svn.orig/northbridge/intel/i945/raminit.c
> +++ svn/northbridge/intel/i945/raminit.c
> @@ -1070,7 +1070,7 @@ static void sdram_rcomp_buffer_strength_
>    };
>
> const u8 * strength_multiplier;
> const u8* const * slew_group_lookup;
> const u8 * slew_group_lookup;

That should be
> const u8 *slew_group_lookup;

rather.
> int idx;
>
> /* Set Strength Multipliers */
> Index: svn/southbridge/intel/i82801gx/pci.c
> ===================================================================
> --- svn.orig/southbridge/intel/i82801gx/pci.c
> +++ svn/southbridge/intel/i82801gx/pci.c
> @@ -57,7 +57,6 @@ static void pci_init(struct device *dev)
>  static void ich_pci_dev_enable_resources(struct device *dev)
>  {
>    const struct pci_operations *ops;
>
> u16 command;
>
> /* Set the subsystem vendor and device id for mainboard devices */
> ops = ops_pci(dev);
>
> @@ -72,6 +71,7 @@ static void ich_pci_dev_enable_resources
>    }
>
> #if 0
> u16 command;
> /* If we write to PCI_COMMAND, on some systems
> this will cause the ROM and APICs not being visible
> anymore.
>
> @@ -103,7 +103,7 @@ static void ich_pci_bus_enable_resources
>    enable_childrens_resources(dev);
>  }
>
> -static void set_subsystem(struct device * dev, u16 vendor, u16 device)
> +static void set_subsystem(struct device * dev, unsigned vendor, unsigned device)

Nope, u16 is the correct one, please don't change in this direction,
rather change more *int* to u8/u16/u32 where appropriate.
> {
>  #if 0
>    /* Currently disabled because it causes a "BAR 9" memory resource
> Index: svn/southbridge/intel/i82801gx/pcie.c
> ===================================================================
> --- svn.orig/southbridge/intel/i82801gx/pcie.c
> +++ svn/southbridge/intel/i82801gx/pcie.c
> @@ -67,7 +67,7 @@ static void pci_init(struct device *dev)
>    printk(BIOS_DEBUG, "    PMLU32 = 0x%08x\n", reg32);
>  }
>
> -static void set_subsystem(struct device * dev, u16 vendor, u16 device)
> +static void set_subsystem(struct device * dev, unsigned vendor, unsigned device)

Ditto.
> {
>    u32 pci_id;
> Index: svn/mainboard/kontron/986lcd-m/stage1.c
>
> --- svn.orig/mainboard/kontron/986lcd-m/stage1.c
> +++ svn/mainboard/kontron/986lcd-m/stage1.c
> @@ -207,7 +207,6 @@ void hardware_stage1(void)
>  {
>    void 	early_superio_config_w83627thg(void);
>    void 	ich7_enable_lpc(void);
> int boot_mode = 0;

Not used/needed?

Uwe.

[Myles Watson]

On Wed, Dec 10, 2008 at 11:02 AM, Uwe Hermann  wrote:
> On Wed, Dec 10, 2008 at 09:57:21AM -0700, Myles Watson wrote:
> > This patch fixes some warnings in the Kontron build and a little bit of
> > whitespace.
> >
> > Signed-off-by: Myles Watson
> >
> > Thanks,
> > Myles
> > Index: svn/northbridge/intel/i945/raminit.c
> >
> > --- svn.orig/northbridge/intel/i945/raminit.c
> > +++ svn/northbridge/intel/i945/raminit.c
> > @@ -1070,7 +1070,7 @@ static void sdram_rcomp_buffer_strength_
> >       };
> >   const u8 * strength_multiplier;
> > const u8* const * slew_group_lookup;
> > const u8 * slew_group_lookup;
>
> That should be
> > const u8 *slew_group_lookup;

fixed.
> > -static void set_subsystem(struct device * dev, u16 vendor, u16 device)
> > +static void set_subsystem(struct device * dev, unsigned vendor, unsigned
>
> device)
>
> Nope, u16 is the correct one, please don't change in this direction,
> rather change more *int* to u8/u16/u32 where appropriate.

That was Ron's comment too.  Fixed.
> > Index: svn/mainboard/kontron/986lcd-m/stage1.c
> >
> > --- svn.orig/mainboard/kontron/986lcd-m/stage1.c
> > +++ svn/mainboard/kontron/986lcd-m/stage1.c
> > @@ -207,7 +207,6 @@ void hardware_stage1(void)
> >  {
> >       void    early_superio_config_w83627thg(void);
> >       void    ich7_enable_lpc(void);
> > int boot_mode = 0;
>
> Not used/needed?

Not used.

Thanks for the quick review.  Sorry our mails crossed.

Myles

[Stefan Reinauer]

Uwe Hermann wrote:
> > Index: svn/mainboard/kontron/986lcd-m/stage1.c
> >
> > --- svn.orig/mainboard/kontron/986lcd-m/stage1.c
> > +++ svn/mainboard/kontron/986lcd-m/stage1.c
> > @@ -207,7 +207,6 @@ void hardware_stage1(void)
> >  {
> >    void 	early_superio_config_w83627thg(void);
> >    void 	ich7_enable_lpc(void);
> > int boot_mode = 0;
>
> Not used/needed?

This might be a bug in v3, still. But the boot_mode is required to find
out whether you're on the reset, resume or cold power on path. if it's
gone, rebooting won't work.

Stefan

[Myles Watson]

On Wed, Dec 10, 2008 at 11:07 AM, Stefan Reinauer wrote:
> Uwe Hermann wrote:
> > > Index: svn/mainboard/kontron/986lcd-m/stage1.c
> > >
> > > --- svn.orig/mainboard/kontron/986lcd-m/stage1.c
> > > +++ svn/mainboard/kontron/986lcd-m/stage1.c
> > > @@ -207,7 +207,6 @@ void hardware_stage1(void)
> > >  {
> > >      void    early_superio_config_w83627thg(void);
> > >      void    ich7_enable_lpc(void);
> > > int boot_mode = 0;
> >
> > Not used/needed?
>
> This might be a bug in v3, still. But the boot_mode is required to find
> out whether you're on the reset, resume or cold power on path. if it's
> gone, rebooting won't work.

So I should replace it with a #warning that rebooting won't work until the
code is moved over?

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 10.12.2008 17:57, Myles Watson wrote:
> This patch fixes some warnings in the Kontron build and a little bit of
> whitespace.
>
> Signed-off-by: Myles Watson

See my comments below. With the comments addressed, this is
Acked-by: Carl-Daniel Hailfinger 
If you feel that addressing the comments needs more work, feel free to
do a partial commit.

Regards,
Carl-Daniel
> Thanks,
> Myles
> Index: svn/northbridge/intel/i945/northbridge.c
> ===================================================================
> --- svn.orig/northbridge/intel/i945/northbridge.c
> +++ svn/northbridge/intel/i945/northbridge.c
> @@ -179,17 +179,17 @@ static unsigned int i945_pci_domain_scan
>  #warning get number of 945 pci domain ops
>  struct device_operations i945_pci_domain_ops = {
>    .id = {.type = DEVICE_ID_PCI,
> {.pci = {.vendor = PCI_VENDOR_ID_INTEL,
> 	      .device = 0x6789}}},
>       {.pci = {.vendor = PCI_VENDOR_ID_INTEL,
> 	.device = 0x6789}}},
> .constructor		 = default_device_constructor,
> .reset_bus		= pci_bus_reset,
> .reset_bus		 = pci_bus_reset,
> .phase3_scan		 = i945_pci_domain_scan_bus,
> .phase4_read_resources	 = I945_pci_domain_read_resources,
> .phase4_set_resources	 = I945_pci_domain_set_resources,
> .phase5_enable_resources = enable_childrens_resources,
> .phase6_init		 = NULL,
> .ops_pci		 = &pci_dev_ops_pci,
> .ops_pci_bus      = &pci_cf8_conf1,	/* Do we want to use the memory mapped space here? */
> .ops_pci_bus		 = &pci_cf8_conf1,	/* Do we want to use the memory mapped space here? */
>
> };
>
> static void mc_read_resources(struct device * dev)
> @@ -212,8 +212,8 @@ static void mc_read_resources(struct dev
>        IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_STORED |
>        IORESOURCE_ASSIGNED;
> printk(BIOS_DEBUG, "Adding PCIe enhanced config space BAR 0x%08lx-0x%08lx.\n",
>      (u64) resource->base, (u64) (resource->base + resource->size));
> printk(BIOS_DEBUG, "Adding PCIe enhanced config space BAR 0x%08llx-0x%08llx.\n",
>       resource->base, resource->base + resource->size);
>
> }
>
> static void mc_set_resources(struct device * dev)
> Index: svn/northbridge/intel/i945/raminit.c
> ===================================================================
> --- svn.orig/northbridge/intel/i945/raminit.c
> +++ svn/northbridge/intel/i945/raminit.c
> @@ -1070,7 +1070,7 @@ static void sdram_rcomp_buffer_strength_
>    };
>
> const u8 * strength_multiplier;
> const u8* const * slew_group_lookup;
> const u8 * slew_group_lookup;

I'm curious. This is a change in semantics. Are you sure the code will
work with that change? Can you get Stefan to confirm? He was a bit wary
of changes to the Intel code because it exploded on seemingly obvious
changes.
> int idx;
>
> /* Set Strength Multipliers */
> Index: svn/southbridge/intel/i82801gx/pci.c
> ===================================================================
> --- svn.orig/southbridge/intel/i82801gx/pci.c
> +++ svn/southbridge/intel/i82801gx/pci.c
> @@ -57,7 +57,6 @@ static void pci_init(struct device *dev)
>  static void ich_pci_dev_enable_resources(struct device *dev)
>  {
>    const struct pci_operations *ops;
>
> u16 command;
>
> /* Set the subsystem vendor and device id for mainboard devices */
> ops = ops_pci(dev);
>
> @@ -72,6 +71,7 @@ static void ich_pci_dev_enable_resources
>    }
>
> #if 0
> u16 command;
> /* If we write to PCI_COMMAND, on some systems
> this will cause the ROM and APICs not being visible
> anymore.
>
> @@ -103,7 +103,7 @@ static void ich_pci_bus_enable_resources
>    enable_childrens_resources(dev);
>  }
>
> -static void set_subsystem(struct device * dev, u16 vendor, u16 device)
> +static void set_subsystem(struct device * dev, unsigned vendor, unsigned device)

This change is the exact opposite of your set_subsystem patch.
> {
>  #if 0
>    /* Currently disabled because it causes a "BAR 9" memory resource
> Index: svn/southbridge/intel/i82801gx/pcie.c
> ===================================================================
> --- svn.orig/southbridge/intel/i82801gx/pcie.c
> +++ svn/southbridge/intel/i82801gx/pcie.c
> @@ -67,7 +67,7 @@ static void pci_init(struct device *dev)
>    printk(BIOS_DEBUG, "    PMLU32 = 0x%08x\n", reg32);
>  }
>
> -static void set_subsystem(struct device * dev, u16 vendor, u16 device)
> +static void set_subsystem(struct device * dev, unsigned vendor, unsigned device)

Same here.
> {
>    u32 pci_id;
> Index: svn/mainboard/kontron/986lcd-m/stage1.c
>
> --- svn.orig/mainboard/kontron/986lcd-m/stage1.c
> +++ svn/mainboard/kontron/986lcd-m/stage1.c
> @@ -207,7 +207,6 @@ void hardware_stage1(void)
>  {
>    void 	early_superio_config_w83627thg(void);
>    void 	ich7_enable_lpc(void);
> int boot_mode = 0;
>
> #if 1
>    /* hack */
>    pci_conf1_write_config32(PCI_BDF(0, 0x1f, 0), RCBA, DEFAULT_RCBA | 1);

[Myles Watson]

On Wed, Dec 10, 2008 at 11:12 AM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> On 10.12.2008 17:57, Myles Watson wrote:
> > This patch fixes some warnings in the Kontron build and a little bit of
> > whitespace.
> >
> > Signed-off-by: Myles Watson
>
> See my comments below. With the comments addressed, this is
> Acked-by: Carl-Daniel Hailfinger 
> If you feel that addressing the comments needs more work, feel free to
> do a partial commit.

Rev 1068.

Thanks,
Myles
> Regards,
> Carl-Daniel
> > Thanks,
> > Myles
> > Index: svn/northbridge/intel/i945/northbridge.c
> > ===================================================================
> > --- svn.orig/northbridge/intel/i945/northbridge.c
> > +++ svn/northbridge/intel/i945/northbridge.c
> > @@ -179,17 +179,17 @@ static unsigned int i945_pci_domain_scan
> >  #warning get number of 945 pci domain ops
> >  struct device_operations i945_pci_domain_ops = {
> >       .id = {.type = DEVICE_ID_PCI,
> >         {.pci = {.vendor = PCI_VENDOR_ID_INTEL,
> >                       .device = 0x6789}}},
> >        {.pci = {.vendor = PCI_VENDOR_ID_INTEL,
> >                 .device = 0x6789}}},
> > .constructor             = default_device_constructor,
> > .reset_bus              = pci_bus_reset,
> > .reset_bus               = pci_bus_reset,
> > .phase3_scan             = i945_pci_domain_scan_bus,
> > .phase4_read_resources   = I945_pci_domain_read_resources,
> > .phase4_set_resources    = I945_pci_domain_set_resources,
> > .phase5_enable_resources = enable_childrens_resources,
> > .phase6_init             = NULL,
> > .ops_pci                 = &pci_dev_ops_pci,
> > .ops_pci_bus      = &pci_cf8_conf1,     /* Do we want to use the
>
> memory mapped space here? */
> > .ops_pci_bus             = &pci_cf8_conf1,      /* Do we want to
>
> use the memory mapped space here? */
> > };
> >
> > static void mc_read_resources(struct device * dev)
> > @@ -212,8 +212,8 @@ static void mc_read_resources(struct dev
> >           IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_STORED |
> >           IORESOURCE_ASSIGNED;
> > printk(BIOS_DEBUG, "Adding PCIe enhanced config space BAR
>
> 0x%08lx-0x%08lx.\n",
> >              (u64) resource->base, (u64) (resource->base +
>
> resource->size));
> > printk(BIOS_DEBUG, "Adding PCIe enhanced config space BAR
>
> 0x%08llx-0x%08llx.\n",
> >        resource->base, resource->base + resource->size);
> >
> > }
> >
> > static void mc_set_resources(struct device * dev)
> > Index: svn/northbridge/intel/i945/raminit.c
> > ===================================================================
> > --- svn.orig/northbridge/intel/i945/raminit.c
> > +++ svn/northbridge/intel/i945/raminit.c
> > @@ -1070,7 +1070,7 @@ static void sdram_rcomp_buffer_strength_
> >       };
> >   const u8 * strength_multiplier;
> > const u8* const * slew_group_lookup;
> > const u8 * slew_group_lookup;
>
> I'm curious. This is a change in semantics. Are you sure the code will
> work with that change? Can you get Stefan to confirm? He was a bit wary
> of changes to the Intel code because it exploded on seemingly obvious
> changes.

I can't confirm that it will work, but I think it is the correct change.
This pointer is assigned a const u8 *, then passed to a function as a const
u8 *.  That's its only usage.
> >   int idx;
> >
> >   /* Set Strength Multipliers */
> > Index: svn/southbridge/intel/i82801gx/pci.c
> >
> > --- svn.orig/southbridge/intel/i82801gx/pci.c
> > +++ svn/southbridge/intel/i82801gx/pci.c
> > @@ -57,7 +57,6 @@ static void pci_init(struct device *dev)
> >  static void ich_pci_dev_enable_resources(struct device *dev)
> >  {
> >       const struct pci_operations *ops;
> > u16 command;
> >
> > /* Set the subsystem vendor and device id for mainboard devices */
> > ops = ops_pci(dev);
> >
> > @@ -72,6 +71,7 @@ static void ich_pci_dev_enable_resources
> >       }
> >
> > #if 0
> > u16 command;
> > /* If we write to PCI_COMMAND, on some systems
> >  * this will cause the ROM and APICs not being visible
> >  * anymore.
> >
> > @@ -103,7 +103,7 @@ static void ich_pci_bus_enable_resources
> >       enable_childrens_resources(dev);
> >  }
> >
> > -static void set_subsystem(struct device * dev, u16 vendor, u16 device)
> > +static void set_subsystem(struct device * dev, unsigned vendor, unsigned
>
> device)
> >
>
> This change is the exact opposite of your set_subsystem patch.

Yes.  Lots of fast reviews :)  I won't include it.