| Message ID | 20240920-pmbus-wp-v1-4-d679ef31c483@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (pmbus/core) support write protected pmbus regulators | expand |
On 9/20/24 09:47, Jerome Brunet wrote: > Writing PMBus protected registers does succeed from the smbus perspective, > even if the write is ignored by the device and a communication fault is > raised. This fault will silently be caught and cleared by pmbus irq if one > has been registered. > > This means that the regulator call may return succeed although the > operation was ignored. > > With this change, the operation which are not supported will be properly > flagged as such and the regulator framework won't even try to execute them. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/hwmon/pmbus/pmbus.h | 4 ++++ > drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++- > include/linux/pmbus.h | 14 ++++++++++++++ > 3 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h > index 5d5dc774187b..76cff65f38d5 100644 > --- a/drivers/hwmon/pmbus/pmbus.h > +++ b/drivers/hwmon/pmbus/pmbus.h > @@ -481,6 +481,8 @@ struct pmbus_driver_info { > /* Regulator ops */ > > extern const struct regulator_ops pmbus_regulator_ops; > +int pmbus_regulator_init_cb(struct regulator_dev *rdev, > + struct regulator_config *config); > > /* Macros for filling in array of struct regulator_desc */ > #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV) \ > @@ -495,6 +497,7 @@ extern const struct regulator_ops pmbus_regulator_ops; > .n_voltages = _voltages, \ > .uV_step = _step, \ > .min_uV = _min_uV, \ > + .init_cb = pmbus_regulator_init_cb, \ > } > > #define PMBUS_REGULATOR(_name, _id) PMBUS_REGULATOR_STEP(_name, _id, 0, 0, 0) > @@ -510,6 +513,7 @@ extern const struct regulator_ops pmbus_regulator_ops; > .n_voltages = _voltages, \ > .uV_step = _step, \ > .min_uV = _min_uV, \ > + .init_cb = pmbus_regulator_init_cb, \ > } > > #define PMBUS_REGULATOR_ONE(_name) PMBUS_REGULATOR_STEP_ONE(_name, 0, 0, 0) > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 82522fc9090a..363def768888 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -2714,8 +2714,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, > if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) { > ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT); > > - if (ret > 0 && (ret & PB_WP_ANY)) > + switch (ret) { > + case PB_WP_ALL: > + data->flags |= PMBUS_OP_PROTECTED; > + fallthrough; > + case PB_WP_OP: > + data->flags |= PMBUS_VOUT_PROTECTED; > + fallthrough; > + case PB_WP_VOUT: > data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK; > + break; > + > + default: > + /* Ignore manufacturer specific and invalid as well as errors */ > + break; > + } > } > > if (data->info->pages) > @@ -3172,8 +3185,12 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev, > { > struct device *dev = rdev_get_dev(rdev); > struct i2c_client *client = to_i2c_client(dev->parent); > + struct pmbus_data *data = i2c_get_clientdata(client); > int val, low, high; > > + if (data->flags & PMBUS_VOUT_PROTECTED) > + return 0; > + > if (selector >= rdev->desc->n_voltages || > selector < rdev->desc->linear_min_sel) > return -EINVAL; > @@ -3208,6 +3225,22 @@ const struct regulator_ops pmbus_regulator_ops = { > }; > EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS); > > +int pmbus_regulator_init_cb(struct regulator_dev *rdev, > + struct regulator_config *config) > +{ > + struct pmbus_data *data = config->driver_data; > + struct regulation_constraints *constraints = rdev->constraints; > + > + if (data->flags & PMBUS_OP_PROTECTED) > + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; > + > + if (data->flags & PMBUS_VOUT_PROTECTED) > + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; > + > + return 0; > +} > +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); > + I am a bit at loss trying to understand why the constraints can't be passed with the regulator init_data when registering the regulator. Care to explain ? Thanks, Guenter
On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@roeck-us.net> wrote: > On 9/20/24 09:47, Jerome Brunet wrote: >> Writing PMBus protected registers does succeed from the smbus perspective, >> even if the write is ignored by the device and a communication fault is >> raised. This fault will silently be caught and cleared by pmbus irq if one >> has been registered. >> This means that the regulator call may return succeed although the >> operation was ignored. >> With this change, the operation which are not supported will be properly >> flagged as such and the regulator framework won't even try to execute them. >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/hwmon/pmbus/pmbus.h | 4 ++++ >> drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++- >> include/linux/pmbus.h | 14 ++++++++++++++ >> 3 files changed, 52 insertions(+), 1 deletion(-) >> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h >> index 5d5dc774187b..76cff65f38d5 100644 >> --- a/drivers/hwmon/pmbus/pmbus.h >> +++ b/drivers/hwmon/pmbus/pmbus.h >> @@ -481,6 +481,8 @@ struct pmbus_driver_info { >> /* Regulator ops */ >> extern const struct regulator_ops pmbus_regulator_ops; >> +int pmbus_regulator_init_cb(struct regulator_dev *rdev, >> + struct regulator_config *config); >> /* Macros for filling in array of struct regulator_desc */ >> #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV) \ >> @@ -495,6 +497,7 @@ extern const struct regulator_ops pmbus_regulator_ops; >> .n_voltages = _voltages, \ >> .uV_step = _step, \ >> .min_uV = _min_uV, \ >> + .init_cb = pmbus_regulator_init_cb, \ >> } >> #define PMBUS_REGULATOR(_name, _id) PMBUS_REGULATOR_STEP(_name, >> _id, 0, 0, 0) >> @@ -510,6 +513,7 @@ extern const struct regulator_ops pmbus_regulator_ops; >> .n_voltages = _voltages, \ >> .uV_step = _step, \ >> .min_uV = _min_uV, \ >> + .init_cb = pmbus_regulator_init_cb, \ >> } >> #define PMBUS_REGULATOR_ONE(_name) PMBUS_REGULATOR_STEP_ONE(_name, >> 0, 0, 0) >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >> index 82522fc9090a..363def768888 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -2714,8 +2714,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, >> if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) { >> ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT); >> - if (ret > 0 && (ret & PB_WP_ANY)) >> + switch (ret) { >> + case PB_WP_ALL: >> + data->flags |= PMBUS_OP_PROTECTED; >> + fallthrough; >> + case PB_WP_OP: >> + data->flags |= PMBUS_VOUT_PROTECTED; >> + fallthrough; >> + case PB_WP_VOUT: >> data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK; >> + break; >> + >> + default: >> + /* Ignore manufacturer specific and invalid as well as errors */ >> + break; >> + } >> } >> if (data->info->pages) >> @@ -3172,8 +3185,12 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev, >> { >> struct device *dev = rdev_get_dev(rdev); >> struct i2c_client *client = to_i2c_client(dev->parent); >> + struct pmbus_data *data = i2c_get_clientdata(client); >> int val, low, high; >> + if (data->flags & PMBUS_VOUT_PROTECTED) >> + return 0; >> + >> if (selector >= rdev->desc->n_voltages || >> selector < rdev->desc->linear_min_sel) >> return -EINVAL; >> @@ -3208,6 +3225,22 @@ const struct regulator_ops pmbus_regulator_ops = { >> }; >> EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS); >> +int pmbus_regulator_init_cb(struct regulator_dev *rdev, >> + struct regulator_config *config) >> +{ >> + struct pmbus_data *data = config->driver_data; >> + struct regulation_constraints *constraints = rdev->constraints; >> + >> + if (data->flags & PMBUS_OP_PROTECTED) >> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; >> + >> + if (data->flags & PMBUS_VOUT_PROTECTED) >> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); >> + > > I am a bit at loss trying to understand why the constraints can't be passed > with the regulator init_data when registering the regulator. Care to explain ? Sure it something I found while working the problem out. Simply put: * you should be able to, in theory. * currently it would not work * when fixed I think it would still be more complex to do so. ATM, if you pass init_data, it will be ignored on DT platforms in favor of the internal DT parsing of the regulator framework. The DT parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is not set ... including for write protected regulator obviously. This is something that might get fix with this change [1]. Even with that fixed, passing init_data systematically would be convenient only if you plan on skipping DT provided constraints (there are lot of those), or redo the parsing in PMBus. Also a callback can be attached to regulator using the pmbus_ops, and only those. PMBus core just collect regulators provided by the drivers in pmbus_regulator_register(), there could other type of regulators in the provided table (something the tps25990 could use). It is difficult to set/modify the init_data (or the ops) in pmbus_regulator_register() because of that. Using a callback allows to changes almost nothing to what is currently done, so it is safe and address the problem regardless of the platform type. I think the solution is fairly simple for both regulator and pmbus. It could be viewed as just as extending an existing callback. I chose to replace it make things more clear. Changes [1] and this patchset are independent because of how I implement the solution and [1] would still be relevant if this patchset was rejected. It is the reason why I sent it separately. [1]: https://lore.kernel.org/r/20240920-regulator-ignored-data-v1-1-7ea4abfe1b0a@baylibre.com > > Thanks, > Guenter
On 9/21/24 04:32, Jerome Brunet wrote: > On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@roeck-us.net> wrote: [ ... ] >>> +int pmbus_regulator_init_cb(struct regulator_dev *rdev, >>> + struct regulator_config *config) >>> +{ >>> + struct pmbus_data *data = config->driver_data; >>> + struct regulation_constraints *constraints = rdev->constraints; >>> + >>> + if (data->flags & PMBUS_OP_PROTECTED) >>> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; >>> + >>> + if (data->flags & PMBUS_VOUT_PROTECTED) >>> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); >>> + >> >> I am a bit at loss trying to understand why the constraints can't be passed >> with the regulator init_data when registering the regulator. Care to explain ? > > Sure it something I found while working the problem out. > > Simply put: > * you should be able to, in theory. > * currently it would not work > * when fixed I think it would still be more complex to do so. > > ATM, if you pass init_data, it will be ignored on DT platforms in > favor of the internal DT parsing of the regulator framework. The DT > parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is > not set ... including for write protected regulator obviously. > If the chip is read-only, I'd argue that the always-on property should be set in devicetree. After all, that is what it is if the chip is in read-only state. In other words, if always-on is _not_ set in regulator constraints, I'd see that as request to override write-protect in the driver if there is a change request from regulator code. > This is something that might get fix with this change [1]. Even with that > fixed, passing init_data systematically would be convenient only if you > plan on skipping DT provided constraints (there are lot of those), or > redo the parsing in PMBus. > I disagree. I am perfectly fine with DT overriding constraints provided by the driver. The driver can provide its own constraints, and if dt overrides them, so be it. This is not different to the current code. The driver provides a variety of limits to the regulator core. If dt says "No, I don't believe that the minimum voltage is 1.234V, I insist that it is 0.934V", it is not the driver's fault if setting the voltage to anything below 1.234V fails. I would actually argue that this is intentional, and that the driver should not on its own try to override values provided through devicetree. After all, this is a two-way problem: Devicetree may also limit voltage or current ranges to less than the range reported by the driver. Again, if devicetree provides constraints, and those do not include the always-on property, we should see that as request to override any chip write protection in the driver while the command is executed. We should not try to override devicetree constraints. > Also a callback can be attached to regulator using the pmbus_ops, and > only those. PMBus core just collect regulators provided by the drivers > in pmbus_regulator_register(), there could other type of regulators in > the provided table (something the tps25990 could use). It is difficult > to set/modify the init_data (or the ops) in pmbus_regulator_register() > because of that. > The solution would be to copy the init data before manipulating it. I don't see why that would be a problem. After all, the data is not needed after the call to regulator_register() since the regulator code keeps its own copy. > Using a callback allows to changes almost nothing to what is currently > done, so it is safe and address the problem regardless of the > platform type. I think the solution is fairly simple for both regulator > and pmbus. It could be viewed as just as extending an existing > callback. I chose to replace it make things more clear. > At the same time I see it as unnecessary and possibly even harmful. Maybe we have a different understanding of complexity, but I don't think that copying the init data and attaching constraints to it in the PMBus core would be more complex than introducing a new regulator callback and implementing it. Thanks, Guenter
On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote: > On 9/21/24 04:32, Jerome Brunet wrote: >> On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@roeck-us.net> wrote: > [ ... ] > >>>> +int pmbus_regulator_init_cb(struct regulator_dev *rdev, >>>> + struct regulator_config *config) >>>> +{ >>>> + struct pmbus_data *data = config->driver_data; >>>> + struct regulation_constraints *constraints = rdev->constraints; >>>> + >>>> + if (data->flags & PMBUS_OP_PROTECTED) >>>> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; >>>> + >>>> + if (data->flags & PMBUS_VOUT_PROTECTED) >>>> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); >>>> + >>> >>> I am a bit at loss trying to understand why the constraints can't be passed >>> with the regulator init_data when registering the regulator. Care to explain ? >> Sure it something I found while working the problem out. >> Simply put: >> * you should be able to, in theory. >> * currently it would not work >> * when fixed I think it would still be more complex to do so. >> ATM, if you pass init_data, it will be ignored on DT platforms in >> favor of the internal DT parsing of the regulator framework. The DT >> parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is >> not set ... including for write protected regulator obviously. >> > > If the chip is read-only, I'd argue that the always-on property should > be set in devicetree. After all, that is what it is if the chip is > in read-only state. I'm not touching that. If always-on is set DT, REGULATOR_CHANGE_STATUS won't be set. What I'm proposing does not change that. > In other words, if always-on is _not_ set in > regulator constraints, I'd see that as request to override write-protect > in the driver if there is a change request from regulator code. That's very much different from what we initially discussed. It can certainly be done, what is proposed here already does 90% of the job in that direction. However, I'm not sure that is what people intended when they did not put anything. A chip that was previously locked, would be unlocked following such change. It's an important behaviour change. > >> This is something that might get fix with this change [1]. Even with that >> fixed, passing init_data systematically would be convenient only if you >> plan on skipping DT provided constraints (there are lot of those), or >> redo the parsing in PMBus. >> > > I disagree. I am perfectly fine with DT overriding constraints provided > by the driver. The driver can provide its own constraints, and if dt > overrides them, so be it. That's not what the regulator framework does. At the moment, it is DT and nothing else. After the linked change, it would be DT if no init_data is passed - otherwise, the init_data. If a something in between, whichever the one you want to give priority to, that will have to re-implemented on the caller side. This is what I meant by redo the parsing on pmbus side. > This is not different to the current code. > The driver provides a variety of limits to the regulator core. > If dt says "No, I don't believe that the minimum voltage is 1.234V, I > insist that it is 0.934V", it is not the driver's fault if setting > the voltage to anything below 1.234V fails. I would actually argue > that this is intentional, and that the driver should not on its own > try to override values provided through devicetree. After all, this > is a two-way problem: Devicetree may also limit voltage or current > ranges to less than the range reported by the driver. It goes way beyond what I'm proposing. The only thing done here is something you simply cannot put in DT because DT is static. Following init, if the chip write protected, REGULATOR_CHANGE_STATUS should not be set, regardless of what is in DT. If it is not set, I'm not adding it. Also, what I'm proposing does not get in the way of DT, or anything else, providing constraints. What I propose allow to make adjustement in the HW based on the constraint, if this is what you want to do. It also allows to update the constaints based on what the HW actually is. If the chip cannot be written, regulator needs to know. > > Again, if devicetree provides constraints, and those do not include > the always-on property, we should see that as request to override any > chip write protection in the driver while the command is executed. I'm fine adding that. The init callback is also the place to do it. As pointed above, this may not be what current user intended. Also, implementing that means that, for a chip with multiple pmbus regulators, a single always-on missing will unlock the chip. Also pmbus will need to adjusted so the hwmon attributes are registered after the regulators, to get the permission right. > We should not try to override devicetree constraints. I don't think I am. I'm just reading the chip state and adjusting the constraint. Even after implementing what is suggested above, it will still be necessary to readback and adjust the constraint based the read protection. Unlock is not guranteed to succeed, the chip may be permanently lock. Some provide the option to do that. > >> Also a callback can be attached to regulator using the pmbus_ops, and >> only those. PMBus core just collect regulators provided by the drivers >> in pmbus_regulator_register(), there could other type of regulators in >> the provided table (something the tps25990 could use). It is difficult >> to set/modify the init_data (or the ops) in pmbus_regulator_register() >> because of that. >> > > The solution would be to copy the init data before manipulating it. > I don't see why that would be a problem. After all, the data is not needed > after the call to regulator_register() since the regulator code keeps > its own copy. The type regulator being registered is not known at this point, unless you to use something as weak as comparing the ops pointer to pmbus_ops. Without that, I don't really seee how you safely manipulate the constraints. If it is not the generic pmbus regulator, the constraints should not be touched by pmbus_core. > >> Using a callback allows to changes almost nothing to what is currently >> done, so it is safe and address the problem regardless of the >> platform type. I think the solution is fairly simple for both regulator >> and pmbus. It could be viewed as just as extending an existing >> callback. I chose to replace it make things more clear. >> > > At the same time I see it as unnecessary and possibly even harmful. > Maybe we have a different understanding of complexity, but I don't > think that copying the init data and attaching constraints to it in > the PMBus core would be more complex than introducing a new regulator > callback and implementing it. There is an infinity of ways to merge the constraints between what regulator_register() gets and what the framework will parse DT. It is impossible to get right on the regulator side. Regulator is just picking one and that's it (always DT at the moment). That's the only sane thing the regulator framework can do IMO. If you want a merge between runtime based constraints, such as write protect, and possibly DT - all that ready before regulator registration in init_data, then yes, a lot of the DT parsing will have to redone in PMBus before regulator_register is called. It is also something that will have to be done only for regulator using the pmbus_ops(). I don't really see how to make that happen in pmbus_regulator_register() without some sort of callback. > > Thanks, > Guenter
On 9/21/24 09:49, Jerome Brunet wrote: > On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote: > >> On 9/21/24 04:32, Jerome Brunet wrote: >>> On Fri 20 Sep 2024 at 14:13, Guenter Roeck <linux@roeck-us.net> wrote: >> [ ... ] >> >>>>> +int pmbus_regulator_init_cb(struct regulator_dev *rdev, >>>>> + struct regulator_config *config) >>>>> +{ >>>>> + struct pmbus_data *data = config->driver_data; >>>>> + struct regulation_constraints *constraints = rdev->constraints; >>>>> + >>>>> + if (data->flags & PMBUS_OP_PROTECTED) >>>>> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; >>>>> + >>>>> + if (data->flags & PMBUS_VOUT_PROTECTED) >>>>> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; >>>>> + >>>>> + return 0; >>>>> +} >>>>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); >>>>> + >>>> >>>> I am a bit at loss trying to understand why the constraints can't be passed >>>> with the regulator init_data when registering the regulator. Care to explain ? >>> Sure it something I found while working the problem out. >>> Simply put: >>> * you should be able to, in theory. >>> * currently it would not work >>> * when fixed I think it would still be more complex to do so. >>> ATM, if you pass init_data, it will be ignored on DT platforms in >>> favor of the internal DT parsing of the regulator framework. The DT >>> parsing sets REGULATOR_CHANGE_STATUS as long as the always-on prop is >>> not set ... including for write protected regulator obviously. >>> >> >> If the chip is read-only, I'd argue that the always-on property should >> be set in devicetree. After all, that is what it is if the chip is >> in read-only state. > > I'm not touching that. If always-on is set DT, REGULATOR_CHANGE_STATUS > won't be set. What I'm proposing does not change that. > >> In other words, if always-on is _not_ set in >> regulator constraints, I'd see that as request to override write-protect >> in the driver if there is a change request from regulator code. > > That's very much different from what we initially discussed. It can > certainly be done, what is proposed here already does 90% of the job in > that direction. However, I'm not sure that is what people intended when > they did not put anything. A chip that was previously locked, would be > unlocked following such change. It's an important behaviour change. > >> >>> This is something that might get fix with this change [1]. Even with that >>> fixed, passing init_data systematically would be convenient only if you >>> plan on skipping DT provided constraints (there are lot of those), or >>> redo the parsing in PMBus. >>> >> >> I disagree. I am perfectly fine with DT overriding constraints provided >> by the driver. The driver can provide its own constraints, and if dt >> overrides them, so be it. > > That's not what the regulator framework does. At the moment, it is DT > and nothing else. After the linked change, it would be DT if no > init_data is passed - otherwise, the init_data. > > If a something in between, whichever the one you want to give priority > to, that will have to re-implemented on the caller side. > This is what I meant by redo the parsing on pmbus side. > >> This is not different to the current code. >> The driver provides a variety of limits to the regulator core. >> If dt says "No, I don't believe that the minimum voltage is 1.234V, I >> insist that it is 0.934V", it is not the driver's fault if setting >> the voltage to anything below 1.234V fails. I would actually argue >> that this is intentional, and that the driver should not on its own >> try to override values provided through devicetree. After all, this >> is a two-way problem: Devicetree may also limit voltage or current >> ranges to less than the range reported by the driver. > > It goes way beyond what I'm proposing. > The only thing done here is something you simply cannot put in DT > because DT is static. Following init, if the chip write protected, > REGULATOR_CHANGE_STATUS should not be set, regardless of what is in DT. > If it is not set, I'm not adding it. > > Also, what I'm proposing does not get in the way of DT, or anything > else, providing constraints. What I propose allow to make adjustement in > the HW based on the constraint, if this is what you want to do. It also > allows to update the constaints based on what the HW actually is. > If the chip cannot be written, regulator needs to know. > >> >> Again, if devicetree provides constraints, and those do not include >> the always-on property, we should see that as request to override any >> chip write protection in the driver while the command is executed. > > I'm fine adding that. The init callback is also the place to do it. > As pointed above, this may not be what current user intended. Also, > implementing that means that, for a chip with multiple pmbus regulators, > a single always-on missing will unlock the chip. Also pmbus will need to > adjusted so the hwmon attributes are registered after the regulators, to > get the permission right. > >> We should not try to override devicetree constraints. > > I don't think I am. I'm just reading the chip state and adjusting the > constraint. Even after implementing what is suggested above, it will > still be necessary to readback and adjust the constraint based the > read protection. Unlock is not guranteed to succeed, the chip may be > permanently lock. Some provide the option to do that. > >> >>> Also a callback can be attached to regulator using the pmbus_ops, and >>> only those. PMBus core just collect regulators provided by the drivers >>> in pmbus_regulator_register(), there could other type of regulators in >>> the provided table (something the tps25990 could use). It is difficult >>> to set/modify the init_data (or the ops) in pmbus_regulator_register() >>> because of that. >>> >> >> The solution would be to copy the init data before manipulating it. >> I don't see why that would be a problem. After all, the data is not needed >> after the call to regulator_register() since the regulator code keeps >> its own copy. > > The type regulator being registered is not known at this point, unless > you to use something as weak as comparing the ops pointer to > pmbus_ops. Without that, I don't really seee how you safely manipulate > the constraints. If it is not the generic pmbus regulator, the > constraints should not be touched by pmbus_core. > >> >>> Using a callback allows to changes almost nothing to what is currently >>> done, so it is safe and address the problem regardless of the >>> platform type. I think the solution is fairly simple for both regulator >>> and pmbus. It could be viewed as just as extending an existing >>> callback. I chose to replace it make things more clear. >>> >> >> At the same time I see it as unnecessary and possibly even harmful. >> Maybe we have a different understanding of complexity, but I don't >> think that copying the init data and attaching constraints to it in >> the PMBus core would be more complex than introducing a new regulator >> callback and implementing it. > > There is an infinity of ways to merge the constraints between what > regulator_register() gets and what the framework will parse DT. It is > impossible to get right on the regulator side. Regulator is just picking > one and that's it (always DT at the moment). That's the only sane thing > the regulator framework can do IMO. > > If you want a merge between runtime based constraints, such as write > protect, and possibly DT - all that ready before regulator registration > in init_data, then yes, a lot of the DT parsing will have to redone in > PMBus before regulator_register is called. It is also something that > will have to be done only for regulator using the pmbus_ops(). I don't > really see how to make that happen in pmbus_regulator_register() without > some sort of callback. > Looks like we'll have to agree to disagree. Let's see what the regulator maintainer has to say about your callback. Guenter
On Sat, Sep 21, 2024 at 06:49:34PM +0200, Jerome Brunet wrote: > On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote: > > In other words, if always-on is _not_ set in > > regulator constraints, I'd see that as request to override write-protect > > in the driver if there is a change request from regulator code. > That's very much different from what we initially discussed. It can > certainly be done, what is proposed here already does 90% of the job in > that direction. However, I'm not sure that is what people intended when > they did not put anything. A chip that was previously locked, would be > unlocked following such change. It's an important behaviour change. The general approach we take for regulators is to not touch the hardware state unless we were explicitly asked to do something by firmware configuration. The theory is that this avoids us doing anything that causes physical damage by mistake. > >> This is something that might get fix with this change [1]. Even with that > >> fixed, passing init_data systematically would be convenient only if you > >> plan on skipping DT provided constraints (there are lot of those), or > >> redo the parsing in PMBus. > > I disagree. I am perfectly fine with DT overriding constraints provided > > by the driver. The driver can provide its own constraints, and if dt > > overrides them, so be it. > That's not what the regulator framework does. At the moment, it is DT > and nothing else. After the linked change, it would be DT if no > init_data is passed - otherwise, the init_data. > If a something in between, whichever the one you want to give priority > to, that will have to re-implemented on the caller side. > This is what I meant by redo the parsing on pmbus side. Right, and I've got a feeling that any attempt to combine constraints is going to need to be done in a case by case manner since what's tasteful is going to vary depending on how much we trust the various sources of information. > It goes way beyond what I'm proposing. > The only thing done here is something you simply cannot put in DT > because DT is static. Following init, if the chip write protected, > REGULATOR_CHANGE_STATUS should not be set, regardless of what is in DT. > If it is not set, I'm not adding it. > Also, what I'm proposing does not get in the way of DT, or anything > else, providing constraints. What I propose allow to make adjustement in > the HW based on the constraint, if this is what you want to do. It also > allows to update the constaints based on what the HW actually is. > If the chip cannot be written, regulator needs to know. So, I know we talked about this a bit on IRC but I didn't register the specific use case. Now I see that it's coming down to the fact that the chip is simply write protected I'm wondering if it might not be simpler to handle this at the ops level rather than the constraints level. When registering the driver could check for write protection and then instead of registering the normal operations register an alternative set with the relevant write operations removed. That would have the same effect that you're going for AFAICT. Sorry for not thinking of it earlier. > > We should not try to override devicetree constraints. > I don't think I am. I'm just reading the chip state and adjusting the > constraint. Even after implementing what is suggested above, it will > still be necessary to readback and adjust the constraint based the > read protection. Unlock is not guranteed to succeed, the chip may be > permanently lock. Some provide the option to do that. I'm not familiar with this hardware so I'll defer to the two of you on what's tasteful with regard to handling this, based on the above it might be a per device thing depending on how reversable the write protection is. It looks like currently we don't change this at runtime but I might not be looking properly?
On Fri, Sep 20, 2024 at 06:47:05PM +0200, Jerome Brunet wrote: > +int pmbus_regulator_init_cb(struct regulator_dev *rdev, > + struct regulator_config *config) > +{ > + struct pmbus_data *data = config->driver_data; > + struct regulation_constraints *constraints = rdev->constraints; > + > + if (data->flags & PMBUS_OP_PROTECTED) > + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; > + > + if (data->flags & PMBUS_VOUT_PROTECTED) > + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; > + > + return 0; > +} > +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); I'm fairly comfortable with this from a regulator point of view, modulo the suggestion I posted in the other message about registering separate ops. The fact that there's three combinations of ops is annoying but doesn't feel too bad, though I didn't actually write it out so perhaps it looks horrible. In general removing permissions is safe, and without separate steps to remove write protect (which I see in your patch 5) the writes wouldn't actually work anyway.
On 9/23/24 06:21, Mark Brown wrote: > On Fri, Sep 20, 2024 at 06:47:05PM +0200, Jerome Brunet wrote: > >> +int pmbus_regulator_init_cb(struct regulator_dev *rdev, >> + struct regulator_config *config) >> +{ >> + struct pmbus_data *data = config->driver_data; >> + struct regulation_constraints *constraints = rdev->constraints; >> + >> + if (data->flags & PMBUS_OP_PROTECTED) >> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; >> + >> + if (data->flags & PMBUS_VOUT_PROTECTED) >> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); > > I'm fairly comfortable with this from a regulator point of view, modulo > the suggestion I posted in the other message about registering separate > ops. The fact that there's three combinations of ops is annoying but > doesn't feel too bad, though I didn't actually write it out so perhaps > it looks horrible. In general removing permissions is safe, and without > separate steps to remove write protect (which I see in your patch 5) the > writes wouldn't actually work anyway. I still consider the callback to be unnecessary, but I don't really have time to implement a better solution myself. If you accept the regulator patches, I'll have another look at the series as-is. Guenter
On Mon 23 Sep 2024 at 14:21, Mark Brown <broonie@kernel.org> wrote: > On Sat, Sep 21, 2024 at 06:49:34PM +0200, Jerome Brunet wrote: >> On Sat 21 Sep 2024 at 08:22, Guenter Roeck <linux@roeck-us.net> wrote: > >> > In other words, if always-on is _not_ set in >> > regulator constraints, I'd see that as request to override write-protect >> > in the driver if there is a change request from regulator code. > >> That's very much different from what we initially discussed. It can >> certainly be done, what is proposed here already does 90% of the job in >> that direction. However, I'm not sure that is what people intended when >> they did not put anything. A chip that was previously locked, would be >> unlocked following such change. It's an important behaviour change. > > The general approach we take for regulators is to not touch the hardware > state unless we were explicitly asked to do something by firmware > configuration. The theory is that this avoids us doing anything that > causes physical damage by mistake. > >> >> This is something that might get fix with this change [1]. Even with that >> >> fixed, passing init_data systematically would be convenient only if you >> >> plan on skipping DT provided constraints (there are lot of those), or >> >> redo the parsing in PMBus. > >> > I disagree. I am perfectly fine with DT overriding constraints provided >> > by the driver. The driver can provide its own constraints, and if dt >> > overrides them, so be it. > >> That's not what the regulator framework does. At the moment, it is DT >> and nothing else. After the linked change, it would be DT if no >> init_data is passed - otherwise, the init_data. > >> If a something in between, whichever the one you want to give priority >> to, that will have to re-implemented on the caller side. >> This is what I meant by redo the parsing on pmbus side. > > Right, and I've got a feeling that any attempt to combine constraints is > going to need to be done in a case by case manner since what's tasteful > is going to vary depending on how much we trust the various sources of > information. > >> It goes way beyond what I'm proposing. >> The only thing done here is something you simply cannot put in DT >> because DT is static. Following init, if the chip write protected, >> REGULATOR_CHANGE_STATUS should not be set, regardless of what is in DT. >> If it is not set, I'm not adding it. > >> Also, what I'm proposing does not get in the way of DT, or anything >> else, providing constraints. What I propose allow to make adjustement in >> the HW based on the constraint, if this is what you want to do. It also >> allows to update the constaints based on what the HW actually is. >> If the chip cannot be written, regulator needs to know. > > So, I know we talked about this a bit on IRC but I didn't register the > specific use case. Now I see that it's coming down to the fact that the > chip is simply write protected I'm wondering if it might not be simpler > to handle this at the ops level rather than the constraints level. When > registering the driver could check for write protection and then instead > of registering the normal operations register an alternative set with > the relevant write operations removed. That would have the same effect > that you're going for AFAICT. Sorry for not thinking of it earlier. Actually I thaught about it, that was my first idea. There is tiny difference between the 2 approach: * A regulator that does not provide enable()/disable() is considered always-on, so it is not really checked if it is enabled or not * A regulator that does provide enable()/disable() but disallow status change will be checked with is_enabled() In the second case, we will pick up on regulator that is disabled and we can't enable. In the first case, I don't think we do. I don't know if it is a bug of not but that makes the 2nd case more correct for what we do with pmbus regs I think The other thing, although is more a pmbus implemantion consideration, is that the type regulator is opaque in pmbus_regulator_register. Different types could be registered so manipulating the ops is tricky. That's where a callback is helpful If building the ops dynamically is the preferred way, I'll find a way to make it happen. I'll need to way to identify which one need it, loose the 'const' qualifier in a lot of place, etc ... that will be a bit hackish I'm afraid. > >> > We should not try to override devicetree constraints. > >> I don't think I am. I'm just reading the chip state and adjusting the >> constraint. Even after implementing what is suggested above, it will >> still be necessary to readback and adjust the constraint based the >> read protection. Unlock is not guranteed to succeed, the chip may be >> permanently lock. Some provide the option to do that. > > I'm not familiar with this hardware so I'll defer to the two of you on > what's tasteful with regard to handling this, based on the above it > might be a per device thing depending on how reversable the write > protection is. It looks like currently we don't change this at runtime > but I might not be looking properly? At the moment, it does not. With this patch it might but only a registration. We've been discussing other modes but it would not impact regulator I think.
On Mon, Sep 23, 2024 at 06:53:07PM +0200, Jerome Brunet wrote: > On Mon 23 Sep 2024 at 14:21, Mark Brown <broonie@kernel.org> wrote: > > So, I know we talked about this a bit on IRC but I didn't register the > > specific use case. Now I see that it's coming down to the fact that the > > chip is simply write protected I'm wondering if it might not be simpler > > to handle this at the ops level rather than the constraints level. When > > registering the driver could check for write protection and then instead > > of registering the normal operations register an alternative set with > > the relevant write operations removed. That would have the same effect > > that you're going for AFAICT. Sorry for not thinking of it earlier. > Actually I thaught about it, that was my first idea. > There is tiny difference between the 2 approach: > * A regulator that does not provide enable()/disable() is considered > always-on, so it is not really checked if it is enabled or not > * A regulator that does provide enable()/disable() but disallow status > change will be checked with is_enabled() > In the second case, we will pick up on regulator that is disabled and we > can't enable. In the first case, I don't think we do. I don't know if it > is a bug of not but that makes the 2nd case more correct for what we do > with pmbus regs I think I think for that we should just always use the is_enabled() operation if it's available. This is simply an oversight caused by not imagining a case where a regulator could have an enable control which is locked out like this, the normal case is that either you can control enable or the regulator is always on. > The other thing, although is more a pmbus implemantion consideration, > is that the type regulator is opaque in > pmbus_regulator_register. Different types could be registered so > manipulating the ops is tricky. That's where a callback is helpful > If building the ops dynamically is the preferred way, I'll find a way to > make it happen. I'll need to way to identify which one need it, loose > the 'const' qualifier in a lot of place, etc ... that will be a bit > hackish I'm afraid. With only a limited set of options it might be better to just have a set of static structs and pick one to register (some other drivers do this based on hardware options), but the number of combinations with this is getting a bit big for that.
On Mon 23 Sep 2024 at 09:44, Guenter Roeck <linux@roeck-us.net> wrote: > On 9/23/24 06:21, Mark Brown wrote: >> On Fri, Sep 20, 2024 at 06:47:05PM +0200, Jerome Brunet wrote: >> >>> +int pmbus_regulator_init_cb(struct regulator_dev *rdev, >>> + struct regulator_config *config) >>> +{ >>> + struct pmbus_data *data = config->driver_data; >>> + struct regulation_constraints *constraints = rdev->constraints; >>> + >>> + if (data->flags & PMBUS_OP_PROTECTED) >>> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; >>> + >>> + if (data->flags & PMBUS_VOUT_PROTECTED) >>> + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); >> I'm fairly comfortable with this from a regulator point of view, modulo >> the suggestion I posted in the other message about registering separate >> ops. The fact that there's three combinations of ops is annoying but >> doesn't feel too bad, though I didn't actually write it out so perhaps >> it looks horrible. In general removing permissions is safe, and without >> separate steps to remove write protect (which I see in your patch 5) the >> writes wouldn't actually work anyway. > > > I still consider the callback to be unnecessary, but I don't really have time > to implement a better solution myself. If you accept the regulator patches, > I'll have another look at the series as-is. I'll group the regulator patches and resend to Mark, adjusted as requested. Guenter, should I the resend the hwmon patches here grouped with the tps25990 series ? Or is there something you'd like me change before ? > > Guenter
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h index 5d5dc774187b..76cff65f38d5 100644 --- a/drivers/hwmon/pmbus/pmbus.h +++ b/drivers/hwmon/pmbus/pmbus.h @@ -481,6 +481,8 @@ struct pmbus_driver_info { /* Regulator ops */ extern const struct regulator_ops pmbus_regulator_ops; +int pmbus_regulator_init_cb(struct regulator_dev *rdev, + struct regulator_config *config); /* Macros for filling in array of struct regulator_desc */ #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV) \ @@ -495,6 +497,7 @@ extern const struct regulator_ops pmbus_regulator_ops; .n_voltages = _voltages, \ .uV_step = _step, \ .min_uV = _min_uV, \ + .init_cb = pmbus_regulator_init_cb, \ } #define PMBUS_REGULATOR(_name, _id) PMBUS_REGULATOR_STEP(_name, _id, 0, 0, 0) @@ -510,6 +513,7 @@ extern const struct regulator_ops pmbus_regulator_ops; .n_voltages = _voltages, \ .uV_step = _step, \ .min_uV = _min_uV, \ + .init_cb = pmbus_regulator_init_cb, \ } #define PMBUS_REGULATOR_ONE(_name) PMBUS_REGULATOR_STEP_ONE(_name, 0, 0, 0) diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 82522fc9090a..363def768888 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -2714,8 +2714,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) { ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT); - if (ret > 0 && (ret & PB_WP_ANY)) + switch (ret) { + case PB_WP_ALL: + data->flags |= PMBUS_OP_PROTECTED; + fallthrough; + case PB_WP_OP: + data->flags |= PMBUS_VOUT_PROTECTED; + fallthrough; + case PB_WP_VOUT: data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK; + break; + + default: + /* Ignore manufacturer specific and invalid as well as errors */ + break; + } } if (data->info->pages) @@ -3172,8 +3185,12 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev, { struct device *dev = rdev_get_dev(rdev); struct i2c_client *client = to_i2c_client(dev->parent); + struct pmbus_data *data = i2c_get_clientdata(client); int val, low, high; + if (data->flags & PMBUS_VOUT_PROTECTED) + return 0; + if (selector >= rdev->desc->n_voltages || selector < rdev->desc->linear_min_sel) return -EINVAL; @@ -3208,6 +3225,22 @@ const struct regulator_ops pmbus_regulator_ops = { }; EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS); +int pmbus_regulator_init_cb(struct regulator_dev *rdev, + struct regulator_config *config) +{ + struct pmbus_data *data = config->driver_data; + struct regulation_constraints *constraints = rdev->constraints; + + if (data->flags & PMBUS_OP_PROTECTED) + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS; + + if (data->flags & PMBUS_VOUT_PROTECTED) + constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE; + + return 0; +} +EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS); + static int pmbus_regulator_register(struct pmbus_data *data) { struct device *dev = data->dev; diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h index fa9f08164c36..884040e1383b 100644 --- a/include/linux/pmbus.h +++ b/include/linux/pmbus.h @@ -73,6 +73,20 @@ */ #define PMBUS_USE_COEFFICIENTS_CMD BIT(5) +/* + * PMBUS_OP_PROTECTED + * Set if the chip OPERATION command is protected and protection is not + * determined by the standard WRITE_PROTECT command. + */ +#define PMBUS_OP_PROTECTED BIT(6) + +/* + * PMBUS_VOUT_PROTECTED + * Set if the chip VOUT_COMMAND command is protected and protection is not + * determined by the standard WRITE_PROTECT command. + */ +#define PMBUS_VOUT_PROTECTED BIT(7) + struct pmbus_platform_data { u32 flags; /* Device specific flags */
Writing PMBus protected registers does succeed from the smbus perspective, even if the write is ignored by the device and a communication fault is raised. This fault will silently be caught and cleared by pmbus irq if one has been registered. This means that the regulator call may return succeed although the operation was ignored. With this change, the operation which are not supported will be properly flagged as such and the regulator framework won't even try to execute them. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/hwmon/pmbus/pmbus.h | 4 ++++ drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++- include/linux/pmbus.h | 14 ++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-)