Thanks, 5.4 kernel, I'll try >Hi > >On 3/18/24 10:48 AM, Mika Westerberg wrote: >> Hi, >> >> On Mon, Mar 18, 2024 at 04:24:46PM +0800, Youwan Wang wrote: >>> fix crash because of null pointers >>> >>> [ 190.538113] kernel NULL pointer dereference at virtual address 0000000000000000 >>> [ 190.538115] Mem abort info: >>> [ 190.538116] ESR = 0x96000004 >>> [ 190.538118] Exception class = DABT (current EL), IL = 32 bits >>> [ 190.538119] SET = 0, FnV = 0 >>> [ 190.538120] EA = 0, S1PTW = 0 >>> [ 190.538121] Data abort info: >>> [ 190.538122] ISV = 0, ISS = 0x00000004 >>> [ 190.538123] CM = 0, WnR = 0 >>> [ 190.538125] user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000cf937cf2 >>> [ 190.538126] [0000000000000000] pgd=0000000000000000 >>> [ 190.538129] Internal error: Oops: 96000004 [#1] SMP >>> [ 190.538172] Process swapper/0 (pid: 0, stack limit = 0x000000000179ba77) >>> [ 190.538175] CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Tainted: G >>> [ 190.538178] pstate: 40000085 (nZcv daIf -PAN -UAO) >>> [ 190.538183] pc : i2c_dw_isr+0x2e4/0x614 [i2c_designware_core] >>> [ 190.538185] lr : i2c_dw_isr+0x9c/0x614 [i2c_designware_core] >>> [ 190.538186] sp : ffff000008003e40 >>> [ 190.538187] x29: ffff000008003e40 x28: ffffd9cfad997200 >>> [ 190.538189] x27: ffff5f18a4f2c018 x26: 0000000000000000 >>> [ 190.538191] x25: ffff5f18a52d9fe8 x24: 0000000000000010 >>> [ 190.538193] x23: 0000000000000000 x22: 0000000000000000 >>> [ 190.538194] x21: 0000000000000510 x20: 0000000000000000 >>> [ 190.538196] x19: ffffd9cfa08d6080 x18: 00000000fffffffe >>> [ 190.538197] x17: 0000000000000000 x16: 0000000000000000 >>> [ 190.538199] x15: 0000000000000000 x14: 0000000000000000 >>> [ 190.538200] x13: 0000000000000000 x12: 000000000000028d >>> [ 190.538202] x11: 0000000000000040 x10: ffff5f18a52fe340 >>> [ 190.538203] x9 : ffffd9cfaf4a28f0 x8 : 0000000000000000 >>> [ 190.538205] x7 : ffffd9cfaf401b88 x6 : ffffd9cfaf401b60 >>> [ 190.538206] x5 : 0000000000000000 x4 : 0000000000000000 >>> [ 190.538208] x3 : 0000000000000000 x2 : ffff000008125000 >>> [ 190.538209] x1 : 0000000000000000 x0 : 0000000000000000 >>> [ 190.538211] Call trace: >>> [ 190.538213] i2c_dw_isr+0x2e4/0x614 [i2c_designware_core] >>> [ 190.538218] __handle_irq_event_percpu+0x68/0x230 >>> [ 190.538219] handle_irq_event+0x6c/0x130 >>> [ 190.538222] handle_fasteoi_irq+0xc0/0x220 >>> [ 190.538223] __handle_domain_irq+0x80/0xe0 >>> [ 190.538226] gic_handle_irq+0x84/0x188 >>> [ 190.538227] el1_irq+0xb0/0x140 >>> [ 190.538229] arch_cpu_idle+0x38/0x1c0 >>> [ 190.538232] do_idle+0x238/0x2a4 >>> [ 190.538234] cpu_startup_entry+0x2c/0x50 >>> [ 190.538237] rest_init+0xbc/0xc8 >>> [ 190.538240] start_kernel+0x4a8/0x4dc >>> [ 190.538242] Code: 937c7c80 b940727a 8b0002c1 f9403e77 (78606ac0) >>> [ 190.538248] SMP: stopping secondary CPUs >>> [ 190.538400] Starting crashdump kernel... >>> [ 190.538406] Bye! >>> >>> Signed-off-by: Youwan Wang <youwan@nfschina.com> >>> --- >>> drivers/i2c/busses/i2c-designware-master.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c >>> index ca1035e010c7..849e8a3e85ed 100644 >>> --- a/drivers/i2c/busses/i2c-designware-master.c >>> +++ b/drivers/i2c/busses/i2c-designware-master.c >>> @@ -429,12 +429,17 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev) >>> struct i2c_msg *msgs = dev->msgs; >>> u32 intr_mask; >>> int tx_limit, rx_limit; >>> - u32 addr = msgs[dev->msg_write_idx].addr; >>> + u32 addr; >>> u32 buf_len = dev->tx_buf_len; >>> u8 *buf = dev->tx_buf; >>> bool need_restart = false; >>> unsigned int flr; >>> >>> + if (WARN_ON(!msgs)) >>> + return; >> >> Instead of treating the symptom, I suggest figuring out why this happens >> in the first place. If the controller interrupt is enabled and triggered >> it is expected that there is some transfer going on and dev->msgs != >> NULL. > >What is the kernel version this happens? Does it include commit >301c8f5c32c8 ("i2c: designware: Fix handling of real but unexpected >device interrupts")?
Hi Radu, kernel test robot noticed the following build errors: [auto build test ERROR on groeck-staging/hwmon-next] [also build test ERROR on robh/for-next linus/master v6.8 next-20240318] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Radu-Sabau/dt-bindings-hwmon-pmbus-adp1050-add-bindings/20240318-202619 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20240318112140.385244-3-radu.sabau%40analog.com patch subject: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240319/202403190800.h8cSGROp-lkp@intel.com/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 8f68022f8e6e54d1aeae4ed301f5a015963089b7) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240319/202403190800.h8cSGROp-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202403190800.h8cSGROp-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from drivers/hwmon/pmbus/adp1050.c:9: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 547 | val = __raw_readb(PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) | ^ In file included from drivers/hwmon/pmbus/adp1050.c:9: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) | ^ In file included from drivers/hwmon/pmbus/adp1050.c:9: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 584 | __raw_writeb(value, PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ In file included from drivers/hwmon/pmbus/adp1050.c:9: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:20: In file included from include/linux/mm.h:2188: include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ >> drivers/hwmon/pmbus/adp1050.c:47:60: error: too few arguments to function call, expected at least 3, have 2 47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n"); | ~~~~~~~~~~~~~ ^ include/linux/dev_printk.h:277:20: note: 'dev_err_probe' declared here 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/hwmon/pmbus/adp1050.c:53:63: error: too few arguments to function call, expected at least 3, have 2 53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); | ~~~~~~~~~~~~~ ^ include/linux/dev_printk.h:277:20: note: 'dev_err_probe' declared here 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 7 warnings and 2 errors generated. vim +47 drivers/hwmon/pmbus/adp1050.c 32 33 static int adp1050_probe(struct i2c_client *client) 34 { 35 u32 vin_scale_monitor, iin_scale_monitor; 36 int ret; 37 38 if (!i2c_check_functionality(client->adapter, 39 I2C_FUNC_SMBUS_WRITE_WORD_DATA)) 40 return -ENODEV; 41 42 /* Unlock CHIP's password in order to be able to read/write to it's 43 * VIN_SCALE and IIN_SCALE registers. 44 */ 45 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); 46 if (ret < 0) { > 47 dev_err_probe(&client->dev, "Device can't be unlocked.\n"); 48 return ret; 49 } 50 51 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); 52 if (ret < 0) { 53 dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); 54 return ret; 55 } 56 57 /* If adi,vin-scale-monitor isn't set or is set to 0 means that the 58 * VIN monitor isn't used, therefore 0 is used as scale in order 59 * for the readings to return 0. 60 */ 61 if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor", 62 &vin_scale_monitor)) 63 vin_scale_monitor = 0; 64 65 /* If adi,iin-scale-monitor isn't set or is set to 0 means that the 66 * IIN monitor isn't used, therefore 0 is used as scale in order 67 * for the readings to return 0. 68 */ 69 if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor", 70 &iin_scale_monitor)) 71 iin_scale_monitor = 0; 72 73 ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR, 74 vin_scale_monitor); 75 if (ret < 0) 76 return ret; 77 78 ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR, 79 iin_scale_monitor); 80 if (ret < 0) 81 return ret; 82 83 return pmbus_do_probe(client, &adp1050_info); 84 } 85 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Hi Radu, kernel test robot noticed the following build errors: [auto build test ERROR on groeck-staging/hwmon-next] [also build test ERROR on robh/for-next linus/master v6.8 next-20240318] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Radu-Sabau/dt-bindings-hwmon-pmbus-adp1050-add-bindings/20240318-202619 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20240318112140.385244-3-radu.sabau%40analog.com patch subject: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240319/202403190552.U4RHYvqc-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240319/202403190552.U4RHYvqc-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202403190552.U4RHYvqc-lkp@intel.com/ All error/warnings (new ones prefixed by >>): drivers/hwmon/pmbus/adp1050.c: In function 'adp1050_probe': >> drivers/hwmon/pmbus/adp1050.c:47:45: warning: passing argument 2 of 'dev_err_probe' makes integer from pointer without a cast [-Wint-conversion] 47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | char * In file included from include/linux/device.h:15, from include/linux/acpi.h:14, from include/linux/i2c.h:13, from drivers/hwmon/pmbus/adp1050.c:9: include/linux/dev_printk.h:277:64: note: expected 'int' but argument is of type 'char *' 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ~~~~^~~ >> drivers/hwmon/pmbus/adp1050.c:47:17: error: too few arguments to function 'dev_err_probe' 47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n"); | ^~~~~~~~~~~~~ include/linux/dev_printk.h:277:20: note: declared here 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ^~~~~~~~~~~~~ drivers/hwmon/pmbus/adp1050.c:53:45: warning: passing argument 2 of 'dev_err_probe' makes integer from pointer without a cast [-Wint-conversion] 53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | char * include/linux/dev_printk.h:277:64: note: expected 'int' but argument is of type 'char *' 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ~~~~^~~ drivers/hwmon/pmbus/adp1050.c:53:17: error: too few arguments to function 'dev_err_probe' 53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); | ^~~~~~~~~~~~~ include/linux/dev_printk.h:277:20: note: declared here 277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...); | ^~~~~~~~~~~~~ vim +/dev_err_probe +47 drivers/hwmon/pmbus/adp1050.c > 9 #include <linux/i2c.h> 10 #include <linux/init.h> 11 #include <linux/kernel.h> 12 #include <linux/module.h> 13 #include <linux/of.h> 14 #include "pmbus.h" 15 16 #define ADP1050_CHIP_PASSWORD 0xD7 17 18 #define ADP1050_VIN_SCALE_MONITOR 0xD8 19 #define ADP1050_IIN_SCALE_MONITOR 0xD9 20 21 static struct pmbus_driver_info adp1050_info = { 22 .pages = 1, 23 .format[PSC_VOLTAGE_IN] = linear, 24 .format[PSC_VOLTAGE_OUT] = linear, 25 .format[PSC_CURRENT_IN] = linear, 26 .format[PSC_TEMPERATURE] = linear, 27 .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT 28 | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT 29 | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP 30 | PMBUS_HAVE_STATUS_TEMP, 31 }; 32 33 static int adp1050_probe(struct i2c_client *client) 34 { 35 u32 vin_scale_monitor, iin_scale_monitor; 36 int ret; 37 38 if (!i2c_check_functionality(client->adapter, 39 I2C_FUNC_SMBUS_WRITE_WORD_DATA)) 40 return -ENODEV; 41 42 /* Unlock CHIP's password in order to be able to read/write to it's 43 * VIN_SCALE and IIN_SCALE registers. 44 */ 45 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); 46 if (ret < 0) { > 47 dev_err_probe(&client->dev, "Device can't be unlocked.\n"); 48 return ret; 49 } 50 51 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); 52 if (ret < 0) { 53 dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); 54 return ret; 55 } 56 57 /* If adi,vin-scale-monitor isn't set or is set to 0 means that the 58 * VIN monitor isn't used, therefore 0 is used as scale in order 59 * for the readings to return 0. 60 */ 61 if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor", 62 &vin_scale_monitor)) 63 vin_scale_monitor = 0; 64 65 /* If adi,iin-scale-monitor isn't set or is set to 0 means that the 66 * IIN monitor isn't used, therefore 0 is used as scale in order 67 * for the readings to return 0. 68 */ 69 if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor", 70 &iin_scale_monitor)) 71 iin_scale_monitor = 0; 72 73 ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR, 74 vin_scale_monitor); 75 if (ret < 0) 76 return ret; 77 78 ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR, 79 iin_scale_monitor); 80 if (ret < 0) 81 return ret; 82 83 return pmbus_do_probe(client, &adp1050_info); 84 } 85 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
On 18/03/2024 17:48, Guenter Roeck wrote:
> On 3/18/24 09:12, Krzysztof Kozlowski wrote:
>> On 18/03/2024 12:21, Radu Sabau wrote:
>>> Add support for ADP1050 Digital Controller for Isolated Power Supplies
>>> with PMBus interface Voltage, Current and Temperature Monitor.
>>>
>>
>> ...
>>
>>> +static int adp1050_probe(struct i2c_client *client)
>>> +{
>>> + u32 vin_scale_monitor, iin_scale_monitor;
>>> + int ret;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
>>> + return -ENODEV;
>>> +
>>> + /* Unlock CHIP's password in order to be able to read/write to it's
>>> + * VIN_SCALE and IIN_SCALE registers.
>>> + */
>>> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
>>> + if (ret < 0) {
>>> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
>>
>> Syntax is: return dev_err_probe(). Same in other places.
>>
>
> dev_err_probe() expects the error number as second parameter, so I don't
> really understand how the above even compiles.
I did not explain the arguments, because they are obvious... but if you
need so:
return dev_err_probe(&client->dev, ret, "Device can't be unlocked.\n");
Best regards,
Krzysztof
On 3/18/24 09:12, Krzysztof Kozlowski wrote:
> On 18/03/2024 12:21, Radu Sabau wrote:
>> Add support for ADP1050 Digital Controller for Isolated Power Supplies
>> with PMBus interface Voltage, Current and Temperature Monitor.
>>
>
> ...
>
>> +static int adp1050_probe(struct i2c_client *client)
>> +{
>> + u32 vin_scale_monitor, iin_scale_monitor;
>> + int ret;
>> +
>> + if (!i2c_check_functionality(client->adapter,
>> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
>> + return -ENODEV;
>> +
>> + /* Unlock CHIP's password in order to be able to read/write to it's
>> + * VIN_SCALE and IIN_SCALE registers.
>> + */
>> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
>> + if (ret < 0) {
>> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
>
> Syntax is: return dev_err_probe(). Same in other places.
>
dev_err_probe() expects the error number as second parameter, so I don't
really understand how the above even compiles.
Guenter
On 18/03/2024 12:21, Radu Sabau wrote: > Add support for ADP1050 Digital Controller for Isolated Power Supplies > with PMBus interface Voltage, Current and Temperature Monitor. > ... > +static int adp1050_probe(struct i2c_client *client) > +{ > + u32 vin_scale_monitor, iin_scale_monitor; > + int ret; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) > + return -ENODEV; > + > + /* Unlock CHIP's password in order to be able to read/write to it's > + * VIN_SCALE and IIN_SCALE registers. > + */ > + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); > + if (ret < 0) { > + dev_err_probe(&client->dev, "Device can't be unlocked.\n"); Syntax is: return dev_err_probe(). Same in other places. > + return ret; > + } > + > + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); > + if (ret < 0) { > + dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); > + return ret; > + } > + > + /* If adi,vin-scale-monitor isn't set or is set to 0 means that the > + * VIN monitor isn't used, therefore 0 is used as scale in order > + * for the readings to return 0. > + */ Please use Linux coding style comments. /* and aligned */. > + if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor", > + &vin_scale_monitor)) > + vin_scale_monitor = 0; > + > + /* If adi,iin-scale-monitor isn't set or is set to 0 means that the > + * IIN monitor isn't used, therefore 0 is used as scale in order > + * for the readings to return 0. > + */ > + if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor", > + &iin_scale_monitor)) > + iin_scale_monitor = 0; > + > + ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR, > + vin_scale_monitor); > + if (ret < 0) > + return ret; > + > + ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR, > + iin_scale_monitor); > + if (ret < 0) > + return ret; > + > + return pmbus_do_probe(client, &adp1050_info); > +} > + > +static const struct i2c_device_id adp1050_id[] = { > + {"adp1050", 0}, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, adp1050_id); > + > +static const struct of_device_id adp1050_of_match[] = { > + { .compatible = "adi,adp1050"}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, adp1050_of_match); > + > +static struct i2c_driver adp1050_driver = { > + .driver = { > + .name = "adp1050", > + .of_match_table = of_match_ptr(adp1050_of_match), Drop of_match_ptr, you will have here warnings. > + }, > + .probe = adp1050_probe, > + .id_table = adp1050_id, > +}; > +module_i2c_driver(adp1050_driver); > + > +MODULE_AUTHOR("Radu Sabau <radu.sabau@analog.com>"); > +MODULE_DESCRIPTION("Analog Devices ADP1050 HWMON PMBus Driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(PMBUS); Best regards, Krzysztof
On 18/03/2024 12:21, Radu Sabau wrote: > Add dt-bindings for adp1050 digital controller for isolated power supply > with pmbus interface voltage, current and temperature monitor. > > Signed-off-by: Radu Sabau <radu.sabau@analog.com> Subject: drop space before ':' > --- > .../bindings/hwmon/pmbus/adi,adp1050.yaml | 65 +++++++++++++++++++ > MAINTAINERS | 8 +++ > 2 files changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > new file mode 100644 > index 000000000000..e3162d0df0e2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > @@ -0,0 +1,65 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > + Drop > +$id: htpps://devicetree.org/schemas/hwmon/pmbus/adi,adp1050.yaml# > +$schema: htpps://devicetree.org/meta-schemes/core.yaml# > + > +title: Analog Devices ADP1050 digital controller with PMBus interface > + > +maintainers: > + - Radu Sabau <radu.sabau@analog.com> > + > +description: | > + The ADP1050 is used to monitor system voltages, currents and temperatures. > + Through the PMBus interface, the ADP1050 targets isolated power supplies > + and has four individual monitors for input/output voltage, input current > + and temperature. > + Datasheet: > + https://www.analog.com/en/products/adp1050.html Missing blank line > +properties: > + compatbile: Typo. And you did not test it... > + const: adi,adp1050 > + > + reg: > + maxItems: 1 > + > + vcc-supply: true > + > + adi,vin-scale-monitor: > + description: > + The value of the input voltage scale used by the internal ADP1050 ADC in > + order to read correct voltage values. > + $ref: /schemas/typees.yaml#/definitions/uint16 Missing blank line. > + adi,iin-scale-monitor: > + description: > + The value of the input current scale used by the internal ADP1050 ADC in > + order to read carrect current values. > + $ref: /schemas/typees.yaml#/definitions/uint16 > + > +required: > + - compatible > + - reg > + - vcc-supply > + - adi,vin-scale-monitor > + - adi,iin-scale-monitor > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #adress-cells = <1>; Totally messed indentation. Use 4 spaces for example indentation. > + #size-cells = <0>; > + clock-frequency = <100000>; > + adp1050@70 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + #adress-cells = <1>; > + #size-cells = <0>; > + compatible = "adi,adp1050"; > + reg = <0x70>; > + adi,vin-scale-monitor = <0xB030>; > + adi,iin-scale-monitor = <0x1>; > + vcc-supply = <&vcc>; > + }; > +... > + > diff --git a/MAINTAINERS b/MAINTAINERS > index f4d7f7cb7577..c90140859988 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -479,6 +479,14 @@ L: linux-wireless@vger.kernel.org > S: Orphan > F: drivers/net/wireless/admtek/adm8211.* > > +ADP1050 HARDWARE MONITOR DRIVER > +M: Radu Sabau <radu.sabau@analog.com> > +L: linux-hwmon@vger.kernel.org > +S: Supported > +W: https://ez.analog.com/linux-software-drivers > +F: Dcumentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > +F: drivers/hwmon/pmbus/adp1050.c There is no such file... Best regards, Krzysztof
Convert the NXP PNX I2C Controller bindings to DT schema. Keep only one example in DT schema to remove redundancy. Signed-off-by: Animesh Agarwal <animeshagarwal28@gmail.com> --- Changes in v2: - Changed the file name from nxp,i2c-pnx.yaml to nxp,pnx-i2c.yaml. - Dropped properties which were already defined in the top level $ref. - Dropped unused labels in example. --- .../devicetree/bindings/i2c/i2c-pnx.txt | 34 -------------- .../devicetree/bindings/i2c/nxp,pnx-i2c.yaml | 46 +++++++++++++++++++ 2 files changed, 46 insertions(+), 34 deletions(-) delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-pnx.txt create mode 100644 Documentation/devicetree/bindings/i2c/nxp,pnx-i2c.yaml diff --git a/Documentation/devicetree/bindings/i2c/i2c-pnx.txt b/Documentation/devicetree/bindings/i2c/i2c-pnx.txt deleted file mode 100644 index 2a59006cf79e..000000000000 --- a/Documentation/devicetree/bindings/i2c/i2c-pnx.txt +++ /dev/null @@ -1,34 +0,0 @@ -* NXP PNX I2C Controller - -Required properties: - - - reg: Offset and length of the register set for the device - - compatible: should be "nxp,pnx-i2c" - - interrupts: configure one interrupt line - - #address-cells: always 1 (for i2c addresses) - - #size-cells: always 0 - -Optional properties: - - - clock-frequency: desired I2C bus clock frequency in Hz, Default: 100000 Hz - -Examples: - - i2c1: i2c@400a0000 { - compatible = "nxp,pnx-i2c"; - reg = <0x400a0000 0x100>; - interrupt-parent = <&mic>; - interrupts = <51 0>; - #address-cells = <1>; - #size-cells = <0>; - }; - - i2c2: i2c@400a8000 { - compatible = "nxp,pnx-i2c"; - reg = <0x400a8000 0x100>; - interrupt-parent = <&mic>; - interrupts = <50 0>; - #address-cells = <1>; - #size-cells = <0>; - clock-frequency = <100000>; - }; diff --git a/Documentation/devicetree/bindings/i2c/nxp,pnx-i2c.yaml b/Documentation/devicetree/bindings/i2c/nxp,pnx-i2c.yaml new file mode 100644 index 000000000000..3125b2f5891e --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/nxp,pnx-i2c.yaml @@ -0,0 +1,46 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/nxp,pnx-i2c.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP PNX I2C Controller + +maintainers: + - Animesh Agarwal<animeshagarwal28@gmail.com> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + const: nxp,pnx-i2c + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clock-frequency: + default: 100000 + +required: + - compatible + - reg + - interrupts + - "#address-cells" + - "#size-cells" + +unevaluatedProperties: false + +examples: + - | + i2c@400a0000 { + compatible = "nxp,pnx-i2c"; + reg = <0x400a0000 0x100>; + interrupt-parent = <&mic>; + interrupts = <51 0>; + #address-cells = <1>; + #size-cells = <0>; + }; -- 2.44.0
On 3/18/24 04:21, Radu Sabau wrote:
> Add dt-bindings for adp1050 digital controller for isolated power supply
> with pmbus interface voltage, current and temperature monitor.
>
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---
> .../bindings/hwmon/pmbus/adi,adp1050.yaml | 65 +++++++++++++++++++
> MAINTAINERS | 8 +++
> 2 files changed, 73 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
> new file mode 100644
> index 000000000000..e3162d0df0e2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: htpps://devicetree.org/schemas/hwmon/pmbus/adi,adp1050.yaml#
> +$schema: htpps://devicetree.org/meta-schemes/core.yaml#
> +
> +title: Analog Devices ADP1050 digital controller with PMBus interface
> +
> +maintainers:
> + - Radu Sabau <radu.sabau@analog.com>
> +
> +description: |
> + The ADP1050 is used to monitor system voltages, currents and temperatures.
> + Through the PMBus interface, the ADP1050 targets isolated power supplies
> + and has four individual monitors for input/output voltage, input current
> + and temperature.
> + Datasheet:
> + https://www.analog.com/en/products/adp1050.html
> +properties:
> + compatbile:
> + const: adi,adp1050
> +
> + reg:
> + maxItems: 1
> +
> + vcc-supply: true
> +
> + adi,vin-scale-monitor:
> + description:
> + The value of the input voltage scale used by the internal ADP1050 ADC in
> + order to read correct voltage values.
> + $ref: /schemas/typees.yaml#/definitions/uint16
> + adi,iin-scale-monitor:
> + description:
> + The value of the input current scale used by the internal ADP1050 ADC in
> + order to read carrect current values.
> + $ref: /schemas/typees.yaml#/definitions/uint16
> +
I don't see value in "-monitor" in those property names.
Also, I don't see why those properties should be mandatory since the chip
has the ability to store its configuration in eeprom.
The proposed driver code disables vin and iin monitoring if the properties
are set to 0 or not provided. I disagree that this should be possible in
the first place (I don't see its value), but if disabling vin and iin
monitoring is supported it should be documented.
Last but not least, I think those values should be abstracted with some
scale instead of reflecting raw (and unexplained) register values.
Thanks,
Guenter
On Mon, Mar 18, 2024 at 7:53 PM Rob Herring <robh@kernel.org> wrote: > Doesn't quite match the compatible string. Will change the file name to nxp,pnx-i2c.yaml > These 2 are defined in i2c-controller.yaml, so drop. > Drop unused labels. Making these fixes in v2. Thanks, Animesh
On 3/18/24 04:21, Radu Sabau wrote: > Add support for ADP1050 Digital Controller for Isolated Power Supplies > with PMBus interface Voltage, Current and Temperature Monitor. > > The ADP1050 implements several features to enable a robust > system of parallel and redundant operation for customers who > require high availability. The device can measure voltage, > current and temperature that can be used in different > techniques to identify and safely shut down an erroneous > power supply in parallel operation mode. > > Signed-off-by: Radu Sabau <radu.sabau@analog.com> > --- > Documentation/hwmon/adp1050.rst | 69 ++++++++++++++++++++ > Documentation/hwmon/index.rst | 1 + > drivers/hwmon/pmbus/Kconfig | 10 +++ > drivers/hwmon/pmbus/Makefile | 1 + > drivers/hwmon/pmbus/adp1050.c | 111 ++++++++++++++++++++++++++++++++ > 5 files changed, 192 insertions(+) > create mode 100644 Documentation/hwmon/adp1050.rst > create mode 100644 drivers/hwmon/pmbus/adp1050.c > > diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst > new file mode 100644 > index 000000000000..e3e5bb650a51 > --- /dev/null > +++ b/Documentation/hwmon/adp1050.rst > @@ -0,0 +1,69 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +Kernel driver adp1050 > +===================== > + > +Supported chips: > + > + * Analog Devices ADP1050 > + > + Prefix: 'adp1050' > + > + Addresses scanned: I2C 0x70 - 0x77 > + > + Datasheet: https://www.analog.com/media/en/technical-documentation/data- > +sheets/ADP1050.pdf > + > +Authors: > + > + - Radu Sabau <radu.sabau@analog.com> > + > + > +Description > +----------- > + > +This driver supprts hardware monitoring for Analog Devices ADP1050 Digital > +Controller for Isolated Power Supply with PMBus interface. > + > +The ADP1050 is an advanced digital controller with a PMBus™ > +interface targeting high density, high efficiency dc-to-dc power > +conversion used to monitor system temperatures, voltages and currents. > +Through the PMBus interface, the device can monitor input/output voltages, > +input current and temperature. > + > +Usage Notes > +----------- > + > +This driver does not auto-detect devices. You will have to instantiate > +the devices explicitly. > +Please see Documentation/i2c/instantiating-devices.rst for details. > + > +The vin scale monitor value and iin scale monitor value can be > +configured by a device tree property;see > +Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml for details > + > +Platform data support > +--------------------- > + > +The driver supports standard PMBus driver platform data. > + > +Sysfs Attributes > +---------------- > + > +================= ======================================== > +in1_label "vin" > +in1_input Measured input voltage > +in1_alarm Input voltage alarm > +in2_label "vout1" > +in2_input Measured output voltage > +in2_crit Critical maximum output voltage > +in2_crit_alarm Output voltage high alarm > +in2_lcrit Critical minimum output voltage > +in2_lcrit_alarm Output voltage critical low alarm > +curr1_label "iin" > +curr1_input Measured input current. > +curr1_alarm Input current alarm > +temp1_input Measured temperature > +temp1_crit Critical high temperature > +temp1_crit_alarm Chip temperature critical high alarm > +================= ======================================== > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > index 1ca7a4fe1f8f..9a4fd576e6f6 100644 > --- a/Documentation/hwmon/index.rst > +++ b/Documentation/hwmon/index.rst > @@ -33,6 +33,7 @@ Hardware Monitoring Kernel Drivers > adm1266 > adm1275 > adm9240 > + adp1050 > ads7828 > adt7410 > adt7411 > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > index 557ae0c414b0..38e794d83cc3 100644 > --- a/drivers/hwmon/pmbus/Kconfig > +++ b/drivers/hwmon/pmbus/Kconfig > @@ -57,6 +57,16 @@ config SENSORS_ADM1275 > This driver can also be built as a module. If so, the module will > be called adm1275. > > +config SENSORS_ADP1050 > + tristate "Analog Devices ADP1050 digital controller for Power Supplies" > + help > + If you say yes here you get hardware monitoring support for Analog > + Devices ADP1050 digital controller for isolated power supply with > + PMBus interface. > + > + This driver can also be built as a module. If so, the module will > + be called adp1050. > + > config SENSORS_BEL_PFE > tristate "Bel PFE Compatible Power Supplies" > help > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > index f14ecf03ad77..95a8dea5e5ed 100644 > --- a/drivers/hwmon/pmbus/Makefile > +++ b/drivers/hwmon/pmbus/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o > obj-$(CONFIG_SENSORS_ACBEL_FSG032) += acbel-fsg032.o > obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o > obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o > +obj-$(CONFIG_SENSORS_ADP1050) += adp1050.o > obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o > obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o > obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o > diff --git a/drivers/hwmon/pmbus/adp1050.c b/drivers/hwmon/pmbus/adp1050.c > new file mode 100644 > index 000000000000..53198d858156 > --- /dev/null > +++ b/drivers/hwmon/pmbus/adp1050.c > @@ -0,0 +1,111 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Hardware monitoring driver for Analog Devices ADP1050 > + * > + * Copyright (C) 2024 Analog Devices, Inc. > + */ > +#include <linux/bits.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include "pmbus.h" > + > +#define ADP1050_CHIP_PASSWORD 0xD7 > + > +#define ADP1050_VIN_SCALE_MONITOR 0xD8 > +#define ADP1050_IIN_SCALE_MONITOR 0xD9 > + > +static struct pmbus_driver_info adp1050_info = { > + .pages = 1, > + .format[PSC_VOLTAGE_IN] = linear, > + .format[PSC_VOLTAGE_OUT] = linear, > + .format[PSC_CURRENT_IN] = linear, > + .format[PSC_TEMPERATURE] = linear, > + .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT > + | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT > + | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP > + | PMBUS_HAVE_STATUS_TEMP, > +}; > + > +static int adp1050_probe(struct i2c_client *client) > +{ > + u32 vin_scale_monitor, iin_scale_monitor; > + int ret; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) > + return -ENODEV; > + > + /* Unlock CHIP's password in order to be able to read/write to it's > + * VIN_SCALE and IIN_SCALE registers. > + */ > + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); > + if (ret < 0) { > + dev_err_probe(&client->dev, "Device can't be unlocked.\n"); > + return ret; > + } > + > + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); > + if (ret < 0) { > + dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); > + return ret; > + } > + The datasheet says that the factory default password is 0xffff. What if it isn't ? Refusing to instantiate the chip because it can't be unlocked seems a bit extreme. After all, the password _can_ be changed. > + /* If adi,vin-scale-monitor isn't set or is set to 0 means that the > + * VIN monitor isn't used, therefore 0 is used as scale in order > + * for the readings to return 0. > + */ > + if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor", > + &vin_scale_monitor)) > + vin_scale_monitor = 0; > + > + /* If adi,iin-scale-monitor isn't set or is set to 0 means that the > + * IIN monitor isn't used, therefore 0 is used as scale in order > + * for the readings to return 0. > + */ > + if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor", > + &iin_scale_monitor)) > + iin_scale_monitor = 0; > + I don't think the "-monitor" extensions in those property names add any value. > + ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR, > + vin_scale_monitor); > + if (ret < 0) > + return ret; > + > + ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR, > + iin_scale_monitor); > + if (ret < 0) > + return ret; > + If vin and iin monitoring is disabled on purpose, why still set PMBUS_HAVE_VIN and PMBUS_HAVE_IIN above ? Reporting input values as 0 if the values are explicitly not monitored does not make sense to me. I am puzzled about this to start with. Unless I am missing something, the data sheet has no indication that VIN monitoring and/or IIN monitoring is actually turned off if iin_scale/vin_scale are set to 0. Why support the option to disable them ? That doesn't make sense to me. Please explain how/when this makes sense. Please also explain why chip support is made to depend on devicetree support. Register values may have been be set by rommon/bios or (much more likely) by manufacturing, or users could just want to keep the chip default. Why force-override it or even force-disable it ? Thanks, Guenter
On Mon, Mar 18, 2024 at 02:49:05PM +0530, Animesh Agarwal wrote: > Convert the NXP PNX I2C Controller bindings to DT schema. > Keep only one example in DT schema to remove redundancy. > > Signed-off-by: Animesh Agarwal <animeshagarwal28@gmail.com> > --- > .../devicetree/bindings/i2c/i2c-pnx.txt | 34 ------------ > .../devicetree/bindings/i2c/nxp,i2c-pnx.yaml | 52 +++++++++++++++++++ > 2 files changed, 52 insertions(+), 34 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-pnx.txt > create mode 100644 Documentation/devicetree/bindings/i2c/nxp,i2c-pnx.yaml > diff --git a/Documentation/devicetree/bindings/i2c/nxp,i2c-pnx.yaml b/Documentation/devicetree/bindings/i2c/nxp,i2c-pnx.yaml > new file mode 100644 > index 000000000000..b44e4f995b73 > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/nxp,i2c-pnx.yaml Doesn't quite match the compatible string. > @@ -0,0 +1,52 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/i2c/nxp,i2c-pnx.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NXP PNX I2C Controller > + > +maintainers: > + - Animesh Agarwal<animeshagarwal28@gmail.com> > + > +allOf: > + - $ref: /schemas/i2c/i2c-controller.yaml# > + > +properties: > + compatible: > + const: nxp,pnx-i2c > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 These 2 are defined in i2c-controller.yaml, so drop. > + > + clock-frequency: > + default: 100000 > + > +required: > + - compatible > + - reg > + - interrupts > + - "#address-cells" > + - "#size-cells" > + > +unevaluatedProperties: false > + > +examples: > + - | > + i2c1: i2c@400a0000 { Drop unused labels. > + compatible = "nxp,pnx-i2c"; > + reg = <0x400a0000 0x100>; > + interrupt-parent = <&mic>; > + interrupts = <51 0>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > -- > 2.44.0 >
On Mon, 18 Mar 2024 13:21:34 +0200, Radu Sabau wrote: > Add dt-bindings for adp1050 digital controller for isolated power supply > with pmbus interface voltage, current and temperature monitor. > > Signed-off-by: Radu Sabau <radu.sabau@analog.com> > --- > .../bindings/hwmon/pmbus/adi,adp1050.yaml | 65 +++++++++++++++++++ > MAINTAINERS | 8 +++ > 2 files changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Traceback (most recent call last): File "/usr/local/lib/python3.11/dist-packages/jsonschema/validators.py", line 909, in resolve_from_url document = self.store[url] ~~~~~~~~~~^^^^^ File "/usr/local/lib/python3.11/dist-packages/jsonschema/_utils.py", line 28, in __getitem__ return self.store[self.normalize(uri)] ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^ KeyError: 'htpps://devicetree.org/meta-schemes/core.yaml' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/usr/local/lib/python3.11/dist-packages/jsonschema/validators.py", line 912, in resolve_from_url document = self.resolve_remote(url) ^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/jsonschema/validators.py", line 1018, in resolve_remote with urlopen(uri) as url: ^^^^^^^^^^^^ File "/usr/lib/python3.11/urllib/request.py", line 216, in urlopen return opener.open(url, data, timeout) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/urllib/request.py", line 519, in open response = self._open(req, data) ^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/urllib/request.py", line 541, in _open return self._call_chain(self.handle_open, 'unknown', ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/urllib/request.py", line 496, in _call_chain result = func(*args) ^^^^^^^^^^^ File "/usr/lib/python3.11/urllib/request.py", line 1419, in unknown_open raise URLError('unknown url type: %s' % type) urllib.error.URLError: <urlopen error unknown url type: htpps> During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/usr/local/bin/dt-doc-validate", line 8, in <module> sys.exit(main()) ^^^^^^ File "/usr/local/lib/python3.11/dist-packages/dtschema/doc_validate.py", line 66, in main ret |= check_doc(f) ^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/dtschema/doc_validate.py", line 29, in check_doc for error in sorted(dtsch.iter_errors(), key=lambda e: e.linecol): ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/dtschema/schema.py", line 120, in iter_errors meta_schema = self.resolver.resolve_from_url(self['$schema']) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/jsonschema/validators.py", line 914, in resolve_from_url raise exceptions.RefResolutionError(exc) jsonschema.exceptions.RefResolutionError: <urlopen error unknown url type: htpps> Error: Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.example.dts:33.3-34.1 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1428: dt_binding_check] Error 2 make: *** [Makefile:240: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240318112140.385244-2-radu.sabau@analog.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Add dt-bindings for adp1050 digital controller for isolated power supply with pmbus interface voltage, current and temperature monitor. Signed-off-by: Radu Sabau <radu.sabau@analog.com> --- .../bindings/hwmon/pmbus/adi,adp1050.yaml | 65 +++++++++++++++++++ MAINTAINERS | 8 +++ 2 files changed, 73 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml new file mode 100644 index 000000000000..e3162d0df0e2 --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml @@ -0,0 +1,65 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- + +$id: htpps://devicetree.org/schemas/hwmon/pmbus/adi,adp1050.yaml# +$schema: htpps://devicetree.org/meta-schemes/core.yaml# + +title: Analog Devices ADP1050 digital controller with PMBus interface + +maintainers: + - Radu Sabau <radu.sabau@analog.com> + +description: | + The ADP1050 is used to monitor system voltages, currents and temperatures. + Through the PMBus interface, the ADP1050 targets isolated power supplies + and has four individual monitors for input/output voltage, input current + and temperature. + Datasheet: + https://www.analog.com/en/products/adp1050.html +properties: + compatbile: + const: adi,adp1050 + + reg: + maxItems: 1 + + vcc-supply: true + + adi,vin-scale-monitor: + description: + The value of the input voltage scale used by the internal ADP1050 ADC in + order to read correct voltage values. + $ref: /schemas/typees.yaml#/definitions/uint16 + adi,iin-scale-monitor: + description: + The value of the input current scale used by the internal ADP1050 ADC in + order to read carrect current values. + $ref: /schemas/typees.yaml#/definitions/uint16 + +required: + - compatible + - reg + - vcc-supply + - adi,vin-scale-monitor + - adi,iin-scale-monitor + +additionalProperties: false + +examples: + - | + i2c { + #adress-cells = <1>; + #size-cells = <0>; + clock-frequency = <100000>; + adp1050@70 { + #adress-cells = <1>; + #size-cells = <0>; + compatible = "adi,adp1050"; + reg = <0x70>; + adi,vin-scale-monitor = <0xB030>; + adi,iin-scale-monitor = <0x1>; + vcc-supply = <&vcc>; + }; +... + diff --git a/MAINTAINERS b/MAINTAINERS index f4d7f7cb7577..c90140859988 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -479,6 +479,14 @@ L: linux-wireless@vger.kernel.org S: Orphan F: drivers/net/wireless/admtek/adm8211.* +ADP1050 HARDWARE MONITOR DRIVER +M: Radu Sabau <radu.sabau@analog.com> +L: linux-hwmon@vger.kernel.org +S: Supported +W: https://ez.analog.com/linux-software-drivers +F: Dcumentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml +F: drivers/hwmon/pmbus/adp1050.c + ADP1653 FLASH CONTROLLER DRIVER M: Sakari Ailus <sakari.ailus@iki.fi> L: linux-media@vger.kernel.org -- 2.34.1
The ADP1050 is an advanced digital controller with a PMBus™ interface targeting high density, high efficiency dc-to-dc power conversion which can measure input/output voltages, input currents and temperature. Radu Sabau (2): dt-bindings: hwmon: pmbus: adp1050 : add bindings hwmon: pmbus: adp1050 : Add driver support .../bindings/hwmon/pmbus/adi,adp1050.yaml | 65 ++++++++++ Documentation/hwmon/adp1050.rst | 69 +++++++++++ Documentation/hwmon/index.rst | 1 + MAINTAINERS | 8 ++ drivers/hwmon/pmbus/Kconfig | 10 ++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/adp1050.c | 111 ++++++++++++++++++ 7 files changed, 265 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml create mode 100644 Documentation/hwmon/adp1050.rst create mode 100644 drivers/hwmon/pmbus/adp1050.c -- 2.34.1
Hi
On 3/18/24 10:48 AM, Mika Westerberg wrote:
> Hi,
>
> On Mon, Mar 18, 2024 at 04:24:46PM +0800, Youwan Wang wrote:
>> fix crash because of null pointers
>>
>> [ 190.538113] kernel NULL pointer dereference at virtual address 0000000000000000
>> [ 190.538115] Mem abort info:
>> [ 190.538116] ESR = 0x96000004
>> [ 190.538118] Exception class = DABT (current EL), IL = 32 bits
>> [ 190.538119] SET = 0, FnV = 0
>> [ 190.538120] EA = 0, S1PTW = 0
>> [ 190.538121] Data abort info:
>> [ 190.538122] ISV = 0, ISS = 0x00000004
>> [ 190.538123] CM = 0, WnR = 0
>> [ 190.538125] user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000cf937cf2
>> [ 190.538126] [0000000000000000] pgd=0000000000000000
>> [ 190.538129] Internal error: Oops: 96000004 [#1] SMP
>> [ 190.538172] Process swapper/0 (pid: 0, stack limit = 0x000000000179ba77)
>> [ 190.538175] CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Tainted: G
>> [ 190.538178] pstate: 40000085 (nZcv daIf -PAN -UAO)
>> [ 190.538183] pc : i2c_dw_isr+0x2e4/0x614 [i2c_designware_core]
>> [ 190.538185] lr : i2c_dw_isr+0x9c/0x614 [i2c_designware_core]
>> [ 190.538186] sp : ffff000008003e40
>> [ 190.538187] x29: ffff000008003e40 x28: ffffd9cfad997200
>> [ 190.538189] x27: ffff5f18a4f2c018 x26: 0000000000000000
>> [ 190.538191] x25: ffff5f18a52d9fe8 x24: 0000000000000010
>> [ 190.538193] x23: 0000000000000000 x22: 0000000000000000
>> [ 190.538194] x21: 0000000000000510 x20: 0000000000000000
>> [ 190.538196] x19: ffffd9cfa08d6080 x18: 00000000fffffffe
>> [ 190.538197] x17: 0000000000000000 x16: 0000000000000000
>> [ 190.538199] x15: 0000000000000000 x14: 0000000000000000
>> [ 190.538200] x13: 0000000000000000 x12: 000000000000028d
>> [ 190.538202] x11: 0000000000000040 x10: ffff5f18a52fe340
>> [ 190.538203] x9 : ffffd9cfaf4a28f0 x8 : 0000000000000000
>> [ 190.538205] x7 : ffffd9cfaf401b88 x6 : ffffd9cfaf401b60
>> [ 190.538206] x5 : 0000000000000000 x4 : 0000000000000000
>> [ 190.538208] x3 : 0000000000000000 x2 : ffff000008125000
>> [ 190.538209] x1 : 0000000000000000 x0 : 0000000000000000
>> [ 190.538211] Call trace:
>> [ 190.538213] i2c_dw_isr+0x2e4/0x614 [i2c_designware_core]
>> [ 190.538218] __handle_irq_event_percpu+0x68/0x230
>> [ 190.538219] handle_irq_event+0x6c/0x130
>> [ 190.538222] handle_fasteoi_irq+0xc0/0x220
>> [ 190.538223] __handle_domain_irq+0x80/0xe0
>> [ 190.538226] gic_handle_irq+0x84/0x188
>> [ 190.538227] el1_irq+0xb0/0x140
>> [ 190.538229] arch_cpu_idle+0x38/0x1c0
>> [ 190.538232] do_idle+0x238/0x2a4
>> [ 190.538234] cpu_startup_entry+0x2c/0x50
>> [ 190.538237] rest_init+0xbc/0xc8
>> [ 190.538240] start_kernel+0x4a8/0x4dc
>> [ 190.538242] Code: 937c7c80 b940727a 8b0002c1 f9403e77 (78606ac0)
>> [ 190.538248] SMP: stopping secondary CPUs
>> [ 190.538400] Starting crashdump kernel...
>> [ 190.538406] Bye!
>>
>> Signed-off-by: Youwan Wang <youwan@nfschina.com>
>> ---
>> drivers/i2c/busses/i2c-designware-master.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
>> index ca1035e010c7..849e8a3e85ed 100644
>> --- a/drivers/i2c/busses/i2c-designware-master.c
>> +++ b/drivers/i2c/busses/i2c-designware-master.c
>> @@ -429,12 +429,17 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>> struct i2c_msg *msgs = dev->msgs;
>> u32 intr_mask;
>> int tx_limit, rx_limit;
>> - u32 addr = msgs[dev->msg_write_idx].addr;
>> + u32 addr;
>> u32 buf_len = dev->tx_buf_len;
>> u8 *buf = dev->tx_buf;
>> bool need_restart = false;
>> unsigned int flr;
>>
>> + if (WARN_ON(!msgs))
>> + return;
>
> Instead of treating the symptom, I suggest figuring out why this happens
> in the first place. If the controller interrupt is enabled and triggered
> it is expected that there is some transfer going on and dev->msgs !=
> NULL.
What is the kernel version this happens? Does it include commit
301c8f5c32c8 ("i2c: designware: Fix handling of real but unexpected
device interrupts")?
Add support for ADP1050 Digital Controller for Isolated Power Supplies with PMBus interface Voltage, Current and Temperature Monitor. The ADP1050 implements several features to enable a robust system of parallel and redundant operation for customers who require high availability. The device can measure voltage, current and temperature that can be used in different techniques to identify and safely shut down an erroneous power supply in parallel operation mode. Signed-off-by: Radu Sabau <radu.sabau@analog.com> --- Documentation/hwmon/adp1050.rst | 69 ++++++++++++++++++++ Documentation/hwmon/index.rst | 1 + drivers/hwmon/pmbus/Kconfig | 10 +++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/adp1050.c | 111 ++++++++++++++++++++++++++++++++ 5 files changed, 192 insertions(+) create mode 100644 Documentation/hwmon/adp1050.rst create mode 100644 drivers/hwmon/pmbus/adp1050.c diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst new file mode 100644 index 000000000000..e3e5bb650a51 --- /dev/null +++ b/Documentation/hwmon/adp1050.rst @@ -0,0 +1,69 @@ +.. SPDX-License-Identifier: GPL-2.0 + +Kernel driver adp1050 +===================== + +Supported chips: + + * Analog Devices ADP1050 + + Prefix: 'adp1050' + + Addresses scanned: I2C 0x70 - 0x77 + + Datasheet: https://www.analog.com/media/en/technical-documentation/data- +sheets/ADP1050.pdf + +Authors: + + - Radu Sabau <radu.sabau@analog.com> + + +Description +----------- + +This driver supprts hardware monitoring for Analog Devices ADP1050 Digital +Controller for Isolated Power Supply with PMBus interface. + +The ADP1050 is an advanced digital controller with a PMBus™ +interface targeting high density, high efficiency dc-to-dc power +conversion used to monitor system temperatures, voltages and currents. +Through the PMBus interface, the device can monitor input/output voltages, +input current and temperature. + +Usage Notes +----------- + +This driver does not auto-detect devices. You will have to instantiate +the devices explicitly. +Please see Documentation/i2c/instantiating-devices.rst for details. + +The vin scale monitor value and iin scale monitor value can be +configured by a device tree property;see +Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml for details + +Platform data support +--------------------- + +The driver supports standard PMBus driver platform data. + +Sysfs Attributes +---------------- + +================= ======================================== +in1_label "vin" +in1_input Measured input voltage +in1_alarm Input voltage alarm +in2_label "vout1" +in2_input Measured output voltage +in2_crit Critical maximum output voltage +in2_crit_alarm Output voltage high alarm +in2_lcrit Critical minimum output voltage +in2_lcrit_alarm Output voltage critical low alarm +curr1_label "iin" +curr1_input Measured input current. +curr1_alarm Input current alarm +temp1_input Measured temperature +temp1_crit Critical high temperature +temp1_crit_alarm Chip temperature critical high alarm +================= ======================================== diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index 1ca7a4fe1f8f..9a4fd576e6f6 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -33,6 +33,7 @@ Hardware Monitoring Kernel Drivers adm1266 adm1275 adm9240 + adp1050 ads7828 adt7410 adt7411 diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index 557ae0c414b0..38e794d83cc3 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -57,6 +57,16 @@ config SENSORS_ADM1275 This driver can also be built as a module. If so, the module will be called adm1275. +config SENSORS_ADP1050 + tristate "Analog Devices ADP1050 digital controller for Power Supplies" + help + If you say yes here you get hardware monitoring support for Analog + Devices ADP1050 digital controller for isolated power supply with + PMBus interface. + + This driver can also be built as a module. If so, the module will + be called adp1050. + config SENSORS_BEL_PFE tristate "Bel PFE Compatible Power Supplies" help diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index f14ecf03ad77..95a8dea5e5ed 100644 --- a/drivers/hwmon/pmbus/Makefile +++ b/drivers/hwmon/pmbus/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o obj-$(CONFIG_SENSORS_ACBEL_FSG032) += acbel-fsg032.o obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o +obj-$(CONFIG_SENSORS_ADP1050) += adp1050.o obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o diff --git a/drivers/hwmon/pmbus/adp1050.c b/drivers/hwmon/pmbus/adp1050.c new file mode 100644 index 000000000000..53198d858156 --- /dev/null +++ b/drivers/hwmon/pmbus/adp1050.c @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Hardware monitoring driver for Analog Devices ADP1050 + * + * Copyright (C) 2024 Analog Devices, Inc. + */ +#include <linux/bits.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include "pmbus.h" + +#define ADP1050_CHIP_PASSWORD 0xD7 + +#define ADP1050_VIN_SCALE_MONITOR 0xD8 +#define ADP1050_IIN_SCALE_MONITOR 0xD9 + +static struct pmbus_driver_info adp1050_info = { + .pages = 1, + .format[PSC_VOLTAGE_IN] = linear, + .format[PSC_VOLTAGE_OUT] = linear, + .format[PSC_CURRENT_IN] = linear, + .format[PSC_TEMPERATURE] = linear, + .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT + | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT + | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP + | PMBUS_HAVE_STATUS_TEMP, +}; + +static int adp1050_probe(struct i2c_client *client) +{ + u32 vin_scale_monitor, iin_scale_monitor; + int ret; + + if (!i2c_check_functionality(client->adapter, + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) + return -ENODEV; + + /* Unlock CHIP's password in order to be able to read/write to it's + * VIN_SCALE and IIN_SCALE registers. + */ + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); + if (ret < 0) { + dev_err_probe(&client->dev, "Device can't be unlocked.\n"); + return ret; + } + + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF); + if (ret < 0) { + dev_err_probe(&client->dev, "Device couldn't be unlocked.\n"); + return ret; + } + + /* If adi,vin-scale-monitor isn't set or is set to 0 means that the + * VIN monitor isn't used, therefore 0 is used as scale in order + * for the readings to return 0. + */ + if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor", + &vin_scale_monitor)) + vin_scale_monitor = 0; + + /* If adi,iin-scale-monitor isn't set or is set to 0 means that the + * IIN monitor isn't used, therefore 0 is used as scale in order + * for the readings to return 0. + */ + if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor", + &iin_scale_monitor)) + iin_scale_monitor = 0; + + ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR, + vin_scale_monitor); + if (ret < 0) + return ret; + + ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR, + iin_scale_monitor); + if (ret < 0) + return ret; + + return pmbus_do_probe(client, &adp1050_info); +} + +static const struct i2c_device_id adp1050_id[] = { + {"adp1050", 0}, + {} +}; +MODULE_DEVICE_TABLE(i2c, adp1050_id); + +static const struct of_device_id adp1050_of_match[] = { + { .compatible = "adi,adp1050"}, + {} +}; +MODULE_DEVICE_TABLE(of, adp1050_of_match); + +static struct i2c_driver adp1050_driver = { + .driver = { + .name = "adp1050", + .of_match_table = of_match_ptr(adp1050_of_match), + }, + .probe = adp1050_probe, + .id_table = adp1050_id, +}; +module_i2c_driver(adp1050_driver); + +MODULE_AUTHOR("Radu Sabau <radu.sabau@analog.com>"); +MODULE_DESCRIPTION("Analog Devices ADP1050 HWMON PMBus Driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(PMBUS); -- 2.34.1
On Mon, Mar 18, 2024 at 11:01 AM Nam Cao <namcao@linutronix.de> wrote: > Nice! I assume I can add > Reported-and-tested-by: Eva Kurchatova <nyandarknessgirl@gmail.com> > to the patch? > Yes. You can also add link to upstream RVVM repo if anyone is interested in reproduction. This RVVM patch applied to 0.6 makes a keystroke storm: (window_update() is called on each display redraw and has access to hid_kb) diff --git a/src/devices/fb_window.c b/src/devices/fb_window.c index f170e2d..17e2519 100644 --- a/src/devices/fb_window.c +++ b/src/devices/fb_window.c @@ -77,6 +77,11 @@ static const uint8_t rvvm_logo_pix[] = { static void window_update(rvvm_mmio_dev_t* device) { + fb_window_t* win = device->data; + for (size_t i=0; i<100000; ++i) { + hid_keyboard_press(win->keyboard, HID_KEY_LEFTCTRL); + hid_keyboard_release(win->keyboard, HID_KEY_LEFTCTRL); + } fb_window_update((fb_window_t*)device->data); } > I am still confused why RT throttling doesn't unstuck the kernel in this > case. I will consult some people and investigate more on this. But I think > this patch is good on its own, so I will send a proper patch shortly. > > Best regards, > Nam RT throttling kicked in *very* rarely, in most cases where the unpatched kernel was actually stuck RT throttling wasn't reported at all. It didn't hang every time either, so it's possible that RT throttling helped sometimes, but not enough to always recover from such a loop: [incoming IRQ]->[IRQ claimed]->[no handling]->[IRQ completion (which immediately triggers phase 1 again)] Best regards, Eva
Convert the NXP PNX I2C Controller bindings to DT schema. Keep only one example in DT schema to remove redundancy. Signed-off-by: Animesh Agarwal <animeshagarwal28@gmail.com> --- .../devicetree/bindings/i2c/i2c-pnx.txt | 34 ------------ .../devicetree/bindings/i2c/nxp,i2c-pnx.yaml | 52 +++++++++++++++++++ 2 files changed, 52 insertions(+), 34 deletions(-) delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-pnx.txt create mode 100644 Documentation/devicetree/bindings/i2c/nxp,i2c-pnx.yaml diff --git a/Documentation/devicetree/bindings/i2c/i2c-pnx.txt b/Documentation/devicetree/bindings/i2c/i2c-pnx.txt deleted file mode 100644 index 2a59006cf79e..000000000000 --- a/Documentation/devicetree/bindings/i2c/i2c-pnx.txt +++ /dev/null @@ -1,34 +0,0 @@ -* NXP PNX I2C Controller - -Required properties: - - - reg: Offset and length of the register set for the device - - compatible: should be "nxp,pnx-i2c" - - interrupts: configure one interrupt line - - #address-cells: always 1 (for i2c addresses) - - #size-cells: always 0 - -Optional properties: - - - clock-frequency: desired I2C bus clock frequency in Hz, Default: 100000 Hz - -Examples: - - i2c1: i2c@400a0000 { - compatible = "nxp,pnx-i2c"; - reg = <0x400a0000 0x100>; - interrupt-parent = <&mic>; - interrupts = <51 0>; - #address-cells = <1>; - #size-cells = <0>; - }; - - i2c2: i2c@400a8000 { - compatible = "nxp,pnx-i2c"; - reg = <0x400a8000 0x100>; - interrupt-parent = <&mic>; - interrupts = <50 0>; - #address-cells = <1>; - #size-cells = <0>; - clock-frequency = <100000>; - }; diff --git a/Documentation/devicetree/bindings/i2c/nxp,i2c-pnx.yaml b/Documentation/devicetree/bindings/i2c/nxp,i2c-pnx.yaml new file mode 100644 index 000000000000..b44e4f995b73 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/nxp,i2c-pnx.yaml @@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/nxp,i2c-pnx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP PNX I2C Controller + +maintainers: + - Animesh Agarwal<animeshagarwal28@gmail.com> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + const: nxp,pnx-i2c + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + clock-frequency: + default: 100000 + +required: + - compatible + - reg + - interrupts + - "#address-cells" + - "#size-cells" + +unevaluatedProperties: false + +examples: + - | + i2c1: i2c@400a0000 { + compatible = "nxp,pnx-i2c"; + reg = <0x400a0000 0x100>; + interrupt-parent = <&mic>; + interrupts = <51 0>; + #address-cells = <1>; + #size-cells = <0>; + }; -- 2.44.0
On 18/Mar/2024 Eva Kurchatova wrote: > On Sun, Mar 17, 2024 at 11:27 PM Nam Cao <namcao@linutronix.de> wrote: > > It seems I2C HID's interrupt handler (i2c_hid_irq) returns immediately if > > I2C_HID_READ_PENDING is set. This flag is supposed to be cleared in > > i2c_hid_xfer(), but since the (threaded) interrupt handler runs at higher > > priority, the flag is never cleared. So we have a lock-up: interrupt > > handler won't do anything unless the flag is cleared, but the clearing of > > this flag is done in a lower priority task which never gets scheduled while > > the interrupt handler is active. > > > > There is RT throttling to prevent RT tasks from locking up the system like > > this. I don't know much about scheduling stuffs, so I am not really sure > > why RT throttling does not work. I think because RT throttling triggers > > when RT tasks take too much CPU time, but in this case hard interrupt > > handlers take lots of CPU time too (~50% according to my measurement), so > > RT throttling doesn't trigger often enough (in this case, it triggers once > > and never again). Again, I don't know much about scheduler so I may be > > talking nonsense here. > > > > The flag I2C_HID_READ_PENDING seems to be used to make sure that only 1 > > I2C operation can happen at a time. But this seems pointless, because I2C > > subsystem already takes care of this. So I think we can just remove it. > > > > Can you give the below patch a try? > > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > > index 2735cd585af0..799ad0ef9c4a 100644 > > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > > @@ -64,7 +64,6 @@ > > /* flags */ > > #define I2C_HID_STARTED 0 > > #define I2C_HID_RESET_PENDING 1 > > -#define I2C_HID_READ_PENDING 2 > > > > #define I2C_HID_PWR_ON 0x00 > > #define I2C_HID_PWR_SLEEP 0x01 > > @@ -190,15 +189,10 @@ static int i2c_hid_xfer(struct i2c_hid *ihid, > > msgs[n].len = recv_len; > > msgs[n].buf = recv_buf; > > n++; > > - > > - set_bit(I2C_HID_READ_PENDING, &ihid->flags); > > } > > > > ret = i2c_transfer(client->adapter, msgs, n); > > > > - if (recv_len) > > - clear_bit(I2C_HID_READ_PENDING, &ihid->flags); > > - > > if (ret != n) > > return ret < 0 ? ret : -EIO; > > > > @@ -566,9 +560,6 @@ static irqreturn_t i2c_hid_irq(int irq, void *dev_id) > > { > > struct i2c_hid *ihid = dev_id; > > > > - if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) > > - return IRQ_HANDLED; > > - > > i2c_hid_get_input(ihid); > > > > return IRQ_HANDLED; > > Patch applied cleanly on top of 6.7.9, builds OK (No warns, etc). > > This indeed fixes the hang completely. Nice! I assume I can add Reported-and-tested-by: Eva Kurchatova <nyandarknessgirl@gmail.com> to the patch? > I modified RVVM to send millions of keystroke events per second, > and put `reboot` as a service hook in the guest. It has been continuously > rebooting without a hitch for the last 30 minutes or so (Full boot takes > around 2 seconds), whereas unpatched 6.7.9 hangs almost immediately > in such conditions (Reverted your patch & rebuilt to be sure). > > Thank you very much for this! Hope to see it upstreamed soon Thank you for the report, your investigation helped a lot. I am still confused why RT throttling doesn't unstuck the kernel in this case. I will consult some people and investigate more on this. But I think this patch is good on its own, so I will send a proper patch shortly. Best regards, Nam
Hi,
On Mon, Mar 18, 2024 at 04:24:46PM +0800, Youwan Wang wrote:
> fix crash because of null pointers
>
> [ 190.538113] kernel NULL pointer dereference at virtual address 0000000000000000
> [ 190.538115] Mem abort info:
> [ 190.538116] ESR = 0x96000004
> [ 190.538118] Exception class = DABT (current EL), IL = 32 bits
> [ 190.538119] SET = 0, FnV = 0
> [ 190.538120] EA = 0, S1PTW = 0
> [ 190.538121] Data abort info:
> [ 190.538122] ISV = 0, ISS = 0x00000004
> [ 190.538123] CM = 0, WnR = 0
> [ 190.538125] user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000cf937cf2
> [ 190.538126] [0000000000000000] pgd=0000000000000000
> [ 190.538129] Internal error: Oops: 96000004 [#1] SMP
> [ 190.538172] Process swapper/0 (pid: 0, stack limit = 0x000000000179ba77)
> [ 190.538175] CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Tainted: G
> [ 190.538178] pstate: 40000085 (nZcv daIf -PAN -UAO)
> [ 190.538183] pc : i2c_dw_isr+0x2e4/0x614 [i2c_designware_core]
> [ 190.538185] lr : i2c_dw_isr+0x9c/0x614 [i2c_designware_core]
> [ 190.538186] sp : ffff000008003e40
> [ 190.538187] x29: ffff000008003e40 x28: ffffd9cfad997200
> [ 190.538189] x27: ffff5f18a4f2c018 x26: 0000000000000000
> [ 190.538191] x25: ffff5f18a52d9fe8 x24: 0000000000000010
> [ 190.538193] x23: 0000000000000000 x22: 0000000000000000
> [ 190.538194] x21: 0000000000000510 x20: 0000000000000000
> [ 190.538196] x19: ffffd9cfa08d6080 x18: 00000000fffffffe
> [ 190.538197] x17: 0000000000000000 x16: 0000000000000000
> [ 190.538199] x15: 0000000000000000 x14: 0000000000000000
> [ 190.538200] x13: 0000000000000000 x12: 000000000000028d
> [ 190.538202] x11: 0000000000000040 x10: ffff5f18a52fe340
> [ 190.538203] x9 : ffffd9cfaf4a28f0 x8 : 0000000000000000
> [ 190.538205] x7 : ffffd9cfaf401b88 x6 : ffffd9cfaf401b60
> [ 190.538206] x5 : 0000000000000000 x4 : 0000000000000000
> [ 190.538208] x3 : 0000000000000000 x2 : ffff000008125000
> [ 190.538209] x1 : 0000000000000000 x0 : 0000000000000000
> [ 190.538211] Call trace:
> [ 190.538213] i2c_dw_isr+0x2e4/0x614 [i2c_designware_core]
> [ 190.538218] __handle_irq_event_percpu+0x68/0x230
> [ 190.538219] handle_irq_event+0x6c/0x130
> [ 190.538222] handle_fasteoi_irq+0xc0/0x220
> [ 190.538223] __handle_domain_irq+0x80/0xe0
> [ 190.538226] gic_handle_irq+0x84/0x188
> [ 190.538227] el1_irq+0xb0/0x140
> [ 190.538229] arch_cpu_idle+0x38/0x1c0
> [ 190.538232] do_idle+0x238/0x2a4
> [ 190.538234] cpu_startup_entry+0x2c/0x50
> [ 190.538237] rest_init+0xbc/0xc8
> [ 190.538240] start_kernel+0x4a8/0x4dc
> [ 190.538242] Code: 937c7c80 b940727a 8b0002c1 f9403e77 (78606ac0)
> [ 190.538248] SMP: stopping secondary CPUs
> [ 190.538400] Starting crashdump kernel...
> [ 190.538406] Bye!
>
> Signed-off-by: Youwan Wang <youwan@nfschina.com>
> ---
> drivers/i2c/busses/i2c-designware-master.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index ca1035e010c7..849e8a3e85ed 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -429,12 +429,17 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> struct i2c_msg *msgs = dev->msgs;
> u32 intr_mask;
> int tx_limit, rx_limit;
> - u32 addr = msgs[dev->msg_write_idx].addr;
> + u32 addr;
> u32 buf_len = dev->tx_buf_len;
> u8 *buf = dev->tx_buf;
> bool need_restart = false;
> unsigned int flr;
>
> + if (WARN_ON(!msgs))
> + return;
Instead of treating the symptom, I suggest figuring out why this happens
in the first place. If the controller interrupt is enabled and triggered
it is expected that there is some transfer going on and dev->msgs !=
NULL.
On Sun, Mar 17, 2024 at 11:27 PM Nam Cao <namcao@linutronix.de> wrote:
>
> Cc: HID folks
>
> On 14/Mar/2024 Eva Kurchatova wrote:
> > If an I2C-HID controller level-triggered IRQ line is routed directly as
> > a PLIC IRQ, and we spam input early enough in kernel boot process
> > (Somewhere between initializing NET, ALSA subsystems and before
> > i2c-hid driver init), then there is a chance of kernel locking up
> > completely and not going any further.
> >
> > There are no kernel messages printed with all the IRQ, task hang
> > debugging enabled - other than (sometimes) it reports sched RT
> > throttling after a few seconds. Basic timer interrupt handling is
> > intact - fbdev tty cursor is still blinking.
> >
> > It appears that in such a case the I2C-HID IRQ line is raised; PLIC
> > notifies the (single) boot system hart, kernel claims the IRQ and
> > immediately completes it by writing to CLAIM/COMPLETE register.
> > No access to the I2C controller (OpenCores) or I2C-HID registers
> > is made, so the HID report is never consumed and IRQ line stays
> > raised forever. The kernel endlessly claims & completes IRQs
> > without doing any work with the device. It doesn't always end up this
> > way; sometimes boot process completes and there are no signs of
> > interrupt storm or stuck IRQ processing afterwards.
>
> It seems I2C HID's interrupt handler (i2c_hid_irq) returns immediately if
> I2C_HID_READ_PENDING is set. This flag is supposed to be cleared in
> i2c_hid_xfer(), but since the (threaded) interrupt handler runs at higher
> priority, the flag is never cleared. So we have a lock-up: interrupt
> handler won't do anything unless the flag is cleared, but the clearing of
> this flag is done in a lower priority task which never gets scheduled while
> the interrupt handler is active.
>
> There is RT throttling to prevent RT tasks from locking up the system like
> this. I don't know much about scheduling stuffs, so I am not really sure
> why RT throttling does not work. I think because RT throttling triggers
> when RT tasks take too much CPU time, but in this case hard interrupt
> handlers take lots of CPU time too (~50% according to my measurement), so
> RT throttling doesn't trigger often enough (in this case, it triggers once
> and never again). Again, I don't know much about scheduler so I may be
> talking nonsense here.
>
> The flag I2C_HID_READ_PENDING seems to be used to make sure that only 1
> I2C operation can happen at a time. But this seems pointless, because I2C
> subsystem already takes care of this. So I think we can just remove it.
>
> Can you give the below patch a try?
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 2735cd585af0..799ad0ef9c4a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -64,7 +64,6 @@
> /* flags */
> #define I2C_HID_STARTED 0
> #define I2C_HID_RESET_PENDING 1
> -#define I2C_HID_READ_PENDING 2
>
> #define I2C_HID_PWR_ON 0x00
> #define I2C_HID_PWR_SLEEP 0x01
> @@ -190,15 +189,10 @@ static int i2c_hid_xfer(struct i2c_hid *ihid,
> msgs[n].len = recv_len;
> msgs[n].buf = recv_buf;
> n++;
> -
> - set_bit(I2C_HID_READ_PENDING, &ihid->flags);
> }
>
> ret = i2c_transfer(client->adapter, msgs, n);
>
> - if (recv_len)
> - clear_bit(I2C_HID_READ_PENDING, &ihid->flags);
> -
> if (ret != n)
> return ret < 0 ? ret : -EIO;
>
> @@ -566,9 +560,6 @@ static irqreturn_t i2c_hid_irq(int irq, void *dev_id)
> {
> struct i2c_hid *ihid = dev_id;
>
> - if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
> - return IRQ_HANDLED;
> -
> i2c_hid_get_input(ihid);
>
> return IRQ_HANDLED;
Patch applied cleanly on top of 6.7.9, builds OK (No warns, etc).
This indeed fixes the hang completely.
I modified RVVM to send millions of keystroke events per second,
and put `reboot` as a service hook in the guest. It has been continuously
rebooting without a hitch for the last 30 minutes or so (Full boot takes
around 2 seconds), whereas unpatched 6.7.9 hangs almost immediately
in such conditions (Reverted your patch & rebuilt to be sure).
Thank you very much for this! Hope to see it upstreamed soon
fix crash because of null pointers [ 190.538113] kernel NULL pointer dereference at virtual address 0000000000000000 [ 190.538115] Mem abort info: [ 190.538116] ESR = 0x96000004 [ 190.538118] Exception class = DABT (current EL), IL = 32 bits [ 190.538119] SET = 0, FnV = 0 [ 190.538120] EA = 0, S1PTW = 0 [ 190.538121] Data abort info: [ 190.538122] ISV = 0, ISS = 0x00000004 [ 190.538123] CM = 0, WnR = 0 [ 190.538125] user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000cf937cf2 [ 190.538126] [0000000000000000] pgd=0000000000000000 [ 190.538129] Internal error: Oops: 96000004 [#1] SMP [ 190.538172] Process swapper/0 (pid: 0, stack limit = 0x000000000179ba77) [ 190.538175] CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Tainted: G [ 190.538178] pstate: 40000085 (nZcv daIf -PAN -UAO) [ 190.538183] pc : i2c_dw_isr+0x2e4/0x614 [i2c_designware_core] [ 190.538185] lr : i2c_dw_isr+0x9c/0x614 [i2c_designware_core] [ 190.538186] sp : ffff000008003e40 [ 190.538187] x29: ffff000008003e40 x28: ffffd9cfad997200 [ 190.538189] x27: ffff5f18a4f2c018 x26: 0000000000000000 [ 190.538191] x25: ffff5f18a52d9fe8 x24: 0000000000000010 [ 190.538193] x23: 0000000000000000 x22: 0000000000000000 [ 190.538194] x21: 0000000000000510 x20: 0000000000000000 [ 190.538196] x19: ffffd9cfa08d6080 x18: 00000000fffffffe [ 190.538197] x17: 0000000000000000 x16: 0000000000000000 [ 190.538199] x15: 0000000000000000 x14: 0000000000000000 [ 190.538200] x13: 0000000000000000 x12: 000000000000028d [ 190.538202] x11: 0000000000000040 x10: ffff5f18a52fe340 [ 190.538203] x9 : ffffd9cfaf4a28f0 x8 : 0000000000000000 [ 190.538205] x7 : ffffd9cfaf401b88 x6 : ffffd9cfaf401b60 [ 190.538206] x5 : 0000000000000000 x4 : 0000000000000000 [ 190.538208] x3 : 0000000000000000 x2 : ffff000008125000 [ 190.538209] x1 : 0000000000000000 x0 : 0000000000000000 [ 190.538211] Call trace: [ 190.538213] i2c_dw_isr+0x2e4/0x614 [i2c_designware_core] [ 190.538218] __handle_irq_event_percpu+0x68/0x230 [ 190.538219] handle_irq_event+0x6c/0x130 [ 190.538222] handle_fasteoi_irq+0xc0/0x220 [ 190.538223] __handle_domain_irq+0x80/0xe0 [ 190.538226] gic_handle_irq+0x84/0x188 [ 190.538227] el1_irq+0xb0/0x140 [ 190.538229] arch_cpu_idle+0x38/0x1c0 [ 190.538232] do_idle+0x238/0x2a4 [ 190.538234] cpu_startup_entry+0x2c/0x50 [ 190.538237] rest_init+0xbc/0xc8 [ 190.538240] start_kernel+0x4a8/0x4dc [ 190.538242] Code: 937c7c80 b940727a 8b0002c1 f9403e77 (78606ac0) [ 190.538248] SMP: stopping secondary CPUs [ 190.538400] Starting crashdump kernel... [ 190.538406] Bye! Signed-off-by: Youwan Wang <youwan@nfschina.com> --- drivers/i2c/busses/i2c-designware-master.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index ca1035e010c7..849e8a3e85ed 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -429,12 +429,17 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev) struct i2c_msg *msgs = dev->msgs; u32 intr_mask; int tx_limit, rx_limit; - u32 addr = msgs[dev->msg_write_idx].addr; + u32 addr; u32 buf_len = dev->tx_buf_len; u8 *buf = dev->tx_buf; bool need_restart = false; unsigned int flr; + if (WARN_ON(!msgs)) + return; + + addr = msgs[dev->msg_write_idx].addr; + intr_mask = DW_IC_INTR_MASTER_MASK; for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) { -- 2.25.1
Cc: HID folks On 14/Mar/2024 Eva Kurchatova wrote: > If an I2C-HID controller level-triggered IRQ line is routed directly as > a PLIC IRQ, and we spam input early enough in kernel boot process > (Somewhere between initializing NET, ALSA subsystems and before > i2c-hid driver init), then there is a chance of kernel locking up > completely and not going any further. > > There are no kernel messages printed with all the IRQ, task hang > debugging enabled - other than (sometimes) it reports sched RT > throttling after a few seconds. Basic timer interrupt handling is > intact - fbdev tty cursor is still blinking. > > It appears that in such a case the I2C-HID IRQ line is raised; PLIC > notifies the (single) boot system hart, kernel claims the IRQ and > immediately completes it by writing to CLAIM/COMPLETE register. > No access to the I2C controller (OpenCores) or I2C-HID registers > is made, so the HID report is never consumed and IRQ line stays > raised forever. The kernel endlessly claims & completes IRQs > without doing any work with the device. It doesn't always end up this > way; sometimes boot process completes and there are no signs of > interrupt storm or stuck IRQ processing afterwards. It seems I2C HID's interrupt handler (i2c_hid_irq) returns immediately if I2C_HID_READ_PENDING is set. This flag is supposed to be cleared in i2c_hid_xfer(), but since the (threaded) interrupt handler runs at higher priority, the flag is never cleared. So we have a lock-up: interrupt handler won't do anything unless the flag is cleared, but the clearing of this flag is done in a lower priority task which never gets scheduled while the interrupt handler is active. There is RT throttling to prevent RT tasks from locking up the system like this. I don't know much about scheduling stuffs, so I am not really sure why RT throttling does not work. I think because RT throttling triggers when RT tasks take too much CPU time, but in this case hard interrupt handlers take lots of CPU time too (~50% according to my measurement), so RT throttling doesn't trigger often enough (in this case, it triggers once and never again). Again, I don't know much about scheduler so I may be talking nonsense here. The flag I2C_HID_READ_PENDING seems to be used to make sure that only 1 I2C operation can happen at a time. But this seems pointless, because I2C subsystem already takes care of this. So I think we can just remove it. Can you give the below patch a try? diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 2735cd585af0..799ad0ef9c4a 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -64,7 +64,6 @@ /* flags */ #define I2C_HID_STARTED 0 #define I2C_HID_RESET_PENDING 1 -#define I2C_HID_READ_PENDING 2 #define I2C_HID_PWR_ON 0x00 #define I2C_HID_PWR_SLEEP 0x01 @@ -190,15 +189,10 @@ static int i2c_hid_xfer(struct i2c_hid *ihid, msgs[n].len = recv_len; msgs[n].buf = recv_buf; n++; - - set_bit(I2C_HID_READ_PENDING, &ihid->flags); } ret = i2c_transfer(client->adapter, msgs, n); - if (recv_len) - clear_bit(I2C_HID_READ_PENDING, &ihid->flags); - if (ret != n) return ret < 0 ? ret : -EIO; @@ -566,9 +560,6 @@ static irqreturn_t i2c_hid_irq(int irq, void *dev_id) { struct i2c_hid *ihid = dev_id; - if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) - return IRQ_HANDLED; - i2c_hid_get_input(ihid); return IRQ_HANDLED;
On Fri, 15 Mar 2024 10:30:30 +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Document support for the I2C Bus Interface (RIIC) available in the
> Renesas RZ/V2H(P) (R9A09G057) SoC.
>
> The RIIC interface in the Renesas RZ/V2H(P) differs from RZ/A in a
> couple of ways:
> - Register offsets for the RZ/V2H(P) SoC differ from those of the
> RZ/A SoC.
> - RZ/V2H register access is limited to 8-bit, whereas RZ/A supports
> 8/16/32-bit.
> - RZ/V2H has bit differences in the slave address register.
>
> To accommodate these differences in the existing driver, a new compatible
> string "renesas,riic-r9a09g057" is added.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v1->v2
> - Used a const for V2H SoC instead of enum in items list
> ---
> .../devicetree/bindings/i2c/renesas,riic.yaml | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
Acked-by: Rob Herring <robh@kernel.org>