| Message ID | 20241024-tps25990-v3-3-b6a6e9d4b506@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: pmbus: add tps25990 efuse support | expand |
On Thu, Oct 24, 2024 at 08:10:37PM +0200, Jerome Brunet wrote: > Add a module parameter to force the write protection mode of pmbus chips. > > 2 protections modes are provided to start with: > * 0: Remove the write protection if possible > * 1: Enable full write protection if possible > > Of course, if the parameter is not provided, the default write protection > status of the pmbus chips is left untouched. > > Suggested-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > Documentation/admin-guide/kernel-parameters.txt | 4 ++ > drivers/hwmon/pmbus/pmbus_core.c | 74 ++++++++++++++++++------- > 2 files changed, 59 insertions(+), 19 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 1518343bbe2237f1d577df5656339d6224b769be..aa79242fe0a9238f618182289f18563ed63cba1c 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -4733,6 +4733,10 @@ > Format: { parport<nr> | timid | 0 } > See also Documentation/admin-guide/parport.rst. > > + pmbus.wp= [HW] PMBus Chips write protection forced mode > + Format: { 0 | 1 } > + See drivers/hwmon/pmbus/pmbus_core.c > + I have always seen that file as applicable for core kernel parameters, not for driver kernel parameters. I can not accept a global change like this without guidance. Please explain why it is desirable to have this parameter documented here and not in driver documentation. > pmtmr= [X86] Manual setup of pmtmr I/O Port. > Override pmtimer IOPort with a hex value. > e.g. pmtmr=0x508 > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 7bdd8f2ffcabc51500437182f411e9826cd7a55d..ce697ca03de01c0e5a352f8f6b72671137721868 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -31,6 +31,20 @@ > #define PMBUS_ATTR_ALLOC_SIZE 32 > #define PMBUS_NAME_SIZE 24 > > +/* > + * PMBus write protect forced mode: > + * PMBus may come up with a variety of write protection configuration. > + * 'pmbus_wp' may be used if a particular write protection is necessary. > + * The ability to actually alter the protection may also depend on the chip > + * so the actual runtime write protection configuration may differ from > + * the requested one. pmbus_core currently support the following value: > + * - 0: write protection removed > + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND > + * registers. Chips essentially become read-only with this. Would it be desirable to also suppport the ability to set the output voltage but not limits (PB_WP_VOUT) ? Guenter
On Fri 01 Nov 2024 at 08:08, Guenter Roeck <linux@roeck-us.net> wrote: > On Thu, Oct 24, 2024 at 08:10:37PM +0200, Jerome Brunet wrote: >> Add a module parameter to force the write protection mode of pmbus chips. >> >> 2 protections modes are provided to start with: >> * 0: Remove the write protection if possible >> * 1: Enable full write protection if possible >> >> Of course, if the parameter is not provided, the default write protection >> status of the pmbus chips is left untouched. >> >> Suggested-by: Guenter Roeck <linux@roeck-us.net> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> Documentation/admin-guide/kernel-parameters.txt | 4 ++ >> drivers/hwmon/pmbus/pmbus_core.c | 74 ++++++++++++++++++------- >> 2 files changed, 59 insertions(+), 19 deletions(-) >> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index 1518343bbe2237f1d577df5656339d6224b769be..aa79242fe0a9238f618182289f18563ed63cba1c 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -4733,6 +4733,10 @@ >> Format: { parport<nr> | timid | 0 } >> See also Documentation/admin-guide/parport.rst. >> >> + pmbus.wp= [HW] PMBus Chips write protection forced mode >> + Format: { 0 | 1 } >> + See drivers/hwmon/pmbus/pmbus_core.c >> + > > I have always seen that file as applicable for core kernel parameters, > not for driver kernel parameters. I can not accept a global change like > this without guidance. Please explain why it is desirable to have this > parameter documented here and not in driver documentation. No reason other than trying to document things the best I can. Other subsystemw are documenting things in here too, I just did the same See 'regulator_ignore_unused' for example. I don't mind dropping this hunk if you prefer it that way. > >> pmtmr= [X86] Manual setup of pmtmr I/O Port. >> Override pmtimer IOPort with a hex value. >> e.g. pmtmr=0x508 >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >> index 7bdd8f2ffcabc51500437182f411e9826cd7a55d..ce697ca03de01c0e5a352f8f6b72671137721868 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -31,6 +31,20 @@ >> #define PMBUS_ATTR_ALLOC_SIZE 32 >> #define PMBUS_NAME_SIZE 24 >> >> +/* >> + * PMBus write protect forced mode: >> + * PMBus may come up with a variety of write protection configuration. >> + * 'pmbus_wp' may be used if a particular write protection is necessary. >> + * The ability to actually alter the protection may also depend on the chip >> + * so the actual runtime write protection configuration may differ from >> + * the requested one. pmbus_core currently support the following value: >> + * - 0: write protection removed >> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND >> + * registers. Chips essentially become read-only with this. > > Would it be desirable to also suppport the ability to set the output voltage > but not limits (PB_WP_VOUT) ? I was starting simple, it is doable sure. It is not something I will be able to test on actual since does not support that. Do you want me to add "2: write protection enable execpt for VOUT_COMMAND." ? > > Guenter
On 11/4/24 00:43, Jerome Brunet wrote: > On Fri 01 Nov 2024 at 08:08, Guenter Roeck <linux@roeck-us.net> wrote: > >> On Thu, Oct 24, 2024 at 08:10:37PM +0200, Jerome Brunet wrote: >>> Add a module parameter to force the write protection mode of pmbus chips. >>> >>> 2 protections modes are provided to start with: >>> * 0: Remove the write protection if possible >>> * 1: Enable full write protection if possible >>> >>> Of course, if the parameter is not provided, the default write protection >>> status of the pmbus chips is left untouched. >>> >>> Suggested-by: Guenter Roeck <linux@roeck-us.net> >>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >>> --- >>> Documentation/admin-guide/kernel-parameters.txt | 4 ++ >>> drivers/hwmon/pmbus/pmbus_core.c | 74 ++++++++++++++++++------- >>> 2 files changed, 59 insertions(+), 19 deletions(-) >>> >>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >>> index 1518343bbe2237f1d577df5656339d6224b769be..aa79242fe0a9238f618182289f18563ed63cba1c 100644 >>> --- a/Documentation/admin-guide/kernel-parameters.txt >>> +++ b/Documentation/admin-guide/kernel-parameters.txt >>> @@ -4733,6 +4733,10 @@ >>> Format: { parport<nr> | timid | 0 } >>> See also Documentation/admin-guide/parport.rst. >>> >>> + pmbus.wp= [HW] PMBus Chips write protection forced mode >>> + Format: { 0 | 1 } >>> + See drivers/hwmon/pmbus/pmbus_core.c >>> + >> >> I have always seen that file as applicable for core kernel parameters, >> not for driver kernel parameters. I can not accept a global change like >> this without guidance. Please explain why it is desirable to have this >> parameter documented here and not in driver documentation. > > No reason other than trying to document things the best I can. > Other subsystemw are documenting things in here too, I just did the same > > See 'regulator_ignore_unused' for example. > Basic rule: You can find examples for everything in the kernel, including dell_smm_hwmon in the same file. That doesn't make it better. It only shows that I did not pay attention. Trying to add all the other 80+ module parameters of hwmon drivers, or all the 5,000+ module parameters from all drivers, into the same file would not scale well. Please document the module parameter in Documentation/hwmon/pmbus-core.rst. Thanks, Guenter
On 11/4/24 00:43, Jerome Brunet wrote: >>> +/* >>> + * PMBus write protect forced mode: >>> + * PMBus may come up with a variety of write protection configuration. >>> + * 'pmbus_wp' may be used if a particular write protection is necessary. >>> + * The ability to actually alter the protection may also depend on the chip >>> + * so the actual runtime write protection configuration may differ from >>> + * the requested one. pmbus_core currently support the following value: >>> + * - 0: write protection removed >>> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND >>> + * registers. Chips essentially become read-only with this. >> >> Would it be desirable to also suppport the ability to set the output voltage >> but not limits (PB_WP_VOUT) ? > > I was starting simple, it is doable sure. > It is not something I will be able to test on actual since does not > support that. > > Do you want me to add "2: write protection enable execpt for VOUT_COMMAND." ? > Please add it. I have a number of PMBus test boards and will be able to test it. Thee are three options, though. Per specification: 1000 0000 Disable all writes except to the WRITE_PROTECT command 0100 0000 Disable all writes except to the WRITE_PROTECT, OPERATION and PAGE commands 0010 0000 Disable all writes except to the WRITE_PROTECT, OPERATION, PAGE, ON_OFF_CONFIG and VOUT_COMMAND commands The driver uses OPERATION and VOUT_COMMAND, so we should have options to disable them separately. It may be desirable, for example, to be able to turn on a regulator but not to change the voltages. Also, since full write protection also disables writes to the page register, the impact of full write protection on multi-page chips needs to be documented. Thanks, Guenter
On Mon 04 Nov 2024 at 06:18, Guenter Roeck <linux@roeck-us.net> wrote: > On 11/4/24 00:43, Jerome Brunet wrote: > >>>> +/* >>>> + * PMBus write protect forced mode: >>>> + * PMBus may come up with a variety of write protection configuration. >>>> + * 'pmbus_wp' may be used if a particular write protection is necessary. >>>> + * The ability to actually alter the protection may also depend on the chip >>>> + * so the actual runtime write protection configuration may differ from >>>> + * the requested one. pmbus_core currently support the following value: >>>> + * - 0: write protection removed >>>> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND >>>> + * registers. Chips essentially become read-only with this. >>> >>> Would it be desirable to also suppport the ability to set the output voltage >>> but not limits (PB_WP_VOUT) ? >> I was starting simple, it is doable sure. >> It is not something I will be able to test on actual since does not >> support that. >> Do you want me to add "2: write protection enable execpt for >> VOUT_COMMAND." ? >> > > Please add it. I have a number of PMBus test boards and will be able to test it. > > Thee are three options, though. Per specification: Any preference for the value mapped to each mode ? Using the one from the spec does not seem practical (32768, 16384, 8192). The bit number, maybe (7, 6, 5) ? or just by order strongest locking ? > > 1000 0000 Disable all writes except to the WRITE_PROTECT command 3 > 0100 0000 Disable all writes except to the WRITE_PROTECT, OPERATION and > PAGE commands 2 > 0010 0000 Disable all writes except to the WRITE_PROTECT, OPERATION, > PAGE, ON_OFF_CONFIG and VOUT_COMMAND commands 1 ? > > The driver uses OPERATION and VOUT_COMMAND, so we should have options > to disable them separately. It may be desirable, for example, to be able > to turn on a regulator but not to change the voltages. Also, since > full write protection also disables writes to the page register, > the impact of full write protection on multi-page chips needs to be > documented. Noted. I'll update the documentation. > > Thanks, > Guenter
On 11/4/24 06:39, Jerome Brunet wrote: > On Mon 04 Nov 2024 at 06:18, Guenter Roeck <linux@roeck-us.net> wrote: > >> On 11/4/24 00:43, Jerome Brunet wrote: >> >>>>> +/* >>>>> + * PMBus write protect forced mode: >>>>> + * PMBus may come up with a variety of write protection configuration. >>>>> + * 'pmbus_wp' may be used if a particular write protection is necessary. >>>>> + * The ability to actually alter the protection may also depend on the chip >>>>> + * so the actual runtime write protection configuration may differ from >>>>> + * the requested one. pmbus_core currently support the following value: >>>>> + * - 0: write protection removed >>>>> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND >>>>> + * registers. Chips essentially become read-only with this. >>>> >>>> Would it be desirable to also suppport the ability to set the output voltage >>>> but not limits (PB_WP_VOUT) ? >>> I was starting simple, it is doable sure. >>> It is not something I will be able to test on actual since does not >>> support that. >>> Do you want me to add "2: write protection enable execpt for >>> VOUT_COMMAND." ? >>> >> >> Please add it. I have a number of PMBus test boards and will be able to test it. >> >> Thee are three options, though. Per specification: > > Any preference for the value mapped to each mode ? Using the one from > the spec does not seem practical (32768, 16384, 8192). > > The bit number, maybe (7, 6, 5) ? > > or just by order strongest locking ? > >> >> 1000 0000 Disable all writes except to the WRITE_PROTECT command > > 3 > >> 0100 0000 Disable all writes except to the WRITE_PROTECT, OPERATION and >> PAGE commands > > 2 > >> 0010 0000 Disable all writes except to the WRITE_PROTECT, OPERATION, >> PAGE, ON_OFF_CONFIG and VOUT_COMMAND commands > > 1 ? > Bit number does not make sense since those are just commands which happen to use individual bits. Also, module parameters should as much as possible be abstract and not reflect HW 1:1. Strongest locking as you suggested as second option makes more sense, since 0 means "no locking". Thanks, Guenter
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 1518343bbe2237f1d577df5656339d6224b769be..aa79242fe0a9238f618182289f18563ed63cba1c 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -4733,6 +4733,10 @@ Format: { parport<nr> | timid | 0 } See also Documentation/admin-guide/parport.rst. + pmbus.wp= [HW] PMBus Chips write protection forced mode + Format: { 0 | 1 } + See drivers/hwmon/pmbus/pmbus_core.c + pmtmr= [X86] Manual setup of pmtmr I/O Port. Override pmtimer IOPort with a hex value. e.g. pmtmr=0x508 diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 7bdd8f2ffcabc51500437182f411e9826cd7a55d..ce697ca03de01c0e5a352f8f6b72671137721868 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -31,6 +31,20 @@ #define PMBUS_ATTR_ALLOC_SIZE 32 #define PMBUS_NAME_SIZE 24 +/* + * PMBus write protect forced mode: + * PMBus may come up with a variety of write protection configuration. + * 'pmbus_wp' may be used if a particular write protection is necessary. + * The ability to actually alter the protection may also depend on the chip + * so the actual runtime write protection configuration may differ from + * the requested one. pmbus_core currently support the following value: + * - 0: write protection removed + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND + * registers. Chips essentially become read-only with this. + */ +static int wp = -1; +module_param(wp, int, 0444); + struct pmbus_sensor { struct pmbus_sensor *next; char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */ @@ -2665,6 +2679,45 @@ static void pmbus_remove_pec(void *dev) device_remove_file(dev, &dev_attr_pec); } +static void pmbus_init_wp(struct i2c_client *client, struct pmbus_data *data) +{ + int ret; + + switch (wp) { + case 0: + _pmbus_write_byte_data(client, 0xff, + PMBUS_WRITE_PROTECT, 0); + break; + + case 1: + _pmbus_write_byte_data(client, 0xff, + PMBUS_WRITE_PROTECT, PB_WP_ALL); + break; + + default: + /* Ignore the other values */ + break; + } + + ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT); + + 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; + } +} + static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, struct pmbus_driver_info *info) { @@ -2718,25 +2771,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, * faults, and we should not try it. Also, in that case, writes into * limit registers need to be disabled. */ - if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) { - ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT); - - 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->flags & PMBUS_NO_WRITE_PROTECT)) + pmbus_init_wp(client, data); ret = i2c_smbus_read_byte_data(client, PMBUS_REVISION); if (ret >= 0)
Add a module parameter to force the write protection mode of pmbus chips. 2 protections modes are provided to start with: * 0: Remove the write protection if possible * 1: Enable full write protection if possible Of course, if the parameter is not provided, the default write protection status of the pmbus chips is left untouched. Suggested-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- Documentation/admin-guide/kernel-parameters.txt | 4 ++ drivers/hwmon/pmbus/pmbus_core.c | 74 ++++++++++++++++++------- 2 files changed, 59 insertions(+), 19 deletions(-)