[coreboot] [PATCH] Enable/Disable IDE 0/1 i82801xx

[Joseph Smith]

On Wed, 27 May 2009 09:29:22 +0200, Stefan Reinauer <stepan at coresystems.de>
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