Hi Hans,
[...]
> +#define IICCR_L 0x00
> +#define MST_RST BIT(7)
> +#define MST_RST_PATCH BIT(6)
> +#define CPU_RDY BIT(3)
> +#define TX_END BIT(2)
> +#define RX_ACK BIT(1)
> +#define IICCR_H 0x01
> +#define FIFO_EN BIT(6)
> +#define IICSLVADDR 0x02
> +#define IICTCR 0x03
> +#define FAST_SEL BIT(7)
> +#define MASTER_RECV BIT(6)
> +#define HS_SEL BIT(5)
> +#define IICSR 0x04
> +#define SDA_I BIT(3)
> +#define SCL_I BIT(2)
> +#define READY BIT(1)
> +#define RCV_NACK BIT(0)
> +#define IICISR 0x06
> +#define IRQ_STS_BYTENACK BIT(5)
> +#define IRQ_STS_FIFONACK BIT(4)
> +#define IRQ_STS_FIFOEND BIT(3)
> +#define IRQ_SCL_TIMEOUT BIT(2)
> +#define IRQ_STS_BYTEEND BIT(1)
> +#define IRQ_STS_ADDRNACK BIT(0)
> +#define IRQ_STS_MASK GENMASK(3, 0)
> +#define IICIMR 0x08
> +#define IRQ_EN_FIFOEND BIT(3)
> +#define IRQ_EN_TIMEOUT BIT(2)
> +#define IRQ_EN_BYTEEND BIT(1)
> +#define IRQ_EN_ADDRNACK BIT(0)
> +#define IICDATA2IIC 0x0A
> +#define IICDATA2CPU 0x0B
> +#define IICTR_FSTP 0x0C
> +#define IICTR_SCLTP 0x0D
> +#define IICMCR 0x0E
> +#define DYCLK_EN BIT(0)
> +#define IIC_MST_CODE 0x0F
> +#define IICCS 0x10
> +#define CLKSEL_50M BIT(0)
> +#define IICREV 0x11
> +#define IICHCR 0x12
> +#define FIFO_RST GENMASK(1, 0)
> +#define IICHTDR 0x13
> +#define IICHRDR 0x14
> +#define IICHTLR 0x15
> +#define IICHRLR 0x16
> +#define IICHWCNTR 0x18
> +#define IICHRCNTR 0x19
IIC is not a prefix of this driver. Below you have used ZXI2C (or
even ZX), can we use that as prefix?
> +#define FIFO_SIZE 32
> +#define ZXI2C_TIMEOUT 1000
[...]
> +#define set_byte(r, d) iowrite8(d, r + IICDATA2IIC)
> +#define get_byte(r) ioread8(r + IICDATA2CPU)
> +#define is_ready(r) (ioread8(r + IICSR) & READY)
> +#define is_nack(r) (ioread8(r + IICSR) & RCV_NACK)
> +#define get_irq_status(r) ioread8(r + IICISR)
> +#define get_reversion(r) ioread8(r + IICREV)
> +#define clear_irq_status(r) iowrite8(IRQ_STS_MASK, r + IICISR)
> +#define set_fifo_byte(r, d) iowrite8(d, r + IICHTDR)
> +#define get_fifo_byte(r) ioread8(r + IICHRDR)
> +#define set_fifo_wr_len(r, d) iowrite8(d, r + IICHTLR)
> +#define set_fifo_rd_len(r, d) iowrite8(d, r + IICHRLR)
> +#define get_fifo_wr_cnt(r) ioread8(r + IICHWCNTR)
> +#define get_fifo_rd_cnt(r) ioread8(r + IICHRCNTR)
> +#define master_regs_reset(r) iowrite8(MST_RST | 0x41, r + IICCR_L)
> +#define set_dynamic_clock(r, d) iowrite8(d, r + IICMCR)
> +#define get_dynamic_clock(r) (ioread8(r + IICMCR) & DYCLK_EN)
> +#define stop_write_byte(r) iowrite8(TX_END | 0x41, r + IICCR_L)
> +#define get_fstp_value(r) ioread8(r + IICTR_FSTP)
> +#define enable_fifo_mode(r) iowrite8(FIFO_EN, r + IICCR_H)
> +#define enable_byte_mode(r) iowrite8(0, r + IICCR_H)
same here set_byte/get_byte/is_ready/is_nack/... can we have a
prefix for them, as well?
zxi2c_set_byte/zxi2c_get_byte/zxi2c_is_read/zxi2c_is_nack/... (or
just zx_set_byte/zx_get_byte/zx_is_read/zx_is_nack/... as long
as you are consistent)
[...]
> +static void zxi2c_reset_fifo(struct zxi2c *i2c)
> +{
> + u8 tmp;
> + u8 count;
> + void __iomem *regs = i2c->regs;
> +
> + tmp = ioread8(regs + IICHCR);
> + iowrite8(tmp | FIFO_RST, regs + IICHCR);
> + for (count = 0; count < 50; count++)
> + if (!(ioread8(regs + IICHCR) & FIFO_RST))
> + break;
> + if (count >= 50)
> + dev_warn(i2c->dev, "%s failed\n", __func__);
are you sure you want to try 50 times? Is it expected to fail
resetting this many times? Besides, how long does it take? don't
we want to have exclusive access during this period?
> +}
[...]
> +static const u32 speed_params_table[][3] = {
> + /* speed, IICTCR, IICTR_FSTP, IICCS, IICTR_SCLTP */
> + {I2C_MAX_STANDARD_MODE_FREQ, 0, 0xF3},
> + {I2C_MAX_FAST_MODE_FREQ, FAST_SEL, 0x38},
> + {I2C_MAX_FAST_MODE_PLUS_FREQ, FAST_SEL, 0x13},
> + {I2C_MAX_HIGH_SPEED_MODE_FREQ, HS_SEL, 0x37},
> +};
what are the third values? Maybe we want to define them, as well?
> +
> +static void zxi2c_set_bus_speed(struct zxi2c *i2c)
> +{
> + u8 i, count;
> + const u32 *params = NULL;
> +
> + count = ARRAY_SIZE(speed_params_table);
> + for (i = 0; i < count; i++) {
> + if (speed_params_table[i][0] == i2c->speed) {
> + params = speed_params_table[i];
> + break;
> + }
> + }
> +
> + iowrite8(params[1], i2c->regs + IICTCR);
> + if (abs(i2c->fstp - params[2]) > 0x10) {
> + /* if BIOS setting value far from golden value,
> + * use golden value and warn user
> + */
0x10 is the golden value... can you please explain what does it
stand for?
> + dev_warn_once(i2c->dev, "speed:%d, fstp:0x%x, golden:0x%x\n",
> + i2c->speed, i2c->fstp, params[2]);
> + iowrite8(params[2], i2c->regs + IICTR_FSTP);
> + } else {
> + iowrite8(i2c->fstp, i2c->regs + IICTR_FSTP);
> + }
> +
> + iowrite8(CLKSEL_50M, i2c->regs + IICCS);
> + iowrite8(0xff, i2c->regs + IICTR_SCLTP);
> +
> + /* for Hs-mode, use 0000 1000 as master code */
> + if (i2c->speed == I2C_MAX_HIGH_SPEED_MODE_FREQ)
> + iowrite8(0x08, i2c->regs + IIC_MST_CODE);
you have defined all the registers and masks above so clearly,
can define 0x08, as well?
> +}
[...]
> +static int zxi2c_wait_event(struct zxi2c *i2c, u8 event)
> +{
> + int timeout;
> +
> + timeout = wait_event_interruptible_timeout(i2c->waitq,
> + i2c->event != 0,
> + msecs_to_jiffies(ZXI2C_TIMEOUT));
> +
> + if (timeout == 0) {
> + dev_err(i2c->dev, "timeout(SW), ID: 0x%X\n", i2c->addr);
> + /* Clock streching timeout, do recovery */
> + if (!is_nack(i2c->regs))
> + dev_err(i2c->dev, "device hang? reset, ID: 0x%X\n",
> + i2c->addr);
> +
> + master_regs_reset(i2c->regs);
> + zxi2c_set_bus_speed(i2c);
> +
> + return -EIO;
> + }
> +
> + if (i2c->event & event) {
> + if (timeout == 1)
> + dev_warn(i2c->dev, "thread may be blocked\n");
> + return 0;
> + }
thanks!
> + return -EIO;
> +}
> +
> +static int zxi2c_byte_xfer(struct zxi2c *i2c, struct i2c_msg *msgs, int num)
> +{
> + u16 i, finished;
> + u8 index;
> + int ret = 0;
> + struct i2c_msg *msg;
> + void __iomem *regs = i2c->regs;
> +
> + clear_irq_status(regs);
why do we need to clear the irq_status for each transfer? In this
device isn't it enough to do it in the irq handler?
> + enable_byte_mode(regs);
> + zxi2c_enable_irq(regs, IRQ_EN_BYTEEND, true);
[...]
> +static int zxi2c_fifo_xfer(struct zxi2c *i2c, struct i2c_msg *msgs)
> +{
> + void __iomem *regs = i2c->regs;
> + struct i2c_msg *msg = msgs;
> + int i;
> + u8 finished;
> +
> + i2c->addr = msg->addr;
> + i2c->is_read = !!(msg->flags & I2C_M_RD);
> + i2c->len = msg->len;
> +
> + zxi2c_reset_fifo(i2c);
this means that at every fifo_xfer we are going to try resetting
up to 50 times, right?
> + enable_fifo_mode(regs);
> +
> + clear_irq_status(regs);
> + zxi2c_enable_irq(regs, IRQ_EN_FIFOEND, true);
> +
> + zxi2c_set_wr(regs, i2c->is_read);
> + if (i2c->is_read) {
> + set_fifo_rd_len(regs, msg->len - 1);
> + } else {
> + set_fifo_wr_len(regs, msg->len - 1);
> + for (i = 0; i < msg->len; i++)
> + set_fifo_byte(regs, msg->buf[i]);
> + }
> +
> + zxi2c_start(i2c);
> + if (zxi2c_wait_event(i2c, IRQ_STS_FIFOEND))
> + return -ENODEV;
> +
> + if (i2c->is_read) {
> + finished = get_fifo_rd_cnt(regs);
> + for (i = 0; i < finished; i++)
> + msg->buf[i] = get_fifo_byte(regs);
> + } else {
> + finished = get_fifo_wr_cnt(regs);
> + }
> +
> + /* check if NACK during transmitting */
> + if (finished != msg->len) {
> + if (finished)
> + dev_err(i2c->dev,
> + "%s: %s only finished %d/%d bytes: %*ph\n",
> + __func__, i2c->is_read ? "read" : "write",
> + finished, msg->len, finished, msg->buf);
> + return -EAGAIN;
> + }
> +
> + zxi2c_enable_irq(regs, IRQ_EN_FIFOEND, false);
> + return 1;
> +}
> +
> +static int zxi2c_master_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + int error;
> + struct zxi2c *i2c = (struct zxi2c *)i2c_get_adapdata(adap);
> +
> + if (!is_ready(i2c->regs)) {
> + zxi2c_module_reset(i2c);
> + zxi2c_set_bus_speed(i2c);
> + dev_err(i2c->dev, "controller busy, reset try again\n");
> +
> + return -EAGAIN;
before returning we could eventually try checking again whether
the controller is ready:
if (!is_ready(i2c->regs)) {
dev_warn("device not ready\n");
module_reset();
set_bus_speed();
if (!is_ready(i2c->regs)) {
dev_err()
return -EBUSY;
}
}
> + }
> +
> + if (num == 1 && msgs->len <= FIFO_SIZE && msgs->len >= 3)
> + error = zxi2c_fifo_xfer(i2c, msgs);
> + else
> + error = zxi2c_byte_xfer(i2c, msgs, num);
> +
> + return error;
> +}
> +
> +static u32 zxi2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
how is smbus supported here?
> +}
[...]
> +static int zxi2c_probe(struct platform_device *pdev)
> +{
> + int error;
> + struct zxi2c *i2c;
> + struct pci_dev *pci;
> + struct device *dev;
> +
> + /* make sure this is zhaoxin platform */
> + dev = pdev->dev.parent;
> + if (dev && dev_is_pci(dev)) {
> + pci = to_pci_dev(dev);
> + if (pci->vendor != 0x1d17 || pci->device != 0x1001)
let's define vendor and device values?
> + return -ENODEV;
> + } else {
> + return -ENODEV;
> + }
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> + if (IS_ERR(i2c)) {
> + return dev_err_probe(&pdev->dev, -ENOMEM,
> + "devm_kzalloc failed\n");
> + }
no need for brackets here
Looks good!
Thanks,
Andi
> +
> + i2c->pci = pci;
> + i2c->dev = &pdev->dev;
> + error = zxi2c_parse_resources(i2c);
> + if (error)
> + return error;
