Message ID | 20220220035321.3870-12-warp5tw@gmail.com |
---|---|
State | New |
Headers | show |
Series | i2c: npcm: Bug fixes timeout, spurious interrupts | expand |
On 20/02/2022 04:53, Tyrone Ting wrote: > From: Tyrone Ting <kfting@nuvoton.com> > > Add NPCM8XX I2C support. > The NPCM8XX uses a similar i2c module as NPCM7XX. > The internal HW FIFO is larger in NPCM8XX. > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > Signed-off-by: Tali Perry <tali.perry1@gmail.com> > --- > drivers/i2c/busses/Kconfig | 8 +-- > drivers/i2c/busses/Makefile | 2 +- > drivers/i2c/busses/i2c-npcm7xx.c | 87 ++++++++++++++++++++++---------- > 3 files changed, 66 insertions(+), 31 deletions(-) > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 42da31c1ab70..ab9ee2de5e00 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -817,13 +817,13 @@ config I2C_NOMADIK > I2C interface from ST-Ericsson's Nomadik and Ux500 architectures, > as well as the STA2X11 PCIe I/O HUB. > > -config I2C_NPCM7XX > +config I2C_NPCM > tristate "Nuvoton I2C Controller" > - depends on ARCH_NPCM7XX || COMPILE_TEST > + depends on ARCH_NPCM || COMPILE_TEST > help > If you say yes to this option, support will be included for the > - Nuvoton I2C controller, which is available on the NPCM7xx BMC > - controller. > + Nuvoton I2C controller, which is available on the NPCM BMC > + controllers. > Driver can also support slave mode (select I2C_SLAVE). > > config I2C_OCORES > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 1d00dce77098..01fdf74a5565 100644 > --- a/drivers/i2c/busses/Makefile > +++ b/drivers/i2c/busses/Makefile > @@ -80,7 +80,7 @@ obj-$(CONFIG_I2C_MT7621) += i2c-mt7621.o > obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o > obj-$(CONFIG_I2C_MXS) += i2c-mxs.o > obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o > -obj-$(CONFIG_I2C_NPCM7XX) += i2c-npcm7xx.o > +obj-$(CONFIG_I2C_NPCM) += i2c-npcm7xx.o > obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o > obj-$(CONFIG_I2C_OMAP) += i2c-omap.o > obj-$(CONFIG_I2C_OWL) += i2c-owl.o > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c > index 2cbf9c679aed..b281e0424e3e 100644 > --- a/drivers/i2c/busses/i2c-npcm7xx.c > +++ b/drivers/i2c/busses/i2c-npcm7xx.c > @@ -17,6 +17,7 @@ > #include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > > @@ -91,7 +92,7 @@ enum i2c_addr { > > /* init register and default value required to enable module */ > #define NPCM_I2CSEGCTL 0xE4 > -#define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000 > +#define NPCM_I2CSEGCTL_INIT_VAL bus->data->segctl_init_val > > /* Common regs */ > #define NPCM_I2CSDA 0x00 > @@ -228,8 +229,7 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { > #define NPCM_I2CFIF_CTS_CLR_FIFO BIT(6) > #define NPCM_I2CFIF_CTS_SLVRSTR BIT(7) > > -/* NPCM_I2CTXF_CTL reg fields */ > -#define NPCM_I2CTXF_CTL_TX_THR GENMASK(4, 0) > +/* NPCM_I2CTXF_CTL reg field */ > #define NPCM_I2CTXF_CTL_THR_TXIE BIT(6) > > /* NPCM_I2CT_OUT reg fields */ > @@ -238,22 +238,22 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { > #define NPCM_I2CT_OUT_T_OUTST BIT(7) > > /* NPCM_I2CTXF_STS reg fields */ > -#define NPCM_I2CTXF_STS_TX_BYTES GENMASK(4, 0) > +#define NPCM_I2CTXF_STS_TX_BYTES bus->data->txf_sts_tx_bytes It's not a clean code to use defines for complex types. It's not a constant anymore, so just use bus->data->txf_sts_tx_bytes directly. The same in other places. > #define NPCM_I2CTXF_STS_TX_THST BIT(6) > Best regards, Krzysztof
Hi Krzysztof: Thank you for your comments and they'll be addressed. Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> 於 2022年2月20日 週日 下午5:36寫道: > > On 20/02/2022 04:53, Tyrone Ting wrote: > > From: Tyrone Ting <kfting@nuvoton.com> > > > > Add NPCM8XX I2C support. > > The NPCM8XX uses a similar i2c module as NPCM7XX. > > The internal HW FIFO is larger in NPCM8XX. > > > > Signed-off-by: Tyrone Ting <kfting@nuvoton.com> > > Signed-off-by: Tali Perry <tali.perry1@gmail.com> > > --- > > drivers/i2c/busses/Kconfig | 8 +-- > > drivers/i2c/busses/Makefile | 2 +- > > drivers/i2c/busses/i2c-npcm7xx.c | 87 ++++++++++++++++++++++---------- > > 3 files changed, 66 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index 42da31c1ab70..ab9ee2de5e00 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -817,13 +817,13 @@ config I2C_NOMADIK > > I2C interface from ST-Ericsson's Nomadik and Ux500 architectures, > > as well as the STA2X11 PCIe I/O HUB. > > > > -config I2C_NPCM7XX > > +config I2C_NPCM > > tristate "Nuvoton I2C Controller" > > - depends on ARCH_NPCM7XX || COMPILE_TEST > > + depends on ARCH_NPCM || COMPILE_TEST > > help > > If you say yes to this option, support will be included for the > > - Nuvoton I2C controller, which is available on the NPCM7xx BMC > > - controller. > > + Nuvoton I2C controller, which is available on the NPCM BMC > > + controllers. > > Driver can also support slave mode (select I2C_SLAVE). > > > > config I2C_OCORES > > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > > index 1d00dce77098..01fdf74a5565 100644 > > --- a/drivers/i2c/busses/Makefile > > +++ b/drivers/i2c/busses/Makefile > > @@ -80,7 +80,7 @@ obj-$(CONFIG_I2C_MT7621) += i2c-mt7621.o > > obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o > > obj-$(CONFIG_I2C_MXS) += i2c-mxs.o > > obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o > > -obj-$(CONFIG_I2C_NPCM7XX) += i2c-npcm7xx.o > > +obj-$(CONFIG_I2C_NPCM) += i2c-npcm7xx.o > > obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o > > obj-$(CONFIG_I2C_OMAP) += i2c-omap.o > > obj-$(CONFIG_I2C_OWL) += i2c-owl.o > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c > > index 2cbf9c679aed..b281e0424e3e 100644 > > --- a/drivers/i2c/busses/i2c-npcm7xx.c > > +++ b/drivers/i2c/busses/i2c-npcm7xx.c > > @@ -17,6 +17,7 @@ > > #include <linux/mfd/syscon.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_device.h> > > #include <linux/platform_device.h> > > #include <linux/regmap.h> > > > > @@ -91,7 +92,7 @@ enum i2c_addr { > > > > /* init register and default value required to enable module */ > > #define NPCM_I2CSEGCTL 0xE4 > > -#define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000 > > +#define NPCM_I2CSEGCTL_INIT_VAL bus->data->segctl_init_val > > > > /* Common regs */ > > #define NPCM_I2CSDA 0x00 > > @@ -228,8 +229,7 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { > > #define NPCM_I2CFIF_CTS_CLR_FIFO BIT(6) > > #define NPCM_I2CFIF_CTS_SLVRSTR BIT(7) > > > > -/* NPCM_I2CTXF_CTL reg fields */ > > -#define NPCM_I2CTXF_CTL_TX_THR GENMASK(4, 0) > > +/* NPCM_I2CTXF_CTL reg field */ > > #define NPCM_I2CTXF_CTL_THR_TXIE BIT(6) > > > > /* NPCM_I2CT_OUT reg fields */ > > @@ -238,22 +238,22 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { > > #define NPCM_I2CT_OUT_T_OUTST BIT(7) > > > > /* NPCM_I2CTXF_STS reg fields */ > > -#define NPCM_I2CTXF_STS_TX_BYTES GENMASK(4, 0) > > +#define NPCM_I2CTXF_STS_TX_BYTES bus->data->txf_sts_tx_bytes > > It's not a clean code to use defines for complex types. It's not a > constant anymore, so just use bus->data->txf_sts_tx_bytes directly. > > The same in other places. > > > #define NPCM_I2CTXF_STS_TX_THST BIT(6) > > > > Best regards, > Krzysztof Best regards, Tyrone
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 42da31c1ab70..ab9ee2de5e00 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -817,13 +817,13 @@ config I2C_NOMADIK I2C interface from ST-Ericsson's Nomadik and Ux500 architectures, as well as the STA2X11 PCIe I/O HUB. -config I2C_NPCM7XX +config I2C_NPCM tristate "Nuvoton I2C Controller" - depends on ARCH_NPCM7XX || COMPILE_TEST + depends on ARCH_NPCM || COMPILE_TEST help If you say yes to this option, support will be included for the - Nuvoton I2C controller, which is available on the NPCM7xx BMC - controller. + Nuvoton I2C controller, which is available on the NPCM BMC + controllers. Driver can also support slave mode (select I2C_SLAVE). config I2C_OCORES diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 1d00dce77098..01fdf74a5565 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -80,7 +80,7 @@ obj-$(CONFIG_I2C_MT7621) += i2c-mt7621.o obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o obj-$(CONFIG_I2C_MXS) += i2c-mxs.o obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o -obj-$(CONFIG_I2C_NPCM7XX) += i2c-npcm7xx.o +obj-$(CONFIG_I2C_NPCM) += i2c-npcm7xx.o obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o obj-$(CONFIG_I2C_OMAP) += i2c-omap.o obj-$(CONFIG_I2C_OWL) += i2c-owl.o diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c index 2cbf9c679aed..b281e0424e3e 100644 --- a/drivers/i2c/busses/i2c-npcm7xx.c +++ b/drivers/i2c/busses/i2c-npcm7xx.c @@ -17,6 +17,7 @@ #include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/regmap.h> @@ -91,7 +92,7 @@ enum i2c_addr { /* init register and default value required to enable module */ #define NPCM_I2CSEGCTL 0xE4 -#define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000 +#define NPCM_I2CSEGCTL_INIT_VAL bus->data->segctl_init_val /* Common regs */ #define NPCM_I2CSDA 0x00 @@ -228,8 +229,7 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { #define NPCM_I2CFIF_CTS_CLR_FIFO BIT(6) #define NPCM_I2CFIF_CTS_SLVRSTR BIT(7) -/* NPCM_I2CTXF_CTL reg fields */ -#define NPCM_I2CTXF_CTL_TX_THR GENMASK(4, 0) +/* NPCM_I2CTXF_CTL reg field */ #define NPCM_I2CTXF_CTL_THR_TXIE BIT(6) /* NPCM_I2CT_OUT reg fields */ @@ -238,22 +238,22 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { #define NPCM_I2CT_OUT_T_OUTST BIT(7) /* NPCM_I2CTXF_STS reg fields */ -#define NPCM_I2CTXF_STS_TX_BYTES GENMASK(4, 0) +#define NPCM_I2CTXF_STS_TX_BYTES bus->data->txf_sts_tx_bytes #define NPCM_I2CTXF_STS_TX_THST BIT(6) /* NPCM_I2CRXF_STS reg fields */ -#define NPCM_I2CRXF_STS_RX_BYTES GENMASK(4, 0) +#define NPCM_I2CRXF_STS_RX_BYTES bus->data->rxf_sts_rx_bytes #define NPCM_I2CRXF_STS_RX_THST BIT(6) /* NPCM_I2CFIF_CTL reg fields */ #define NPCM_I2CFIF_CTL_FIFO_EN BIT(4) /* NPCM_I2CRXF_CTL reg fields */ -#define NPCM_I2CRXF_CTL_RX_THR GENMASK(4, 0) -#define NPCM_I2CRXF_CTL_LAST_PEC BIT(5) +#define NPCM_I2CRXF_CTL_LAST_PEC bus->data->rxf_ctl_last_pec #define NPCM_I2CRXF_CTL_THR_RXIE BIT(6) -#define I2C_HW_FIFO_SIZE 16 +#define MAX_I2C_HW_FIFO_SIZE 32 +#define I2C_HW_FIFO_SIZE bus->data->fifo_size /* I2C_VER reg fields */ #define I2C_VER_VERSION GENMASK(6, 0) @@ -270,11 +270,36 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { #define I2C_FREQ_MIN_HZ 10000 #define I2C_FREQ_MAX_HZ I2C_MAX_FAST_MODE_PLUS_FREQ +struct npcm_i2c_data { + u8 fifo_size; + u32 segctl_init_val; + u8 txf_sts_tx_bytes; + u8 rxf_sts_rx_bytes; + u8 rxf_ctl_last_pec; +}; + +static const struct npcm_i2c_data npxm7xx_i2c_data = { + .fifo_size = 16, + .segctl_init_val = 0x0333F000, + .txf_sts_tx_bytes = GENMASK(4, 0), + .rxf_sts_rx_bytes = GENMASK(4, 0), + .rxf_ctl_last_pec = BIT(5) +}; + +static const struct npcm_i2c_data npxm8xx_i2c_data = { + .fifo_size = 32, + .segctl_init_val = 0x9333F000, + .txf_sts_tx_bytes = GENMASK(5, 0), + .rxf_sts_rx_bytes = GENMASK(5, 0), + .rxf_ctl_last_pec = BIT(7) +}; + /* Status of one I2C module */ struct npcm_i2c { struct i2c_adapter adap; struct device *dev; unsigned char __iomem *reg; + const struct npcm_i2c_data *data; spinlock_t lock; /* IRQ synchronization */ struct completion cmd_complete; int cmd_err; @@ -307,8 +332,8 @@ struct npcm_i2c { int slv_rd_ind; int slv_wr_size; int slv_wr_ind; - u8 slv_rd_buf[I2C_HW_FIFO_SIZE]; - u8 slv_wr_buf[I2C_HW_FIFO_SIZE]; + u8 slv_rd_buf[MAX_I2C_HW_FIFO_SIZE]; + u8 slv_wr_buf[MAX_I2C_HW_FIFO_SIZE]; #endif struct dentry *debugfs; /* debugfs device directory */ u64 ber_cnt; @@ -743,11 +768,11 @@ static void npcm_i2c_callback(struct npcm_i2c *bus, static u8 npcm_i2c_fifo_usage(struct npcm_i2c *bus) { if (bus->operation == I2C_WRITE_OPER) - return FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES, - ioread8(bus->reg + NPCM_I2CTXF_STS)); + return (NPCM_I2CTXF_STS_TX_BYTES & + ioread8(bus->reg + NPCM_I2CTXF_STS)); if (bus->operation == I2C_READ_OPER) - return FIELD_GET(NPCM_I2CRXF_STS_RX_BYTES, - ioread8(bus->reg + NPCM_I2CRXF_STS)); + return (NPCM_I2CRXF_STS_RX_BYTES & + ioread8(bus->reg + NPCM_I2CRXF_STS)); return 0; } @@ -882,10 +907,10 @@ static void npcm_i2c_write_fifo_slave(struct npcm_i2c *bus, u16 max_bytes) while (max_bytes-- && I2C_HW_FIFO_SIZE != npcm_i2c_fifo_usage(bus)) { if (bus->slv_wr_size <= 0) break; - bus->slv_wr_ind = bus->slv_wr_ind % I2C_HW_FIFO_SIZE; + bus->slv_wr_ind = bus->slv_wr_ind & (I2C_HW_FIFO_SIZE - 1); npcm_i2c_wr_byte(bus, bus->slv_wr_buf[bus->slv_wr_ind]); bus->slv_wr_ind++; - bus->slv_wr_ind = bus->slv_wr_ind % I2C_HW_FIFO_SIZE; + bus->slv_wr_ind = bus->slv_wr_ind & (I2C_HW_FIFO_SIZE - 1); bus->slv_wr_size--; } } @@ -900,7 +925,7 @@ static void npcm_i2c_read_fifo_slave(struct npcm_i2c *bus, u8 bytes_in_fifo) while (bytes_in_fifo--) { data = npcm_i2c_rd_byte(bus); - bus->slv_rd_ind = bus->slv_rd_ind % I2C_HW_FIFO_SIZE; + bus->slv_rd_ind = bus->slv_rd_ind & (I2C_HW_FIFO_SIZE - 1); bus->slv_rd_buf[bus->slv_rd_ind] = data; bus->slv_rd_ind++; @@ -927,7 +952,7 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus) } else { i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value); } - ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE; + ind = (bus->slv_wr_ind + bus->slv_wr_size) & (I2C_HW_FIFO_SIZE - 1); bus->slv_wr_buf[ind] = value; bus->slv_wr_size++; } @@ -999,8 +1024,8 @@ static void npcm_i2c_slave_wr_buf_sync(struct npcm_i2c *bus) { int left_in_fifo; - left_in_fifo = FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES, - ioread8(bus->reg + NPCM_I2CTXF_STS)); + left_in_fifo = (NPCM_I2CTXF_STS_TX_BYTES & + ioread8(bus->reg + NPCM_I2CTXF_STS)); /* fifo already full: */ if (left_in_fifo >= I2C_HW_FIFO_SIZE || @@ -2265,12 +2290,21 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev, bus->debugfs = d; } +static const struct of_device_id npcm_i2c_bus_of_table[] = { + { .compatible = "nuvoton,npcm750-i2c", .data = &npxm7xx_i2c_data }, + { .compatible = "nuvoton,npcm845-i2c", .data = &npxm8xx_i2c_data }, + {} +}; +MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table); + static int npcm_i2c_probe_bus(struct platform_device *pdev) { struct npcm_i2c *bus; struct i2c_adapter *adap; struct clk *i2c_clk; static struct regmap *gcr_regmap; + struct device *dev = &pdev->dev; + const struct of_device_id *match; int irq; int ret; struct device_node *np = pdev->dev.of_node; @@ -2281,6 +2315,13 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev) bus->dev = &pdev->dev; + match = of_match_device(npcm_i2c_bus_of_table, dev); + if (!match) { + dev_err(dev, "OF data missing\n"); + return -EINVAL; + } + bus->data = match->data; + bus->num = of_alias_get_id(pdev->dev.of_node, "i2c"); /* core clk must be acquired to calculate module timing settings */ i2c_clk = devm_clk_get(&pdev->dev, NULL); @@ -2352,12 +2393,6 @@ static int npcm_i2c_remove_bus(struct platform_device *pdev) return 0; } -static const struct of_device_id npcm_i2c_bus_of_table[] = { - { .compatible = "nuvoton,npcm750-i2c", }, - {} -}; -MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table); - static struct platform_driver npcm_i2c_bus_driver = { .probe = npcm_i2c_probe_bus, .remove = npcm_i2c_remove_bus,