From: Thor Thayer <[email protected]>
This patch series adds the Altera I2C Controller driver for use in FPGAs.
This version includes cleanup and a bug fix noticed by the Linux
community.
Thor Thayer (3):
MAINTAINERS: Add Altera I2C Controller Driver
dt-bindings: i2c: Add Altera I2C Controller
i2c: altera: Add Altera I2C Controller driver
.../devicetree/bindings/i2c/i2c-altera.txt | 39 ++
MAINTAINERS | 5 +
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-altera.c | 510 +++++++++++++++++++++
5 files changed, 565 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-altera.txt
create mode 100644 drivers/i2c/busses/i2c-altera.c
--
2.7.4
From: Thor Thayer <[email protected]>
Add the documentation to support the Altera synthesizable
logic I2C Controller in FPGA.
Signed-off-by: Thor Thayer <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
v2 Change the subject to identify dt-bindings
Add synthesizable logic to description.
Change compatible string to "altr,softip-i2c"
Make description of clock phandle singular since 1 clock.
Remove altr, from fifo-size
Specify fifo-size is in bytes.
v3 Add version to compatible string "altr,softip-i2c-v1.0"
Add for use in FPGA in description.
v4 No Change - Add Acked-by
v5 No Change
---
.../devicetree/bindings/i2c/i2c-altera.txt | 39 ++++++++++++++++++++++
1 file changed, 39 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-altera.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-altera.txt b/Documentation/devicetree/bindings/i2c/i2c-altera.txt
new file mode 100644
index 0000000..767664f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-altera.txt
@@ -0,0 +1,39 @@
+* Altera I2C Controller
+* This is Altera's synthesizable logic block I2C Controller for use
+* in Altera's FPGAs.
+
+Required properties :
+ - compatible : should be "altr,softip-i2c-v1.0"
+ - reg : Offset and length of the register set for the device
+ - interrupts : <IRQ> where IRQ is the interrupt number.
+ - clocks : phandle to input clock.
+ - #address-cells = <1>;
+ - #size-cells = <0>;
+
+Recommended properties :
+ - clock-frequency : desired I2C bus clock frequency in Hz.
+
+Optional properties :
+ - fifo-size : Size of the RX and TX FIFOs in bytes.
+ - Child nodes conforming to i2c bus binding
+
+Example :
+
+ i2c@100080000 {
+ compatible = "altr,softip-i2c-v1.0";
+ reg = <0x00000001 0x00080000 0x00000040>;
+ interrupt-parent = <&intc>;
+ interrupts = <0 43 4>;
+ clocks = <&clk_0>;
+ clock-frequency = <100000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ fifo-size = <4>;
+
+ eeprom@51 {
+ compatible = "atmel,24c32";
+ reg = <0x51>;
+ pagesize = <32>;
+ };
+ };
+
--
2.7.4
From: Thor Thayer <[email protected]>
Add driver support for the Altera I2C Controller. The I2C
controller is soft IP for use in FPGAs.
Signed-off-by: Thor Thayer <[email protected]>
---
v2 Remove altr, from fifo-size to agree with bindings.
Change compatible string to "altr,softip-i2c"
v3 Add version to compatible string "altr,softip-i2c-v1.0"
v4 Add default 100kHz message. Cleanup IRQ error message.
v5 Cleanup header usage - remove init.h.
Fix 1 byte RX bug and add convenience function for transfer shared by RX and TX.
Use readl_poll_timeout_atomic() function.
Cache the I2C Interrupt Status Enable Register.
Cleanup of irq_mask by combining into 1 function.
Use i2c_8bit_addr_from_msg() for cleaner xfer_msg implementation.
Restructure altr_i2c_xfer() without arrays and indexes.
Remove redundant assignments.
Remove function calls from inside if() statements.
Convert of_property_read_u32() to device_property_read_u32().
Call i2c_m_read() once in irq.
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-altera.c | 510 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 521 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-altera.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 1006b23..668147bf 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -328,6 +328,16 @@ config I2C_POWERMAC
comment "I2C system bus drivers (mostly embedded / system-on-chip)"
+config I2C_ALTERA
+ tristate "Altera Soft IP I2C"
+ depends on (ARCH_SOCFPGA || NIOS2) && OF
+ help
+ If you say yes to this option, support will be included for the
+ Altera Soft IP I2C interfaces on SoCFPGA and Nios2 architectures.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-altera.
+
config I2C_ASPEED
tristate "Aspeed I2C Controller"
depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 1b2fc81..1395dcb 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
# Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_ALTERA) += i2c-altera.o
obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
obj-$(CONFIG_I2C_AT91) += i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
diff --git a/drivers/i2c/busses/i2c-altera.c b/drivers/i2c/busses/i2c-altera.c
new file mode 100644
index 0000000..e818901
--- /dev/null
+++ b/drivers/i2c/busses/i2c-altera.c
@@ -0,0 +1,510 @@
+/*
+ * Copyright Intel Corporation (C) 2017.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Based on the i2c-axxia.c driver.
+ */
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+
+#define ALTR_I2C_TFR_CMD 0x00 /* Transfer Command register */
+#define ALTR_I2C_TFR_CMD_STA BIT(9) /* send START before byte */
+#define ALTR_I2C_TFR_CMD_STO BIT(8) /* send STOP after byte */
+#define ALTR_I2C_TFR_CMD_RW_D BIT(0) /* Direction of transfer */
+#define ALTR_I2C_RX_DATA 0x04 /* RX data FIFO register */
+#define ALTR_I2C_CTRL 0x08 /* Control register */
+#define ALTR_I2C_CTRL_RXT_SHFT 4 /* RX FIFO Threshold */
+#define ALTR_I2C_CTRL_TCT_SHFT 2 /* TFER CMD FIFO Threshold */
+#define ALTR_I2C_CTRL_BSPEED BIT(1) /* Bus Speed (1=Fast) */
+#define ALTR_I2C_CTRL_EN BIT(0) /* Enable Core (1=Enable) */
+#define ALTR_I2C_ISER 0x0C /* Interrupt Status Enable register */
+#define ALTR_I2C_ISER_RXOF_EN BIT(4) /* Enable RX OVERFLOW IRQ */
+#define ALTR_I2C_ISER_ARB_EN BIT(3) /* Enable ARB LOST IRQ */
+#define ALTR_I2C_ISER_NACK_EN BIT(2) /* Enable NACK DET IRQ */
+#define ALTR_I2C_ISER_RXRDY_EN BIT(1) /* Enable RX Ready IRQ */
+#define ALTR_I2C_ISER_TXRDY_EN BIT(0) /* Enable TX Ready IRQ */
+#define ALTR_I2C_ISR 0x10 /* Interrupt Status register */
+#define ALTR_I2C_ISR_RXOF BIT(4) /* RX OVERFLOW IRQ */
+#define ALTR_I2C_ISR_ARB BIT(3) /* ARB LOST IRQ */
+#define ALTR_I2C_ISR_NACK BIT(2) /* NACK DET IRQ */
+#define ALTR_I2C_ISR_RXRDY BIT(1) /* RX Ready IRQ */
+#define ALTR_I2C_ISR_TXRDY BIT(0) /* TX Ready IRQ */
+#define ALTR_I2C_STATUS 0x14 /* Status register */
+#define ALTR_I2C_STAT_CORE BIT(0) /* Core Status (0=idle) */
+#define ALTR_I2C_TC_FIFO_LVL 0x18 /* Transfer FIFO LVL register */
+#define ALTR_I2C_RX_FIFO_LVL 0x1C /* Receive FIFO LVL register */
+#define ALTR_I2C_SCL_LOW 0x20 /* SCL low count register */
+#define ALTR_I2C_SCL_HIGH 0x24 /* SCL high count register */
+#define ALTR_I2C_SDA_HOLD 0x28 /* SDA hold count register */
+
+#define ALTR_I2C_ALL_IRQ (ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | \
+ ALTR_I2C_ISR_NACK | ALTR_I2C_ISR_RXRDY | \
+ ALTR_I2C_ISR_TXRDY)
+
+#define ALTR_I2C_THRESHOLD 0 /* IRQ Threshold at 1 element */
+#define ALTR_I2C_DFLT_FIFO_SZ 4
+#define ALTR_I2C_TIMEOUT 100000 /* 100ms */
+#define ALTR_I2C_XFER_TIMEOUT (msecs_to_jiffies(250))
+
+/**
+ * altr_i2c_dev - I2C device context
+ * @base: pointer to register struct
+ * @msg: pointer to current message
+ * @msg_len: number of bytes transferred in msg
+ * @msg_err: error code for completed message
+ * @msg_complete: xfer completion object
+ * @dev: device reference
+ * @adapter: core i2c abstraction
+ * @i2c_clk: clock reference for i2c input clock
+ * @bus_clk_rate: current i2c bus clock rate
+ * @buf: ptr to msg buffer for easier use.
+ * @fifo_size: size of the FIFO passed in.
+ */
+struct altr_i2c_dev {
+ void __iomem *base;
+ struct i2c_msg *msg;
+ size_t msg_len;
+ int msg_err;
+ struct completion msg_complete;
+ struct device *dev;
+ struct i2c_adapter adapter;
+ struct clk *i2c_clk;
+ u32 bus_clk_rate;
+ u8 *buf;
+ u32 fifo_size;
+ u32 isr_mask;
+};
+
+static void i2c_int_enable(struct altr_i2c_dev *idev, u32 mask, bool enable)
+{
+ u32 int_en = readl(idev->base + ALTR_I2C_ISER);
+
+ if (enable)
+ idev->isr_mask = int_en | mask;
+ else
+ idev->isr_mask = int_en & ~mask;
+
+ writel(idev->isr_mask, idev->base + ALTR_I2C_ISER);
+}
+
+static void i2c_int_clear(struct altr_i2c_dev *idev, u32 mask)
+{
+ u32 int_en = readl(idev->base + ALTR_I2C_ISR);
+
+ writel(int_en | mask, idev->base + ALTR_I2C_ISR);
+}
+
+static void altr_i2c_core_disable(struct altr_i2c_dev *idev)
+{
+ u32 tmp = readl(idev->base + ALTR_I2C_CTRL);
+
+ writel(tmp & ~ALTR_I2C_CTRL_EN, idev->base + ALTR_I2C_CTRL);
+}
+
+static void altr_i2c_core_enable(struct altr_i2c_dev *idev)
+{
+ u32 tmp = readl(idev->base + ALTR_I2C_CTRL);
+
+ writel(tmp | ALTR_I2C_CTRL_EN, idev->base + ALTR_I2C_CTRL);
+}
+
+static void altr_i2c_reset(struct altr_i2c_dev *idev)
+{
+ altr_i2c_core_disable(idev);
+ altr_i2c_core_enable(idev);
+}
+
+static void altr_i2c_recover(struct altr_i2c_dev *idev)
+{
+ altr_i2c_reset(idev);
+ /* Clock start bit + 8 bits + stop bit out */
+ writel(ALTR_I2C_TFR_CMD_STA | ALTR_I2C_TFR_CMD_STO,
+ idev->base + ALTR_I2C_TFR_CMD);
+ altr_i2c_reset(idev);
+
+ i2c_recover_bus(&idev->adapter);
+}
+
+static inline void altr_i2c_stop(struct altr_i2c_dev *idev)
+{
+ writel(ALTR_I2C_TFR_CMD_STO, idev->base + ALTR_I2C_TFR_CMD);
+}
+
+static void altr_i2c_init(struct altr_i2c_dev *idev)
+{
+ u32 divisor = clk_get_rate(idev->i2c_clk) / idev->bus_clk_rate;
+ u32 clk_mhz = clk_get_rate(idev->i2c_clk) / 1000000;
+ u32 tmp = (ALTR_I2C_THRESHOLD << ALTR_I2C_CTRL_RXT_SHFT) |
+ (ALTR_I2C_THRESHOLD << ALTR_I2C_CTRL_TCT_SHFT);
+ u32 t_high, t_low;
+
+ if (idev->bus_clk_rate <= 100000) {
+ tmp &= ~ALTR_I2C_CTRL_BSPEED;
+ /* Standard mode SCL 50/50 */
+ t_high = divisor * 1 / 2;
+ t_low = divisor * 1 / 2;
+ } else {
+ tmp |= ALTR_I2C_CTRL_BSPEED;
+ /* Fast mode SCL 33/66 */
+ t_high = divisor * 1 / 3;
+ t_low = divisor * 2 / 3;
+ }
+ writel(tmp, idev->base + ALTR_I2C_CTRL);
+
+ dev_dbg(idev->dev, "rate=%uHz per_clk=%uMHz -> ratio=1:%u\n",
+ idev->bus_clk_rate, clk_mhz, divisor);
+
+ /* Reset controller */
+ altr_i2c_reset(idev);
+
+ /* SCL High Time */
+ writel(t_high, idev->base + ALTR_I2C_SCL_HIGH);
+ /* SCL Low Time */
+ writel(t_low, idev->base + ALTR_I2C_SCL_LOW);
+ /* SDA Hold Time, 300ns */
+ writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD);
+
+ /* Mask all master interrupt bits */
+ i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
+}
+
+static int i2c_m_rd(const struct i2c_msg *msg)
+{
+ return (msg->flags & I2C_M_RD) != 0;
+}
+
+/**
+ * altr_i2c_transfer - On the last byte to be transmitted, send
+ * a Stop bit on the last byte.
+ */
+static void altr_i2c_transfer(struct altr_i2c_dev *idev, u32 data)
+{
+ /* On the last byte to be transmitted, send STOP */
+ if (idev->msg_len == 1)
+ data |= ALTR_I2C_TFR_CMD_STO;
+ if (idev->msg_len > 0)
+ writel(data, idev->base + ALTR_I2C_TFR_CMD);
+}
+
+/**
+ * altr_i2c_empty_rx_fifo - Fetch data from RX FIFO until end of
+ * transfer. Send a Stop bit on the last byte.
+ */
+static void altr_i2c_empty_rx_fifo(struct altr_i2c_dev *idev)
+{
+ size_t rx_fifo_avail = readl(idev->base + ALTR_I2C_RX_FIFO_LVL);
+ int bytes_to_transfer = min(rx_fifo_avail, idev->msg_len);
+
+ while (bytes_to_transfer-- > 0) {
+ *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
+ idev->msg_len--;
+ altr_i2c_transfer(idev, 0);
+ }
+}
+
+/**
+ * altr_i2c_fill_tx_fifo - Fill TX FIFO from current message buffer.
+ * @return: Number of bytes left to transfer.
+ */
+static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
+{
+ size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
+ ALTR_I2C_TC_FIFO_LVL);
+ int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
+ int ret = idev->msg_len - bytes_to_transfer;
+
+ while (bytes_to_transfer-- > 0) {
+ altr_i2c_transfer(idev, *idev->buf++);
+ idev->msg_len--;
+ }
+
+ return ret;
+}
+
+static irqreturn_t altr_i2c_isr(int irq, void *_dev)
+{
+ int ret, read;
+ struct altr_i2c_dev *idev = _dev;
+ /* Read IRQ status but only interested in Enabled IRQs. */
+ u32 status = readl(idev->base + ALTR_I2C_ISR) & idev->isr_mask;
+
+ if (!idev->msg) {
+ dev_warn(idev->dev, "unexpected interrupt\n");
+ goto out;
+ }
+ read = i2c_m_rd(idev->msg);
+
+ /* handle Lost Arbitration */
+ if (unlikely(status & ALTR_I2C_ISR_ARB)) {
+ dev_err(idev->dev, "%s: arbitration lost\n", __func__);
+ i2c_int_clear(idev, ALTR_I2C_ISR_ARB);
+ idev->msg_err = -EAGAIN;
+ goto complete;
+ }
+
+ if (unlikely(status & ALTR_I2C_ISR_NACK)) {
+ dev_dbg(idev->dev, "%s: could not get ACK\n", __func__);
+ idev->msg_err = -ENXIO;
+ i2c_int_clear(idev, ALTR_I2C_ISR_NACK);
+ altr_i2c_stop(idev);
+ goto complete;
+ }
+
+ /* handle RX FIFO Overflow */
+ if (read && unlikely(status & ALTR_I2C_ISR_RXOF)) {
+ altr_i2c_empty_rx_fifo(idev);
+ i2c_int_clear(idev, ALTR_I2C_ISR_RXRDY);
+ altr_i2c_stop(idev);
+ dev_err(idev->dev, "%s: RX FIFO Overflow\n", __func__);
+ goto complete;
+ }
+
+ /* RX FIFO needs service? */
+ if (read && (status & ALTR_I2C_ISR_RXRDY)) {
+ altr_i2c_empty_rx_fifo(idev);
+ i2c_int_clear(idev, ALTR_I2C_ISR_RXRDY);
+ if (!idev->msg_len)
+ goto complete;
+
+ goto out;
+ }
+
+ /* TX FIFO needs service? */
+ if (!read && (status & ALTR_I2C_ISR_TXRDY)) {
+ i2c_int_clear(idev, ALTR_I2C_ISR_TXRDY);
+ if (!altr_i2c_fill_tx_fifo(idev))
+ goto complete;
+
+ goto out;
+ }
+
+complete:
+ /* Wait for the Core to finish */
+ ret = readl_poll_timeout_atomic(idev->base + ALTR_I2C_STATUS, status,
+ ((status & ALTR_I2C_STAT_CORE) == 0),
+ 1, ALTR_I2C_TIMEOUT);
+ if (ret)
+ dev_err(idev->dev, "%s: message timeout\n", __func__);
+ i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
+ i2c_int_clear(idev, ALTR_I2C_ALL_IRQ);
+ complete(&idev->msg_complete);
+ dev_dbg(idev->dev, "%s: Message Complete\n", __func__);
+
+out:
+ return IRQ_HANDLED;
+}
+
+static int altr_i2c_xfer_msg(struct altr_i2c_dev *idev, struct i2c_msg *msg)
+{
+ u32 imask = ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | ALTR_I2C_ISR_NACK;
+ unsigned long time_left;
+ u32 value;
+ u8 addr = i2c_8bit_addr_from_msg(msg);
+
+ idev->msg = msg;
+ idev->msg_len = msg->len;
+ idev->buf = msg->buf;
+ idev->msg_err = 0;
+ reinit_completion(&idev->msg_complete);
+ altr_i2c_core_enable(idev);
+
+ /* Make sure RX FIFO is empty */
+ do {
+ readl(idev->base + ALTR_I2C_RX_DATA);
+ } while (readl(idev->base + ALTR_I2C_RX_FIFO_LVL));
+
+ writel(ALTR_I2C_TFR_CMD_STA | addr, idev->base + ALTR_I2C_TFR_CMD);
+
+ if (i2c_m_rd(msg)) {
+ imask |= ALTR_I2C_ISER_RXOF_EN | ALTR_I2C_ISER_RXRDY_EN;
+ i2c_int_enable(idev, imask, true);
+ /* write the first byte to start the RX */
+ altr_i2c_transfer(idev, 0);
+ } else {
+ imask |= ALTR_I2C_ISR_TXRDY;
+ i2c_int_enable(idev, imask, true);
+ altr_i2c_fill_tx_fifo(idev);
+ }
+
+ time_left = wait_for_completion_timeout(&idev->msg_complete,
+ ALTR_I2C_XFER_TIMEOUT);
+ i2c_int_enable(idev, imask, false);
+
+ value = readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE;
+ if (value)
+ dev_err(idev->dev, "%s: Core Status not IDLE...\n", __func__);
+
+ if (time_left == 0) {
+ idev->msg_err = -ETIMEDOUT;
+ dev_err(idev->dev, "%s: Transaction timed out.\n", __func__);
+ altr_i2c_recover(idev);
+ }
+
+ if (unlikely(idev->msg_err) && idev->msg_err != -ENXIO)
+ altr_i2c_init(idev);
+
+ altr_i2c_core_disable(idev);
+
+ return idev->msg_err;
+}
+
+static int
+altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
+ int i, ret;
+
+ for (i = 0; i < num; i++) {
+ ret = altr_i2c_xfer_msg(idev, msgs++);
+ if (ret)
+ return ret;
+ }
+ return num;
+}
+
+static u32 altr_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C;
+}
+
+static const struct i2c_algorithm altr_i2c_algo = {
+ .master_xfer = altr_i2c_xfer,
+ .functionality = altr_i2c_func,
+};
+
+static int altr_i2c_probe(struct platform_device *pdev)
+{
+ struct altr_i2c_dev *idev = NULL;
+ struct resource *res;
+ int irq, ret;
+ u32 val;
+
+ idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
+ if (!idev)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ idev->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(idev->base))
+ return PTR_ERR(idev->base);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "missing interrupt resource\n");
+ return irq;
+ }
+
+ idev->i2c_clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(idev->i2c_clk)) {
+ dev_err(&pdev->dev, "missing clock\n");
+ return PTR_ERR(idev->i2c_clk);
+ }
+
+ idev->dev = &pdev->dev;
+ init_completion(&idev->msg_complete);
+
+ val = device_property_read_u32(idev->dev, "fifo-size",
+ &idev->fifo_size);
+ if (val) {
+ dev_err(&pdev->dev, "FIFO size set to default of %d\n",
+ ALTR_I2C_DFLT_FIFO_SZ);
+ idev->fifo_size = ALTR_I2C_DFLT_FIFO_SZ;
+ }
+
+ val = device_property_read_u32(idev->dev, "clock-frequency",
+ &idev->bus_clk_rate);
+ if (val) {
+ dev_err(&pdev->dev, "Default to 100kHz\n");
+ idev->bus_clk_rate = 100000; /* default clock rate */
+ }
+
+ if (idev->bus_clk_rate > 400000) {
+ dev_err(&pdev->dev, "invalid clock-frequency %d\n",
+ idev->bus_clk_rate);
+ return -EINVAL;
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq, altr_i2c_isr, 0,
+ pdev->name, idev);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to claim IRQ %d\n", irq);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(idev->i2c_clk);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to enable clock\n");
+ return ret;
+ }
+
+ altr_i2c_init(idev);
+
+ i2c_set_adapdata(&idev->adapter, idev);
+ strlcpy(idev->adapter.name, pdev->name, sizeof(idev->adapter.name));
+ idev->adapter.owner = THIS_MODULE;
+ idev->adapter.algo = &altr_i2c_algo;
+ idev->adapter.dev.parent = &pdev->dev;
+ idev->adapter.dev.of_node = pdev->dev.of_node;
+
+ platform_set_drvdata(pdev, idev);
+
+ ret = i2c_add_adapter(&idev->adapter);
+ if (ret) {
+ clk_disable_unprepare(idev->i2c_clk);
+ return ret;
+ }
+ dev_info(&pdev->dev, "Altera SoftIP I2C Probe Complete\n");
+
+ return 0;
+}
+
+static int altr_i2c_remove(struct platform_device *pdev)
+{
+ struct altr_i2c_dev *idev = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(idev->i2c_clk);
+ i2c_del_adapter(&idev->adapter);
+
+ return 0;
+}
+
+/* Match table for of_platform binding */
+static const struct of_device_id altr_i2c_of_match[] = {
+ { .compatible = "altr,softip-i2c-v1.0" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altr_i2c_of_match);
+
+static struct platform_driver altr_i2c_driver = {
+ .probe = altr_i2c_probe,
+ .remove = altr_i2c_remove,
+ .driver = {
+ .name = "altera-i2c",
+ .of_match_table = altr_i2c_of_match,
+ },
+};
+
+module_platform_driver(altr_i2c_driver);
+
+MODULE_DESCRIPTION("Altera Soft IP I2C bus driver");
+MODULE_AUTHOR("Thor Thayer <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.7.4
From: Thor Thayer <[email protected]>
Add maintainer for the Altera I2C Controller Driver.
Signed-off-by: Thor Thayer <[email protected]>
---
v2-5 No change
---
MAINTAINERS | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 6f08ce7..5ee85d8a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -636,6 +636,11 @@ ALPS PS/2 TOUCHPAD DRIVER
R: Pali Rohár <[email protected]>
F: drivers/input/mouse/alps.*
+ALTERA I2C CONTROLLER DRIVER
+M: Thor Thayer <[email protected]>
+S: Maintained
+F: drivers/i2c/busses/i2c-altera.c
+
ALTERA MAILBOX DRIVER
M: Ley Foon Tan <[email protected]>
L: [email protected] (moderated for non-subscribers)
--
2.7.4
On Mon, Jul 17, 2017 at 7:35 PM, <[email protected]> wrote:
> From: Thor Thayer <[email protected]>
>
> Add driver support for the Altera I2C Controller. The I2C
> controller is soft IP for use in FPGAs.
>
So, there are few more things that can be improved, like removing
__func__ parameter from dev_printk() calls or _isr() refactoring,
though I leave it to be judged by Wolfram and others.
To avoid any further bikeshedding it looks good enough to me, FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Thor Thayer <[email protected]>
> ---
> v2 Remove altr, from fifo-size to agree with bindings.
> Change compatible string to "altr,softip-i2c"
> v3 Add version to compatible string "altr,softip-i2c-v1.0"
> v4 Add default 100kHz message. Cleanup IRQ error message.
> v5 Cleanup header usage - remove init.h.
> Fix 1 byte RX bug and add convenience function for transfer shared by RX and TX.
> Use readl_poll_timeout_atomic() function.
> Cache the I2C Interrupt Status Enable Register.
> Cleanup of irq_mask by combining into 1 function.
> Use i2c_8bit_addr_from_msg() for cleaner xfer_msg implementation.
> Restructure altr_i2c_xfer() without arrays and indexes.
> Remove redundant assignments.
> Remove function calls from inside if() statements.
> Convert of_property_read_u32() to device_property_read_u32().
> Call i2c_m_read() once in irq.
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-altera.c | 510 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 521 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-altera.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 1006b23..668147bf 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -328,6 +328,16 @@ config I2C_POWERMAC
>
> comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>
> +config I2C_ALTERA
> + tristate "Altera Soft IP I2C"
> + depends on (ARCH_SOCFPGA || NIOS2) && OF
> + help
> + If you say yes to this option, support will be included for the
> + Altera Soft IP I2C interfaces on SoCFPGA and Nios2 architectures.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-altera.
> +
> config I2C_ASPEED
> tristate "Aspeed I2C Controller"
> depends on ARCH_ASPEED || COMPILE_TEST
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1b2fc81..1395dcb 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
> obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
>
> # Embedded system I2C/SMBus host controller drivers
> +obj-$(CONFIG_I2C_ALTERA) += i2c-altera.o
> obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
> obj-$(CONFIG_I2C_AT91) += i2c-at91.o
> obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
> diff --git a/drivers/i2c/busses/i2c-altera.c b/drivers/i2c/busses/i2c-altera.c
> new file mode 100644
> index 0000000..e818901
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-altera.c
> @@ -0,0 +1,510 @@
> +/*
> + * Copyright Intel Corporation (C) 2017.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Based on the i2c-axxia.c driver.
> + */
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +
> +#define ALTR_I2C_TFR_CMD 0x00 /* Transfer Command register */
> +#define ALTR_I2C_TFR_CMD_STA BIT(9) /* send START before byte */
> +#define ALTR_I2C_TFR_CMD_STO BIT(8) /* send STOP after byte */
> +#define ALTR_I2C_TFR_CMD_RW_D BIT(0) /* Direction of transfer */
> +#define ALTR_I2C_RX_DATA 0x04 /* RX data FIFO register */
> +#define ALTR_I2C_CTRL 0x08 /* Control register */
> +#define ALTR_I2C_CTRL_RXT_SHFT 4 /* RX FIFO Threshold */
> +#define ALTR_I2C_CTRL_TCT_SHFT 2 /* TFER CMD FIFO Threshold */
> +#define ALTR_I2C_CTRL_BSPEED BIT(1) /* Bus Speed (1=Fast) */
> +#define ALTR_I2C_CTRL_EN BIT(0) /* Enable Core (1=Enable) */
> +#define ALTR_I2C_ISER 0x0C /* Interrupt Status Enable register */
> +#define ALTR_I2C_ISER_RXOF_EN BIT(4) /* Enable RX OVERFLOW IRQ */
> +#define ALTR_I2C_ISER_ARB_EN BIT(3) /* Enable ARB LOST IRQ */
> +#define ALTR_I2C_ISER_NACK_EN BIT(2) /* Enable NACK DET IRQ */
> +#define ALTR_I2C_ISER_RXRDY_EN BIT(1) /* Enable RX Ready IRQ */
> +#define ALTR_I2C_ISER_TXRDY_EN BIT(0) /* Enable TX Ready IRQ */
> +#define ALTR_I2C_ISR 0x10 /* Interrupt Status register */
> +#define ALTR_I2C_ISR_RXOF BIT(4) /* RX OVERFLOW IRQ */
> +#define ALTR_I2C_ISR_ARB BIT(3) /* ARB LOST IRQ */
> +#define ALTR_I2C_ISR_NACK BIT(2) /* NACK DET IRQ */
> +#define ALTR_I2C_ISR_RXRDY BIT(1) /* RX Ready IRQ */
> +#define ALTR_I2C_ISR_TXRDY BIT(0) /* TX Ready IRQ */
> +#define ALTR_I2C_STATUS 0x14 /* Status register */
> +#define ALTR_I2C_STAT_CORE BIT(0) /* Core Status (0=idle) */
> +#define ALTR_I2C_TC_FIFO_LVL 0x18 /* Transfer FIFO LVL register */
> +#define ALTR_I2C_RX_FIFO_LVL 0x1C /* Receive FIFO LVL register */
> +#define ALTR_I2C_SCL_LOW 0x20 /* SCL low count register */
> +#define ALTR_I2C_SCL_HIGH 0x24 /* SCL high count register */
> +#define ALTR_I2C_SDA_HOLD 0x28 /* SDA hold count register */
> +
> +#define ALTR_I2C_ALL_IRQ (ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | \
> + ALTR_I2C_ISR_NACK | ALTR_I2C_ISR_RXRDY | \
> + ALTR_I2C_ISR_TXRDY)
> +
> +#define ALTR_I2C_THRESHOLD 0 /* IRQ Threshold at 1 element */
> +#define ALTR_I2C_DFLT_FIFO_SZ 4
> +#define ALTR_I2C_TIMEOUT 100000 /* 100ms */
> +#define ALTR_I2C_XFER_TIMEOUT (msecs_to_jiffies(250))
> +
> +/**
> + * altr_i2c_dev - I2C device context
> + * @base: pointer to register struct
> + * @msg: pointer to current message
> + * @msg_len: number of bytes transferred in msg
> + * @msg_err: error code for completed message
> + * @msg_complete: xfer completion object
> + * @dev: device reference
> + * @adapter: core i2c abstraction
> + * @i2c_clk: clock reference for i2c input clock
> + * @bus_clk_rate: current i2c bus clock rate
> + * @buf: ptr to msg buffer for easier use.
> + * @fifo_size: size of the FIFO passed in.
> + */
> +struct altr_i2c_dev {
> + void __iomem *base;
> + struct i2c_msg *msg;
> + size_t msg_len;
> + int msg_err;
> + struct completion msg_complete;
> + struct device *dev;
> + struct i2c_adapter adapter;
> + struct clk *i2c_clk;
> + u32 bus_clk_rate;
> + u8 *buf;
> + u32 fifo_size;
> + u32 isr_mask;
> +};
> +
> +static void i2c_int_enable(struct altr_i2c_dev *idev, u32 mask, bool enable)
> +{
> + u32 int_en = readl(idev->base + ALTR_I2C_ISER);
> +
> + if (enable)
> + idev->isr_mask = int_en | mask;
> + else
> + idev->isr_mask = int_en & ~mask;
> +
> + writel(idev->isr_mask, idev->base + ALTR_I2C_ISER);
> +}
> +
> +static void i2c_int_clear(struct altr_i2c_dev *idev, u32 mask)
> +{
> + u32 int_en = readl(idev->base + ALTR_I2C_ISR);
> +
> + writel(int_en | mask, idev->base + ALTR_I2C_ISR);
> +}
> +
> +static void altr_i2c_core_disable(struct altr_i2c_dev *idev)
> +{
> + u32 tmp = readl(idev->base + ALTR_I2C_CTRL);
> +
> + writel(tmp & ~ALTR_I2C_CTRL_EN, idev->base + ALTR_I2C_CTRL);
> +}
> +
> +static void altr_i2c_core_enable(struct altr_i2c_dev *idev)
> +{
> + u32 tmp = readl(idev->base + ALTR_I2C_CTRL);
> +
> + writel(tmp | ALTR_I2C_CTRL_EN, idev->base + ALTR_I2C_CTRL);
> +}
> +
> +static void altr_i2c_reset(struct altr_i2c_dev *idev)
> +{
> + altr_i2c_core_disable(idev);
> + altr_i2c_core_enable(idev);
> +}
> +
> +static void altr_i2c_recover(struct altr_i2c_dev *idev)
> +{
> + altr_i2c_reset(idev);
> + /* Clock start bit + 8 bits + stop bit out */
> + writel(ALTR_I2C_TFR_CMD_STA | ALTR_I2C_TFR_CMD_STO,
> + idev->base + ALTR_I2C_TFR_CMD);
> + altr_i2c_reset(idev);
> +
> + i2c_recover_bus(&idev->adapter);
> +}
> +
> +static inline void altr_i2c_stop(struct altr_i2c_dev *idev)
> +{
> + writel(ALTR_I2C_TFR_CMD_STO, idev->base + ALTR_I2C_TFR_CMD);
> +}
> +
> +static void altr_i2c_init(struct altr_i2c_dev *idev)
> +{
> + u32 divisor = clk_get_rate(idev->i2c_clk) / idev->bus_clk_rate;
> + u32 clk_mhz = clk_get_rate(idev->i2c_clk) / 1000000;
> + u32 tmp = (ALTR_I2C_THRESHOLD << ALTR_I2C_CTRL_RXT_SHFT) |
> + (ALTR_I2C_THRESHOLD << ALTR_I2C_CTRL_TCT_SHFT);
> + u32 t_high, t_low;
> +
> + if (idev->bus_clk_rate <= 100000) {
> + tmp &= ~ALTR_I2C_CTRL_BSPEED;
> + /* Standard mode SCL 50/50 */
> + t_high = divisor * 1 / 2;
> + t_low = divisor * 1 / 2;
> + } else {
> + tmp |= ALTR_I2C_CTRL_BSPEED;
> + /* Fast mode SCL 33/66 */
> + t_high = divisor * 1 / 3;
> + t_low = divisor * 2 / 3;
> + }
> + writel(tmp, idev->base + ALTR_I2C_CTRL);
> +
> + dev_dbg(idev->dev, "rate=%uHz per_clk=%uMHz -> ratio=1:%u\n",
> + idev->bus_clk_rate, clk_mhz, divisor);
> +
> + /* Reset controller */
> + altr_i2c_reset(idev);
> +
> + /* SCL High Time */
> + writel(t_high, idev->base + ALTR_I2C_SCL_HIGH);
> + /* SCL Low Time */
> + writel(t_low, idev->base + ALTR_I2C_SCL_LOW);
> + /* SDA Hold Time, 300ns */
> + writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD);
> +
> + /* Mask all master interrupt bits */
> + i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
> +}
> +
> +static int i2c_m_rd(const struct i2c_msg *msg)
> +{
> + return (msg->flags & I2C_M_RD) != 0;
> +}
> +
> +/**
> + * altr_i2c_transfer - On the last byte to be transmitted, send
> + * a Stop bit on the last byte.
> + */
> +static void altr_i2c_transfer(struct altr_i2c_dev *idev, u32 data)
> +{
> + /* On the last byte to be transmitted, send STOP */
> + if (idev->msg_len == 1)
> + data |= ALTR_I2C_TFR_CMD_STO;
> + if (idev->msg_len > 0)
> + writel(data, idev->base + ALTR_I2C_TFR_CMD);
> +}
> +
> +/**
> + * altr_i2c_empty_rx_fifo - Fetch data from RX FIFO until end of
> + * transfer. Send a Stop bit on the last byte.
> + */
> +static void altr_i2c_empty_rx_fifo(struct altr_i2c_dev *idev)
> +{
> + size_t rx_fifo_avail = readl(idev->base + ALTR_I2C_RX_FIFO_LVL);
> + int bytes_to_transfer = min(rx_fifo_avail, idev->msg_len);
> +
> + while (bytes_to_transfer-- > 0) {
> + *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
> + idev->msg_len--;
> + altr_i2c_transfer(idev, 0);
> + }
> +}
> +
> +/**
> + * altr_i2c_fill_tx_fifo - Fill TX FIFO from current message buffer.
> + * @return: Number of bytes left to transfer.
> + */
> +static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
> +{
> + size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
> + ALTR_I2C_TC_FIFO_LVL);
> + int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
> + int ret = idev->msg_len - bytes_to_transfer;
> +
> + while (bytes_to_transfer-- > 0) {
> + altr_i2c_transfer(idev, *idev->buf++);
> + idev->msg_len--;
> + }
> +
> + return ret;
> +}
> +
> +static irqreturn_t altr_i2c_isr(int irq, void *_dev)
> +{
> + int ret, read;
> + struct altr_i2c_dev *idev = _dev;
> + /* Read IRQ status but only interested in Enabled IRQs. */
> + u32 status = readl(idev->base + ALTR_I2C_ISR) & idev->isr_mask;
> +
> + if (!idev->msg) {
> + dev_warn(idev->dev, "unexpected interrupt\n");
> + goto out;
> + }
> + read = i2c_m_rd(idev->msg);
> +
> + /* handle Lost Arbitration */
> + if (unlikely(status & ALTR_I2C_ISR_ARB)) {
> + dev_err(idev->dev, "%s: arbitration lost\n", __func__);
> + i2c_int_clear(idev, ALTR_I2C_ISR_ARB);
> + idev->msg_err = -EAGAIN;
> + goto complete;
> + }
> +
> + if (unlikely(status & ALTR_I2C_ISR_NACK)) {
> + dev_dbg(idev->dev, "%s: could not get ACK\n", __func__);
> + idev->msg_err = -ENXIO;
> + i2c_int_clear(idev, ALTR_I2C_ISR_NACK);
> + altr_i2c_stop(idev);
> + goto complete;
> + }
> +
> + /* handle RX FIFO Overflow */
> + if (read && unlikely(status & ALTR_I2C_ISR_RXOF)) {
> + altr_i2c_empty_rx_fifo(idev);
> + i2c_int_clear(idev, ALTR_I2C_ISR_RXRDY);
> + altr_i2c_stop(idev);
> + dev_err(idev->dev, "%s: RX FIFO Overflow\n", __func__);
> + goto complete;
> + }
> +
> + /* RX FIFO needs service? */
> + if (read && (status & ALTR_I2C_ISR_RXRDY)) {
> + altr_i2c_empty_rx_fifo(idev);
> + i2c_int_clear(idev, ALTR_I2C_ISR_RXRDY);
> + if (!idev->msg_len)
> + goto complete;
> +
> + goto out;
> + }
> +
> + /* TX FIFO needs service? */
> + if (!read && (status & ALTR_I2C_ISR_TXRDY)) {
> + i2c_int_clear(idev, ALTR_I2C_ISR_TXRDY);
> + if (!altr_i2c_fill_tx_fifo(idev))
> + goto complete;
> +
> + goto out;
> + }
> +
> +complete:
> + /* Wait for the Core to finish */
> + ret = readl_poll_timeout_atomic(idev->base + ALTR_I2C_STATUS, status,
> + ((status & ALTR_I2C_STAT_CORE) == 0),
> + 1, ALTR_I2C_TIMEOUT);
> + if (ret)
> + dev_err(idev->dev, "%s: message timeout\n", __func__);
> + i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
> + i2c_int_clear(idev, ALTR_I2C_ALL_IRQ);
> + complete(&idev->msg_complete);
> + dev_dbg(idev->dev, "%s: Message Complete\n", __func__);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +static int altr_i2c_xfer_msg(struct altr_i2c_dev *idev, struct i2c_msg *msg)
> +{
> + u32 imask = ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | ALTR_I2C_ISR_NACK;
> + unsigned long time_left;
> + u32 value;
> + u8 addr = i2c_8bit_addr_from_msg(msg);
> +
> + idev->msg = msg;
> + idev->msg_len = msg->len;
> + idev->buf = msg->buf;
> + idev->msg_err = 0;
> + reinit_completion(&idev->msg_complete);
> + altr_i2c_core_enable(idev);
> +
> + /* Make sure RX FIFO is empty */
> + do {
> + readl(idev->base + ALTR_I2C_RX_DATA);
> + } while (readl(idev->base + ALTR_I2C_RX_FIFO_LVL));
> +
> + writel(ALTR_I2C_TFR_CMD_STA | addr, idev->base + ALTR_I2C_TFR_CMD);
> +
> + if (i2c_m_rd(msg)) {
> + imask |= ALTR_I2C_ISER_RXOF_EN | ALTR_I2C_ISER_RXRDY_EN;
> + i2c_int_enable(idev, imask, true);
> + /* write the first byte to start the RX */
> + altr_i2c_transfer(idev, 0);
> + } else {
> + imask |= ALTR_I2C_ISR_TXRDY;
> + i2c_int_enable(idev, imask, true);
> + altr_i2c_fill_tx_fifo(idev);
> + }
> +
> + time_left = wait_for_completion_timeout(&idev->msg_complete,
> + ALTR_I2C_XFER_TIMEOUT);
> + i2c_int_enable(idev, imask, false);
> +
> + value = readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE;
> + if (value)
> + dev_err(idev->dev, "%s: Core Status not IDLE...\n", __func__);
> +
> + if (time_left == 0) {
> + idev->msg_err = -ETIMEDOUT;
> + dev_err(idev->dev, "%s: Transaction timed out.\n", __func__);
> + altr_i2c_recover(idev);
> + }
> +
> + if (unlikely(idev->msg_err) && idev->msg_err != -ENXIO)
> + altr_i2c_init(idev);
> +
> + altr_i2c_core_disable(idev);
> +
> + return idev->msg_err;
> +}
> +
> +static int
> +altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> + struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
> + int i, ret;
> +
> + for (i = 0; i < num; i++) {
> + ret = altr_i2c_xfer_msg(idev, msgs++);
> + if (ret)
> + return ret;
> + }
> + return num;
> +}
> +
> +static u32 altr_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C;
> +}
> +
> +static const struct i2c_algorithm altr_i2c_algo = {
> + .master_xfer = altr_i2c_xfer,
> + .functionality = altr_i2c_func,
> +};
> +
> +static int altr_i2c_probe(struct platform_device *pdev)
> +{
> + struct altr_i2c_dev *idev = NULL;
> + struct resource *res;
> + int irq, ret;
> + u32 val;
> +
> + idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
> + if (!idev)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + idev->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(idev->base))
> + return PTR_ERR(idev->base);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "missing interrupt resource\n");
> + return irq;
> + }
> +
> + idev->i2c_clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(idev->i2c_clk)) {
> + dev_err(&pdev->dev, "missing clock\n");
> + return PTR_ERR(idev->i2c_clk);
> + }
> +
> + idev->dev = &pdev->dev;
> + init_completion(&idev->msg_complete);
> +
> + val = device_property_read_u32(idev->dev, "fifo-size",
> + &idev->fifo_size);
> + if (val) {
> + dev_err(&pdev->dev, "FIFO size set to default of %d\n",
> + ALTR_I2C_DFLT_FIFO_SZ);
> + idev->fifo_size = ALTR_I2C_DFLT_FIFO_SZ;
> + }
> +
> + val = device_property_read_u32(idev->dev, "clock-frequency",
> + &idev->bus_clk_rate);
> + if (val) {
> + dev_err(&pdev->dev, "Default to 100kHz\n");
> + idev->bus_clk_rate = 100000; /* default clock rate */
> + }
> +
> + if (idev->bus_clk_rate > 400000) {
> + dev_err(&pdev->dev, "invalid clock-frequency %d\n",
> + idev->bus_clk_rate);
> + return -EINVAL;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, irq, altr_i2c_isr, 0,
> + pdev->name, idev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to claim IRQ %d\n", irq);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(idev->i2c_clk);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to enable clock\n");
> + return ret;
> + }
> +
> + altr_i2c_init(idev);
> +
> + i2c_set_adapdata(&idev->adapter, idev);
> + strlcpy(idev->adapter.name, pdev->name, sizeof(idev->adapter.name));
> + idev->adapter.owner = THIS_MODULE;
> + idev->adapter.algo = &altr_i2c_algo;
> + idev->adapter.dev.parent = &pdev->dev;
> + idev->adapter.dev.of_node = pdev->dev.of_node;
> +
> + platform_set_drvdata(pdev, idev);
> +
> + ret = i2c_add_adapter(&idev->adapter);
> + if (ret) {
> + clk_disable_unprepare(idev->i2c_clk);
> + return ret;
> + }
> + dev_info(&pdev->dev, "Altera SoftIP I2C Probe Complete\n");
> +
> + return 0;
> +}
> +
> +static int altr_i2c_remove(struct platform_device *pdev)
> +{
> + struct altr_i2c_dev *idev = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(idev->i2c_clk);
> + i2c_del_adapter(&idev->adapter);
> +
> + return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id altr_i2c_of_match[] = {
> + { .compatible = "altr,softip-i2c-v1.0" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, altr_i2c_of_match);
> +
> +static struct platform_driver altr_i2c_driver = {
> + .probe = altr_i2c_probe,
> + .remove = altr_i2c_remove,
> + .driver = {
> + .name = "altera-i2c",
> + .of_match_table = altr_i2c_of_match,
> + },
> +};
> +
> +module_platform_driver(altr_i2c_driver);
> +
> +MODULE_DESCRIPTION("Altera Soft IP I2C bus driver");
> +MODULE_AUTHOR("Thor Thayer <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
Hi Thor,
thanks for the patches! A few comments and questions from me.
On Mon, Jul 17, 2017 at 11:35:14AM -0500, [email protected] wrote:
> From: Thor Thayer <[email protected]>
>
> Add driver support for the Altera I2C Controller. The I2C
> controller is soft IP for use in FPGAs.
>
> Signed-off-by: Thor Thayer <[email protected]>
> ---
> v2 Remove altr, from fifo-size to agree with bindings.
> Change compatible string to "altr,softip-i2c"
> v3 Add version to compatible string "altr,softip-i2c-v1.0"
> v4 Add default 100kHz message. Cleanup IRQ error message.
> v5 Cleanup header usage - remove init.h.
> Fix 1 byte RX bug and add convenience function for transfer shared by RX and TX.
> Use readl_poll_timeout_atomic() function.
> Cache the I2C Interrupt Status Enable Register.
> Cleanup of irq_mask by combining into 1 function.
> Use i2c_8bit_addr_from_msg() for cleaner xfer_msg implementation.
> Restructure altr_i2c_xfer() without arrays and indexes.
> Remove redundant assignments.
> Remove function calls from inside if() statements.
> Convert of_property_read_u32() to device_property_read_u32().
> Call i2c_m_read() once in irq.
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-altera.c | 510 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 521 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-altera.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 1006b23..668147bf 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -328,6 +328,16 @@ config I2C_POWERMAC
>
> comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>
> +config I2C_ALTERA
> + tristate "Altera Soft IP I2C"
> + depends on (ARCH_SOCFPGA || NIOS2) && OF
> + help
> + If you say yes to this option, support will be included for the
> + Altera Soft IP I2C interfaces on SoCFPGA and Nios2 architectures.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-altera.
> +
> config I2C_ASPEED
> tristate "Aspeed I2C Controller"
> depends on ARCH_ASPEED || COMPILE_TEST
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1b2fc81..1395dcb 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
> obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
>
> # Embedded system I2C/SMBus host controller drivers
> +obj-$(CONFIG_I2C_ALTERA) += i2c-altera.o
> obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
> obj-$(CONFIG_I2C_AT91) += i2c-at91.o
> obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
> diff --git a/drivers/i2c/busses/i2c-altera.c b/drivers/i2c/busses/i2c-altera.c
> new file mode 100644
> index 0000000..e818901
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-altera.c
> @@ -0,0 +1,510 @@
> +/*
> + * Copyright Intel Corporation (C) 2017.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Based on the i2c-axxia.c driver.
> + */
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +
> +#define ALTR_I2C_TFR_CMD 0x00 /* Transfer Command register */
> +#define ALTR_I2C_TFR_CMD_STA BIT(9) /* send START before byte */
> +#define ALTR_I2C_TFR_CMD_STO BIT(8) /* send STOP after byte */
> +#define ALTR_I2C_TFR_CMD_RW_D BIT(0) /* Direction of transfer */
> +#define ALTR_I2C_RX_DATA 0x04 /* RX data FIFO register */
> +#define ALTR_I2C_CTRL 0x08 /* Control register */
> +#define ALTR_I2C_CTRL_RXT_SHFT 4 /* RX FIFO Threshold */
> +#define ALTR_I2C_CTRL_TCT_SHFT 2 /* TFER CMD FIFO Threshold */
> +#define ALTR_I2C_CTRL_BSPEED BIT(1) /* Bus Speed (1=Fast) */
> +#define ALTR_I2C_CTRL_EN BIT(0) /* Enable Core (1=Enable) */
> +#define ALTR_I2C_ISER 0x0C /* Interrupt Status Enable register */
> +#define ALTR_I2C_ISER_RXOF_EN BIT(4) /* Enable RX OVERFLOW IRQ */
> +#define ALTR_I2C_ISER_ARB_EN BIT(3) /* Enable ARB LOST IRQ */
> +#define ALTR_I2C_ISER_NACK_EN BIT(2) /* Enable NACK DET IRQ */
> +#define ALTR_I2C_ISER_RXRDY_EN BIT(1) /* Enable RX Ready IRQ */
> +#define ALTR_I2C_ISER_TXRDY_EN BIT(0) /* Enable TX Ready IRQ */
> +#define ALTR_I2C_ISR 0x10 /* Interrupt Status register */
> +#define ALTR_I2C_ISR_RXOF BIT(4) /* RX OVERFLOW IRQ */
> +#define ALTR_I2C_ISR_ARB BIT(3) /* ARB LOST IRQ */
> +#define ALTR_I2C_ISR_NACK BIT(2) /* NACK DET IRQ */
> +#define ALTR_I2C_ISR_RXRDY BIT(1) /* RX Ready IRQ */
> +#define ALTR_I2C_ISR_TXRDY BIT(0) /* TX Ready IRQ */
> +#define ALTR_I2C_STATUS 0x14 /* Status register */
> +#define ALTR_I2C_STAT_CORE BIT(0) /* Core Status (0=idle) */
> +#define ALTR_I2C_TC_FIFO_LVL 0x18 /* Transfer FIFO LVL register */
> +#define ALTR_I2C_RX_FIFO_LVL 0x1C /* Receive FIFO LVL register */
> +#define ALTR_I2C_SCL_LOW 0x20 /* SCL low count register */
> +#define ALTR_I2C_SCL_HIGH 0x24 /* SCL high count register */
> +#define ALTR_I2C_SDA_HOLD 0x28 /* SDA hold count register */
> +
> +#define ALTR_I2C_ALL_IRQ (ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | \
> + ALTR_I2C_ISR_NACK | ALTR_I2C_ISR_RXRDY | \
> + ALTR_I2C_ISR_TXRDY)
> +
> +#define ALTR_I2C_THRESHOLD 0 /* IRQ Threshold at 1 element */
> +#define ALTR_I2C_DFLT_FIFO_SZ 4
> +#define ALTR_I2C_TIMEOUT 100000 /* 100ms */
> +#define ALTR_I2C_XFER_TIMEOUT (msecs_to_jiffies(250))
> +
> +/**
> + * altr_i2c_dev - I2C device context
> + * @base: pointer to register struct
> + * @msg: pointer to current message
> + * @msg_len: number of bytes transferred in msg
> + * @msg_err: error code for completed message
> + * @msg_complete: xfer completion object
> + * @dev: device reference
> + * @adapter: core i2c abstraction
> + * @i2c_clk: clock reference for i2c input clock
> + * @bus_clk_rate: current i2c bus clock rate
> + * @buf: ptr to msg buffer for easier use.
> + * @fifo_size: size of the FIFO passed in.
> + */
> +struct altr_i2c_dev {
> + void __iomem *base;
> + struct i2c_msg *msg;
> + size_t msg_len;
> + int msg_err;
> + struct completion msg_complete;
> + struct device *dev;
> + struct i2c_adapter adapter;
> + struct clk *i2c_clk;
> + u32 bus_clk_rate;
> + u8 *buf;
> + u32 fifo_size;
> + u32 isr_mask;
The last one is not described in the kernel docs.
> +};
> +
> +static void i2c_int_enable(struct altr_i2c_dev *idev, u32 mask, bool enable)
> +{
> + u32 int_en = readl(idev->base + ALTR_I2C_ISER);
> +
> + if (enable)
> + idev->isr_mask = int_en | mask;
> + else
> + idev->isr_mask = int_en & ~mask;
> +
> + writel(idev->isr_mask, idev->base + ALTR_I2C_ISER);
> +}
> +
> +static void i2c_int_clear(struct altr_i2c_dev *idev, u32 mask)
> +{
> + u32 int_en = readl(idev->base + ALTR_I2C_ISR);
> +
> + writel(int_en | mask, idev->base + ALTR_I2C_ISR);
> +}
Those two functions would also suit an altr_ prefix.
> +
> +static void altr_i2c_core_disable(struct altr_i2c_dev *idev)
> +{
> + u32 tmp = readl(idev->base + ALTR_I2C_CTRL);
> +
> + writel(tmp & ~ALTR_I2C_CTRL_EN, idev->base + ALTR_I2C_CTRL);
> +}
> +
> +static void altr_i2c_core_enable(struct altr_i2c_dev *idev)
> +{
> + u32 tmp = readl(idev->base + ALTR_I2C_CTRL);
> +
> + writel(tmp | ALTR_I2C_CTRL_EN, idev->base + ALTR_I2C_CTRL);
> +}
> +
> +static void altr_i2c_reset(struct altr_i2c_dev *idev)
> +{
> + altr_i2c_core_disable(idev);
> + altr_i2c_core_enable(idev);
> +}
> +
> +static void altr_i2c_recover(struct altr_i2c_dev *idev)
> +{
> + altr_i2c_reset(idev);
> + /* Clock start bit + 8 bits + stop bit out */
Really 8? It should be 9 (ACK bit).
> + writel(ALTR_I2C_TFR_CMD_STA | ALTR_I2C_TFR_CMD_STO,
> + idev->base + ALTR_I2C_TFR_CMD);
> + altr_i2c_reset(idev);
Why the second reset?
> +
> + i2c_recover_bus(&idev->adapter);
This will return an error since you don't have a struct
i2c_bus_recovery_info defined. This whole recovery looks fishy, maybe it
should be dropped now and added incrementally?
> +}
> +
> +static inline void altr_i2c_stop(struct altr_i2c_dev *idev)
> +{
> + writel(ALTR_I2C_TFR_CMD_STO, idev->base + ALTR_I2C_TFR_CMD);
> +}
> +
> +static void altr_i2c_init(struct altr_i2c_dev *idev)
> +{
> + u32 divisor = clk_get_rate(idev->i2c_clk) / idev->bus_clk_rate;
> + u32 clk_mhz = clk_get_rate(idev->i2c_clk) / 1000000;
> + u32 tmp = (ALTR_I2C_THRESHOLD << ALTR_I2C_CTRL_RXT_SHFT) |
> + (ALTR_I2C_THRESHOLD << ALTR_I2C_CTRL_TCT_SHFT);
> + u32 t_high, t_low;
> +
> + if (idev->bus_clk_rate <= 100000) {
> + tmp &= ~ALTR_I2C_CTRL_BSPEED;
> + /* Standard mode SCL 50/50 */
> + t_high = divisor * 1 / 2;
> + t_low = divisor * 1 / 2;
> + } else {
> + tmp |= ALTR_I2C_CTRL_BSPEED;
> + /* Fast mode SCL 33/66 */
> + t_high = divisor * 1 / 3;
> + t_low = divisor * 2 / 3;
> + }
> + writel(tmp, idev->base + ALTR_I2C_CTRL);
> +
> + dev_dbg(idev->dev, "rate=%uHz per_clk=%uMHz -> ratio=1:%u\n",
> + idev->bus_clk_rate, clk_mhz, divisor);
> +
> + /* Reset controller */
> + altr_i2c_reset(idev);
> +
> + /* SCL High Time */
> + writel(t_high, idev->base + ALTR_I2C_SCL_HIGH);
> + /* SCL Low Time */
> + writel(t_low, idev->base + ALTR_I2C_SCL_LOW);
> + /* SDA Hold Time, 300ns */
> + writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD);
> +
> + /* Mask all master interrupt bits */
> + i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
> +}
> +
> +static int i2c_m_rd(const struct i2c_msg *msg)
> +{
> + return (msg->flags & I2C_M_RD) != 0;
> +}
I'll leave it to you, but I'd think an extra function doesn't really
improve readability here.
> +
> +/**
> + * altr_i2c_transfer - On the last byte to be transmitted, send
> + * a Stop bit on the last byte.
> + */
> +static void altr_i2c_transfer(struct altr_i2c_dev *idev, u32 data)
> +{
> + /* On the last byte to be transmitted, send STOP */
> + if (idev->msg_len == 1)
> + data |= ALTR_I2C_TFR_CMD_STO;
> + if (idev->msg_len > 0)
> + writel(data, idev->base + ALTR_I2C_TFR_CMD);
> +}
> +
> +/**
> + * altr_i2c_empty_rx_fifo - Fetch data from RX FIFO until end of
> + * transfer. Send a Stop bit on the last byte.
> + */
> +static void altr_i2c_empty_rx_fifo(struct altr_i2c_dev *idev)
> +{
> + size_t rx_fifo_avail = readl(idev->base + ALTR_I2C_RX_FIFO_LVL);
> + int bytes_to_transfer = min(rx_fifo_avail, idev->msg_len);
> +
> + while (bytes_to_transfer-- > 0) {
> + *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
> + idev->msg_len--;
> + altr_i2c_transfer(idev, 0);
> + }
> +}
> +
> +/**
> + * altr_i2c_fill_tx_fifo - Fill TX FIFO from current message buffer.
> + * @return: Number of bytes left to transfer.
> + */
> +static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
> +{
> + size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
> + ALTR_I2C_TC_FIFO_LVL);
> + int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
> + int ret = idev->msg_len - bytes_to_transfer;
> +
> + while (bytes_to_transfer-- > 0) {
> + altr_i2c_transfer(idev, *idev->buf++);
> + idev->msg_len--;
> + }
> +
> + return ret;
> +}
> +
> +static irqreturn_t altr_i2c_isr(int irq, void *_dev)
> +{
> + int ret, read;
> + struct altr_i2c_dev *idev = _dev;
> + /* Read IRQ status but only interested in Enabled IRQs. */
> + u32 status = readl(idev->base + ALTR_I2C_ISR) & idev->isr_mask;
> +
> + if (!idev->msg) {
> + dev_warn(idev->dev, "unexpected interrupt\n");
> + goto out;
Won't you get an interrupt storm if you don't use any i2c_int_clear()?
> + }
> + read = i2c_m_rd(idev->msg);
> +
> + /* handle Lost Arbitration */
> + if (unlikely(status & ALTR_I2C_ISR_ARB)) {
> + dev_err(idev->dev, "%s: arbitration lost\n", __func__);
Drop the message, it is not an error, but a to be expected bus condition
in multi-master setups.
> + i2c_int_clear(idev, ALTR_I2C_ISR_ARB);
> + idev->msg_err = -EAGAIN;
> + goto complete;
> + }
> +
> + if (unlikely(status & ALTR_I2C_ISR_NACK)) {
> + dev_dbg(idev->dev, "%s: could not get ACK\n", __func__);
> + idev->msg_err = -ENXIO;
> + i2c_int_clear(idev, ALTR_I2C_ISR_NACK);
> + altr_i2c_stop(idev);
> + goto complete;
> + }
> +
> + /* handle RX FIFO Overflow */
> + if (read && unlikely(status & ALTR_I2C_ISR_RXOF)) {
> + altr_i2c_empty_rx_fifo(idev);
> + i2c_int_clear(idev, ALTR_I2C_ISR_RXRDY);
> + altr_i2c_stop(idev);
> + dev_err(idev->dev, "%s: RX FIFO Overflow\n", __func__);
> + goto complete;
> + }
> +
> + /* RX FIFO needs service? */
> + if (read && (status & ALTR_I2C_ISR_RXRDY)) {
> + altr_i2c_empty_rx_fifo(idev);
> + i2c_int_clear(idev, ALTR_I2C_ISR_RXRDY);
> + if (!idev->msg_len)
> + goto complete;
> +
> + goto out;
> + }
> +
> + /* TX FIFO needs service? */
> + if (!read && (status & ALTR_I2C_ISR_TXRDY)) {
> + i2c_int_clear(idev, ALTR_I2C_ISR_TXRDY);
> + if (!altr_i2c_fill_tx_fifo(idev))
> + goto complete;
> +
> + goto out;
A bit too much goto in here for my taste. What about using a boolean
'complete' variable which gets set or cleared and have an 'if
(complete)' at the end of this routine?
> + }
> +
> +complete:
> + /* Wait for the Core to finish */
> + ret = readl_poll_timeout_atomic(idev->base + ALTR_I2C_STATUS, status,
> + ((status & ALTR_I2C_STAT_CORE) == 0),
> + 1, ALTR_I2C_TIMEOUT);
Why do you need this polling in an interrupt?
> + if (ret)
> + dev_err(idev->dev, "%s: message timeout\n", __func__);
> + i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
> + i2c_int_clear(idev, ALTR_I2C_ALL_IRQ);
> + complete(&idev->msg_complete);
> + dev_dbg(idev->dev, "%s: Message Complete\n", __func__);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +static int altr_i2c_xfer_msg(struct altr_i2c_dev *idev, struct i2c_msg *msg)
> +{
> + u32 imask = ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | ALTR_I2C_ISR_NACK;
> + unsigned long time_left;
> + u32 value;
> + u8 addr = i2c_8bit_addr_from_msg(msg);
> +
> + idev->msg = msg;
> + idev->msg_len = msg->len;
> + idev->buf = msg->buf;
> + idev->msg_err = 0;
> + reinit_completion(&idev->msg_complete);
> + altr_i2c_core_enable(idev);
> +
> + /* Make sure RX FIFO is empty */
> + do {
> + readl(idev->base + ALTR_I2C_RX_DATA);
> + } while (readl(idev->base + ALTR_I2C_RX_FIFO_LVL));
> +
> + writel(ALTR_I2C_TFR_CMD_STA | addr, idev->base + ALTR_I2C_TFR_CMD);
> +
> + if (i2c_m_rd(msg)) {
> + imask |= ALTR_I2C_ISER_RXOF_EN | ALTR_I2C_ISER_RXRDY_EN;
> + i2c_int_enable(idev, imask, true);
> + /* write the first byte to start the RX */
> + altr_i2c_transfer(idev, 0);
> + } else {
> + imask |= ALTR_I2C_ISR_TXRDY;
> + i2c_int_enable(idev, imask, true);
> + altr_i2c_fill_tx_fifo(idev);
> + }
> +
> + time_left = wait_for_completion_timeout(&idev->msg_complete,
> + ALTR_I2C_XFER_TIMEOUT);
> + i2c_int_enable(idev, imask, false);
> +
> + value = readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE;
> + if (value)
> + dev_err(idev->dev, "%s: Core Status not IDLE...\n", __func__);
__func__ with dev_* is 99,9% superfluous.
> +
> + if (time_left == 0) {
> + idev->msg_err = -ETIMEDOUT;
> + dev_err(idev->dev, "%s: Transaction timed out.\n", __func__);
No error message here. This can happen when an EEPROM is currently
erasing.
> + altr_i2c_recover(idev);
And no need to reset the bus! Only if SDA is stuck low.
> + }
> +
> + if (unlikely(idev->msg_err) && idev->msg_err != -ENXIO)
> + altr_i2c_init(idev);
Why do you need to reinit on NACK?
> +
> + altr_i2c_core_disable(idev);
> +
> + return idev->msg_err;
> +}
> +
> +static int
> +altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> + struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
> + int i, ret;
> +
> + for (i = 0; i < num; i++) {
> + ret = altr_i2c_xfer_msg(idev, msgs++);
> + if (ret)
> + return ret;
> + }
> + return num;
> +}
> +
> +static u32 altr_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C;
> +}
No emulated SMBUS?
> +
> +static const struct i2c_algorithm altr_i2c_algo = {
> + .master_xfer = altr_i2c_xfer,
> + .functionality = altr_i2c_func,
> +};
> +
> +static int altr_i2c_probe(struct platform_device *pdev)
> +{
> + struct altr_i2c_dev *idev = NULL;
> + struct resource *res;
> + int irq, ret;
> + u32 val;
> +
> + idev = devm_kzalloc(&pdev->dev, sizeof(*idev), GFP_KERNEL);
> + if (!idev)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + idev->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(idev->base))
> + return PTR_ERR(idev->base);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "missing interrupt resource\n");
> + return irq;
> + }
> +
> + idev->i2c_clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(idev->i2c_clk)) {
> + dev_err(&pdev->dev, "missing clock\n");
> + return PTR_ERR(idev->i2c_clk);
> + }
> +
> + idev->dev = &pdev->dev;
> + init_completion(&idev->msg_complete);
> +
> + val = device_property_read_u32(idev->dev, "fifo-size",
> + &idev->fifo_size);
> + if (val) {
> + dev_err(&pdev->dev, "FIFO size set to default of %d\n",
> + ALTR_I2C_DFLT_FIFO_SZ);
> + idev->fifo_size = ALTR_I2C_DFLT_FIFO_SZ;
> + }
> +
> + val = device_property_read_u32(idev->dev, "clock-frequency",
> + &idev->bus_clk_rate);
> + if (val) {
> + dev_err(&pdev->dev, "Default to 100kHz\n");
> + idev->bus_clk_rate = 100000; /* default clock rate */
> + }
> +
> + if (idev->bus_clk_rate > 400000) {
> + dev_err(&pdev->dev, "invalid clock-frequency %d\n",
> + idev->bus_clk_rate);
> + return -EINVAL;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, irq, altr_i2c_isr, 0,
> + pdev->name, idev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to claim IRQ %d\n", irq);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(idev->i2c_clk);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to enable clock\n");
> + return ret;
> + }
> +
> + altr_i2c_init(idev);
> +
> + i2c_set_adapdata(&idev->adapter, idev);
> + strlcpy(idev->adapter.name, pdev->name, sizeof(idev->adapter.name));
> + idev->adapter.owner = THIS_MODULE;
> + idev->adapter.algo = &altr_i2c_algo;
> + idev->adapter.dev.parent = &pdev->dev;
> + idev->adapter.dev.of_node = pdev->dev.of_node;
> +
> + platform_set_drvdata(pdev, idev);
> +
> + ret = i2c_add_adapter(&idev->adapter);
> + if (ret) {
> + clk_disable_unprepare(idev->i2c_clk);
> + return ret;
> + }
> + dev_info(&pdev->dev, "Altera SoftIP I2C Probe Complete\n");
> +
> + return 0;
> +}
> +
> +static int altr_i2c_remove(struct platform_device *pdev)
> +{
> + struct altr_i2c_dev *idev = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(idev->i2c_clk);
> + i2c_del_adapter(&idev->adapter);
> +
> + return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id altr_i2c_of_match[] = {
> + { .compatible = "altr,softip-i2c-v1.0" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, altr_i2c_of_match);
> +
> +static struct platform_driver altr_i2c_driver = {
> + .probe = altr_i2c_probe,
> + .remove = altr_i2c_remove,
> + .driver = {
> + .name = "altera-i2c",
> + .of_match_table = altr_i2c_of_match,
> + },
> +};
> +
> +module_platform_driver(altr_i2c_driver);
> +
> +MODULE_DESCRIPTION("Altera Soft IP I2C bus driver");
> +MODULE_AUTHOR("Thor Thayer <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
Hi Wolfram,
Thank you for reviewing.
On 07/31/2017 08:31 AM, Wolfram Sang wrote:
> Hi Thor,
>
> thanks for the patches! A few comments and questions from me.
>
> On Mon, Jul 17, 2017 at 11:35:14AM -0500, [email protected] wrote:
>> From: Thor Thayer <[email protected]>
>>
>> Add driver support for the Altera I2C Controller. The I2C
>> controller is soft IP for use in FPGAs.
>>
>> Signed-off-by: Thor Thayer <[email protected]>
>> ---
>> v2 Remove altr, from fifo-size to agree with bindings.
>> Change compatible string to "altr,softip-i2c"
>> v3 Add version to compatible string "altr,softip-i2c-v1.0"
>> v4 Add default 100kHz message. Cleanup IRQ error message.
>> v5 Cleanup header usage - remove init.h.
>> Fix 1 byte RX bug and add convenience function for transfer shared by RX and TX.
>> Use readl_poll_timeout_atomic() function.
>> Cache the I2C Interrupt Status Enable Register.
>> Cleanup of irq_mask by combining into 1 function.
>> Use i2c_8bit_addr_from_msg() for cleaner xfer_msg implementation.
>> Restructure altr_i2c_xfer() without arrays and indexes.
>> Remove redundant assignments.
>> Remove function calls from inside if() statements.
>> Convert of_property_read_u32() to device_property_read_u32().
>> Call i2c_m_read() once in irq.
>> ---
>> drivers/i2c/busses/Kconfig | 10 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-altera.c | 510 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 521 insertions(+)
<snip>
>> new file mode 100644
>> index 0000000..e818901
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-altera.c
>> @@ -0,0 +1,510 @@
<snip>
>> +
>> +#define ALTR_I2C_THRESHOLD 0 /* IRQ Threshold at 1 element */
>> +#define ALTR_I2C_DFLT_FIFO_SZ 4
>> +#define ALTR_I2C_TIMEOUT 100000 /* 100ms */
>> +#define ALTR_I2C_XFER_TIMEOUT (msecs_to_jiffies(250))
>> +
>> +/**
>> + * altr_i2c_dev - I2C device context
>> + * @base: pointer to register struct
>> + * @msg: pointer to current message
>> + * @msg_len: number of bytes transferred in msg
>> + * @msg_err: error code for completed message
>> + * @msg_complete: xfer completion object
>> + * @dev: device reference
>> + * @adapter: core i2c abstraction
>> + * @i2c_clk: clock reference for i2c input clock
>> + * @bus_clk_rate: current i2c bus clock rate
>> + * @buf: ptr to msg buffer for easier use.
>> + * @fifo_size: size of the FIFO passed in.
>> + */
>> +struct altr_i2c_dev {
>> + void __iomem *base;
>> + struct i2c_msg *msg;
>> + size_t msg_len;
>> + int msg_err;
>> + struct completion msg_complete;
>> + struct device *dev;
>> + struct i2c_adapter adapter;
>> + struct clk *i2c_clk;
>> + u32 bus_clk_rate;
>> + u8 *buf;
>> + u32 fifo_size;
>> + u32 isr_mask;
>
> The last one is not described in the kernel docs.
>
Whoops. Yes, I will add this.
>> +};
>> +
>> +static void i2c_int_enable(struct altr_i2c_dev *idev, u32 mask, bool enable)
>> +{
>> + u32 int_en = readl(idev->base + ALTR_I2C_ISER);
>> +
>> + if (enable)
>> + idev->isr_mask = int_en | mask;
>> + else
>> + idev->isr_mask = int_en & ~mask;
>> +
>> + writel(idev->isr_mask, idev->base + ALTR_I2C_ISER);
>> +}
>> +
>> +static void i2c_int_clear(struct altr_i2c_dev *idev, u32 mask)
>> +{
>> + u32 int_en = readl(idev->base + ALTR_I2C_ISR);
>> +
>> + writel(int_en | mask, idev->base + ALTR_I2C_ISR);
>> +}
>
> Those two functions would also suit an altr_ prefix.
>
Yes. I will fix this.
>> +
>> +static void altr_i2c_core_disable(struct altr_i2c_dev *idev)
>> +{
>> + u32 tmp = readl(idev->base + ALTR_I2C_CTRL);
>> +
>> + writel(tmp & ~ALTR_I2C_CTRL_EN, idev->base + ALTR_I2C_CTRL);
>> +}
>> +
>> +static void altr_i2c_core_enable(struct altr_i2c_dev *idev)
>> +{
>> + u32 tmp = readl(idev->base + ALTR_I2C_CTRL);
>> +
>> + writel(tmp | ALTR_I2C_CTRL_EN, idev->base + ALTR_I2C_CTRL);
>> +}
>> +
>> +static void altr_i2c_reset(struct altr_i2c_dev *idev)
>> +{
>> + altr_i2c_core_disable(idev);
>> + altr_i2c_core_enable(idev);
>> +}
>> +
>> +static void altr_i2c_recover(struct altr_i2c_dev *idev)
>> +{
>> + altr_i2c_reset(idev);
>> + /* Clock start bit + 8 bits + stop bit out */
>
> Really 8? It should be 9 (ACK bit).
>
Yes, I'll update the comment.
>> + writel(ALTR_I2C_TFR_CMD_STA | ALTR_I2C_TFR_CMD_STO,
>> + idev->base + ALTR_I2C_TFR_CMD);
>> + altr_i2c_reset(idev);
>
> Why the second reset?
>
I wanted to start from a known clean condition. The first reset would
reset the hardware. The 2nd would ensure the hardware is ready after
clearing the bus. Maybe I'm being overly cautious but I don't have a way
to check the status of the SDA line.
>> +
>> + i2c_recover_bus(&idev->adapter);
>
> This will return an error since you don't have a struct
> i2c_bus_recovery_info defined. This whole recovery looks fishy, maybe it
> should be dropped now and added incrementally?
>
Yes. I'll drop it for now. Thanks.
>> +}
>> +
>> +static inline void altr_i2c_stop(struct altr_i2c_dev *idev)
>> +{
>> + writel(ALTR_I2C_TFR_CMD_STO, idev->base + ALTR_I2C_TFR_CMD);
>> +}
>> +
>> +static void altr_i2c_init(struct altr_i2c_dev *idev)
>> +{
>> + u32 divisor = clk_get_rate(idev->i2c_clk) / idev->bus_clk_rate;
>> + u32 clk_mhz = clk_get_rate(idev->i2c_clk) / 1000000;
>> + u32 tmp = (ALTR_I2C_THRESHOLD << ALTR_I2C_CTRL_RXT_SHFT) |
>> + (ALTR_I2C_THRESHOLD << ALTR_I2C_CTRL_TCT_SHFT);
>> + u32 t_high, t_low;
>> +
>> + if (idev->bus_clk_rate <= 100000) {
>> + tmp &= ~ALTR_I2C_CTRL_BSPEED;
>> + /* Standard mode SCL 50/50 */
>> + t_high = divisor * 1 / 2;
>> + t_low = divisor * 1 / 2;
>> + } else {
>> + tmp |= ALTR_I2C_CTRL_BSPEED;
>> + /* Fast mode SCL 33/66 */
>> + t_high = divisor * 1 / 3;
>> + t_low = divisor * 2 / 3;
>> + }
>> + writel(tmp, idev->base + ALTR_I2C_CTRL);
>> +
>> + dev_dbg(idev->dev, "rate=%uHz per_clk=%uMHz -> ratio=1:%u\n",
>> + idev->bus_clk_rate, clk_mhz, divisor);
>> +
>> + /* Reset controller */
>> + altr_i2c_reset(idev);
>> +
>> + /* SCL High Time */
>> + writel(t_high, idev->base + ALTR_I2C_SCL_HIGH);
>> + /* SCL Low Time */
>> + writel(t_low, idev->base + ALTR_I2C_SCL_LOW);
>> + /* SDA Hold Time, 300ns */
>> + writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD);
>> +
>> + /* Mask all master interrupt bits */
>> + i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
>> +}
>> +
>> +static int i2c_m_rd(const struct i2c_msg *msg)
>> +{
>> + return (msg->flags & I2C_M_RD) != 0;
>> +}
>
> I'll leave it to you, but I'd think an extra function doesn't really
> improve readability here.
>
OK. I'd seen others using it elsewhere and wanted to stay with
convention but I'll just handle it directly. Thanks.
>> +
>> +/**
>> + * altr_i2c_transfer - On the last byte to be transmitted, send
>> + * a Stop bit on the last byte.
>> + */
>> +static void altr_i2c_transfer(struct altr_i2c_dev *idev, u32 data)
>> +{
>> + /* On the last byte to be transmitted, send STOP */
>> + if (idev->msg_len == 1)
>> + data |= ALTR_I2C_TFR_CMD_STO;
>> + if (idev->msg_len > 0)
>> + writel(data, idev->base + ALTR_I2C_TFR_CMD);
>> +}
>> +
>> +/**
>> + * altr_i2c_empty_rx_fifo - Fetch data from RX FIFO until end of
>> + * transfer. Send a Stop bit on the last byte.
>> + */
>> +static void altr_i2c_empty_rx_fifo(struct altr_i2c_dev *idev)
>> +{
>> + size_t rx_fifo_avail = readl(idev->base + ALTR_I2C_RX_FIFO_LVL);
>> + int bytes_to_transfer = min(rx_fifo_avail, idev->msg_len);
>> +
>> + while (bytes_to_transfer-- > 0) {
>> + *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
>> + idev->msg_len--;
>> + altr_i2c_transfer(idev, 0);
>> + }
>> +}
>> +
>> +/**
>> + * altr_i2c_fill_tx_fifo - Fill TX FIFO from current message buffer.
>> + * @return: Number of bytes left to transfer.
>> + */
>> +static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
>> +{
>> + size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
>> + ALTR_I2C_TC_FIFO_LVL);
>> + int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
>> + int ret = idev->msg_len - bytes_to_transfer;
>> +
>> + while (bytes_to_transfer-- > 0) {
>> + altr_i2c_transfer(idev, *idev->buf++);
>> + idev->msg_len--;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static irqreturn_t altr_i2c_isr(int irq, void *_dev)
>> +{
>> + int ret, read;
>> + struct altr_i2c_dev *idev = _dev;
>> + /* Read IRQ status but only interested in Enabled IRQs. */
>> + u32 status = readl(idev->base + ALTR_I2C_ISR) & idev->isr_mask;
>> +
>> + if (!idev->msg) {
>> + dev_warn(idev->dev, "unexpected interrupt\n");
>> + goto out;
>
> Won't you get an interrupt storm if you don't use any i2c_int_clear()?
>
Yes. I missed that. Thanks.
>> + }
>> + read = i2c_m_rd(idev->msg);
>> +
>> + /* handle Lost Arbitration */
>> + if (unlikely(status & ALTR_I2C_ISR_ARB)) {
>> + dev_err(idev->dev, "%s: arbitration lost\n", __func__);
>
> Drop the message, it is not an error, but a to be expected bus condition
> in multi-master setups.
OK. Thanks.
>
>> + i2c_int_clear(idev, ALTR_I2C_ISR_ARB);
>> + idev->msg_err = -EAGAIN;
>> + goto complete;
>> + }
>> +
>> + if (unlikely(status & ALTR_I2C_ISR_NACK)) {
>> + dev_dbg(idev->dev, "%s: could not get ACK\n", __func__);
>> + idev->msg_err = -ENXIO;
>> + i2c_int_clear(idev, ALTR_I2C_ISR_NACK);
>> + altr_i2c_stop(idev);
>> + goto complete;
>> + }
>> +
>> + /* handle RX FIFO Overflow */
>> + if (read && unlikely(status & ALTR_I2C_ISR_RXOF)) {
>> + altr_i2c_empty_rx_fifo(idev);
>> + i2c_int_clear(idev, ALTR_I2C_ISR_RXRDY);
>> + altr_i2c_stop(idev);
>> + dev_err(idev->dev, "%s: RX FIFO Overflow\n", __func__);
>> + goto complete;
>> + }
>> +
>> + /* RX FIFO needs service? */
>> + if (read && (status & ALTR_I2C_ISR_RXRDY)) {
>> + altr_i2c_empty_rx_fifo(idev);
>> + i2c_int_clear(idev, ALTR_I2C_ISR_RXRDY);
>> + if (!idev->msg_len)
>> + goto complete;
>> +
>> + goto out;
>> + }
>> +
>> + /* TX FIFO needs service? */
>> + if (!read && (status & ALTR_I2C_ISR_TXRDY)) {
>> + i2c_int_clear(idev, ALTR_I2C_ISR_TXRDY);
>> + if (!altr_i2c_fill_tx_fifo(idev))
>> + goto complete;
>> +
>> + goto out;
>
> A bit too much goto in here for my taste. What about using a boolean
> 'complete' variable which gets set or cleared and have an 'if
> (complete)' at the end of this routine?
>
I want to exit immediately on an error - without checking any further
errors or RX or TX. Maybe a switch/case statement would be better.
Yes, the boolean would be cleaner. I will look at better ways to handle
the sequence. Andy's last comment indicated something similar. Thanks.
>> + }
>> +
>> +complete:
>> + /* Wait for the Core to finish */
>> + ret = readl_poll_timeout_atomic(idev->base + ALTR_I2C_STATUS, status,
>> + ((status & ALTR_I2C_STAT_CORE) == 0),
>> + 1, ALTR_I2C_TIMEOUT);
>
> Why do you need this polling in an interrupt?
>
I wanted to wait for completion before marking it as complete. As you
point out, this isn't appropriate in an interrupt handler. I will look
at this again. Thanks for pointing this out.
>
>> + if (ret)
>> + dev_err(idev->dev, "%s: message timeout\n", __func__);
>> + i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
>> + i2c_int_clear(idev, ALTR_I2C_ALL_IRQ);
>> + complete(&idev->msg_complete);
>> + dev_dbg(idev->dev, "%s: Message Complete\n", __func__);
>> +
>> +out:
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int altr_i2c_xfer_msg(struct altr_i2c_dev *idev, struct i2c_msg *msg)
>> +{
>> + u32 imask = ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | ALTR_I2C_ISR_NACK;
>> + unsigned long time_left;
>> + u32 value;
>> + u8 addr = i2c_8bit_addr_from_msg(msg);
>> +
>> + idev->msg = msg;
>> + idev->msg_len = msg->len;
>> + idev->buf = msg->buf;
>> + idev->msg_err = 0;
>> + reinit_completion(&idev->msg_complete);
>> + altr_i2c_core_enable(idev);
>> +
>> + /* Make sure RX FIFO is empty */
>> + do {
>> + readl(idev->base + ALTR_I2C_RX_DATA);
>> + } while (readl(idev->base + ALTR_I2C_RX_FIFO_LVL));
>> +
>> + writel(ALTR_I2C_TFR_CMD_STA | addr, idev->base + ALTR_I2C_TFR_CMD);
>> +
>> + if (i2c_m_rd(msg)) {
>> + imask |= ALTR_I2C_ISER_RXOF_EN | ALTR_I2C_ISER_RXRDY_EN;
>> + i2c_int_enable(idev, imask, true);
>> + /* write the first byte to start the RX */
>> + altr_i2c_transfer(idev, 0);
>> + } else {
>> + imask |= ALTR_I2C_ISR_TXRDY;
>> + i2c_int_enable(idev, imask, true);
>> + altr_i2c_fill_tx_fifo(idev);
>> + }
>> +
>> + time_left = wait_for_completion_timeout(&idev->msg_complete,
>> + ALTR_I2C_XFER_TIMEOUT);
>> + i2c_int_enable(idev, imask, false);
>> +
>> + value = readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE;
>> + if (value)
>> + dev_err(idev->dev, "%s: Core Status not IDLE...\n", __func__);
>
> __func__ with dev_* is 99,9% superfluous.
>
I will remove this and the other ones above. Andy indicated this as well.
>> +
>> + if (time_left == 0) {
>> + idev->msg_err = -ETIMEDOUT;
>> + dev_err(idev->dev, "%s: Transaction timed out.\n", __func__);
>
> No error message here. This can happen when an EEPROM is currently
> erasing.
>
OK.
>> + altr_i2c_recover(idev);
>
> And no need to reset the bus! Only if SDA is stuck low.
>
OK. Unfortunately, we can't check the state of the SDA line so I was
being extra cautious. I will remove it.
>> + }
>> +
>> + if (unlikely(idev->msg_err) && idev->msg_err != -ENXIO)
>> + altr_i2c_init(idev);
>
> Why do you need to reinit on NACK?
>
OK. Thanks.
>> +
>> + altr_i2c_core_disable(idev);
>> +
>> + return idev->msg_err;
>> +}
>> +
>> +static int
>> +altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> +{
>> + struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
>> + int i, ret;
>> +
>> + for (i = 0; i < num; i++) {
>> + ret = altr_i2c_xfer_msg(idev, msgs++);
>> + if (ret)
>> + return ret;
>> + }
>> + return num;
>> +}
>> +
>> +static u32 altr_i2c_func(struct i2c_adapter *adap)
>> +{
>> + return I2C_FUNC_I2C;
>> +}
>
> No emulated SMBUS?
>
I was focusing on I2C and wasn't sure about SMBus (particularly the
minimum clock speed). SMBus could be added later but would require some
IP changes.
<snip>
> >>+ writel(ALTR_I2C_TFR_CMD_STA | ALTR_I2C_TFR_CMD_STO,
> >>+ idev->base + ALTR_I2C_TFR_CMD);
> >>+ altr_i2c_reset(idev);
> >
> >Why the second reset?
> >
>
> I wanted to start from a known clean condition. The first reset would reset
> the hardware. The 2nd would ensure the hardware is ready after clearing the
> bus. Maybe I'm being overly cautious but I don't have a way to check the
> status of the SDA line.
Well, it looks like the function will go for now anyhow...
> >>+ altr_i2c_recover(idev);
> >
> >And no need to reset the bus! Only if SDA is stuck low.
> >
>
> OK. Unfortunately, we can't check the state of the SDA line so I was being
> extra cautious. I will remove it.
Thanks. Bus recovery really needs a testcase to be checked against.
> >>+static u32 altr_i2c_func(struct i2c_adapter *adap)
> >>+{
> >>+ return I2C_FUNC_I2C;
> >>+}
> >
> >No emulated SMBUS?
> >
>
> I was focusing on I2C and wasn't sure about SMBus (particularly the minimum
> clock speed). SMBus could be added later but would require some IP changes.
I meant you can add I2C_FUNC_SMBUS_EMUL which makes the I2C core emulate
SMBus-style transactions via I2C messages for you. A lot of drivers
won't work if you don't have this.