Message ID | 20230330133953.21074-3-pmalgujar@marvell.com |
---|---|
State | Changes Requested |
Delegated to: | Andi Shyti |
Headers | show |
Series | i2c: thunderx: Marvell thunderx i2c changes | expand |
Hi Piyush, Thank you for the patch! Yet something to improve: [auto build test ERROR on wsa/i2c/for-next] [also build test ERROR on linus/master v6.3-rc4 next-20230330] [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/Piyush-Malgujar/i2c-thunderx-Clock-divisor-logic-changes/20230330-214626 base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next patch link: https://lore.kernel.org/r/20230330133953.21074-3-pmalgujar%40marvell.com patch subject: [PATCH 2/3] i2c: thunderx: Add support for High speed mode config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20230330/202303302309.SSHrlrqN-lkp@intel.com/config) compiler: riscv64-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/c7866465c9142bf77cc1bc651704bfbfc9b0b411 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Piyush-Malgujar/i2c-thunderx-Clock-divisor-logic-changes/20230330-214626 git checkout c7866465c9142bf77cc1bc651704bfbfc9b0b411 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/i2c/busses/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303302309.SSHrlrqN-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/i2c/busses/i2c-octeon-core.c: In function 'octeon_i2c_wait': >> drivers/i2c/busses/i2c-octeon-core.c:74:33: error: implicit declaration of function '__udelay'; did you mean '__delay'? [-Werror=implicit-function-declaration] 74 | __udelay(1); | ^~~~~~~~ | __delay cc1: some warnings being treated as errors vim +74 drivers/i2c/busses/i2c-octeon-core.c 39 40 /** 41 * octeon_i2c_wait - wait for the IFLG to be set 42 * @i2c: The struct octeon_i2c 43 * 44 * Returns 0 on success, otherwise a negative errno. 45 */ 46 static int octeon_i2c_wait(struct octeon_i2c *i2c) 47 { 48 long time_left; 49 50 /* 51 * Some chip revisions don't assert the irq in the interrupt 52 * controller. So we must poll for the IFLG change. 53 */ 54 if (i2c->broken_irq_mode) { 55 u64 end = get_jiffies_64() + i2c->adap.timeout; 56 57 while (!octeon_i2c_test_iflg(i2c) && 58 time_before64(get_jiffies_64(), end)) 59 usleep_range(I2C_OCTEON_EVENT_WAIT / 2, I2C_OCTEON_EVENT_WAIT); 60 61 return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT; 62 } 63 64 if (IS_LS_FREQ(i2c->twsi_freq)) { 65 i2c->int_enable(i2c); 66 time_left = wait_event_timeout(i2c->queue, 67 octeon_i2c_test_iflg(i2c), 68 i2c->adap.timeout); 69 i2c->int_disable(i2c); 70 } else { 71 time_left = 1000; /* 1ms */ 72 do { 73 if (time_left--) > 74 __udelay(1); 75 } while (!octeon_i2c_test_iflg(i2c) && time_left); 76 } 77 78 if (i2c->broken_irq_check && !time_left && 79 octeon_i2c_test_iflg(i2c)) { 80 dev_err(i2c->dev, "broken irq connection detected, switching to polling mode.\n"); 81 i2c->broken_irq_mode = true; 82 return 0; 83 } 84 85 if (!time_left) 86 return -ETIMEDOUT; 87 88 return 0; 89 } 90
Hi Piyush, Thank you for the patch! Yet something to improve: [auto build test ERROR on wsa/i2c/for-next] [also build test ERROR on linus/master v6.3-rc4 next-20230330] [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/Piyush-Malgujar/i2c-thunderx-Clock-divisor-logic-changes/20230330-214626 base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next patch link: https://lore.kernel.org/r/20230330133953.21074-3-pmalgujar%40marvell.com patch subject: [PATCH 2/3] i2c: thunderx: Add support for High speed mode config: riscv-randconfig-r042-20230329 (https://download.01.org/0day-ci/archive/20230331/202303310153.yx2xXH8s-lkp@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/intel-lab-lkp/linux/commit/c7866465c9142bf77cc1bc651704bfbfc9b0b411 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Piyush-Malgujar/i2c-thunderx-Clock-divisor-logic-changes/20230330-214626 git checkout c7866465c9142bf77cc1bc651704bfbfc9b0b411 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/i2c/busses/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303310153.yx2xXH8s-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/i2c/busses/i2c-octeon-core.c:74:5: error: call to undeclared function '__udelay'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] __udelay(1); ^ drivers/i2c/busses/i2c-octeon-core.c:74:5: note: did you mean '__delay'? arch/riscv/include/asm/delay.h:18:13: note: '__delay' declared here extern void __delay(unsigned long cycles); ^ 1 error generated. vim +/__udelay +74 drivers/i2c/busses/i2c-octeon-core.c 39 40 /** 41 * octeon_i2c_wait - wait for the IFLG to be set 42 * @i2c: The struct octeon_i2c 43 * 44 * Returns 0 on success, otherwise a negative errno. 45 */ 46 static int octeon_i2c_wait(struct octeon_i2c *i2c) 47 { 48 long time_left; 49 50 /* 51 * Some chip revisions don't assert the irq in the interrupt 52 * controller. So we must poll for the IFLG change. 53 */ 54 if (i2c->broken_irq_mode) { 55 u64 end = get_jiffies_64() + i2c->adap.timeout; 56 57 while (!octeon_i2c_test_iflg(i2c) && 58 time_before64(get_jiffies_64(), end)) 59 usleep_range(I2C_OCTEON_EVENT_WAIT / 2, I2C_OCTEON_EVENT_WAIT); 60 61 return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT; 62 } 63 64 if (IS_LS_FREQ(i2c->twsi_freq)) { 65 i2c->int_enable(i2c); 66 time_left = wait_event_timeout(i2c->queue, 67 octeon_i2c_test_iflg(i2c), 68 i2c->adap.timeout); 69 i2c->int_disable(i2c); 70 } else { 71 time_left = 1000; /* 1ms */ 72 do { 73 if (time_left--) > 74 __udelay(1); 75 } while (!octeon_i2c_test_iflg(i2c) && time_left); 76 } 77 78 if (i2c->broken_irq_check && !time_left && 79 octeon_i2c_test_iflg(i2c)) { 80 dev_err(i2c->dev, "broken irq connection detected, switching to polling mode.\n"); 81 i2c->broken_irq_mode = true; 82 return 0; 83 } 84 85 if (!time_left) 86 return -ETIMEDOUT; 87 88 return 0; 89 } 90
Hi Suneel and Piyush, [...] > @@ -61,10 +61,19 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c) > return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT; > } > > - i2c->int_enable(i2c); > - time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c), > - i2c->adap.timeout); > - i2c->int_disable(i2c); > + if (IS_LS_FREQ(i2c->twsi_freq)) { > + i2c->int_enable(i2c); > + time_left = wait_event_timeout(i2c->queue, > + octeon_i2c_test_iflg(i2c), > + i2c->adap.timeout); > + i2c->int_disable(i2c); > + } else { > + time_left = 1000; /* 1ms */ > + do { > + if (time_left--) > + __udelay(1); Are you sure you want to wait 1ms with __udelay(). This is a bit dsruptive, can we use a more relaxed waiting method? > + } while (!octeon_i2c_test_iflg(i2c) && time_left); > + } [...] > * Find divisors to produce target frequency, start with large delta > * to cover wider range of divisors, note thp = TCLK half period. > */ > - int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = huge_delta; > + int ds = 10, thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = huge_delta; unsigned? [...] > + if (octeon_i2c_is_otx2(to_pci_dev(i2c->dev))) { > + u64 mode; > + > + mode = __raw_readq(i2c->twsi_base + MODE(i2c)); > + /* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */ > + if (!IS_LS_FREQ(i2c->twsi_freq)) > + mode |= BIT(4) | BIT(0); > + else > + mode &= ~(BIT(4) | BIT(0)); would be nice to have this defined and with some meaning as comment. > + octeon_i2c_writeq_flush(mode, i2c->twsi_base + MODE(i2c)); > + } Robert, any comment here? Thanks, Andi
diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c index dfd58bbec47b1f0554ae0c100c680b6ba9be61ec..7c49dc8ccbd2ef05fec675d282193b98f2b69835 100644 --- a/drivers/i2c/busses/i2c-octeon-core.c +++ b/drivers/i2c/busses/i2c-octeon-core.c @@ -61,10 +61,19 @@ static int octeon_i2c_wait(struct octeon_i2c *i2c) return octeon_i2c_test_iflg(i2c) ? 0 : -ETIMEDOUT; } - i2c->int_enable(i2c); - time_left = wait_event_timeout(i2c->queue, octeon_i2c_test_iflg(i2c), - i2c->adap.timeout); - i2c->int_disable(i2c); + if (IS_LS_FREQ(i2c->twsi_freq)) { + i2c->int_enable(i2c); + time_left = wait_event_timeout(i2c->queue, + octeon_i2c_test_iflg(i2c), + i2c->adap.timeout); + i2c->int_disable(i2c); + } else { + time_left = 1000; /* 1ms */ + do { + if (time_left--) + __udelay(1); + } while (!octeon_i2c_test_iflg(i2c) && time_left); + } if (i2c->broken_irq_check && !time_left && octeon_i2c_test_iflg(i2c)) { @@ -608,25 +617,27 @@ int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) struct octeon_i2c *i2c = i2c_get_adapdata(adap); int i, ret = 0; - if (num == 1) { - if (msgs[0].len > 0 && msgs[0].len <= 8) { - if (msgs[0].flags & I2C_M_RD) - ret = octeon_i2c_hlc_read(i2c, msgs); - else - ret = octeon_i2c_hlc_write(i2c, msgs); - goto out; - } - } else if (num == 2) { - if ((msgs[0].flags & I2C_M_RD) == 0 && - (msgs[1].flags & I2C_M_RECV_LEN) == 0 && - msgs[0].len > 0 && msgs[0].len <= 2 && - msgs[1].len > 0 && msgs[1].len <= 8 && - msgs[0].addr == msgs[1].addr) { - if (msgs[1].flags & I2C_M_RD) - ret = octeon_i2c_hlc_comp_read(i2c, msgs); - else - ret = octeon_i2c_hlc_comp_write(i2c, msgs); - goto out; + if (IS_LS_FREQ(i2c->twsi_freq)) { + if (num == 1) { + if (msgs[0].len > 0 && msgs[0].len <= 8) { + if (msgs[0].flags & I2C_M_RD) + ret = octeon_i2c_hlc_read(i2c, msgs); + else + ret = octeon_i2c_hlc_write(i2c, msgs); + goto out; + } + } else if (num == 2) { + if ((msgs[0].flags & I2C_M_RD) == 0 && + (msgs[1].flags & I2C_M_RECV_LEN) == 0 && + msgs[0].len > 0 && msgs[0].len <= 2 && + msgs[1].len > 0 && msgs[1].len <= 8 && + msgs[0].addr == msgs[1].addr) { + if (msgs[1].flags & I2C_M_RD) + ret = octeon_i2c_hlc_comp_read(i2c, msgs); + else + ret = octeon_i2c_hlc_comp_write(i2c, msgs); + goto out; + } } } @@ -666,11 +677,13 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c) * Find divisors to produce target frequency, start with large delta * to cover wider range of divisors, note thp = TCLK half period. */ - int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = huge_delta; + int ds = 10, thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = huge_delta; if (octeon_i2c_is_otx2(to_pci_dev(i2c->dev))) { thp = 0x3; mdiv_min = 0; + if (!IS_LS_FREQ(i2c->twsi_freq)) + ds = 15; } for (ndiv_idx = 0; ndiv_idx < 8 && delta_hz != 0; ndiv_idx++) { @@ -683,7 +696,7 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c) * For given ndiv and mdiv values check the * two closest thp values. */ - tclk = i2c->twsi_freq * (mdiv_idx + 1) * 10; + tclk = i2c->twsi_freq * (mdiv_idx + 1) * ds; tclk *= (1 << ndiv_idx); if (octeon_i2c_is_otx2(to_pci_dev(i2c->dev))) thp_base = (i2c->sys_freq / tclk) - 2; @@ -701,7 +714,9 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c) foscl = i2c->sys_freq / (2 * (thp_idx + 1)); foscl = foscl / (1 << ndiv_idx); - foscl = foscl / (mdiv_idx + 1) / 10; + foscl = foscl / (mdiv_idx + 1) / ds; + if (foscl > i2c->twsi_freq) + continue; diff = abs(foscl - i2c->twsi_freq); /* Use it if smaller diff from target */ if (diff < delta_hz) { @@ -715,6 +730,17 @@ void octeon_i2c_set_clock(struct octeon_i2c *i2c) } octeon_i2c_reg_write(i2c, SW_TWSI_OP_TWSI_CLK, thp); octeon_i2c_reg_write(i2c, SW_TWSI_EOP_TWSI_CLKCTL, (mdiv << 3) | ndiv); + if (octeon_i2c_is_otx2(to_pci_dev(i2c->dev))) { + u64 mode; + + mode = __raw_readq(i2c->twsi_base + MODE(i2c)); + /* Set REFCLK_SRC and HS_MODE in TWSX_MODE register */ + if (!IS_LS_FREQ(i2c->twsi_freq)) + mode |= BIT(4) | BIT(0); + else + mode &= ~(BIT(4) | BIT(0)); + octeon_i2c_writeq_flush(mode, i2c->twsi_base + MODE(i2c)); + } } int octeon_i2c_init_lowlevel(struct octeon_i2c *i2c) diff --git a/drivers/i2c/busses/i2c-octeon-core.h b/drivers/i2c/busses/i2c-octeon-core.h index 8a0033c94a8a291fb255b0da03858274035c46f4..89d7d3bb8e30bd5787978d17d5a9b20ab0d41e22 100644 --- a/drivers/i2c/busses/i2c-octeon-core.h +++ b/drivers/i2c/busses/i2c-octeon-core.h @@ -93,11 +93,13 @@ struct octeon_i2c_reg_offset { unsigned int sw_twsi; unsigned int twsi_int; unsigned int sw_twsi_ext; + unsigned int mode; }; #define SW_TWSI(x) (x->roff.sw_twsi) #define TWSI_INT(x) (x->roff.twsi_int) #define SW_TWSI_EXT(x) (x->roff.sw_twsi_ext) +#define MODE(x) (x->roff.mode) struct octeon_i2c { wait_queue_head_t queue; @@ -212,6 +214,7 @@ static inline void octeon_i2c_write_int(struct octeon_i2c *i2c, u64 data) octeon_i2c_writeq_flush(data, i2c->twsi_base + TWSI_INT(i2c)); } +#define IS_LS_FREQ(twsi_freq) ((twsi_freq) <= 400000) #define PCI_SUBSYS_DEVID_9XXX 0xB /** * octeon_i2c_is_otx2 - check for chip ID diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c index eecd27f9f1730e522dcccafc9f12ea891a3b59ef..3dd5a4d798f99e9b5282360cf9d5840042fc8dcc 100644 --- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c +++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c @@ -165,6 +165,7 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev, i2c->roff.sw_twsi = 0x1000; i2c->roff.twsi_int = 0x1010; i2c->roff.sw_twsi_ext = 0x1018; + i2c->roff.mode = 0x1038; i2c->dev = dev; pci_set_drvdata(pdev, i2c); @@ -209,7 +210,7 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev, * For OcteonTX2 chips, set reference frequency to 100MHz * as refclk_src in TWSI_MODE register defaults to 100MHz. */ - if (octeon_i2c_is_otx2(pdev)) + if (octeon_i2c_is_otx2(pdev) && IS_LS_FREQ(i2c->twsi_freq)) i2c->sys_freq = 100000000; octeon_i2c_set_clock(i2c);