[U-Boot] [RFC PATCH 20/29] drivers: i2c: add I2C controller driver for OcteonTX
Simon Glass
sjg at chromium.org
Wed Nov 20 03:00:54 UTC 2019
Hi Suneel,
On Tue, 29 Oct 2019 at 14:08, Suneel Garapati <suneelglinux at gmail.com> wrote:
>
> From: Suneel Garapati <sgarapati at marvell.com>
>
> Adds support for I2C controllers found on OcteonTX or
> OcteonTX2 SoC platforms.
>
> Signed-off-by: Aaron Williams <awilliams at marvell.com>
> Signed-off-by: Suneel Garapati <sgarapati at marvell.com>
> ---
> drivers/i2c/Kconfig | 7 +
> drivers/i2c/Makefile | 1 +
> drivers/i2c/octeontx_i2c.c | 968 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 976 insertions(+)
> create mode 100644 drivers/i2c/octeontx_i2c.c
>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index 03d2fed341..eb47454e76 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -366,6 +366,13 @@ config SYS_I2C_SANDBOX
> bus. Devices can be attached to the bus using the device tree
> which specifies the driver to use. See sandbox.dts as an example.
>
> +config SYS_I2C_OCTEONTX
> + bool "OcteonTX I2C driver"
> + depends on (ARCH_OCTEONTX || ARCH_OCTEONTX2) && DM_I2C
> + default y
> + help
> + Enable I2C support for OcteonTX/TX2 line of processors.
Again please expound on capabilities of hardware and driver.
> +
> config SYS_I2C_S3C24X0
> bool "Samsung I2C driver"
> depends on ARCH_EXYNOS4 && DM_I2C
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index c2f75d8755..e2ae95e770 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -33,6 +33,7 @@ obj-$(CONFIG_SYS_I2C_SH) += sh_i2c.o
> obj-$(CONFIG_SYS_I2C_SOFT) += soft_i2c.o
> obj-$(CONFIG_SYS_I2C_STM32F7) += stm32f7_i2c.o
> obj-$(CONFIG_SYS_I2C_TEGRA) += tegra_i2c.o
> +obj-$(CONFIG_SYS_I2C_OCTEONTX) += octeontx_i2c.o
> obj-$(CONFIG_SYS_I2C_UNIPHIER) += i2c-uniphier.o
> obj-$(CONFIG_SYS_I2C_UNIPHIER_F) += i2c-uniphier-f.o
> obj-$(CONFIG_SYS_I2C_VERSATILE) += i2c-versatile.o
> diff --git a/drivers/i2c/octeontx_i2c.c b/drivers/i2c/octeontx_i2c.c
> new file mode 100644
> index 0000000000..b74424a75f
> --- /dev/null
> +++ b/drivers/i2c/octeontx_i2c.c
> @@ -0,0 +1,968 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Marvell International Ltd.
> + *
> + * https://spdx.org/licenses
> + */
> +
> +#include <common.h>
> +#include <i2c.h>
> +#include <dm.h>
> +#include <asm/io.h>
> +#include <asm/arch/clock.h>
> +
> +#if defined(CONFIG_ARCH_OCTEONTX)
No, these need to be handled at runtime as mentioned earlier. No #ifdefs please.
> +# define TWSI_THP 24
> +#else
> +# define TWSI_THP 3
> +#endif
> +
> +/**
> + * Slave address to use for Thunder when accessed by another master
> + */
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SLAVE_ADDR
> +# define CONFIG_SYS_I2C_OCTEONTX_SLAVE_ADDR 0x77
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_0
> +# ifdef CONFIG_SYS_I2C_SPEED
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_0 CONFIG_SYS_I2C_SPEED
> +# else
> +# define CONFIG_SYS_I2C_SPEED 100000
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_0 CONFIG_SYS_I2C_SPEED
> +# endif
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_1
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_1 CONFIG_SYS_I2C_OCTEONTX_SPEED_0
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_2
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_2 CONFIG_SYS_I2C_OCTEONTX_SPEED_1
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_3
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_3 CONFIG_SYS_I2C_OCTEONTX_SPEED_2
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_4
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_4 CONFIG_SYS_I2C_OCTEONTX_SPEED_3
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_5
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_5 CONFIG_SYS_I2C_OCTEONTX_SPEED_4
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_6
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_6 CONFIG_SYS_I2C_OCTEONTX_SPEED_5
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_7
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_7 CONFIG_SYS_I2C_OCTEONTX_SPEED_6
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_8
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_8 CONFIG_SYS_I2C_OCTEONTX_SPEED_7
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_9
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_9 CONFIG_SYS_I2C_OCTEONTX_SPEED_8
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_10
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_10 CONFIG_SYS_I2C_OCTEONTX_SPEED_9
> +#endif
> +
> +#ifndef CONFIG_SYS_I2C_OCTEONTX_SPEED_11
> +# define CONFIG_SYS_I2C_OCTEONTX_SPEED_11 CONFIG_SYS_I2C_OCTEONTX_SPEED_10
> +#endif
Drop all of that and use device tree clock-frequency property
> +
> +#define TWSI_SW_TWSI 0x1000
> +#define TWSI_TWSI_SW 0x1008
> +#define TWSI_INT 0x1010
> +#define TWSI_SW_TWSI_EXT 0x1018
Should this be a struct variable for a register set?
> +
> +union twsx_sw_twsi {
comment
> + u64 u;
Not clear why this is a union at all?
> + struct {
> + u64 data:32;
> + u64 eop_ia:3;
> + u64 ia:5;
> + u64 addr:10;
> + u64 scr:2;
> + u64 size:3;
> + u64 sovr:1;
> + u64 r:1;
> + u64 op:4;
> + u64 eia:1;
> + u64 slonly:1;
> + u64 v:1;
> + } s;
> +};
> +
> +union twsx_sw_twsi_ext {
> + u64 u;
> + struct {
> + u64 data:32;
> + u64 ia:8;
> + u64 rsvd:24;
> + } s;
> +};
> +
> +union twsx_int {
Again, comment, and why a union?
> + u64 u;
> + struct {
> + u64 st_int:1; /** TWSX_SW_TWSI register update int */
> + u64 ts_int:1; /** TWSX_TWSI_SW register update int */
> + u64 core_int:1; /** TWSI core interrupt, ignored for HLC */
> + u64 rsvd1:5; /** Reserved */
> + u64 sda_ovr:1; /** SDA testing override */
> + u64 scl_ovr:1; /** SCL testing override */
> + u64 sda:1; /** SDA signal */
> + u64 scl:1; /** SCL signal */
> + u64 rsvd2:52; /** Reserved */
> + } s;
> +};
> +
> +enum {
> + TWSI_OP_WRITE = 0,
> + TWSI_OP_READ = 1,
> +};
> +
> +enum {
> + TWSI_EOP_SLAVE_ADDR = 0,
> + TWSI_EOP_CLK_CTL = 3,
> + TWSI_SW_EOP_IA = 6,
> +};
> +
> +enum {
> + TWSI_SLAVEADD = 0,
> + TWSI_DATA = 1,
> + TWSI_CTL = 2,
> + TWSI_CLKCTL = 3,
> + TWSI_STAT = 3,
> + TWSI_SLAVEADD_EXT = 4,
> + TWSI_RST = 7,
> +};
> +
> +enum {
> + TWSI_CTL_AAK = BIT(2),
> + TWSI_CTL_IFLG = BIT(3),
> + TWSI_CTL_STP = BIT(4),
> + TWSI_CTL_STA = BIT(5),
> + TWSI_CTL_ENAB = BIT(6),
> + TWSI_CTL_CE = BIT(7),
> +};
> +
> +enum {
> + /** Bus error */
> + TWSI_STAT_BUS_ERROR = 0x00,
> + /** Start condition transmitted */
> + TWSI_STAT_START = 0x08,
> + /** Repeat start condition transmitted */
> + TWSI_STAT_RSTART = 0x10,
> + /** Address + write bit transmitted, ACK received */
> + TWSI_STAT_TXADDR_ACK = 0x18,
> + /** Address + write bit transmitted, /ACK received */
> + TWSI_STAT_TXADDR_NAK = 0x20,
> + /** Data byte transmitted in master mode, ACK received */
> + TWSI_STAT_TXDATA_ACK = 0x28,
> + /** Data byte transmitted in master mode, ACK received */
> + TWSI_STAT_TXDATA_NAK = 0x30,
> + /** Arbitration lost in address or data byte */
> + TWSI_STAT_TX_ARB_LOST = 0x38,
> + /** Address + read bit transmitted, ACK received */
> + TWSI_STAT_RXADDR_ACK = 0x40,
> + /** Address + read bit transmitted, /ACK received */
> + TWSI_STAT_RXADDR_NAK = 0x48,
> + /** Data byte received in master mode, ACK transmitted */
> + TWSI_STAT_RXDATA_ACK_SENT = 0x50,
> + /** Data byte received, NACK transmitted */
> + TWSI_STAT_RXDATA_NAK_SENT = 0x58,
> + /** Slave address received, sent ACK */
> + TWSI_STAT_SLAVE_RXADDR_ACK = 0x60,
> + /**
> + * Arbitration lost in address as master, slave address + write bit
> + * received, ACK transmitted
> + */
> + TWSI_STAT_TX_ACK_ARB_LOST = 0x68,
> + /** General call address received, ACK transmitted */
> + TWSI_STAT_RX_GEN_ADDR_ACK = 0x70,
> + /**
> + * Arbitration lost in address as master, general call address
> + * received, ACK transmitted
> + */
> + TWSI_STAT_RX_GEN_ADDR_ARB_LOST = 0x78,
> + /** Data byte received after slave address received, ACK transmitted */
> + TWSI_STAT_SLAVE_RXDATA_ACK = 0x80,
> + /** Data byte received after slave address received, /ACK transmitted */
> + TWSI_STAT_SLAVE_RXDATA_NAK = 0x88,
> + /**
> + * Data byte received after general call address received, ACK
> + * transmitted
> + */
> + TWSI_STAT_GEN_RXADDR_ACK = 0x90,
> + /**
> + * Data byte received after general call address received, /ACK
> + * transmitted
> + */
> + TWSI_STAT_GEN_RXADDR_NAK = 0x98,
> + /** STOP or repeated START condition received in slave mode */
> + TWSI_STAT_STOP_MULTI_START = 0xA0,
> + /** Slave address + read bit received, ACK transmitted */
> + TWSI_STAT_SLAVE_RXADDR2_ACK = 0xA8,
Lower-case hex
> + /**
/** is for functions and structs as I understand it, not for ordinary comments.
> + * Arbitration lost in address as master, slave address + read bit
> + * received, ACK transmitted
> + */
> + TWSI_STAT_RXDATA_ACK_ARB_LOST = 0xB0,
> + /** Data byte transmitted in slave mode, ACK received */
> + TWSI_STAT_SLAVE_TXDATA_ACK = 0xB8,
> + /** Data byte transmitted in slave mode, /ACK received */
> + TWSI_STAT_SLAVE_TXDATA_NAK = 0xC0,
> + /** Last byte transmitted in slave mode, ACK received */
> + TWSI_STAT_SLAVE_TXDATA_END_ACK = 0xC8,
> + /** Second address byte + write bit transmitted, ACK received */
> + TWSI_STAT_TXADDR2DATA_ACK = 0xD0,
> + /** Second address byte + write bit transmitted, /ACK received */
> + TWSI_STAT_TXADDR2DATA_NAK = 0xD8,
> + /** No relevant status information */
> + TWSI_STAT_IDLE = 0xF8
> +};
> +
> +struct octeontx_twsi {
Please add missing comments to structs and functions globally.
> + int id;
> + int speed;
> + void *baseaddr;
> +};
> +
> +/** Array of bus speeds */
> +static unsigned int speeds[] = {
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_0,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_1,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_2,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_3,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_4,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_5,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_6,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_7,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_8,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_9,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_10,
> + CONFIG_SYS_I2C_OCTEONTX_SPEED_11,
> +};
> +
> +/** Last i2c id assigned */
> +static int last_id;
Cannot declare statics in a driver. Move it to a private structure.
> +
> +static void twsi_unblock(void *baseaddr);
> +static int twsi_stop(void *baseaddr);
> +
> +/**
> + * Converts the i2c status to a meaningful string
> + *
> + * @param status status to convert
> + *
> + * @return string representation of the status
> + */
> +static const char *twsi_i2c_status_str(uint8_t status)
> +{
> + switch (status) {
> + case TWSI_STAT_BUS_ERROR:
Wow this is a lot of string data. I suppose you have no need for I2C in SPL?
> + return "Bus error";
> + case TWSI_STAT_START:
> + return "START condition transmitted";
> + case TWSI_STAT_RSTART:
> + return "Repeated START condition transmitted";
> + case TWSI_STAT_TXADDR_ACK:
> + return "Address + write bit transmitted, ACK received";
> + case TWSI_STAT_TXADDR_NAK:
> + return "Address + write bit transmitted, NAK received";
> + case TWSI_STAT_TXDATA_ACK:
> + return "Data byte transmitted in master mode, ACK received";
> + case TWSI_STAT_TXDATA_NAK:
> + return "Data byte transmitted in master mode, NAK received";
> + case TWSI_STAT_TX_ARB_LOST:
> + return "Arbitration lost in address or data byte";
> + case TWSI_STAT_RXADDR_ACK:
> + return "Address + read bit transmitted, ACK received";
> + case TWSI_STAT_RXADDR_NAK:
> + return "Address + read bit transmitted, NAK received";
> + case TWSI_STAT_RXDATA_ACK_SENT:
> + return "Data byte received in master mode, ACK transmitted";
> + case TWSI_STAT_RXDATA_NAK_SENT:
> + return "Data byte received in master mode, NAK transmitted";
> + case TWSI_STAT_SLAVE_RXADDR_ACK:
> + return "Slave address + write bit received, ACK transmitted";
> + case TWSI_STAT_TX_ACK_ARB_LOST:
> + return "Arbitration lost in address as master, slave address + write bit received, ACK transmitted";
> + case TWSI_STAT_RX_GEN_ADDR_ACK:
> + return "General call address received, ACK transmitted";
> + case TWSI_STAT_RX_GEN_ADDR_ARB_LOST:
> + return "Arbitration lost in address as master, general call address received, ACK transmitted";
> + case TWSI_STAT_SLAVE_RXDATA_ACK:
> + return "Data byte received after slave address received, ACK transmitted";
> + case TWSI_STAT_SLAVE_RXDATA_NAK:
> + return "Data byte received after slave address received, NAK transmitted";
> + case TWSI_STAT_GEN_RXADDR_ACK:
> + return "Data byte received after general call address received, ACK transmitted";
> + case TWSI_STAT_GEN_RXADDR_NAK:
> + return "Data byte received after general call address received, NAK transmitted";
> + case TWSI_STAT_STOP_MULTI_START:
> + return "STOP or repeated START condition received in slave mode";
> + case TWSI_STAT_SLAVE_RXADDR2_ACK:
> + return "Slave address + read bit received, ACK transmitted";
> + case TWSI_STAT_RXDATA_ACK_ARB_LOST:
> + return "Arbitration lost in address as master, slave address + read bit received, ACK transmitted";
> + case TWSI_STAT_SLAVE_TXDATA_ACK:
> + return "Data byte transmitted in slave mode, ACK received";
> + case TWSI_STAT_SLAVE_TXDATA_NAK:
> + return "Data byte transmitted in slave mode, NAK received";
> + case TWSI_STAT_SLAVE_TXDATA_END_ACK:
> + return "Last byte transmitted in slave mode, ACK received";
> + case TWSI_STAT_TXADDR2DATA_ACK:
> + return "Second address byte + write bit transmitted, ACK received";
> + case TWSI_STAT_TXADDR2DATA_NAK:
> + return "Second address byte + write bit transmitted, NAK received";
> + case TWSI_STAT_IDLE:
> + return "Idle";
> + default:
> + return "Unknown status code";
> + }
> +}
> +
> +/**
> + * Returns true if we lost arbitration
> + *
> + * @param code status code
> + * @param final_read true if this is the final read operation
> + *
> + * @return true if arbitration has been lost, false if it hasn't been lost.
> + */
> +static int twsi_i2c_lost_arb(u8 code, int final_read)
> +{
> + switch (code) {
> + /* Arbitration lost */
> + case TWSI_STAT_TX_ARB_LOST:
> + case TWSI_STAT_TX_ACK_ARB_LOST:
> + case TWSI_STAT_RX_GEN_ADDR_ARB_LOST:
> + case TWSI_STAT_RXDATA_ACK_ARB_LOST:
> + return -EAGAIN;
> +
> + /* Being addressed as slave, should back off and listen */
> + case TWSI_STAT_SLAVE_RXADDR_ACK:
> + case TWSI_STAT_RX_GEN_ADDR_ACK:
> + case TWSI_STAT_GEN_RXADDR_ACK:
> + case TWSI_STAT_GEN_RXADDR_NAK:
> + return -EIO;
> +
> + /* Core busy as slave */
> + case TWSI_STAT_SLAVE_RXDATA_ACK:
> + case TWSI_STAT_SLAVE_RXDATA_NAK:
> + case TWSI_STAT_STOP_MULTI_START:
> + case TWSI_STAT_SLAVE_RXADDR2_ACK:
> + case TWSI_STAT_SLAVE_TXDATA_ACK:
> + case TWSI_STAT_SLAVE_TXDATA_NAK:
> + case TWSI_STAT_SLAVE_TXDATA_END_ACK:
> + return -EIO;
> +
> + /* Ack allowed on pre-terminal bytes only */
> + case TWSI_STAT_RXDATA_ACK_SENT:
> + if (!final_read)
> + return 0;
> + return -EAGAIN;
> +
> + /* NAK allowed on terminal byte only */
> + case TWSI_STAT_RXDATA_NAK_SENT:
> + if (!final_read)
> + return 0;
> + return -EAGAIN;
> +
> + case TWSI_STAT_TXDATA_NAK:
> + case TWSI_STAT_TXADDR_NAK:
> + case TWSI_STAT_RXADDR_NAK:
> + case TWSI_STAT_TXADDR2DATA_NAK:
> + return -EAGAIN;
> + }
> + return 0;
> +}
> +
> +#define RST_BOOT_PNR_MUL(val) (((val) >> 33) & 0x1F)
> +
> +/**
> + * Writes to the MIO_TWS(0..5)_SW_TWSI register
> + *
> + * @param baseaddr Base address of i2c registers
> + * @param sw_twsi value to write
> + *
Drop blank line (globally)
> + * @return 0 for success, otherwise error
> + */
> +static u64 twsi_write_sw(void *baseaddr, union twsx_sw_twsi sw_twsi)
> +{
> + unsigned long start = get_timer(0);
> +
> + sw_twsi.s.r = 0;
This is really cryptic. Please get rid of single-char members, and try
to drop the units
> + sw_twsi.s.v = 1;
> +
> + debug("%s(%p, 0x%llx)\n", __func__, baseaddr, sw_twsi.u);
> + writeq(sw_twsi.u, baseaddr + TWSI_SW_TWSI);
> + do {
> + sw_twsi.u = readq(baseaddr + TWSI_SW_TWSI);
> + } while (sw_twsi.s.v != 0 && get_timer(start) < 50);
> +
> + if (sw_twsi.s.v)
> + debug("%s: timed out\n", __func__);
> + return sw_twsi.u;
> +}
> +
> +/**
> + * Reads the MIO_TWS(0..5)_SW_TWSI register
> + *
> + * @param baseaddr Base address of i2c registers
> + * @param sw_twsi value for eia and op, etc. to read
> + *
> + * @return value of the register
> + */
> +static u64 twsi_read_sw(void *baseaddr, union twsx_sw_twsi sw_twsi)
> +{
> + unsigned long start = get_timer(0);
> +
> + sw_twsi.s.r = 1;
> + sw_twsi.s.v = 1;
> +
> + debug("%s(%p, 0x%llx)\n", __func__, baseaddr, sw_twsi.u);
> + writeq(sw_twsi.u, baseaddr + TWSI_SW_TWSI);
> +
> + do {
> + sw_twsi.u = readq(baseaddr + TWSI_SW_TWSI);
> + } while (sw_twsi.s.v != 0 && get_timer(start) < 50);
Drop != 0
But I think patman will warn about this anyway.
> +
> + if (sw_twsi.s.v)
> + debug("%s: Error writing 0x%llx\n", __func__, sw_twsi.u);
> +
> + debug("%s: Returning 0x%llx\n", __func__, sw_twsi.u);
> + return sw_twsi.u;
> +}
> +
> +/**
> + * Write control register
> + *
> + * @param baseaddr Base address for i2c registers
> + * @param data data to write
> + */
> +static void twsi_write_ctl(void *baseaddr, u8 data)
> +{
> + union twsx_sw_twsi twsi_sw;
> +
> + debug("%s(%p, 0x%x)\n", __func__, baseaddr, data);
> + twsi_sw.u = 0;
> +
> + twsi_sw.s.op = TWSI_SW_EOP_IA;
> + twsi_sw.s.eop_ia = TWSI_CTL;
> + twsi_sw.s.data = data;
> +
> + twsi_write_sw(baseaddr, twsi_sw);
> +}
> +
> +/**
> + * Reads the TWSI Control Register
> + *
> + * @param[in] baseaddr Base address for i2c
> + *
> + * @return 8-bit TWSI control register
> + */
> +static u32 twsi_read_ctl(void *baseaddr)
> +{
> + union twsx_sw_twsi sw_twsi;
> +
> + sw_twsi.u = 0;
> + sw_twsi.s.op = TWSI_SW_EOP_IA;
> + sw_twsi.s.eop_ia = TWSI_CTL;
> +
> + sw_twsi.u = twsi_read_sw(baseaddr, sw_twsi);
> + debug("%s(%p): 0x%x\n", __func__, baseaddr, sw_twsi.s.data);
> + return sw_twsi.s.data;
What is causing sw_twsi.s.data to be set, here?
> +}
> +
> +/**
> + * Read i2c status register
> + *
> + * @param baseaddr Base address of i2c registers
> + *
> + * @return value of status register
> + */
> +static u8 twsi_read_status(void *baseaddr)
> +{
> + union twsx_sw_twsi twsi_sw;
> +
> + twsi_sw.u = 0;
> + twsi_sw.s.op = TWSI_SW_EOP_IA;
> + twsi_sw.s.eop_ia = TWSI_STAT;
> +
> + return twsi_read_sw(baseaddr, twsi_sw);
> +}
> +
> +/**
> + * Waits for an i2c operation to complete
> + *
> + * @param baseaddr Base address of registers
> + *
> + * @return 0 for success, 1 if timeout
> + */
> +static int twsi_wait(void *baseaddr)
> +{
> + unsigned long start = get_timer(0);
> + u8 twsi_ctl;
> +
> + debug("%s(%p)\n", __func__, baseaddr);
> + do {
> + twsi_ctl = twsi_read_ctl(baseaddr);
> + twsi_ctl &= TWSI_CTL_IFLG;
> + } while (!twsi_ctl && get_timer(start) < 50);
> +
> + debug(" return: %u\n", !twsi_ctl);
> + return !twsi_ctl;
> +}
> +
> +/**
> + * Unsticks the i2c bus
> + *
> + * @param baseaddr base address of registers
> + */
> +static int twsi_start_unstick(void *baseaddr)
> +{
> + twsi_stop(baseaddr);
> +
no need for this blank line
> + twsi_unblock(baseaddr);
> +
> + return 0;
> +}
> +
> +/**
> + * Sends an i2c start condition
> + *
> + * @param baseaddr base address of registers
> + *
> + * @return 0 for success, otherwise error
> + */
> +static int twsi_start(void *baseaddr)
> +{
> + int result;
> + u8 stat;
> +
> + debug("%s(%p)\n", __func__, baseaddr);
> + twsi_write_ctl(baseaddr, TWSI_CTL_STA | TWSI_CTL_ENAB);
> + result = twsi_wait(baseaddr);
> + if (result) {
> + stat = twsi_read_status(baseaddr);
> + debug("%s: result: 0x%x, status: 0x%x\n", __func__,
> + result, stat);
> + switch (stat) {
> + case TWSI_STAT_START:
> + case TWSI_STAT_RSTART:
> + return 0;
> + case TWSI_STAT_RXADDR_ACK:
> + default:
> + return twsi_start_unstick(baseaddr);
> + }
> + }
> + debug("%s: success\n", __func__);
> + return 0;
> +}
> +
> +/**
> + * Sends an i2c stop condition
> + *
> + * @param baseaddr register base address
> + *
> + * @return 0 for success, -1 if error
> + */
> +static int twsi_stop(void *baseaddr)
> +{
> + u8 stat;
uint
> +
> + twsi_write_ctl(baseaddr, TWSI_CTL_STP | TWSI_CTL_ENAB);
> +
> + stat = twsi_read_status(baseaddr);
> + if (stat != TWSI_STAT_IDLE) {
> + debug("%s: Bad status on bus@%p\n", __func__, baseaddr);
> + return -1;
> + }
> + return 0;
> +}
> +
> +/**
> + * Writes data to the i2c bus
> + *
> + * @param baseraddr register base address
BTW we tend to use @baseaddr as the style now
> + * @param slave_addr address of slave to write to
> + * @param buffer Pointer to buffer to write
> + * @param length Number of bytes in buffer to write
> + *
> + * @return 0 for success, otherwise error
> + */
> +static int twsi_write_data(void *baseaddr, u8 slave_addr,
> + u8 *buffer, unsigned int length)
> +{
> + union twsx_sw_twsi twsi_sw;
> + unsigned int curr = 0;
> + int result;
> +
> + debug("%s(%p, 0x%x, %p, 0x%x)\n", __func__, baseaddr, slave_addr,
> + buffer, length);
> + result = twsi_start(baseaddr);
> + if (result) {
> + debug("%s: Could not start BUS transaction\n", __func__);
> + return -1;
> + }
> +
> + result = twsi_wait(baseaddr);
> + if (result) {
> + debug("%s: wait failed\n", __func__);
> + return result;
> + }
> +
> + twsi_sw.u = 0;
> + twsi_sw.s.op = TWSI_SW_EOP_IA;
> + twsi_sw.s.eop_ia = TWSI_DATA;
> + twsi_sw.s.data = (u32)(slave_addr << 1) | TWSI_OP_WRITE;
> +
> + twsi_write_sw(baseaddr, twsi_sw);
> + twsi_write_ctl(baseaddr, TWSI_CTL_ENAB);
> +
> + debug("%s: Waiting\n", __func__);
> + result = twsi_wait(baseaddr);
How about 'ret' instead of result? It is shorted and easier to read,
and that is what driver model uses.
> + if (result) {
> + debug("%s: Timed out writing slave address 0x%x to target\n",
> + __func__, slave_addr);
> + return result;
> + }
> + result = twsi_read_status(baseaddr);
> + debug("%s: status: (%d) %s\n", __func__, result,
> + twsi_i2c_status_str(result));
> + if (result != TWSI_STAT_TXADDR_ACK) {
> + debug("%s: status: (%d) %s\n", __func__, result,
> + twsi_i2c_status_str(result));
> + twsi_stop(baseaddr);
> + return twsi_i2c_lost_arb(result, 0);
> + }
> +
> + while (curr < length) {
> + twsi_sw.u = 0;
> + twsi_sw.s.op = TWSI_SW_EOP_IA;
> + twsi_sw.s.eop_ia = TWSI_DATA;
> + twsi_sw.s.data = buffer[curr++];
> +
> + twsi_write_sw(baseaddr, twsi_sw);
> + twsi_write_ctl(baseaddr, TWSI_CTL_ENAB);
> +
> + debug("%s: Writing 0x%x\n", __func__, twsi_sw.s.data);
> +
> + result = twsi_wait(baseaddr);
> + if (result) {
> + debug("%s: Timed out writing data to 0x%x\n",
> + __func__, slave_addr);
> + return result;
> + }
> + result = twsi_read_status(baseaddr);
> + debug("%s: status: (%d) %s\n", __func__, result,
> + twsi_i2c_status_str(result));
> + }
> +
> + debug("%s: Stopping\n", __func__);
> + return twsi_stop(baseaddr);
> +}
> +
> +/**
> + * Manually clear the I2C bus and send a stop
> + */
> +static void twsi_unblock(void *baseaddr)
> +{
> + int i;
> + union twsx_int int_reg;
> +
> + int_reg.u = 0;
> + for (i = 0; i < 9; i++) {
> + int_reg.s.scl_ovr = 0;
> + writeq(int_reg.u, baseaddr + TWSI_INT);
> + udelay(5);
Why do we need udelay()? Please at least add a comment.
> + int_reg.s.scl_ovr = 1;
This is way too cryptic - I think you need to rethink the unions.
> + writeq(int_reg.u, baseaddr + TWSI_INT);
> + udelay(5);
> + }
> + int_reg.s.sda_ovr = 1;
> + writeq(int_reg.u, baseaddr + TWSI_INT);
> + udelay(5);
> + int_reg.s.scl_ovr = 0;
> + writeq(int_reg.u, baseaddr + TWSI_INT);
> + udelay(5);
> + int_reg.u = 0;
> + writeq(int_reg.u, baseaddr + TWSI_INT);
> + udelay(5);
> +}
> +
> +/**
> + * Performs a read transaction on the i2c bus
> + *
> + * @param baseaddr Base address of twsi registers
> + * @param slave_addr i2c bus address to read from
> + * @param buffer buffer to read into
> + * @param length number of bytes to read
> + *
> + * @return 0 for success, otherwise error
> + */
> +static int twsi_read_data(void *baseaddr, u8 slave_addr,
> + u8 *buffer, unsigned int length)
> +{
> + union twsx_sw_twsi twsi_sw;
> + unsigned int curr = 0;
> + int result;
> +
> + debug("%s(%p, 0x%x, %p, %u)\n", __func__, baseaddr, slave_addr,
> + buffer, length);
> + result = twsi_start(baseaddr);
> + if (result) {
> + debug("%s: start failed\n", __func__);
> + return result;
> + }
> +
> + result = twsi_wait(baseaddr);
> + if (result) {
> + debug("%s: wait failed\n", __func__);
> + return result;
> + }
> +
> + twsi_sw.u = 0;
> + twsi_sw.s.op = TWSI_SW_EOP_IA;
> + twsi_sw.s.eop_ia = TWSI_DATA;
> +
> + twsi_sw.s.data = (u32)(slave_addr << 1) | TWSI_OP_READ;
> +
> + twsi_write_sw(baseaddr, twsi_sw);
> + twsi_write_ctl(baseaddr, TWSI_CTL_ENAB);
> +
> + result = twsi_wait(baseaddr);
> + if (result) {
> + debug("%s: waiting for sending addr failed\n", __func__);
> + return result;
> + }
> +
> + result = twsi_read_status(baseaddr);
> + debug("%s: status: (%d) %s\n", __func__, result,
> + twsi_i2c_status_str(result));
> + if (result != TWSI_STAT_RXADDR_ACK) {
> + debug("%s: status: (%d) %s\n", __func__, result,
> + twsi_i2c_status_str(result));
> + twsi_stop(baseaddr);
> + return twsi_i2c_lost_arb(result, 0);
> + }
> +
> + while (curr < length) {
> + twsi_write_ctl(baseaddr, TWSI_CTL_ENAB |
> + ((curr < length - 1) ? TWSI_CTL_AAK : 0));
> +
> + result = twsi_wait(baseaddr);
> + if (result) {
> + debug("%s: waiting for data failed\n", __func__);
> + return result;
> + }
> +
> + twsi_sw.u = twsi_read_sw(baseaddr, twsi_sw);
> + buffer[curr++] = twsi_sw.s.data;
> + }
> +
> + twsi_stop(baseaddr);
> +
> + return 0;
> +}
> +
> +static void twsi_calc_div(unsigned int speed, int *m_div, int *n_div)
> +{
> + int io_clock_hz;
> + int tclk, fsamp;
> + int ndiv, mdiv;
> +
> +#if defined(CONFIG_ARCH_OCTEONTX)
> + io_clock_hz = octeontx_get_io_clock();
Please use a clock driver.
> + tclk = io_clock_hz / 2 * (TWSI_THP + 1);
> +#elif defined(CONFIG_ARCH_OCTEONTX2)
> + /* Refclk src in mode register defaults to 100MHz clock */
> + io_clock_hz = 100000000; /* 100 Mhz */
> + tclk = io_clock_hz / (TWSI_THP + 2);
> +#endif
> + debug("%s( io_clock %u)\n", __func__, io_clock_hz);
> +
> + /* Set the TWSI clock to a conservative TWSI_BUS_FREQ.
But what does this have to do with TWSI_BUS_FREQ?
> + * Compute the clocks M divider based on the SCLK.
> + *
> + * TWSI freq = (core freq) / (10 x (M+1) x 2 * (thp+1) x 2^N)
> + * M = ((core freq) / (10 x (TWSI freq) x 2 * (thp+1) x 2^N)) - 1
> + *
> + * For OcteonTX2 -
> + * TWSI freq = (core freq) / (10 x (M+1) x (thp+2) x 2^N)
> + * M = ((core freq) / (10 x (TWSI freq) x (thp+2) x 2^N)) - 1
> + */
> + for (ndiv = 0; ndiv < 8; ndiv++) {
> + fsamp = tclk / (1 << ndiv);
> + mdiv = fsamp / speed / 10;
> + mdiv -= 1;
> + if (mdiv < 16)
> + break;
> + }
> + *m_div = mdiv;
> + *n_div = ndiv;
> +}
> +
> +static int twsi_init(void *baseaddr, unsigned int speed, int slaveaddr)
> +{
> + int n_div, m_div;
> + union twsx_sw_twsi sw_twsi;
> +
> + debug("%s(%p, %u, 0x%x)\n", __func__, baseaddr, speed, slaveaddr);
> +
> + twsi_calc_div(speed, &m_div, &n_div);
> + if (m_div >= 16)
> + return -1;
> +
> + sw_twsi.u = 0;
> + sw_twsi.s.v = 1; /* Clear valid bit */
> + sw_twsi.s.op = 0x6; /* See EOP field */
> + sw_twsi.s.r = 0; /* Select CLKCTL when R = 0 */
> + sw_twsi.s.eop_ia = 3; /* R=0 selects CLKCTL, R=1 selects STAT */
> + sw_twsi.s.data = ((m_div & 0xf) << 3) | ((n_div & 0x7) << 0);
This is just awful :-)
Can you pass some parameters to your function instead, perhaps with some flags?
> +
> + twsi_write_sw(baseaddr, sw_twsi);
> + /* Only init non-slave ports */
> + debug("%s: Writing 0x%llx to sw_twsi, m_div: 0x%x, n_div: 0x%x\n",
> + __func__, sw_twsi.u, m_div, n_div);
> +
> + sw_twsi.u = 0;
> + sw_twsi.s.v = 1;
> + sw_twsi.s.op = TWSI_SW_EOP_IA;
> + sw_twsi.s.r = 0;
> + sw_twsi.s.eop_ia = 0;
> + sw_twsi.s.data = slaveaddr << 1;
> +
> + twsi_write_sw(baseaddr, sw_twsi);
> +
> + /* Set slave address */
> + sw_twsi.u = 0;
> + sw_twsi.s.v = 1;
> + sw_twsi.s.op = TWSI_SW_EOP_IA;
> + sw_twsi.s.r = 0;
> + sw_twsi.s.eop_ia = TWSI_EOP_SLAVE_ADDR;
> + sw_twsi.s.data = slaveaddr;
> + twsi_write_sw(baseaddr, sw_twsi);
> +
> + return 0;
> +}
> +
> +/**
> + * Transfers data over the i2c bus
> + *
> + * @param bus i2c bus to transfer data over
> + * @param msg Array of i2c messages
> + * @param nmsgs Number of messages to send/receive
> + *
> + * @return 0 for success, otherwise error
> + */
> +static int octeontx_i2c_xfer(struct udevice *bus, struct i2c_msg *msg,
> + int nmsgs)
> +{
> + struct octeontx_twsi *twsi = dev_get_priv(bus);
> + int result;
> +
> + debug("%s: %d messages\n", __func__, nmsgs);
> + for (; nmsgs > 0; nmsgs--, msg++) {
for (i = 0; i < nmsgs; i++, msg++)
> + debug("%s: chip=0x%x, len=0x%x\n", __func__, msg->addr,
> + msg->len);
> + if (msg->flags & I2C_M_RD) {
> + debug("%s: Reading data\n", __func__);
> + result = twsi_read_data(twsi->baseaddr, msg->addr,
> + msg->buf, msg->len);
> + } else {
> + debug("%s: Writing data\n", __func__);
> + result = twsi_write_data(twsi->baseaddr, msg->addr,
> + msg->buf, msg->len);
> + }
> + if (result) {
> + debug("%s: error sending\n", __func__);
> + return -EREMOTEIO;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int octeontx_i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
> +{
> + struct octeontx_twsi *twsi = dev_get_priv(bus);
> + int m_div, n_div;
> + union twsx_sw_twsi sw_twsi;
> + void *baseaddr = twsi->baseaddr;
> +
> + debug("%s(%p, %u)\n", __func__, bus, speed);
> +
> + twsi_calc_div(speed, &m_div, &n_div);
> + if (m_div >= 16)
> + return -1;
> +
> + sw_twsi.u = 0;
> + sw_twsi.s.v = 1; /* Clear valid bit */
> + sw_twsi.s.op = TWSI_SW_EOP_IA; /* See EOP field */
> + sw_twsi.s.r = 0; /* Select CLKCTL when R = 0 */
> + sw_twsi.s.eop_ia = TWSI_CLKCTL; /* R=0 selects CLKCTL, R=1 selects STAT */
> + sw_twsi.s.data = ((m_div & 0xf) << 3) | ((n_div & 0x7) << 0);
> +
> + /* Only init non-slave ports */
> + writeq(sw_twsi.u, baseaddr + TWSI_SW_TWSI);
> +
> + debug("%s: Wrote 0x%llx to sw_twsi\n", __func__, sw_twsi.u);
> + return 0;
> +}
> +
> +static int octeontx_pci_i2c_probe(struct udevice *dev)
> +{
> + struct octeontx_twsi *twsi = dev_get_priv(dev);
> + pci_dev_t bdf = dm_pci_get_bdf(dev);
> +
> + debug("TWSI PCI device: %x\n", bdf);
> + dev->req_seq = PCI_FUNC(bdf);
> +
> + twsi->baseaddr = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> + PCI_REGION_MEM);
> + twsi->id = last_id++;
> +
> + debug("TWSI bus %d at %p\n", dev->seq, twsi->baseaddr);
> +
> + return twsi_init(twsi->baseaddr,
> + twsi->id < ARRAY_SIZE(speeds) ?
> + speeds[twsi->id] : CONFIG_SYS_I2C_SPEED,
> + CONFIG_SYS_I2C_OCTEONTX_SLAVE_ADDR);
> +}
> +
> +static const struct dm_i2c_ops octeontx_i2c_ops = {
> + .xfer = octeontx_i2c_xfer,
> + .set_bus_speed = octeontx_i2c_set_bus_speed,
> +};
> +
> +static const struct udevice_id octeontx_i2c_ids[] = {
> + { .compatible = "cavium,thunder-8890-twsi" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(octeontx_pci_twsi) = {
> + .name = "i2c_octeontx",
> + .id = UCLASS_I2C,
> + .of_match = octeontx_i2c_ids,
> + .probe = octeontx_pci_i2c_probe,
> + .priv_auto_alloc_size = sizeof(struct octeontx_twsi),
> + .ops = &octeontx_i2c_ops,
> +};
> +
> +static struct pci_device_id octeontx_twsi_supported[] = {
> + { PCI_VDEVICE(CAVIUM, 0xa012) },
> + { },
> +};
> +
> +U_BOOT_PCI_DEVICE(octeontx_pci_twsi, octeontx_twsi_supported);
> --
> 2.23.0
>
Regards,
Simon
More information about the U-Boot
mailing list