[PATCH] Enable/Disable IDE 0/1 i82801xx

[Joseph Smith]

Sorry so late, here is the patch to enable/disable IDE 0/1
(Primary/Secondary) interfaces on the i82801xx southbridge.

[Joseph Smith]

On Wed, 27 May 2009 00:06:57 -0400, Joseph Smith 
wrote:
> Sorry so late, here is the patch to enable/disable IDE 0/1
> (Primary/Secondary) interfaces on the i82801xx southbridge.

Oops, helps if the patch is attached.

[Joseph Smith]

On Wed, 27 May 2009 00:08:35 -0400, Joseph Smith 
wrote:
> On Wed, 27 May 2009 00:06:57 -0400, Joseph Smith 
> wrote:
> > Sorry so late, here is the patch to enable/disable IDE 0/1
> > (Primary/Secondary) interfaces on the i82801xx southbridge.
>
> Oops, helps if the patch is attached.

Duh, long day.

Signed-off by: Joseph Smith

[ron minnich]

Looks fine to me, but I can not test.

Acked-by: Ronald G. Minnich

[Stefan Reinauer]

On 27.05.2009 6:08 Uhr, Joseph Smith wrote:
> extern struct chip_operations southbridge_intel_i82801xx_ops;
> Index: src/southbridge/intel/i82801xx/i82801xx_ide.c
> ===================================================================
> --- src/southbridge/intel/i82801xx/i82801xx_ide.c	(revision 4311)
> +++ src/southbridge/intel/i82801xx/i82801xx_ide.c	(working copy)
> @@ -27,29 +27,36 @@
>  #include <device/pci_ids.h>
>  #include "i82801xx.h"
>
> +typedef struct southbridge_intel_i82801xx_config config_t;
>
> static void ide_init(struct device *dev)
>  {
> /* Get the chip configuration */
> config_t *config = dev->chip_info;
> /* TODO: Needs to be tested for compatibility with ICH5(R). */
>  /* Enable IDE devices so the Linux IDE driver will work. */
>  uint16_t ideTimingConfig;
>
> int enable_primary = 1;
>
> int enable_secondary = 1;
>
> ideTimingConfig = pci_read_config16(dev, IDE_TIM_PRI);
> ideTimingConfig &= ~IDE_DECODE_ENABLE;
>
> if (enable_primary) {
> if (config->ide0_enable) {

You should check if config is not NULL before accessing it or you might
silently read a config_t at the start of memory, which tells you that
all interrupts are configured to zero and both IDEs are disabled. This
was a tricky find for me once.

It will be NULL if the device 1f.2 (or whatever your IDE is) is not
present in Config.lb.

Maybe you want to check

if (!config || config->ide0_enable) { ... } // enable is default if
config is NULL

or you print a message that the user should fix the Config.lb file.

Other than that..

Acked-by: Stefan Reinauer

All the best,
Stefan

[Joseph Smith]

On Wed, 27 May 2009 09:29:22 +0200, Stefan Reinauer 
wrote:
> On 27.05.2009 6:08 Uhr, Joseph Smith wrote:
> > extern struct chip_operations southbridge_intel_i82801xx_ops;
> > Index: src/southbridge/intel/i82801xx/i82801xx_ide.c
> > ===================================================================
> > --- src/southbridge/intel/i82801xx/i82801xx_ide.c	(revision 4311)
> > +++ src/southbridge/intel/i82801xx/i82801xx_ide.c	(working copy)
> > @@ -27,29 +27,36 @@
> >  #include <device/pci_ids.h>
> >  #include "i82801xx.h"
> >
> > +typedef struct southbridge_intel_i82801xx_config config_t;
> >
> > static void ide_init(struct device *dev)
> >  {
> > /* Get the chip configuration */
> > config_t *config = dev->chip_info;
> > /* TODO: Needs to be tested for compatibility with ICH5(R). */
> >  /* Enable IDE devices so the Linux IDE driver will work. */
> >  uint16_t ideTimingConfig;
> >
> > int enable_primary = 1;
> >
> > int enable_secondary = 1;
> >
> > ideTimingConfig = pci_read_config16(dev, IDE_TIM_PRI);
> > ideTimingConfig &= ~IDE_DECODE_ENABLE;
> >
> > if (enable_primary) {
> > if (config->ide0_enable) {
>
> You should check if config is not NULL before accessing it or you might
> silently read a config_t at the start of memory, which tells you that
> all interrupts are configured to zero and both IDEs are disabled. This
> was a tricky find for me once.
>
> It will be NULL if the device 1f.2 (or whatever your IDE is) is not
> present in Config.lb.

Why would the IDE device not be present in Config.lb? As Ron has said this
would be manipulating the build system and should not be done. Do you mean
if the IDE device is turned "off" ?
> Maybe you want to check
>
> if (!config || config->ide0_enable) { ... } // enable is default if
> config is NULL
>
> or you print a message that the user should fix the Config.lb file.

Anyways it is a good safety check, thanks, I will make the change. Should I
change this in i82801xx_lpc.c as well for the PIRQ's?

Thanks,
Joseph Smith
Set-Top-Linux
www.settoplinux.org

[Joseph Smith]

> On Wed, 27 May 2009 00:06:57 -0400, Joseph Smith 
> wrote:
> > Sorry so late, here is the patch to enable/disable IDE 0/1
> > (Primary/Secondary) interfaces on the i82801xx southbridge.

All fixed Uwe, thanks Ron and Stefan, r4324