| Message ID | 20240401025620.205068-2-frank@crawford.emu.id.au (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hwmon (it87): Correct handling for configuration mode | expand |
On Mon, Apr 01, 2024 at 01:56:03PM +1100, Frank Crawford wrote: > Rename previous definitions to match the new information that they are > preinitialised by the BIOS and should not receive codes to enter or exit > configuration mode. > That is wrong. NOEXIT is needed for broken chips where two chips are on the sio bus, and disabling sio access on the broken chip results in sio access errors. The description is also wrong, because SIO mode still needs to be _entered_. Also, a BIOS open mode, if it indeed exists, would have to be be board specific, not chip specific. Guenter > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > --- > drivers/hwmon/it87.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index fbe86cec6055..6eeba3a01e3c 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -320,7 +320,7 @@ struct it87_devices { > * second SIO address. Never exit configuration mode on these > * chips to avoid the problem. > */ > -#define FEAT_CONF_NOEXIT BIT(19) /* Chip should not exit conf mode */ > +#define FEAT_CONF_BIOSOPEN BIT(19) /* Chip conf mode enabled by BIOS */ > #define FEAT_FOUR_FANS BIT(20) /* Supports four fans */ > #define FEAT_FOUR_PWM BIT(21) /* Supports four fan controls */ > #define FEAT_FOUR_TEMP BIT(22) > @@ -452,7 +452,7 @@ static const struct it87_devices it87_devices[] = { > .model = "IT8790E", > .features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS > | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL > - | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_CONF_NOEXIT, > + | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_CONF_BIOSOPEN, > .peci_mask = 0x07, > }, > [it8792] = { > @@ -461,7 +461,7 @@ static const struct it87_devices it87_devices[] = { > .features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS > | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI > | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF > - | FEAT_CONF_NOEXIT, > + | FEAT_CONF_BIOSOPEN, > .peci_mask = 0x07, > .old_peci_mask = 0x02, /* Actually reports PCH */ > }, > @@ -507,7 +507,7 @@ static const struct it87_devices it87_devices[] = { > .features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS > | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI > | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF > - | FEAT_CONF_NOEXIT, > + | FEAT_CONF_BIOSOPEN, > .peci_mask = 0x07, > .old_peci_mask = 0x02, /* Actually reports PCH */ > }, > @@ -544,7 +544,7 @@ static const struct it87_devices it87_devices[] = { > #define has_four_temp(data) ((data)->features & FEAT_FOUR_TEMP) > #define has_six_temp(data) ((data)->features & FEAT_SIX_TEMP) > #define has_vin3_5v(data) ((data)->features & FEAT_VIN3_5V) > -#define has_conf_noexit(data) ((data)->features & FEAT_CONF_NOEXIT) > +#define has_conf_biosopen(data) ((data)->features & FEAT_CONF_BIOSOPEN) > #define has_scaling(data) ((data)->features & (FEAT_12MV_ADC | \ > FEAT_10_9MV_ADC)) > #define has_fanctl_onoff(data) ((data)->features & FEAT_FANCTL_ONOFF) > @@ -748,7 +748,7 @@ static int smbus_disable(struct it87_data *data) > superio_select(data->sioaddr, PME); > superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG, > data->ec_special_config & ~data->smbus_bitmap); > - superio_exit(data->sioaddr, has_conf_noexit(data)); > + superio_exit(data->sioaddr, has_conf_biosopen(data)); > } > return 0; > } > @@ -765,7 +765,7 @@ static int smbus_enable(struct it87_data *data) > superio_select(data->sioaddr, PME); > superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG, > data->ec_special_config); > - superio_exit(data->sioaddr, has_conf_noexit(data)); > + superio_exit(data->sioaddr, has_conf_biosopen(data)); > } > return 0; > } > @@ -3143,7 +3143,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, > } > > exit: > - superio_exit(sioaddr, config ? has_conf_noexit(config) : false); > + superio_exit(sioaddr, config ? has_conf_biosopen(config) : false); > return err; > } > > @@ -3540,7 +3540,7 @@ static void it87_resume_sio(struct platform_device *pdev) > reg2c); > } > > - superio_exit(data->sioaddr, has_conf_noexit(data)); > + superio_exit(data->sioaddr, has_conf_biosopen(data)); > } > > static int it87_resume(struct device *dev)
On Wed, 2024-04-10 at 08:10 -0700, Guenter Roeck wrote: > On Mon, Apr 01, 2024 at 01:56:03PM +1100, Frank Crawford wrote: > > Rename previous definitions to match the new information that they > > are > > preinitialised by the BIOS and should not receive codes to enter or > > exit > > configuration mode. > > > > That is wrong. NOEXIT is needed for broken chips where two chips are > on the > sio bus, and disabling sio access on the broken chip results in sio > access > errors. The description is also wrong, because SIO mode still needs > to be > _entered_. As noted in the patch group write up, this change has come from the technical specifications for the chips not for the board. If by SIO mode you mean the sequence "0x87,0x01,0x55,0xAA" then it should not be used for these chips according to people with access to the specification documents. Unfortunately, I don't have direct access to these documents, so cannot quote the full description. Now, the macro name may not be the best (BIOSOPEN), and I'm happy to change it to something better, but the current name of "NOEXIT" is also wrong. However, for the chips that this relates to, and are defined to use in the it87_devices structure, you can access the chip details without the the superio_enter sequence, as that is specifically the read that occurs to find the chipID, and I have tested it on a number of different chips, both of this type and older ones that do need the entry sequence. What makes this a little more difficult is that the chips that it affects also only ever appears to be the second chip on the bus, which may be by design, or just current usage. I will add that the use of enter sequence has been confirmed to fail and cause the exact chip lockup concerned about on the Gigabyte X670E Aorus Master board. While you may say that we should only do this for that board, the information I have received is that it is cause by incorrect access to those chipIDs, not board specific. > > Also, a BIOS open mode, if it indeed exists, would have to be be > board > specific, not chip specific. Now here my description may be wrong in it being BIOS related, but rather it is specifically the chip initialisation, but the details on access came from the chip specifications. > > Guenter Regards Frank > > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > > --- > > drivers/hwmon/it87.c | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > > index fbe86cec6055..6eeba3a01e3c 100644 > > --- a/drivers/hwmon/it87.c > > +++ b/drivers/hwmon/it87.c > > @@ -320,7 +320,7 @@ struct it87_devices { > > * second SIO address. Never exit configuration mode on these > > * chips to avoid the problem. > > */ > > -#define FEAT_CONF_NOEXIT BIT(19) /* Chip should not exit conf mode > > */ > > +#define FEAT_CONF_BIOSOPEN BIT(19) /* Chip conf mode enabled by > > BIOS */ > > #define FEAT_FOUR_FANS BIT(20) /* Supports four fans */ > > #define FEAT_FOUR_PWM BIT(21) /* Supports four fan controls */ > > #define FEAT_FOUR_TEMP BIT(22) > > @@ -452,7 +452,7 @@ static const struct it87_devices it87_devices[] > > = { > > .model = "IT8790E", > > .features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS > > | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL > > - | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_CONF_NOEXIT, > > + | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_CONF_BIOSOPEN, > > .peci_mask = 0x07, > > }, > > [it8792] = { > > @@ -461,7 +461,7 @@ static const struct it87_devices it87_devices[] > > = { > > .features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS > > | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI > > | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF > > - | FEAT_CONF_NOEXIT, > > + | FEAT_CONF_BIOSOPEN, > > .peci_mask = 0x07, > > .old_peci_mask = 0x02, /* Actually reports PCH */ > > }, > > @@ -507,7 +507,7 @@ static const struct it87_devices it87_devices[] > > = { > > .features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS > > | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI > > | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF > > - | FEAT_CONF_NOEXIT, > > + | FEAT_CONF_BIOSOPEN, > > .peci_mask = 0x07, > > .old_peci_mask = 0x02, /* Actually reports PCH */ > > }, > > @@ -544,7 +544,7 @@ static const struct it87_devices it87_devices[] > > = { > > #define has_four_temp(data) ((data)->features & FEAT_FOUR_TEMP) > > #define has_six_temp(data) ((data)->features & FEAT_SIX_TEMP) > > #define has_vin3_5v(data) ((data)->features & FEAT_VIN3_5V) > > -#define has_conf_noexit(data) ((data)->features & > > FEAT_CONF_NOEXIT) > > +#define has_conf_biosopen(data) ((data)->features & > > FEAT_CONF_BIOSOPEN) > > #define has_scaling(data) ((data)->features & (FEAT_12MV_ADC | \ > > FEAT_10_9MV_ADC)) > > #define has_fanctl_onoff(data) ((data)->features & > > FEAT_FANCTL_ONOFF) > > @@ -748,7 +748,7 @@ static int smbus_disable(struct it87_data > > *data) > > superio_select(data->sioaddr, PME); > > superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG, > > data->ec_special_config & ~data->smbus_bitmap); > > - superio_exit(data->sioaddr, has_conf_noexit(data)); > > + superio_exit(data->sioaddr, has_conf_biosopen(data)); > > } > > return 0; > > } > > @@ -765,7 +765,7 @@ static int smbus_enable(struct it87_data *data) > > superio_select(data->sioaddr, PME); > > superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG, > > data->ec_special_config); > > - superio_exit(data->sioaddr, has_conf_noexit(data)); > > + superio_exit(data->sioaddr, has_conf_biosopen(data)); > > } > > return 0; > > } > > @@ -3143,7 +3143,7 @@ static int __init it87_find(int sioaddr, > > unsigned short *address, > > } > > > > exit: > > - superio_exit(sioaddr, config ? has_conf_noexit(config) : false); > > + superio_exit(sioaddr, config ? has_conf_biosopen(config) : > > false); > > return err; > > } > > > > @@ -3540,7 +3540,7 @@ static void it87_resume_sio(struct > > platform_device *pdev) > > reg2c); > > } > > > > - superio_exit(data->sioaddr, has_conf_noexit(data)); > > + superio_exit(data->sioaddr, has_conf_biosopen(data)); > > } > > > > static int it87_resume(struct device *dev)
On Thu, Apr 11, 2024 at 09:03:06PM +1000, Frank Crawford wrote: > On Wed, 2024-04-10 at 08:10 -0700, Guenter Roeck wrote: > > On Mon, Apr 01, 2024 at 01:56:03PM +1100, Frank Crawford wrote: > > > Rename previous definitions to match the new information that they > > > are > > > preinitialised by the BIOS and should not receive codes to enter or > > > exit > > > configuration mode. > > > > > > > That is wrong. NOEXIT is needed for broken chips where two chips are > > on the > > sio bus, and disabling sio access on the broken chip results in sio > > access > > errors. The description is also wrong, because SIO mode still needs > > to be > > _entered_. > > As noted in the patch group write up, this change has come from the > technical specifications for the chips not for the board. If by SIO > mode you mean the sequence "0x87,0x01,0x55,0xAA" then it should not be > used for these chips according to people with access to the > specification documents. > > Unfortunately, I don't have direct access to these documents, so cannot > quote the full description. > > Now, the macro name may not be the best (BIOSOPEN), and I'm happy to > change it to something better, but the current name of "NOEXIT" is also > wrong. > > However, for the chips that this relates to, and are defined to use in > the it87_devices structure, you can access the chip details without the > the superio_enter sequence, as that is specifically the read that > occurs to find the chipID, and I have tested it on a number of > different chips, both of this type and older ones that do need the > entry sequence. > > What makes this a little more difficult is that the chips that it > affects also only ever appears to be the second chip on the bus, which > may be by design, or just current usage. > > I will add that the use of enter sequence has been confirmed to fail > and cause the exact chip lockup concerned about on the Gigabyte X670E > Aorus Master board. While you may say that we should only do this for > that board, the information I have received is that it is cause by > incorrect access to those chipIDs, not board specific. It looks like you fund a better source of support than me. All I ever got was "we do not support Linux". Anyway, please find a better term than BIOSOPEN. I don't really care too much what it is as long as it doesn't suggest to be related to something that it isn't. FEAT_NOCONF or something similar might do. > > > > Also, a BIOS open mode, if it indeed exists, would have to be be > > board > > specific, not chip specific. > > Now here my description may be wrong in it being BIOS related, but > rather it is specifically the chip initialisation, but the details on > access came from the chip specifications. Well, again, BIOS suggests that the BIOS is involved, which would make it board specific. Thanks, Guenter
On Thu, 2024-04-11 at 05:31 -0700, Guenter Roeck wrote: > On Thu, Apr 11, 2024 at 09:03:06PM +1000, Frank Crawford wrote: > > On Wed, 2024-04-10 at 08:10 -0700, Guenter Roeck wrote: > > > On Mon, Apr 01, 2024 at 01:56:03PM +1100, Frank Crawford wrote: > > > > Rename previous definitions to match the new information that > > > > they > > > > are > > > > preinitialised by the BIOS and should not receive codes to > > > > enter or > > > > exit > > > > configuration mode. > > > > > > > > > > That is wrong. NOEXIT is needed for broken chips where two chips > > > are > > > on the > > > sio bus, and disabling sio access on the broken chip results in > > > sio > > > access > > > errors. The description is also wrong, because SIO mode still > > > needs > > > to be > > > _entered_. > > > > As noted in the patch group write up, this change has come from the > > technical specifications for the chips not for the board. If by > > SIO > > mode you mean the sequence "0x87,0x01,0x55,0xAA" then it should not > > be > > used for these chips according to people with access to the > > specification documents. > > > > Unfortunately, I don't have direct access to these documents, so > > cannot > > quote the full description. > > > > Now, the macro name may not be the best (BIOSOPEN), and I'm happy > > to > > change it to something better, but the current name of "NOEXIT" is > > also > > wrong. > > > > However, for the chips that this relates to, and are defined to use > > in > > the it87_devices structure, you can access the chip details without > > the > > the superio_enter sequence, as that is specifically the read that > > occurs to find the chipID, and I have tested it on a number of > > different chips, both of this type and older ones that do need the > > entry sequence. > > > > What makes this a little more difficult is that the chips that it > > affects also only ever appears to be the second chip on the bus, > > which > > may be by design, or just current usage. > > > > I will add that the use of enter sequence has been confirmed to > > fail > > and cause the exact chip lockup concerned about on the Gigabyte > > X670E > > Aorus Master board. While you may say that we should only do this > > for > > that board, the information I have received is that it is cause by > > incorrect access to those chipIDs, not board specific. > > It looks like you fund a better source of support than me. All I ever > got was "we do not support Linux". Yes, although they do have a commercial arrangement with Gigabyte, so they cannot release too much specific information without risking their agreement. > > Anyway, please find a better term than BIOSOPEN. I don't really care > too much what it is as long as it doesn't suggest to be related to > something that it isn't. FEAT_NOCONF or something similar might do. Okay, I will update the macro name and work on making the descriptions better, and then submit an updated patch set. > > > > > > > Also, a BIOS open mode, if it indeed exists, would have to be be > > > board > > > specific, not chip specific. > > > > Now here my description may be wrong in it being BIOS related, but > > rather it is specifically the chip initialisation, but the details > > on > > access came from the chip specifications. > > Well, again, BIOS suggests that the BIOS is involved, which would > make > it board specific. Yes, I'll correct that issue in the description. > > Thanks, > Guenter Regards Frank
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index fbe86cec6055..6eeba3a01e3c 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -320,7 +320,7 @@ struct it87_devices { * second SIO address. Never exit configuration mode on these * chips to avoid the problem. */ -#define FEAT_CONF_NOEXIT BIT(19) /* Chip should not exit conf mode */ +#define FEAT_CONF_BIOSOPEN BIT(19) /* Chip conf mode enabled by BIOS */ #define FEAT_FOUR_FANS BIT(20) /* Supports four fans */ #define FEAT_FOUR_PWM BIT(21) /* Supports four fan controls */ #define FEAT_FOUR_TEMP BIT(22) @@ -452,7 +452,7 @@ static const struct it87_devices it87_devices[] = { .model = "IT8790E", .features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL - | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_CONF_NOEXIT, + | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_CONF_BIOSOPEN, .peci_mask = 0x07, }, [it8792] = { @@ -461,7 +461,7 @@ static const struct it87_devices it87_devices[] = { .features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF - | FEAT_CONF_NOEXIT, + | FEAT_CONF_BIOSOPEN, .peci_mask = 0x07, .old_peci_mask = 0x02, /* Actually reports PCH */ }, @@ -507,7 +507,7 @@ static const struct it87_devices it87_devices[] = { .features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF - | FEAT_CONF_NOEXIT, + | FEAT_CONF_BIOSOPEN, .peci_mask = 0x07, .old_peci_mask = 0x02, /* Actually reports PCH */ }, @@ -544,7 +544,7 @@ static const struct it87_devices it87_devices[] = { #define has_four_temp(data) ((data)->features & FEAT_FOUR_TEMP) #define has_six_temp(data) ((data)->features & FEAT_SIX_TEMP) #define has_vin3_5v(data) ((data)->features & FEAT_VIN3_5V) -#define has_conf_noexit(data) ((data)->features & FEAT_CONF_NOEXIT) +#define has_conf_biosopen(data) ((data)->features & FEAT_CONF_BIOSOPEN) #define has_scaling(data) ((data)->features & (FEAT_12MV_ADC | \ FEAT_10_9MV_ADC)) #define has_fanctl_onoff(data) ((data)->features & FEAT_FANCTL_ONOFF) @@ -748,7 +748,7 @@ static int smbus_disable(struct it87_data *data) superio_select(data->sioaddr, PME); superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG, data->ec_special_config & ~data->smbus_bitmap); - superio_exit(data->sioaddr, has_conf_noexit(data)); + superio_exit(data->sioaddr, has_conf_biosopen(data)); } return 0; } @@ -765,7 +765,7 @@ static int smbus_enable(struct it87_data *data) superio_select(data->sioaddr, PME); superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG, data->ec_special_config); - superio_exit(data->sioaddr, has_conf_noexit(data)); + superio_exit(data->sioaddr, has_conf_biosopen(data)); } return 0; } @@ -3143,7 +3143,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, } exit: - superio_exit(sioaddr, config ? has_conf_noexit(config) : false); + superio_exit(sioaddr, config ? has_conf_biosopen(config) : false); return err; } @@ -3540,7 +3540,7 @@ static void it87_resume_sio(struct platform_device *pdev) reg2c); } - superio_exit(data->sioaddr, has_conf_noexit(data)); + superio_exit(data->sioaddr, has_conf_biosopen(data)); } static int it87_resume(struct device *dev)
Rename previous definitions to match the new information that they are preinitialised by the BIOS and should not receive codes to enter or exit configuration mode. Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> --- drivers/hwmon/it87.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)