| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a8b53088-70a0-f693-2081-4b9576ac492f@roeck-us.net>
Date: Wed, 26 Jul 2023 07:19:07 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Naresh Solanki <naresh.solanki@...ements.com>,
Jean Delvare <jdelvare@...e.com>,
krzysztof.kozlowski+dt@...aro.org
Cc: linux-hwmon@...r.kernel.org,
Patrick Rudolph <patrick.rudolph@...ements.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] hwmon: (pmbus/tda38640) Add workaround for bug in
SVID mode
On 7/26/23 05:22, Naresh Solanki wrote:
> Hi Guenter,
>
> On 25-07-2023 07:51 pm, Guenter Roeck wrote:
>> On 7/25/23 04:40, Naresh Solanki wrote:
>>> From: Patrick Rudolph <patrick.rudolph@...ements.com>
>>>
>>> The TDA38640 supports two operating modes to set the output voltage:
>>> - PMBUS
>>> - SVID
>>>
>>> Due to an undocumented bug the regulator cannot be enabled using BIT7
>>> of OPERATION(01h) register when in SVID mode.
>>> It works when in PMBUS mode. In SVID mode the regulator only cares
>>> about the ENABLE pin.
>>>
>>> Add a workaround that needs the ENABLE pin to be left floating or
>>> tied to a fixed level. The DT must contain the newly introduced
>>> property 'infineon,en-pin-fixed-level' to enable this workaround.
>>>
>>
>> Why is that property even needed ? Isn't the workaround always (and only)
>> required if the chip is in SVID mode ?
> Will add below function to detect SVID mode.
> static bool svid_mode(struct i2c_client *client)
> {
> /* PMBUS_MFR_READ(0xD0) + Address */
> u8 write_buf[] = {0xd0, 0x44, 0x00};
> u8 read_buf[2];
> int ret;
>
> struct i2c_msg msgs[2] = {
> {
> .addr = client->addr,
> .flags = 0,
> .buf = write_buf,
> .len = sizeof(write_buf),
> },
> {
> .addr = client->addr,
> .flags = I2C_M_RD,
> .buf = read_buf,
> .len = sizeof(read_buf),
> }
> };
>
> ret = i2c_transfer(client->adapter, msgs, 2);
>
drop empty line
> if (ret < 0) {
> dev_err(&client->dev, "%s:%d i2c_transfer failed. %d", __func__, __LINE__, ret);
> return ret;
Return type is bool.
> }
> /* 0x44[15] determines PMBus Operating Mode */
> return !!(read_buf[1] & BIT(7));
> }
>
"The application note AN_2203_PL12_2204_210835 having information on the register map
of TDA38640 is under review. The document will be uploaded to the Infineon website
once the review is completed."
How annoying. Is that really the only way to get that information ?
> Based on svid_mode will init accordingly:
> if (!IS_ENABLED(CONFIG_SENSORS_TDA38640_REGULATOR) || !np || !svid_mode(client))
> return pmbus_do_probe(client, &data->info);
>
This is unnecessary complexity. Just add the local read/write
commands and be done with it.
if (svid_mode(client)) {
data->info.read_byte_data = tda38640_read_byte_data;
data->info.write_byte_data = tda38640_write_byte_data;
}
though it would be useful to error check the return value.
ret = svid_mode();
if (ret < 0)
return ret;
if (ret) {
/* consider adding comments here */
data->info.read_byte_data = tda38640_read_byte_data;
data->info.write_byte_data = tda38640_write_byte_data;
}
Guenter
> dev_dbg(&client->dev, "SVID mode");
>
> Regards,
> Naresh
>>
>> Guenter
>>
>>> The workaround will map the PB_OPERATION_CONTROL_ON bit to the
>>> PB_ON_OFF_CONFIG_EN_PIN_REQ bit of the ON_OFF_CONFIG register.
>>> In combination with the fixed level on the ENABLE pin the regulator
>>> can be controlled as expected.
>>>
>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@...ements.com>
>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@...ements.com>
>>> ---
>>> drivers/hwmon/pmbus/tda38640.c | 95 +++++++++++++++++++++++++++++++++-
>>> 1 file changed, 93 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/tda38640.c b/drivers/hwmon/pmbus/tda38640.c
>>> index 450b0273fb59..9d3b89d9845c 100644
>>> --- a/drivers/hwmon/pmbus/tda38640.c
>>> +++ b/drivers/hwmon/pmbus/tda38640.c
>>> @@ -18,6 +18,72 @@ static const struct regulator_desc __maybe_unused tda38640_reg_desc[] = {
>>> PMBUS_REGULATOR("vout", 0),
>>> };
>>> +struct tda38640_data {
>>> + struct pmbus_driver_info info;
>>> + u32 en_pin_lvl;
>>> +};
>>> +
>>> +#define to_tda38640_data(x) container_of(x, struct tda38640_data, info)
>>> +
>>> +/*
>>> + * Map PB_ON_OFF_CONFIG_POLARITY_HIGH to PB_OPERATION_CONTROL_ON.
>>> + */
>>> +static int tda38640_read_byte_data(struct i2c_client *client, int page, int reg)
>>> +{
>>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>>> + struct tda38640_data *data = to_tda38640_data(info);
>>> + int ret, on_off_config, enabled;
>>> +
>>> + if (reg != PMBUS_OPERATION)
>>> + return -ENODATA;
>>> +
>>> + ret = pmbus_read_byte_data(client, page, reg);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + on_off_config = pmbus_read_byte_data(client, page,
>>> + PMBUS_ON_OFF_CONFIG);
>>> + if (on_off_config < 0)
>>> + return on_off_config;
>>> +
>>> + enabled = !!(on_off_config & PB_ON_OFF_CONFIG_POLARITY_HIGH);
>>> +
>>> + enabled ^= data->en_pin_lvl;
>>> + if (enabled)
>>> + ret &= ~PB_OPERATION_CONTROL_ON;
>>> + else
>>> + ret |= PB_OPERATION_CONTROL_ON;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/*
>>> + * Map PB_OPERATION_CONTROL_ON to PB_ON_OFF_CONFIG_POLARITY_HIGH.
>>> + */
>>> +static int tda38640_write_byte_data(struct i2c_client *client, int page,
>>> + int reg, u8 byte)
>>> +{
>>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>>> + struct tda38640_data *data = to_tda38640_data(info);
>>> + int enable, ret;
>>> +
>>> + if (reg != PMBUS_OPERATION)
>>> + return -ENODATA;
>>> +
>>> + enable = !!(byte & PB_OPERATION_CONTROL_ON);
>>> +
>>> + byte &= ~PB_OPERATION_CONTROL_ON;
>>> + ret = pmbus_write_byte_data(client, page, reg, byte);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + enable ^= data->en_pin_lvl;
>>> +
>>> + return pmbus_update_byte_data(client, page, PMBUS_ON_OFF_CONFIG,
>>> + PB_ON_OFF_CONFIG_POLARITY_HIGH,
>>> + enable ? 0 : PB_ON_OFF_CONFIG_POLARITY_HIGH);
>>> +}
>>> +
>>> static struct pmbus_driver_info tda38640_info = {
>>> .pages = 1,
>>> .format[PSC_VOLTAGE_IN] = linear,
>>> @@ -26,7 +92,6 @@ static struct pmbus_driver_info tda38640_info = {
>>> .format[PSC_CURRENT_IN] = linear,
>>> .format[PSC_POWER] = linear,
>>> .format[PSC_TEMPERATURE] = linear,
>>> -
>>> .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
>>> | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
>>> | PMBUS_HAVE_IIN
>>> @@ -41,7 +106,33 @@ static struct pmbus_driver_info tda38640_info = {
>>> static int tda38640_probe(struct i2c_client *client)
>>> {
>>> - return pmbus_do_probe(client, &tda38640_info);
>>> + struct device *dev = &client->dev;
>>> + struct device_node *np = dev_of_node(dev);
>>> + struct tda38640_data *data;
>>> +
>>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> + if (!data)
>>> + return -ENOMEM;
>>> + memcpy(&data->info, &tda38640_info, sizeof(tda38640_info));
>>> +
>>> + if (!CONFIG_SENSORS_TDA38640_REGULATOR || !np ||
>>> + of_property_read_u32(np, "infineon,en-pin-fixed-level", &data->en_pin_lvl))
>>> + return pmbus_do_probe(client, &data->info);
>>> +
>>> + /*
>>> + * Apply ON_OFF_CONFIG workaround as enabling the regulator using the
>>> + * OPERATION register doesn't work in SVID mode.
>>> + */
>>> + data->info.read_byte_data = tda38640_read_byte_data;
>>> + data->info.write_byte_data = tda38640_write_byte_data;
>>> + /*
>>> + * One should configure PMBUS_ON_OFF_CONFIG here, but
>>> + * PB_ON_OFF_CONFIG_POWERUP_CONTROL, PB_ON_OFF_CONFIG_EN_PIN_REQ and
>>> + * PB_ON_OFF_CONFIG_EN_PIN_REQ are ignored by the device.
>>> + * Only PB_ON_OFF_CONFIG_POLARITY_HIGH has an effect.
>>> + */
>>> +
>>> + return pmbus_do_probe(client, &data->info);
>>> }
>>> static const struct i2c_device_id tda38640_id[] = {
>>
Powered by blists - more mailing lists