On 12/19/2018 12:15 PM, Heiko Schocher wrote: > Hello Marek, Hi,
> Am 19.12.2018 um 09:07 schrieb Marek Vasut: >> Add Xilinx AXI I2C controller driver based on the Linux i2c-xiic driver. >> This driver is stripped of all the IRQ handling and uses pure polling, >> yet tries to retain most of the structure of the Linux driver to make >> backporting of fixes easy. >> >> Note that the IP has a known limitation on 255 bytes read and write, >> according to xilinx this is still being worked on [1]. >> >> [1] >> https://forums.xilinx.com/t5/Embedded-Processor-System-Design/AXI-IIC-V2-0-I2C-Master-Reading-multiple-bytes-from-I2C-slave/m-p/854419/highlight/true#M39387 >> >> >> Signed-off-by: Marek Vasut <ma...@denx.de> >> Cc: Michal Simek <mon...@monstr.eu> >> Cc: Michal Simek <michal.si...@xilinx.com> >> Cc: Heiko Schocher <h...@denx.de> >> --- >> drivers/i2c/Kconfig | 6 + >> drivers/i2c/Makefile | 1 + >> drivers/i2c/xilinx_xiic.c | 340 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 347 insertions(+) >> create mode 100644 drivers/i2c/xilinx_xiic.c >> >> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig >> index 5eceab9ea8..810d861f32 100644 >> --- a/drivers/i2c/Kconfig >> +++ b/drivers/i2c/Kconfig >> @@ -437,6 +437,12 @@ config SYS_I2C_BUS_MAX >> help >> Define the maximum number of available I2C buses. >> +config SYS_I2C_XILINX_XIIC >> + bool "Xilinx AXI I2C driver" >> + depends on DM_I2C > > seems some formating problem ... Fixed >> + help >> + Support for Xilinx AXI I2C controller. >> + >> config SYS_I2C_ZYNQ >> bool "Xilinx I2C driver" >> depends on ARCH_ZYNQMP || ARCH_ZYNQ >> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile >> index da368cc02a..94c68c8ba1 100644 >> --- a/drivers/i2c/Makefile >> +++ b/drivers/i2c/Makefile >> @@ -38,6 +38,7 @@ obj-$(CONFIG_SYS_I2C_TEGRA) += tegra_i2c.o >> obj-$(CONFIG_SYS_I2C_UNIPHIER) += i2c-uniphier.o >> obj-$(CONFIG_SYS_I2C_UNIPHIER_F) += i2c-uniphier-f.o >> obj-$(CONFIG_SYS_I2C_ZYNQ) += zynq_i2c.o >> +obj-$(CONFIG_SYS_I2C_XILINX_XIIC) += xilinx_xiic.o > > please sort in alphabetical order. Fixed >> obj-$(CONFIG_TEGRA186_BPMP_I2C) += tegra186_bpmp_i2c.o >> obj-$(CONFIG_I2C_MUX) += muxes/ >> diff --git a/drivers/i2c/xilinx_xiic.c b/drivers/i2c/xilinx_xiic.c >> new file mode 100644 >> index 0000000000..180cd14fec >> --- /dev/null >> +++ b/drivers/i2c/xilinx_xiic.c >> @@ -0,0 +1,340 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Xilinx AXI I2C driver >> + * >> + * Copyright (C) 2018 Marek Vasut <ma...@denx.de> >> + * >> + * Based on Linux 4.14.y i2c-xiic.c >> + * Copyright (c) 2002-2007 Xilinx Inc. >> + * Copyright (c) 2009-2010 Intel Corporation >> + */ >> + >> +#include <common.h> >> +#include <clk.h> >> +#include <dm.h> >> +#include <i2c.h> >> +#include <wait_bit.h> >> +#include <asm/io.h> >> + >> +struct xilinx_xiic_priv { >> + void __iomem *base; >> + struct clk clk; >> +}; >> + >> +#define XIIC_MSB_OFFSET 0 >> +#define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET) >> + >> +/* >> + * Register offsets in bytes from RegisterBase. Three is added to the >> + * base offset to access LSB (IBM style) of the word >> + */ >> +#define XIIC_CR_REG_OFFSET (0x00+XIIC_REG_OFFSET) /* Control >> Register */ >> +#define XIIC_SR_REG_OFFSET (0x04+XIIC_REG_OFFSET) /* Status >> Register */ >> +#define XIIC_DTR_REG_OFFSET (0x08+XIIC_REG_OFFSET) /* Data Tx >> Register */ >> +#define XIIC_DRR_REG_OFFSET (0x0C+XIIC_REG_OFFSET) /* Data Rx >> Register */ >> +#define XIIC_ADR_REG_OFFSET (0x10+XIIC_REG_OFFSET) /* Address >> Register */ >> +#define XIIC_TFO_REG_OFFSET (0x14+XIIC_REG_OFFSET) /* Tx FIFO >> Occupancy */ >> +#define XIIC_RFO_REG_OFFSET (0x18+XIIC_REG_OFFSET) /* Rx FIFO >> Occupancy */ >> +#define XIIC_TBA_REG_OFFSET (0x1C+XIIC_REG_OFFSET) /* 10 Bit >> Address reg */ >> +#define XIIC_RFD_REG_OFFSET (0x20+XIIC_REG_OFFSET) /* Rx FIFO >> Depth reg */ >> +#define XIIC_GPO_REG_OFFSET (0x24+XIIC_REG_OFFSET) /* Output >> Register */ > > A struct would be much more nicer, but as you said it is taken from > linux code ... I am fine with it. Actually, struct is not a good idea. Once you get a slight variance in the register layout, the struct approach breaks down horribly. Using macros for registers seems much better in most cases. >> + >> +/* Control Register masks */ >> +#define XIIC_CR_ENABLE_DEVICE_MASK 0x01 /* Device enable = >> 1 */ >> +#define XIIC_CR_TX_FIFO_RESET_MASK 0x02 /* Transmit FIFO >> reset=1 */ >> +#define XIIC_CR_MSMS_MASK 0x04 /* Master starts >> Txing=1 */ >> +#define XIIC_CR_DIR_IS_TX_MASK 0x08 /* Dir of tx. >> Txing=1 */ >> +#define XIIC_CR_NO_ACK_MASK 0x10 /* Tx Ack. NO ack = >> 1 */ >> +#define XIIC_CR_REPEATED_START_MASK 0x20 /* Repeated start = >> 1 */ >> +#define XIIC_CR_GENERAL_CALL_MASK 0x40 /* Gen Call enabled >> = 1 */ >> + >> +/* Status Register masks */ >> +#define XIIC_SR_GEN_CALL_MASK 0x01 /* 1=a mstr issued >> a GC */ >> +#define XIIC_SR_ADDR_AS_SLAVE_MASK 0x02 /* 1=when addr as >> slave */ >> +#define XIIC_SR_BUS_BUSY_MASK 0x04 /* 1 = bus is >> busy */ >> +#define XIIC_SR_MSTR_RDING_SLAVE_MASK 0x08 /* 1=Dir: mstr <-- >> slave */ >> +#define XIIC_SR_TX_FIFO_FULL_MASK 0x10 /* 1 = Tx FIFO >> full */ >> +#define XIIC_SR_RX_FIFO_FULL_MASK 0x20 /* 1 = Rx FIFO >> full */ >> +#define XIIC_SR_RX_FIFO_EMPTY_MASK 0x40 /* 1 = Rx FIFO >> empty */ >> +#define XIIC_SR_TX_FIFO_EMPTY_MASK 0x80 /* 1 = Tx FIFO >> empty */ >> + >> +/* Interrupt Status Register masks Interrupt occurs when... */ >> +#define XIIC_INTR_ARB_LOST_MASK 0x01 /* 1 = arbitration >> lost */ >> +#define XIIC_INTR_TX_ERROR_MASK 0x02 /* 1=Tx error/msg >> complete */ >> +#define XIIC_INTR_TX_EMPTY_MASK 0x04 /* 1 = Tx FIFO/reg >> empty */ >> +#define XIIC_INTR_RX_FULL_MASK 0x08 /* 1=Rx >> FIFO/reg=OCY level */ >> +#define XIIC_INTR_BNB_MASK 0x10 /* 1 = Bus not >> busy */ >> +#define XIIC_INTR_AAS_MASK 0x20 /* 1 = when addr as >> slave */ >> +#define XIIC_INTR_NAAS_MASK 0x40 /* 1 = not addr as >> slave */ >> +#define XIIC_INTR_TX_HALF_MASK 0x80 /* 1 = TX FIFO half >> empty */ >> + >> +/* The following constants specify the depth of the FIFOs */ >> +#define IIC_RX_FIFO_DEPTH 16 /* Rx fifo >> capacity */ >> +#define IIC_TX_FIFO_DEPTH 16 /* Tx fifo >> capacity */ >> + >> +/* >> + * Tx Fifo upper bit masks. >> + */ >> +#define XIIC_TX_DYN_START_MASK 0x0100 /* 1 = Set dynamic >> start */ >> +#define XIIC_TX_DYN_STOP_MASK 0x0200 /* 1 = Set dynamic >> stop */ >> + >> +/* >> + * The following constants define the register offsets for the Interrupt >> + * registers. There are some holes in the memory map for reserved >> addresses >> + * to allow other registers to be added and still match the memory >> map of the >> + * interrupt controller registers >> + */ >> +#define XIIC_DGIER_OFFSET 0x1C /* Device Global Interrupt Enable >> Register */ >> +#define XIIC_IISR_OFFSET 0x20 /* Interrupt Status Register */ >> +#define XIIC_IIER_OFFSET 0x28 /* Interrupt Enable Register */ >> +#define XIIC_RESETR_OFFSET 0x40 /* Reset Register */ >> + >> +#define XIIC_RESET_MASK 0xAUL >> + >> +static u8 i2c_8bit_addr_from_flags(uint addr, u16 flags) >> +{ >> + return (addr << 1) | (flags & I2C_M_RD ? 1 : 0); >> +} >> + >> +static void xiic_irq_clr(struct xilinx_xiic_priv *priv, u32 mask) >> +{ >> + u32 isr = readl(priv->base + XIIC_IISR_OFFSET); >> + >> + writel(isr & mask, priv->base + XIIC_IISR_OFFSET); >> +} >> + >> +static int xiic_read_rx(struct xilinx_xiic_priv *priv, >> + struct i2c_msg *msg, int nmsgs) >> +{ >> + u8 bytes_in_fifo; >> + u32 pos = 0; >> + int i, ret; >> + >> + while (pos < msg->len) { >> + ret = wait_for_bit_8(priv->base + XIIC_SR_REG_OFFSET, >> + XIIC_SR_RX_FIFO_EMPTY_MASK, false, >> + 1000, true); >> + if (ret) >> + return ret; >> + >> + bytes_in_fifo = readb(priv->base + XIIC_RFO_REG_OFFSET) + 1; >> + >> + if (bytes_in_fifo > msg->len) >> + bytes_in_fifo = msg->len; >> + >> + for (i = 0; i < bytes_in_fifo; i++) { >> + msg->buf[pos++] = readb(priv->base + >> + XIIC_DRR_REG_OFFSET); >> + } > > No need for { brackets here. It's highly recommended for multi-line code to use brackets , even if it's multiline only because of comments. >> + } >> + >> + return 0; >> +} >> + >> +static int xiic_tx_fifo_space(struct xilinx_xiic_priv *priv) >> +{ >> + /* return the actual space left in the FIFO */ >> + return IIC_TX_FIFO_DEPTH - readb(priv->base + >> XIIC_TFO_REG_OFFSET) - 1; >> +} >> + >> +static void xiic_fill_tx_fifo(struct xilinx_xiic_priv *priv, >> + struct i2c_msg *msg, int nmsgs) >> +{ >> + u8 fifo_space = xiic_tx_fifo_space(priv); >> + int len = msg->len; >> + u32 pos = 0; >> + >> + len = (len > fifo_space) ? fifo_space : len; >> + >> + while (len--) { >> + u16 data = msg->buf[pos++]; >> + >> + if (pos == len && nmsgs == 1) { >> + /* last message in transfer -> STOP */ >> + data |= XIIC_TX_DYN_STOP_MASK; >> + } > > No { brackets needed DTTO >> + writew(data, priv->base + XIIC_DTR_REG_OFFSET); >> + } >> +} >> + >> +static void xilinx_xiic_set_addr(struct udevice *dev, u8 addr, >> + u16 flags, u32 len, u32 nmsgs) >> +{ >> + struct xilinx_xiic_priv *priv = dev_get_priv(dev); >> + >> + xiic_irq_clr(priv, XIIC_INTR_TX_ERROR_MASK); >> + >> + if (!(flags & I2C_M_NOSTART)) { >> + /* write the address */ >> + u16 data = i2c_8bit_addr_from_flags(addr, flags) | >> + XIIC_TX_DYN_START_MASK; >> + if (nmsgs == 1 && len == 0) >> + /* no data and last message -> add STOP */ >> + data |= XIIC_TX_DYN_STOP_MASK; >> + >> + writew(data, priv->base + XIIC_DTR_REG_OFFSET); >> + } >> +} >> + >> +static int xilinx_xiic_read_common(struct udevice *dev, struct >> i2c_msg *msg, >> + u32 nmsgs) >> +{ >> + struct xilinx_xiic_priv *priv = dev_get_priv(dev); >> + u8 rx_watermark; >> + >> + /* Clear and enable Rx full interrupt. */ >> + xiic_irq_clr(priv, XIIC_INTR_RX_FULL_MASK | >> XIIC_INTR_TX_ERROR_MASK); >> + >> + /* we want to get all but last byte, because the TX_ERROR IRQ is >> used >> + * to inidicate error ACK on the address, and negative ack on the >> last >> + * received byte, so to not mix them receive all but last. >> + * In the case where there is only one byte to receive >> + * we can check if ERROR and RX full is set at the same time >> + */ >> + rx_watermark = msg->len; >> + if (rx_watermark > IIC_RX_FIFO_DEPTH) >> + rx_watermark = IIC_RX_FIFO_DEPTH; >> + >> + writeb(rx_watermark - 1, priv->base + XIIC_RFD_REG_OFFSET); >> + >> + xilinx_xiic_set_addr(dev, msg->addr, msg->flags, msg->len, nmsgs); >> + >> + xiic_irq_clr(priv, XIIC_INTR_BNB_MASK); >> + >> + writew((msg->len & 0xff) | ((nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK >> : 0), >> + priv->base + XIIC_DTR_REG_OFFSET); >> + >> + if (nmsgs == 1) >> + /* very last, enable bus not busy as well */ >> + xiic_irq_clr(priv, XIIC_INTR_BNB_MASK); >> + >> + return xiic_read_rx(priv, msg, nmsgs); >> +} >> + >> +static int xilinx_xiic_write_common(struct udevice *dev, struct >> i2c_msg *msg, >> + int nmsgs) >> +{ >> + struct xilinx_xiic_priv *priv = dev_get_priv(dev); >> + int ret; >> + >> + xilinx_xiic_set_addr(dev, msg->addr, msg->flags, msg->len, nmsgs); >> + xiic_fill_tx_fifo(priv, msg, nmsgs); >> + >> + ret = wait_for_bit_8(priv->base + XIIC_SR_REG_OFFSET, >> + XIIC_SR_TX_FIFO_EMPTY_MASK, false, 1000, true); >> + if (ret) >> + return ret; >> + >> + /* Clear any pending Tx empty, Tx Error and then enable them. */ >> + xiic_irq_clr(priv, XIIC_INTR_TX_EMPTY_MASK | >> XIIC_INTR_TX_ERROR_MASK | >> + XIIC_INTR_BNB_MASK); >> + >> + return 0; >> +} >> + >> +static void xiic_clear_rx_fifo(struct xilinx_xiic_priv *priv) >> +{ >> + u8 sr; >> + >> + for (sr = readb(priv->base + XIIC_SR_REG_OFFSET); >> + !(sr & XIIC_SR_RX_FIFO_EMPTY_MASK); >> + sr = readb(priv->base + XIIC_SR_REG_OFFSET)) >> + readb(priv->base + XIIC_DRR_REG_OFFSET); >> +} >> + >> +static void xiic_reinit(struct xilinx_xiic_priv *priv) >> +{ >> + writel(XIIC_RESET_MASK, priv->base + XIIC_RESETR_OFFSET); >> + >> + /* Set receive Fifo depth to maximum (zero based). */ >> + writeb(IIC_RX_FIFO_DEPTH - 1, priv->base + XIIC_RFD_REG_OFFSET); >> + >> + /* Reset Tx Fifo. */ >> + writeb(XIIC_CR_TX_FIFO_RESET_MASK, priv->base + XIIC_CR_REG_OFFSET); >> + >> + /* Enable IIC Device, remove Tx Fifo reset & disable general >> call. */ >> + writeb(XIIC_CR_ENABLE_DEVICE_MASK, priv->base + XIIC_CR_REG_OFFSET); >> + >> + /* make sure RX fifo is empty */ >> + xiic_clear_rx_fifo(priv); >> + >> + /* Disable interrupts */ >> + writel(0, priv->base + XIIC_DGIER_OFFSET); >> + >> + xiic_irq_clr(priv, XIIC_INTR_ARB_LOST_MASK); >> +} >> + >> +static int xilinx_xiic_xfer(struct udevice *dev, struct i2c_msg *msg, >> int nmsgs) >> +{ >> + int ret; >> + >> + for (; nmsgs > 0; nmsgs--, msg++) { >> + if (msg->flags & I2C_M_RD) >> + ret = xilinx_xiic_read_common(dev, msg, nmsgs); >> + else >> + ret = xilinx_xiic_write_common(dev, msg, nmsgs); >> + >> + if (ret) >> + return -EREMOTEIO; >> + } >> + >> + return ret; > > Hmm... maybe theoretical, but if nmsgs == 0, return value is not defined. Can that actually happen ? I don't think so ; if nmsgs == 0, this code won't even be called from the i2c core code. >> +} >> + >> +static int xilinx_xiic_probe_chip(struct udevice *dev, uint addr, >> uint flags) >> +{ >> + struct xilinx_xiic_priv *priv = dev_get_priv(dev); >> + u32 reg; >> + int ret; >> + >> + xiic_reinit(priv); >> + >> + xilinx_xiic_set_addr(dev, addr, 0, 0, 1); >> + ret = wait_for_bit_8(priv->base + XIIC_SR_REG_OFFSET, >> + XIIC_SR_BUS_BUSY_MASK, false, 1000, true); >> + if (ret) >> + return ret; >> + >> + reg = readl(priv->base + XIIC_IISR_OFFSET); >> + if (reg & XIIC_INTR_TX_ERROR_MASK) >> + return -ENODEV; >> + >> + return 0; >> +} >> + >> +static int xilinx_xiic_set_speed(struct udevice *dev, uint speed) >> +{ >> + return 0; >> +} >> + >> +static int xilinx_xiic_probe(struct udevice *dev) >> +{ >> + struct xilinx_xiic_priv *priv = dev_get_priv(dev); >> + >> + priv->base = dev_read_addr_ptr(dev); >> + >> + writel(XIIC_CR_TX_FIFO_RESET_MASK, priv->base + XIIC_CR_REG_OFFSET); >> + xiic_reinit(priv); >> + >> + return 0; >> +} >> + >> +static const struct dm_i2c_ops xilinx_xiic_ops = { >> + .xfer = xilinx_xiic_xfer, >> + .probe_chip = xilinx_xiic_probe_chip, >> + .set_bus_speed = xilinx_xiic_set_speed, >> +}; >> + >> +static const struct udevice_id xilinx_xiic_ids[] = { >> + { .compatible = "xlnx,xps-iic-2.00.a" }, >> + { } >> +}; >> + >> +U_BOOT_DRIVER(xilinx_xiic) = { >> + .name = "xilinx_axi_i2c", >> + .id = UCLASS_I2C, >> + .of_match = xilinx_xiic_ids, >> + .probe = xilinx_xiic_probe, >> + .priv_auto_alloc_size = sizeof(struct xilinx_xiic_priv), >> + .ops = &xilinx_xiic_ops, >> +}; >> > > Beside of the nitpicks. > > Reviewed-by: Heiko Schocher <h...@denx.de> > > bye, > Heiko -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
