[v6,4/5] i2c: aspeed: added driver for Aspeed I2C

Added initial master support for Aspeed I2C controller. Supports
fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes for v2:
  - Added single module_init (multiple was breaking some builds).
Changes for v3:
  - Removed "bus" device tree param; now extracted from bus address offset
Changes for v4:
  - I2C adapter number is now generated dynamically unless specified in alias.
Changes for v5:
  - Removed irq_chip used to multiplex IRQ and replaced it with dummy_irq_chip
    along with some other IRQ cleanup.
  - Addressed comments from Cedric, and Vladimir, mostly stylistic things and
    using devm managed resources.
  - Increased max clock frequency before the bus is put in HighSpeed mode, as
    per Kachalov's comment.
Changes for v6:
  - No longer arbitrarily restrict bus to be slave xor master.
  - Pulled out "struct aspeed_i2c_controller" as a interrupt controller.
  - Pulled out slave support into its own commit.
  - Rewrote code that sets clock divider register because the original version
    set it incorrectly.
  - Rewrote the aspeed_i2c_master_irq handler because the old method of
    completing a completion in between restarts was too slow causing devices to
    misbehave.
  - Added support for I2C_M_RECV_LEN which I had incorrectly said was supported
    before.
  - Addressed other comments from Vladimir.
---
 drivers/i2c/busses/Kconfig      |  10 +
 drivers/i2c/busses/Makefile     |   1 +
 drivers/i2c/busses/i2c-aspeed.c | 610 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 621 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-aspeed.c

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> +/* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
> +#define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
> +#define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
> +#define ASPEED_I2CD_TIME_SCL_LOW_MASK                  GENMASK(15, 12)
> +#define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK             GENMASK(3, 0)
> +#define ASPEED_I2CD_TIME_SCL_REG_MAX                   GENMASK(3, 0)
> +/* 0x08 : I2CD Clock and AC Timing Control Register #2 */
> +#define ASPEED_NO_TIMEOUT_CTRL                         0

Those are slightly different between the 2400 and 2500, allowing
slightly more fine grained settings (faster base clock and thus higher
numbers in high/low counts).

I *think* that using the 2400 values as-is might work ok, at least
it does for 100kHz but I would double check.

I'll review the rest tomorrow.

Cheers,
Ben.

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> +       /* Set AC Timing */
> +       if (clk_freq / 1000 > 1000) {
> +               aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> +                                                     ASPEED_I2C_FUN_CTRL_REG) |
> +                               ASPEED_I2CD_M_HIGH_SPEED_EN |
> +                               ASPEED_I2CD_M_SDA_DRIVE_1T_EN |

s/ASPEED_I2CD_M_SDA_DRIVE_1T_EN/ASPEED_I2CD_M_SCL_DRIVE_1T_EN/

(and in the definition too)

> +                               ASPEED_I2CD_SDA_DRIVE_1T_EN,
> +                               ASPEED_I2C_FUN_CTRL_REG);
> +
> +               aspeed_i2c_write(bus, 0x3, ASPEED_I2C_AC_TIMING_REG2);
> +               aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
> +                                ASPEED_I2C_AC_TIMING_REG1);
> +       } else {

I don't think that's right. AFAIK ASPEED_I2CD_M_HIGH_SPEED_EN is about
ignoring the timing register completely and going for full speed which
is a few Mhz (I forgot how much). At least from my (possibly incorrect)
reading of the spec and the SDK driver.

Or maybe that's what you intend by the above ? Anything above 1Mhz ?

I think there's a blurb somewhere that says that setting that bit makes
it ignore the timing register completely. The definition is:

<<
Enable High Speed master mode
0 : normal speed mode
1 : high speed mode (3.4Mbps)
High speed mode can only use buffer mode for transfer. And only master
mode supports speed switching capability
>>

The spec of the base clock field of the timing register also says

<<
When switch to High Speed (HS) mode, the divisor will be switch to 0 by
hardware automatically
>>

Note also that we aren't use buffer mode anyway so this can't work as-
is, we're using byte mode.

The other interesting question is what is the frequency threshold for
setting ASPEED_I2CD_M_SCL_DRIVE_1T_EN (and the SDA one) ? 

Those bits are somewhat orthogonal to ASPEED_I2CD_M_HIGH_SPEED_EN. They
make the device drive the signals for a clock when they go up to "speed
up" the rising edge more than a normal pull up would do.

If you have some fast devices, it would be interesting to scope the
signal see from what speed it becomes interesting to set the 1T enable
bits to speed up rising edges.

Cheers,
Ben.

>> +                               ASPEED_I2CD_M_HIGH_SPEED_EN |
>> +                               ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
>
> s/ASPEED_I2CD_M_SDA_DRIVE_1T_EN/ASPEED_I2CD_M_SCL_DRIVE_1T_EN/
>
> (and in the definition too)

Will fix.

>
>> +                               ASPEED_I2CD_SDA_DRIVE_1T_EN,
>> +                               ASPEED_I2C_FUN_CTRL_REG);
>> +
>> +               aspeed_i2c_write(bus, 0x3, ASPEED_I2C_AC_TIMING_REG2);
>> +               aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
>> +                                ASPEED_I2C_AC_TIMING_REG1);
>> +       } else {
>
> I don't think that's right. AFAIK ASPEED_I2CD_M_HIGH_SPEED_EN is about
> ignoring the timing register completely and going for full speed which
> is a few Mhz (I forgot how much). At least from my (possibly incorrect)
> reading of the spec and the SDK driver.
>
> Or maybe that's what you intend by the above ? Anything above 1Mhz ?
>
> I think there's a blurb somewhere that says that setting that bit makes
> it ignore the timing register completely. The definition is:
>
> <<
> Enable High Speed master mode
> 0 : normal speed mode
> 1 : high speed mode (3.4Mbps)
> High speed mode can only use buffer mode for transfer. And only master
> mode supports speed switching capability
>>>

Yeah, I was picking an arbitrary cutoff and 1MHz seemed reasonable in
part because in order to get above 1MHz you would set the divisor to 0
(1 << 0) anyway because you will only modify the SCL high and low time
for anything less than that. Also because that was the cutoff for fast
mode (as opposed to high speed).

>
> The spec of the base clock field of the timing register also says
>
> <<
> When switch to High Speed (HS) mode, the divisor will be switch to 0 by
> hardware automatically
>>>
>
> Note also that we aren't use buffer mode anyway so this can't work as-
> is, we're using byte mode.
>

Good catch. Yeah, I did not realize it. I should probably remove this
until that is supported then.

> The other interesting question is what is the frequency threshold for
> setting ASPEED_I2CD_M_SCL_DRIVE_1T_EN (and the SDA one) ?

I would guess that we should make them correspond to the cutoff for
high speed mode, or fast mode plus. Not really sure though, the
documentation is not clear on this (or a lot of other things :-P)

>
> Those bits are somewhat orthogonal to ASPEED_I2CD_M_HIGH_SPEED_EN. They
> make the device drive the signals for a clock when they go up to "speed
> up" the rising edge more than a normal pull up would do.
>
> If you have some fast devices, it would be interesting to scope the
> signal see from what speed it becomes interesting to set the 1T enable
> bits to speed up rising edges.

Agreed.

On Tue, Mar 28, 2017 at 3:42 PM, Brendan Higgins
<brendanhiggins@google.com> wrote:
> Added initial master support for Aspeed I2C controller. Supports
> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.

Mention that the driver supports byte at a time access only at this stage.

> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>

Looking good. I've given this a spin on ast2500 hardware and it worked for me.

I've got a bunch of nits below, and one bigger question about weather
we need internal locking in the driver, or if we can rely on the i2c
core for our locks.

> ---
>  drivers/i2c/busses/Kconfig      |  10 +
>  drivers/i2c/busses/Makefile     |   1 +
>  drivers/i2c/busses/i2c-aspeed.c | 610 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 621 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-aspeed.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 8adc0f1d7ad0..e5ea5641a874 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -326,6 +326,16 @@ config I2C_POWERMAC
>
>  comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>
> +config I2C_ASPEED
> +       tristate "Aspeed AST2xxx SoC I2C Controller"

Aspeed I2C Controller

> +       depends on ARCH_ASPEED
> +       help
> +         If you say yes to this option, support will be included for the
> +         Aspeed AST2xxx SoC I2C controller.

And again.

> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called i2c-aspeed.
> +
>  config I2C_AT91
>         tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
>         depends on ARCH_AT91

> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> new file mode 100644
> index 000000000000..04266acc6c46
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-aspeed.c

> +       spin_unlock_irqrestore(&bus->lock, flags);
> +
> +       return ret;
> +}
> +
> +static void do_start(struct aspeed_i2c_bus *bus)

aspeed_i2c_do_start

> +{
> +       u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
> +       struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> +       u8 slave_addr = msg->addr << 1;
> +
> +       bus->master_state = ASPEED_I2C_MASTER_START;
> +       bus->buf_index = 0;
> +
> +       if (msg->flags & I2C_M_RD) {
> +               slave_addr |= 1;
> +               command |= ASPEED_I2CD_M_RX_CMD;
> +               /* Need to let the hardware know to NACK after RX. */
> +               if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
> +                       command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> +       }
> +
> +       aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
> +       aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
> +}
> +
> +static void do_stop(struct aspeed_i2c_bus *bus)

aspeed_i2c_do_stop

> +{
> +       bus->master_state = ASPEED_I2C_MASTER_STOP;
> +       aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> +                        ASPEED_I2C_CMD_REG);
> +}

> +static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> +{
> +       struct aspeed_i2c_bus *bus;
> +       struct resource *res;
> +       int ret;
> +
> +       bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> +       if (!bus)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       bus->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(bus->base)) {
> +               dev_err(&pdev->dev, "failed to devm_ioremap_resource\n");

devm_ioremap_resource shows an error for you, please drop the dev_err here.

> +               return PTR_ERR(bus->base);
> +       }
> +
> +       bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +       ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
> +                              IRQF_SHARED, dev_name(&pdev->dev), bus);

Is this requesting an IRQ from your i2c-irq-controller? In which case
the IRQ won't be shared with any other driver, so you don't need to
set IRQF_SHARED.

> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to request interrupt\n");
> +               return ret;
> +       }
> +
> +       /* Initialize the I2C adapter */
> +       spin_lock_init(&bus->lock);

Do we need this lock at all?

The i2c core provides locking around operations on the bus. I was
browsing some of the other bus drivers and they do not have locking
inside of the driver (eg. i2c-at91.c). I also did a test of an earlier
version of this driver where I removed the locks, and it performed
correctly in my testing (http://patchwork.ozlabs.org/patch/731899/).

> +       init_completion(&bus->cmd_complete);
> +       bus->adap.owner = THIS_MODULE;
> +       bus->adap.retries = 0;
> +       bus->adap.timeout = 5 * HZ;
> +       bus->adap.algo = &aspeed_i2c_algo;
> +       bus->adap.algo_data = bus;
> +       bus->adap.dev.parent = &pdev->dev;
> +       bus->adap.dev.of_node = pdev->dev.of_node;
> +       snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed i2c");

> +static struct platform_driver aspeed_i2c_bus_driver = {
> +       .probe          = aspeed_i2c_probe_bus,
> +       .remove         = aspeed_i2c_remove_bus,
> +       .driver         = {
> +               .name           = "ast-i2c-bus",

aspeed-i2c-bus please.

> +               .of_match_table = aspeed_i2c_bus_of_table,
> +       },
> +};
> +module_platform_driver(aspeed_i2c_bus_driver);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> +MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.12.2.564.g063fe858b8-goog
>

Allright, I finally found some time for reviewing some of this
after splitting the ftgmac100 patch into 54 smaller ones :)

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:

 .../...

> +struct aspeed_i2c_bus {
> +	struct i2c_adapter		adap;
> +	struct device			*dev;
> +	void __iomem			*base;
> +	/* Synchronizes I/O mem access to base. */
> +	spinlock_t			lock;

I am not entirely convinced we need that lock. The i2c core will
take a mutex protecting all operations on the bus. So we only need
to synchronize between our "xfer" code and our interrupt handler.

This probably be done without a lock if we are careful. Not a huge
deal though as Aspeed SoC are currently not SMP so the lock compiles
down to not much unless you have all the debug crap enabled :-)

> +	struct completion		cmd_complete;
> +	int				irq;
> +	/* Transaction state. */
> +	enum aspeed_i2c_master_state	master_state;
> +	struct i2c_msg			*msgs;
> +	size_t				buf_index;
> +	size_t				msgs_index;
> +	size_t				msgs_size;
> +	bool				send_stop;
> +	int				cmd_err;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	struct i2c_client		*slave;
> +	enum aspeed_i2c_slave_state	slave_state;
> +#endif
> +};

Minor nit but the above should probably be in the slave patch no ?

> +static inline void aspeed_i2c_write(struct aspeed_i2c_bus *bus, u32
> val,
> +				    u32 reg)
> +{
> +	writel(val, bus->base + reg);
> +}
> +
> +static inline u32 aspeed_i2c_read(struct aspeed_i2c_bus *bus, u32
> reg)
> +{
> +	return readl(bus->base + reg);
> +}

Another very minor nit, I'm not certain those accessors are a big
win in code size and/or readability but keep them if you want.

> +static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> +{
> +	unsigned long time_left, flags;
> +	int ret = 0;
> +	u32 command;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +	command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
> +
> +	if (command & ASPEED_I2CD_SDA_LINE_STS) {
> +		/* Bus is idle: no recovery needed. */
> +		if (command & ASPEED_I2CD_SCL_LINE_STS)
> +			goto out;
> +		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> +			command);
> +
> +		aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> +				 ASPEED_I2C_CMD_REG);
> +		reinit_completion(&bus->cmd_complete);
> +		spin_unlock_irqrestore(&bus->lock, flags);

See my comment further down in master_xfer, do the reinit before sending
the command, even if currently the lock protects you, it's cleaner.

Now, I don't completely get how your interrupt handler deals with these
"message-less" completions. See the review of the interrupt handler.

> +
> +		time_left = wait_for_completion_timeout(
> +				&bus->cmd_complete, bus->adap.timeout);
> +
> +		spin_lock_irqsave(&bus->lock, flags);
> +		if (time_left == 0)
> +			ret = -ETIMEDOUT;
> +		else if (bus->cmd_err)
> +			ret = -EIO;
> +	/* Bus error. */
> +	} else {
> +		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> +			command);
> +
> +		aspeed_i2c_write(bus, ASPEED_I2CD_BUS_RECOVER_CMD,
> +				 ASPEED_I2C_CMD_REG);
> +		reinit_completion(&bus->cmd_complete);

Same comments as above.

> +		spin_unlock_irqrestore(&bus->lock, flags);
> +
> +		time_left = wait_for_completion_timeout(
> +				&bus->cmd_complete, bus->adap.timeout);
> +
> +		spin_lock_irqsave(&bus->lock, flags);
> +		if (time_left == 0)
> +			ret = -ETIMEDOUT;
> +		else if (bus->cmd_err)
> +			ret = -EIO;
> +		/* Recovery failed. */
> +		else if (!(aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
> +			   ASPEED_I2CD_SDA_LINE_STS))
> +			ret = -EIO;
> +	}

Some of those error states probably also warrant a reset of the controller,
I think aspeed does that in the SDK.

> +out:
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	return ret;
> +}
> +
> +static void do_start(struct aspeed_i2c_bus *bus)
> +{
> +	u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
> +	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> +	u8 slave_addr = msg->addr << 1;
> +
> +	bus->master_state = ASPEED_I2C_MASTER_START;
> +	bus->buf_index = 0;
> +
> +	if (msg->flags & I2C_M_RD) {
> +		slave_addr |= 1;
> +		command |= ASPEED_I2CD_M_RX_CMD;
> +		/* Need to let the hardware know to NACK after RX. */
> +		if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
> +			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> +	}


What about I2C_M_NOSTART ? 

Not that I've ever seen it used... ;-)

> +	aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
> +	aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
> +}
> +
> +static void do_stop(struct aspeed_i2c_bus *bus)
> +{
> +	bus->master_state = ASPEED_I2C_MASTER_STOP;
> +	aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> +			 ASPEED_I2C_CMD_REG);
> +}
> +
> +static void aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +{
> +	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> +	u32 irq_status, status_ack = 0, command = 0;
> +	u8 recv_byte;

If your lock means anything you should probably capture bus->msgs[..]
with the lock held. That said, see my previous comment about the
lock possibly not being terribly useful.

Additionally, if you are doing a bus recovery, won't you be messing
around with a stale or NULL bus->msgs ?

I would at the very least make it

	msg = bus->msgs ? &bus->msgs[bus->msgs_index] : NULL;

That way msg is NULL in the recovery case rather than a random
crap pointer.

> +	spin_lock(&bus->lock);
> +	irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
>

I would "ack" (write back to INTR_STS_REG) immediately. Otherwise
you have a race between status bits set as a result of what happened
before the interrupt handler vs. as a result of what you did.

For example, take TX. You get the TX bit in irq_status. You start
a new character transmission bcs there's more to send *then* you ack
the TX bit. That's racy. If that new transmission is fast enough,
you'll end up acking the wrong one. Again this is extremely unlikely
but code should be written in a way that is completely fool proof
from such races. They can happen for stupid reasons, such as huge
bus delays caused by a peripheral, FIQ going bonkers etc...

In general, you always ACK all interrupts first. Then you handle
the bits you have harvested.

> +	if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> +	    (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP)) {

What happen with recovery completion here ?

Won't we hit !bus->msgs && master state != stop ? Especially
if we hit a timeout where we haven't cleaned up any of our state.

> +		dev_dbg(bus->dev, "received error interrupt: 0x%08x",
> +			irq_status);

This is confusing too in the case of master_state != stop ... any
interrupt will trigger that. I think it would be worthwhile either
commenting a bit more here or having clearer messages depending
on the condition.

> +		bus->cmd_err = -EIO;
> +		do_stop(bus);
> +		goto out_no_complete;
> +	}
> +
> +	if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> +		goto out_complete;
> +	}

I would set master_state to "RECOVERY" (new state ?) and ensure
those things are caught if they happen outside of a recovery.

> +	if (bus->master_state == ASPEED_I2C_MASTER_START) {

Here a comment would be handy as to why you do this before the
switch/case. I understand why but it makes reading the code by
somebody else easier.

> +		if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK)) {

Minor nit:

		if (unlikely(error case)) {
			...
			goto out;
		}
		...

Ie, you don't need the "else", and you make it clear that this
is an error case, allowing the compiler to potentially optimize
the likely branch.

In fact, I would have that on all the error cases above too.

I understand now why you have that 'status_ack'. You are trying
to catch the bits that may be set that shouldn't be.

I think you should still "ack early". However, you could have
status_ack called status_handled or something like that and
at the end, still catch "spurrious" bits.

That said, I notice a lot of duplication in your state machine.

You basically have each state starting with

	if (didn't get the bit I wanted) {
		error
	}

You are also not very consistent as to whether you generate a
stop as a result or not.

I would happily simplify that state machine by just completing
with an error and letting master_xfer() do a stop when done but
if you like to keep it the way it is, you could have a common
goto label that handle error + stop.

> +			dev_dbg(bus->dev,
> +				"no slave present at %02x", msg->addr);
> +			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +			bus->cmd_err = -EIO;
> +			do_stop(bus);
> +			goto out_no_complete;
> +		} else {
> +			status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> +			if (msg->flags & I2C_M_RD)
> +				bus->master_state = ASPEED_I2C_MASTER_RX;
> +			else
> +				bus->master_state = ASPEED_I2C_MASTER_TX_FIRST;

What about the SMBUS_QUICK case ? (0-len transfer). Do we need
to handle this here ? A quick look at the TX_FIRST case makes
me think we are ok there but I'm not sure about the RX case.

I'm not sure the RX case is tight also. What completion does the
HW give you for the address cycle ? Won't you get that before it
has received the first character ? IE. You fall through to
the read case of the state machine with the read potentially
not complete yet no ?

> +		}
> +	}
> +
> +	switch (bus->master_state) {
> +	case ASPEED_I2C_MASTER_TX:
> +		if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> +			dev_dbg(bus->dev, "slave NACKed TX");
> +			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +			bus->cmd_err = -EIO;
> +			do_stop(bus);
> +			goto out_no_complete;

As I said earlier, I would factor all the error cases. I would also
not worry too much about checking that the status bits meet expectation
in the error path.

> +		} else if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> {
> +			dev_err(bus->dev, "slave failed to ACK TX");
> +			goto out_complete;

You should still stop.

> +		}
> +		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> +		/* fallthrough intended */
> +	case ASPEED_I2C_MASTER_TX_FIRST:
> +		if (bus->buf_index < msg->len) {
> +			bus->master_state = ASPEED_I2C_MASTER_TX;
> +			aspeed_i2c_write(bus, msg->buf[bus->buf_index++],
> +					 ASPEED_I2C_BYTE_BUF_REG);
> +			aspeed_i2c_write(bus, ASPEED_I2CD_M_TX_CMD,
> +					 ASPEED_I2C_CMD_REG);
> +		} else if (bus->msgs_index + 1 < bus->msgs_size) {
> +			bus->msgs_index++;
> +			do_start(bus);
> +		} else {
> +			do_stop(bus);
> +		}
> +		goto out_no_complete;
> +	case ASPEED_I2C_MASTER_RX:
> +		if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> +			dev_err(bus->dev, "master failed to RX");
> +			goto out_complete;
> +		}

See my comment above for a bog standard i2c_read. Aren't you getting
the completion for the address before the read is even started ?

> +		status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> +
> +		recv_byte = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
> +		msg->buf[bus->buf_index++] = recv_byte;
> +
> +		if (msg->flags & I2C_M_RECV_LEN &&
> +		    recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> +			msg->len = recv_byte +
> +					((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
> +			msg->flags &= ~I2C_M_RECV_LEN;
> +		}

You need to error out with -EPROTO if the size is too large.

> +
> +		if (bus->buf_index < msg->len) {
> +			bus->master_state = ASPEED_I2C_MASTER_RX;
> +			command = ASPEED_I2CD_M_RX_CMD;
> +			if (bus->buf_index + 1 == msg->len)
> +				command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> +			aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
> +		} else if (bus->msgs_index + 1 < bus->msgs_size) {
> +			bus->msgs_index++;
> +			do_start(bus);
> +		} else {
> +			do_stop(bus);
> +		}

You have some duplication. You could have your "completed message,
switch to the next one" be either a helper or another goto statement.

I would do a little helper that check the index and calls stop or
start.

> +		goto out_no_complete;
> +	case ASPEED_I2C_MASTER_STOP:
> +		if (!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP)) {
> +			dev_err(bus->dev, "master failed to STOP");
> +			bus->cmd_err = -EIO;
> +		}
> +		status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> +
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		goto out_complete;
> +	case ASPEED_I2C_MASTER_INACTIVE:
> +		dev_err(bus->dev,
> +			"master received interrupt 0x%08x, but is inactive",
> +			irq_status);
> +		bus->cmd_err = -EIO;
> +		goto out_complete;
> +	default:
> +		WARN(1, "unknown master state\n");
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		bus->cmd_err = -EIO;
> +		goto out_complete;
> +	}
> +
> +out_complete:
> +	complete(&bus->cmd_complete);
> +out_no_complete:
> +	if (irq_status != status_ack)
> +		dev_err(bus->dev,
> +			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> +			irq_status, status_ack);
> +	aspeed_i2c_write(bus, irq_status, ASPEED_I2C_INTR_STS_REG);
> +	spin_unlock(&bus->lock);
> +}
> +
> +static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> +{
> +	struct aspeed_i2c_bus *bus = dev_id;
> +
> +	aspeed_i2c_master_irq(bus);
> +	return IRQ_HANDLED;
> +}

In theory you want to only return IRQ_HANDLED if you indeed has at
least one IRQ status bit set... Not a huge deal here but it would
be cleaner.

> +static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> +				  struct i2c_msg *msgs, int num)
> +{
> +	struct aspeed_i2c_bus *bus = adap->algo_data;
> +	unsigned long time_left, flags;
> +	int ret = 0;
> +
> +	bus->cmd_err = 0;
> +
> +	/* If bus is busy, attempt recovery. We assume a single master
> +	 * environment.
> +	 */
> +	if (aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
> +	    ASPEED_I2CD_BUS_BUSY_STS) {
> +		ret = aspeed_i2c_recover_bus(bus);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	spin_lock_irqsave(&bus->lock, flags);

See previous comment about the lock.

I would also cleanup all the interrupts before we even start a transfer
(ie write all 1's to the interrupt status reg).

> +	bus->msgs = msgs;
> +	bus->msgs_index = 0;
> +	bus->msgs_size = num;

Minor nit: msgs_count rather than size ?

> +	do_start(bus);
> +	reinit_completion(&bus->cmd_complete);

The reinit_completion call should probably be before do_start.

Currently the spinlock avoids this being a real issue but if as I
suggest you take out the lock, then it will be racy (probably
impossible to hit in practice but still .. :-)
 
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	time_left = wait_for_completion_timeout(&bus->cmd_complete,
> +						bus->adap.timeout);
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +	bus->msgs = NULL;
> +	if (time_left == 0)
> +		ret = -ETIMEDOUT;
> +	else
> +		ret = bus->cmd_err;

If we timed out we may want to sanitize the HW state. I would suggest
resetting the master. We should also sanitize master_state. I would
suggest adding a reset function that cleans everything up.

> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	/* If nothing went wrong, return number of messages transferred. */
> +	if (ret >= 0)
> +		return bus->msgs_index + 1;
> +	else
> +		return ret;
> +}
> +
> +static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm aspeed_i2c_algo = {
> +	.master_xfer	= aspeed_i2c_master_xfer,
> +	.functionality	= aspeed_i2c_functionality,
> +};
> +
> +static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
> +{
> +	u32 base_clk, clk_high, clk_low, tmp;
> +
> +	/*
> +	 * The actual clock frequency of SCL is:
> +	 *	SCL_freq = base_freq * (SCL_high + SCL_low)
> +	 *		 = APB_freq / divisor
> +	 * where base_freq is a programmable clock divider; its value is
> +	 *	base_freq = 1 << base_clk
> +	 * SCL_high is the number of base_freq clock cycles that SCL stays high
> +	 * and SCL_low is the number of base_freq clock cycles that SCL stays
> +	 * low for a period of SCL.
> +	 * The actual register has a minimum SCL_high and SCL_low minimum of 1;
> +	 * thus, they start counting at zero. So
> +	 *	SCL_high = clk_high + 1
> +	 *	SCL_low	 = clk_low + 1
> +	 * Thus,
> +	 *	SCL_freq = (1 << base_clk) * (clk_high + 1 + clk_low + 1)
> +	 * The documentation recommends clk_high >= 8 and clk_low >= 7 when
> +	 * possible; this last constraint gives us the following solution:
> +	 */
> +	base_clk = divisor > 32 ? ilog2(divisor / 16 - 1) : 0;
> +	tmp = divisor / (1 << base_clk);
> +	clk_high = tmp / 2 + tmp % 2;
> +	clk_low = tmp - clk_high;
> +
> +	clk_high -= 1;
> +	clk_low -= 1;
> +
> +	return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> +		& ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> +			| ((clk_low << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> +			   & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> +			| (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> +}

As I think I mentioned earlier, the AST2500 has a slightly different
register layout which support larger values for high and low, thus
allowing a finer granularity.

BTW. In case you haven't, I would suggest you copy/paste the above in
a userspace app and run it for all frequency divisors and see if your
results match the aspeed table :)

> +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> +			       struct platform_device *pdev)
> +{
> +	u32 clk_freq, divisor;
> +	struct clk *pclk;
> +	int ret;
> +
> +	pclk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pclk)) {
> +		dev_err(&pdev->dev, "clk_get failed\n");
> +		return PTR_ERR(pclk);
> +	}
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   "clock-frequency", &clk_freq);

See my previous comment about calling that 'bus-frequency' rather
than 'clock-frequency'.

> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"Could not read clock-frequency property\n");
> +		clk_freq = 100000;
> +	}
> +	divisor = clk_get_rate(pclk) / clk_freq;
> +	/* We just need the clock rate, we don't actually use the clk object. */
> +	devm_clk_put(&pdev->dev, pclk);
> +
> +	/* Set AC Timing */
> +	if (clk_freq / 1000 > 1000) {
> +		aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> +						      ASPEED_I2C_FUN_CTRL_REG) |
> +				ASPEED_I2CD_M_HIGH_SPEED_EN |
> +				ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
> +				ASPEED_I2CD_SDA_DRIVE_1T_EN,
> +				ASPEED_I2C_FUN_CTRL_REG);
> +
> +		aspeed_i2c_write(bus, 0x3, ASPEED_I2C_AC_TIMING_REG2);
> +		aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
> +				 ASPEED_I2C_AC_TIMING_REG1);

I already discussed by doubts about the above. I can try to scope
it with the EVB if you don't get to it. For now I'd rather take the
code out.

We should ask aspeed from what frequency the "1T" stuff is useful.

> +	} else {
> +		aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
> +				 ASPEED_I2C_AC_TIMING_REG1);
> +		aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> +				 ASPEED_I2C_AC_TIMING_REG2);
> +	}
> +
> +	return 0;
> +}
> +
> +static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> +{
> +	struct aspeed_i2c_bus *bus;
> +	struct resource *res;
> +	int ret;
> +
> +	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	bus->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(bus->base)) {
> +		dev_err(&pdev->dev, "failed to devm_ioremap_resource\n");
> +		return PTR_ERR(bus->base);
> +	}
> +
> +	bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +	ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
> +			       IRQF_SHARED, dev_name(&pdev->dev), bus);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to request interrupt\n");
> +		return ret;
> +	}

Again, out of paranoia, make sure the HW is reset and interrupt
off *before* you register the interrupt handler, or a HW left in
a funny state (by uboot for example) might shoot interrupts before
you are ready to take them. I would move the reset you do below
to before devm_request_irq.

> +	/* Initialize the I2C adapter */
> +	spin_lock_init(&bus->lock);
> +	init_completion(&bus->cmd_complete);
> +	bus->adap.owner = THIS_MODULE;
> +	bus->adap.retries = 0;
> +	bus->adap.timeout = 5 * HZ;
> +	bus->adap.algo = &aspeed_i2c_algo;
> +	bus->adap.algo_data = bus;
> +	bus->adap.dev.parent = &pdev->dev;
> +	bus->adap.dev.of_node = pdev->dev.of_node;
> +	snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed i2c");

Another trivial one, should we put some kind of bus number
in that string ?

> +	bus->dev = &pdev->dev;
> +
> +	/* reset device: disable master & slave functions */
> +	aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> +
> +	ret = aspeed_i2c_init_clk(bus, pdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable Master Mode */
> +	aspeed_i2c_write(bus, aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG) |
> +		      ASPEED_I2CD_MASTER_EN |
> +		      ASPEED_I2CD_MULTI_MASTER_DIS, ASPEED_I2C_FUN_CTRL_REG);
> +
> +	/* Set interrupt generation of I2C controller */
> +	aspeed_i2c_write(bus, ASPEED_I2CD_INTR_ALL, ASPEED_I2C_INTR_CTRL_REG);
> +
> +	ret = i2c_add_adapter(&bus->adap);
> +	if (ret < 0)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, bus);
> +
> +	dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
> +		 bus->adap.nr, bus->irq);
> +
> +	return 0;
> +}
> +
> +static int aspeed_i2c_remove_bus(struct platform_device *pdev)
> +{
> +	struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&bus->adap);

Out of paranoia, should we turn off the function and mask the
interrupts here just in case ?

> +	return 0;
> +}
> +
> +static const struct of_device_id aspeed_i2c_bus_of_table[] = {
> +	{ .compatible = "aspeed,ast2400-i2c-bus", },
> +	{ .compatible = "aspeed,ast2500-i2c-bus", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
> +
> +static struct platform_driver aspeed_i2c_bus_driver = {
> +	.probe		= aspeed_i2c_probe_bus,
> +	.remove		= aspeed_i2c_remove_bus,
> +	.driver		= {
> +		.name		= "ast-i2c-bus",
> +		.of_match_table	= aspeed_i2c_bus_of_table,
> +	},
> +};
> +module_platform_driver(aspeed_i2c_bus_driver);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> +MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
> +MODULE_LICENSE("GPL v2");

>> +struct aspeed_i2c_bus {
>> +     struct i2c_adapter              adap;
>> +     struct device                   *dev;
>> +     void __iomem                    *base;
>> +     /* Synchronizes I/O mem access to base. */
>> +     spinlock_t                      lock;
>
> I am not entirely convinced we need that lock. The i2c core will
> take a mutex protecting all operations on the bus. So we only need
> to synchronize between our "xfer" code and our interrupt handler.

You are right if both having slave and master active at the same time
was not possible; however, it is. Imagine the case where the slave is
receiving a request and something in the I2C API gets called. I
suppose we could make the slave IRQ handler lock that lock, but I
think it makes more sense to have a separate lock, since we do not
control that lock making it harder to reason about. Plus, we put
ourselves in a position where an API user has access to a lock that an
interrupt handler needs to acquire, if the user does something dumb,
then we can get interrupt starvation.

>
> This probably be done without a lock if we are careful. Not a huge
> deal though as Aspeed SoC are currently not SMP so the lock compiles
> down to not much unless you have all the debug crap enabled :-)
>
>> +     struct completion               cmd_complete;
>> +     int                             irq;
>> +     /* Transaction state. */
>> +     enum aspeed_i2c_master_state    master_state;
>> +     struct i2c_msg                  *msgs;
>> +     size_t                          buf_index;
>> +     size_t                          msgs_index;
>> +     size_t                          msgs_size;
>> +     bool                            send_stop;
...
>> +             time_left = wait_for_completion_timeout(
>> +                             &bus->cmd_complete, bus->adap.timeout);
>> +
>> +             spin_lock_irqsave(&bus->lock, flags);
>> +             if (time_left == 0)
>> +                     ret = -ETIMEDOUT;
>> +             else if (bus->cmd_err)
>> +                     ret = -EIO;
>> +             /* Recovery failed. */
>> +             else if (!(aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
>> +                        ASPEED_I2CD_SDA_LINE_STS))
>> +                     ret = -EIO;
>> +     }
>
> Some of those error states probably also warrant a reset of the controller,
> I think aspeed does that in the SDK.

For timeout and cmd_err, I do not see any argument against it; it
sounds like we are in a very messed up, very unknown state, so full
reset is probably the best last resort. For SDA staying pulled down, I
think we can say with reasonable confidence that some device on our
bus is behaving very badly and I am not convinced that resetting the
controller is likely to do anything to help; that being said, I really
do not have any good ideas to address that. So maybe praying and
resetting the controller is *the most reasonable thing to do.* I would
like to know what you think we should do in that case.

While I was thinking about this I also realized that the SDA line
check after recovery happens in the else branch, but SCL line check
does not happen after we attempt to STOP if SCL is hung. If we decide
to make special note SDA being hung by a device that won't let go, we
might want to make a special note that SCL is hung by a device that
won't let go. Just a thought.

>
>> +out:
...
> What about I2C_M_NOSTART ?
>
> Not that I've ever seen it used... ;-)

Right now I am not doing any of the protocol mangling options, but I
can add them in if you think it is important for initial support.

>
>> +     aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
>> +     aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
>> +}
...
>
>> +     spin_lock(&bus->lock);
>> +     irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
>>
>
> I would "ack" (write back to INTR_STS_REG) immediately. Otherwise
> you have a race between status bits set as a result of what happened
> before the interrupt handler vs. as a result of what you did.
>
> For example, take TX. You get the TX bit in irq_status. You start
> a new character transmission bcs there's more to send *then* you ack
> the TX bit. That's racy. If that new transmission is fast enough,
> you'll end up acking the wrong one. Again this is extremely unlikely
> but code should be written in a way that is completely fool proof
> from such races. They can happen for stupid reasons, such as huge
> bus delays caused by a peripheral, FIQ going bonkers etc...
>
> In general, you always ACK all interrupts first. Then you handle
> the bits you have harvested.
>

The documentation says to ACK the interrupt after handling in the RX case:

<<<
S/W needs to clear this status bit to allow next data receiving.
>>>

I will double check with Ryan to make sure TX works the same way.

>> +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
>> +         (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP)) {
>
...
>
> I would set master_state to "RECOVERY" (new state ?) and ensure
> those things are caught if they happen outside of a recovery.

Let me know if you still think we need a "RECOVERY" state.

>
>> +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
>
...
>
>> +                     dev_dbg(bus->dev,
>> +                             "no slave present at %02x", msg->addr);
>> +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> +                     bus->cmd_err = -EIO;
>> +                     do_stop(bus);
>> +                     goto out_no_complete;
>> +             } else {
>> +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> +                     if (msg->flags & I2C_M_RD)
>> +                             bus->master_state = ASPEED_I2C_MASTER_RX;
>> +                     else
>> +                             bus->master_state = ASPEED_I2C_MASTER_TX_FIRST;
>
> What about the SMBUS_QUICK case ? (0-len transfer). Do we need
> to handle this here ? A quick look at the TX_FIRST case makes
> me think we are ok there but I'm not sure about the RX case.

I did not think that there is an SMBUS_QUICK RX. Could you point me to
an example?

>
> I'm not sure the RX case is tight also. What completion does the
> HW give you for the address cycle ? Won't you get that before it
> has received the first character ? IE. You fall through to
> the read case of the state machine with the read potentially
> not complete yet no ?
...
>> +     case ASPEED_I2C_MASTER_RX:
>> +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
>> +                     dev_err(bus->dev, "master failed to RX");
>> +                     goto out_complete;
>> +             }
>
> See my comment above for a bog standard i2c_read. Aren't you getting
> the completion for the address before the read is even started ?

In practice no, but it is probably best to be safe :-)

>
>> +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> +
>> +             recv_byte = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> +             msg->buf[bus->buf_index++] = recv_byte;
>> +
>> +             if (msg->flags & I2C_M_RECV_LEN &&
>> +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
>> +                     msg->len = recv_byte +
>> +                                     ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
...
>> +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
>> +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
>> +                     | ((clk_low << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
>> +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
>> +                     | (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>> +}
>
> As I think I mentioned earlier, the AST2500 has a slightly different
> register layout which support larger values for high and low, thus
> allowing a finer granularity.

I am developing against the 2500.

> BTW. In case you haven't, I would suggest you copy/paste the above in
> a userspace app and run it for all frequency divisors and see if your
> results match the aspeed table :)

Good call.

>
>> +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
>> +                            struct platform_device *pdev)
>> +{
>> +     u32 clk_freq, divisor;
>> +     struct clk *pclk;
>> +     int ret;
>> +
>> +     pclk = devm_clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(pclk)) {
>> +             dev_err(&pdev->dev, "clk_get failed\n");
>> +             return PTR_ERR(pclk);
>> +     }
>> +     ret = of_property_read_u32(pdev->dev.of_node,
>> +                                "clock-frequency", &clk_freq);
>
> See my previous comment about calling that 'bus-frequency' rather
> than 'clock-frequency'.
>
>> +     if (ret < 0) {
>> +             dev_err(&pdev->dev,
>> +                     "Could not read clock-frequency property\n");
>> +             clk_freq = 100000;
>> +     }
>> +     divisor = clk_get_rate(pclk) / clk_freq;
>> +     /* We just need the clock rate, we don't actually use the clk object. */
>> +     devm_clk_put(&pdev->dev, pclk);
>> +
>> +     /* Set AC Timing */
>> +     if (clk_freq / 1000 > 1000) {
>> +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
>> +                                                   ASPEED_I2C_FUN_CTRL_REG) |
>> +                             ASPEED_I2CD_M_HIGH_SPEED_EN |
>> +                             ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
>> +                             ASPEED_I2CD_SDA_DRIVE_1T_EN,
>> +                             ASPEED_I2C_FUN_CTRL_REG);
>> +
>> +             aspeed_i2c_write(bus, 0x3, ASPEED_I2C_AC_TIMING_REG2);
>> +             aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
>> +                              ASPEED_I2C_AC_TIMING_REG1);
>
> I already discussed by doubts about the above. I can try to scope
> it with the EVB if you don't get to it. For now I'd rather take the
> code out.
>
> We should ask aspeed from what frequency the "1T" stuff is useful.

Will do, I will try to rope Ryan in on the next review; it will be
good for him to get used to working with upstream anyway.

>
>> +     } else {
>> +             aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
>> +                              ASPEED_I2C_AC_TIMING_REG1);
>> +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
>> +                              ASPEED_I2C_AC_TIMING_REG2);
>> +     }
...
>> +     spin_lock_init(&bus->lock);
>> +     init_completion(&bus->cmd_complete);
>> +     bus->adap.owner = THIS_MODULE;
>> +     bus->adap.retries = 0;
>> +     bus->adap.timeout = 5 * HZ;
>> +     bus->adap.algo = &aspeed_i2c_algo;
>> +     bus->adap.algo_data = bus;
>> +     bus->adap.dev.parent = &pdev->dev;
>> +     bus->adap.dev.of_node = pdev->dev.of_node;
>> +     snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed i2c");
>
> Another trivial one, should we put some kind of bus number
> in that string ?

Whoops, looks like I missed this one; I will get to it in the next revision.

>
>> +     bus->dev = &pdev->dev;
>> +
>> +     /* reset device: disable master & slave functions */
>> +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
...

On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
> > > +struct aspeed_i2c_bus {
> > > +     struct i2c_adapter              adap;
> > > +     struct device                   *dev;
> > > +     void __iomem                    *base;
> > > +     /* Synchronizes I/O mem access to base. */
> > > +     spinlock_t                      lock;
> > 
> > I am not entirely convinced we need that lock. The i2c core will
> > take a mutex protecting all operations on the bus. So we only need
> > to synchronize between our "xfer" code and our interrupt handler.
> 
> You are right if both having slave and master active at the same time
> was not possible; however, it is.

Right, I somewhat forgot about the slave case.

  ...

> > Some of those error states probably also warrant a reset of the
> > controller,
> > I think aspeed does that in the SDK.
> 
> For timeout and cmd_err, I do not see any argument against it; it
> sounds like we are in a very messed up, very unknown state, so full
> reset is probably the best last resort.

Yup.

> For SDA staying pulled down, I
> think we can say with reasonable confidence that some device on our
> bus is behaving very badly and I am not convinced that resetting the
> controller is likely to do anything to help;

Right. Hammering with STOPs and pray ...

>  that being said, I really
> do not have any good ideas to address that. So maybe praying and
> resetting the controller is *the most reasonable thing to do.* I
> would like to know what you think we should do in that case.

Well, there's a (small ?) chance that it's a controller bug asserting
the line so ... but there's little we can do if not.

> While I was thinking about this I also realized that the SDA line
> check after recovery happens in the else branch, but SCL line check
> does not happen after we attempt to STOP if SCL is hung. If we decide
> to make special note SDA being hung by a device that won't let go, we
> might want to make a special note that SCL is hung by a device that
> won't let go. Just a thought.

Maybe. Or just "unrecoverable error"... hopefully these don't happen
too often ... We had cases of a TPM misbehaving like that.

> > > +out:
> 
> ...
> > What about I2C_M_NOSTART ?
> > 
> > Not that I've ever seen it used... ;-)
> 
> Right now I am not doing any of the protocol mangling options, but I
> can add them in if you think it is important for initial support.

No, not important, we can add that later if it ever becomes useful.

 ...

> > In general, you always ACK all interrupts first. Then you handle
> > the bits you have harvested.
> > 
> 
> The documentation says to ACK the interrupt after handling in the RX
> case:
> 
> <<<
> S/W needs to clear this status bit to allow next data receiving.
> > > > 
> 
> I will double check with Ryan to make sure TX works the same way.
> 
> > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> > > +         (!bus->msgs && bus->master_state !=
> > > ASPEED_I2C_MASTER_STOP)) {
> 
> ...
> > 
> > I would set master_state to "RECOVERY" (new state ?) and ensure
> > those things are caught if they happen outside of a recovery.

I replied privately ... as long as we ack before we start a new command
we should be ok but we shouldn't ack after.

Your latest patch still does that. It will do things like start a STOP
command *then* ack the status bits. I'm pretty sure that's bogus.

That way it's a lot simpler to simply move the

	writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);

To either right after the readl of the status reg at the beginning of
aspeed_i2c_master_irq().

I would be very surprised if that didn't work properly and wasn't much
safer than what you are currently doing. 

> Let me know if you still think we need a "RECOVERY" state.

The way you just switch to stop state and store the error for later
should work I think.

> > 
> > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
> 
> ...
> > 
> > > +                     dev_dbg(bus->dev,
> > > +                             "no slave present at %02x", msg-
> > > >addr);
> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> > > +                     bus->cmd_err = -EIO;
> > > +                     do_stop(bus);
> > > +                     goto out_no_complete;
> > > +             } else {
> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> > > +                     if (msg->flags & I2C_M_RD)
> > > +                             bus->master_state =
> > > ASPEED_I2C_MASTER_RX;
> > > +                     else
> > > +                             bus->master_state =
> > > ASPEED_I2C_MASTER_TX_FIRST;
> > 
> > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
> > to handle this here ? A quick look at the TX_FIRST case makes
> > me think we are ok there but I'm not sure about the RX case.
> 
> I did not think that there is an SMBUS_QUICK RX. Could you point me
> to an example?

Not so much an RX, it's more like you are sending a 1-bit data in
the place of the Rd/Wr bit. So you have a read with a lenght of 0,
I don't think in that case you should set ASPEED_I2CD_M_RX_CMD in
__aspeed_i2c_do_start

> > I'm not sure the RX case is tight also. What completion does the
> > HW give you for the address cycle ? Won't you get that before it
> > has received the first character ? IE. You fall through to
> > the read case of the state machine with the read potentially
> > not complete yet no ?
> 
> ...
> > > +     case ASPEED_I2C_MASTER_RX:
> > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> > > +                     dev_err(bus->dev, "master failed to RX");
> > > +                     goto out_complete;
> > > +             }
> > 
> > See my comment above for a bog standard i2c_read. Aren't you
> > getting
> > the completion for the address before the read is even started ?
> 
> In practice no, but it is probably best to be safe :-)

Yup :)
> > 
> > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> > > +
> > > +             recv_byte = aspeed_i2c_read(bus,
> > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
> > > +             msg->buf[bus->buf_index++] = recv_byte;
> > > +
> > > +             if (msg->flags & I2C_M_RECV_LEN &&
> > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> > > +                     msg->len = recv_byte +
> > > +                                     ((msg->flags &
> > > I2C_CLIENT_PEC) ? 2 : 1);
> 
> ...
> > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> > > +                     | ((clk_low <<
> > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> > > +                     | (base_clk &
> > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> > > +}
> > 
> > As I think I mentioned earlier, the AST2500 has a slightly
> > different
> > register layout which support larger values for high and low, thus
> > allowing a finer granularity.
> 
> I am developing against the 2500.

Yes but we'd like the driver to work with both :-)

> > BTW. In case you haven't, I would suggest you copy/paste the above
> > in
> > a userspace app and run it for all frequency divisors and see if
> > your
> > results match the aspeed table :)
> 
> Good call.

If you end up doing that, can you shoot it my way ? I can take care
of making sure it's all good for the 2400.

> > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> > > +                            struct platform_device *pdev)
> > > +{
> > > +     u32 clk_freq, divisor;
> > > +     struct clk *pclk;
> > > +     int ret;
> > > +
> > > +     pclk = devm_clk_get(&pdev->dev, NULL);
> > > +     if (IS_ERR(pclk)) {
> > > +             dev_err(&pdev->dev, "clk_get failed\n");
> > > +             return PTR_ERR(pclk);
> > > +     }
> > > +     ret = of_property_read_u32(pdev->dev.of_node,
> > > +                                "clock-frequency", &clk_freq);
> > 
> > See my previous comment about calling that 'bus-frequency' rather
> > than 'clock-frequency'.
> > 
> > > +     if (ret < 0) {
> > > +             dev_err(&pdev->dev,
> > > +                     "Could not read clock-frequency
> > > property\n");
> > > +             clk_freq = 100000;
> > > +     }
> > > +     divisor = clk_get_rate(pclk) / clk_freq;
> > > +     /* We just need the clock rate, we don't actually use the
> > > clk object. */
> > > +     devm_clk_put(&pdev->dev, pclk);
> > > +
> > > +     /* Set AC Timing */
> > > +     if (clk_freq / 1000 > 1000) {
> > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> > > +                                                   ASPEED_I2C_FU
> > > N_CTRL_REG) |
> > > +                             ASPEED_I2CD_M_HIGH_SPEED_EN |
> > > +                             ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
> > > +                             ASPEED_I2CD_SDA_DRIVE_1T_EN,
> > > +                             ASPEED_I2C_FUN_CTRL_REG);
> > > +
> > > +             aspeed_i2c_write(bus, 0x3,
> > > ASPEED_I2C_AC_TIMING_REG2);
> > > +             aspeed_i2c_write(bus,
> > > aspeed_i2c_get_clk_reg_val(divisor),
> > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > 
> > I already discussed by doubts about the above. I can try to scope
> > it with the EVB if you don't get to it. For now I'd rather take the
> > code out.
> > 
> > We should ask aspeed from what frequency the "1T" stuff is useful.
> 
> Will do, I will try to rope Ryan in on the next review; it will be
> good for him to get used to working with upstream anyway.

Yup. However, for the sake of getting something upstream (and in
OpenBMC 4.10 kernel) asap, I would suggest just dropping support
for those fast speeds for now, we can add them back later.

> > 
> > > +     } else {
> > > +             aspeed_i2c_write(bus,
> > > aspeed_i2c_get_clk_reg_val(divisor),
> > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> > > +                              ASPEED_I2C_AC_TIMING_REG2);
> > > +     }
> 
> ...
> > > +     spin_lock_init(&bus->lock);
> > > +     init_completion(&bus->cmd_complete);
> > > +     bus->adap.owner = THIS_MODULE;
> > > +     bus->adap.retries = 0;
> > > +     bus->adap.timeout = 5 * HZ;
> > > +     bus->adap.algo = &aspeed_i2c_algo;
> > > +     bus->adap.algo_data = bus;
> > > +     bus->adap.dev.parent = &pdev->dev;
> > > +     bus->adap.dev.of_node = pdev->dev.of_node;
> > > +     snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed
> > > i2c");
> > 
> > Another trivial one, should we put some kind of bus number
> > in that string ?
> 
> Whoops, looks like I missed this one; I will get to it in the next
> revision.

Ok. I noticed you missed that in v7, so I assume you mean v8 :-)

> > 
> > > +     bus->dev = &pdev->dev;
> > > +
> > > +     /* reset device: disable master & slave functions */
> > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> 
> ...
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Adding Ryan.

On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
>> > > +struct aspeed_i2c_bus {
>> > > +     struct i2c_adapter              adap;
>> > > +     struct device                   *dev;
>> > > +     void __iomem                    *base;
>> > > +     /* Synchronizes I/O mem access to base. */
>> > > +     spinlock_t                      lock;
>> >
>> > I am not entirely convinced we need that lock. The i2c core will
>> > take a mutex protecting all operations on the bus. So we only need
>> > to synchronize between our "xfer" code and our interrupt handler.
>>
>> You are right if both having slave and master active at the same time
>> was not possible; however, it is.
>
> Right, I somewhat forgot about the slave case.
>
>   ...
>
>> > Some of those error states probably also warrant a reset of the
>> > controller,
>> > I think aspeed does that in the SDK.
>>
>> For timeout and cmd_err, I do not see any argument against it; it
>> sounds like we are in a very messed up, very unknown state, so full
>> reset is probably the best last resort.
>
> Yup.
>
>> For SDA staying pulled down, I
>> think we can say with reasonable confidence that some device on our
>> bus is behaving very badly and I am not convinced that resetting the
>> controller is likely to do anything to help;
>
> Right. Hammering with STOPs and pray ...

I think sending recovery mode sends stops as a part of the recovery
algorithm it executes.

>
>>  that being said, I really
>> do not have any good ideas to address that. So maybe praying and
>> resetting the controller is *the most reasonable thing to do.* I
>> would like to know what you think we should do in that case.
>
> Well, there's a (small ?) chance that it's a controller bug asserting
> the line so ... but there's little we can do if not.

True.

>
>> While I was thinking about this I also realized that the SDA line
>> check after recovery happens in the else branch, but SCL line check
>> does not happen after we attempt to STOP if SCL is hung. If we decide
>> to make special note SDA being hung by a device that won't let go, we
>> might want to make a special note that SCL is hung by a device that
>> won't let go. Just a thought.
>
> Maybe. Or just "unrecoverable error"... hopefully these don't happen
> too often ... We had cases of a TPM misbehaving like that.

Yeah, definitely should print something out.

>
>> > > +out:
>>
>> ...
>> > What about I2C_M_NOSTART ?
>> >
>> > Not that I've ever seen it used... ;-)
>>
>> Right now I am not doing any of the protocol mangling options, but I
>> can add them in if you think it is important for initial support.
>
> No, not important, we can add that later if it ever becomes useful.
>
>  ...
>
>> > In general, you always ACK all interrupts first. Then you handle
>> > the bits you have harvested.
>> >
>>
>> The documentation says to ACK the interrupt after handling in the RX
>> case:
>>
>> <<<
>> S/W needs to clear this status bit to allow next data receiving.
>> > > >
>>
>> I will double check with Ryan to make sure TX works the same way.
>>
>> > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
>> > > +         (!bus->msgs && bus->master_state !=
>> > > ASPEED_I2C_MASTER_STOP)) {
>>
>> ...
>> >
>> > I would set master_state to "RECOVERY" (new state ?) and ensure
>> > those things are caught if they happen outside of a recovery.
>
> I replied privately ... as long as we ack before we start a new command
> we should be ok but we shouldn't ack after.
>
> Your latest patch still does that. It will do things like start a STOP
> command *then* ack the status bits. I'm pretty sure that's bogus.
>
> That way it's a lot simpler to simply move the
>
>         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>
> To either right after the readl of the status reg at the beginning of
> aspeed_i2c_master_irq().
>
> I would be very surprised if that didn't work properly and wasn't much
> safer than what you are currently doing.

I think I tried your way and it worked. In anycase, Ryan will be able
to clarify for us.

>
>> Let me know if you still think we need a "RECOVERY" state.
>
> The way you just switch to stop state and store the error for later
> should work I think.
>
>> >
>> > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
>>
>> ...
>> >
>> > > +                     dev_dbg(bus->dev,
>> > > +                             "no slave present at %02x", msg-
>> > > >addr);
>> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> > > +                     bus->cmd_err = -EIO;
>> > > +                     do_stop(bus);
>> > > +                     goto out_no_complete;
>> > > +             } else {
>> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> > > +                     if (msg->flags & I2C_M_RD)
>> > > +                             bus->master_state =
>> > > ASPEED_I2C_MASTER_RX;
>> > > +                     else
>> > > +                             bus->master_state =
>> > > ASPEED_I2C_MASTER_TX_FIRST;
>> >
>> > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
>> > to handle this here ? A quick look at the TX_FIRST case makes
>> > me think we are ok there but I'm not sure about the RX case.
>>
>> I did not think that there is an SMBUS_QUICK RX. Could you point me
>> to an example?
>
> Not so much an RX, it's more like you are sending a 1-bit data in
> the place of the Rd/Wr bit. So you have a read with a lenght of 0,
> I don't think in that case you should set ASPEED_I2CD_M_RX_CMD in
> __aspeed_i2c_do_start

Forget what I said, I was just not thinking about the fact that SMBus
emulation causes the data bit to be encoded as the R/W flag. I see
what you are saying; you are correct.

>
>> > I'm not sure the RX case is tight also. What completion does the
>> > HW give you for the address cycle ? Won't you get that before it
>> > has received the first character ? IE. You fall through to
>> > the read case of the state machine with the read potentially
>> > not complete yet no ?
>>
>> ...
>> > > +     case ASPEED_I2C_MASTER_RX:
>> > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
>> > > +                     dev_err(bus->dev, "master failed to RX");
>> > > +                     goto out_complete;
>> > > +             }
>> >
>> > See my comment above for a bog standard i2c_read. Aren't you
>> > getting
>> > the completion for the address before the read is even started ?
>>
>> In practice no, but it is probably best to be safe :-)
>
> Yup :)
>> >
>> > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> > > +
>> > > +             recv_byte = aspeed_i2c_read(bus,
>> > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> > > +             msg->buf[bus->buf_index++] = recv_byte;
>> > > +
>> > > +             if (msg->flags & I2C_M_RECV_LEN &&
>> > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
>> > > +                     msg->len = recv_byte +
>> > > +                                     ((msg->flags &
>> > > I2C_CLIENT_PEC) ? 2 : 1);
>>
>> ...
>> > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
>> > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
>> > > +                     | ((clk_low <<
>> > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
>> > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
>> > > +                     | (base_clk &
>> > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>> > > +}
>> >
>> > As I think I mentioned earlier, the AST2500 has a slightly
>> > different
>> > register layout which support larger values for high and low, thus
>> > allowing a finer granularity.
>>
>> I am developing against the 2500.
>
> Yes but we'd like the driver to work with both :-)

Right, I thought you were making an assertion about the 2500, if you
are making an assertion about the 2400, I do not know and do not have
one handy.

>
>> > BTW. In case you haven't, I would suggest you copy/paste the above
>> > in
>> > a userspace app and run it for all frequency divisors and see if
>> > your
>> > results match the aspeed table :)
>>
>> Good call.
>
> If you end up doing that, can you shoot it my way ? I can take care
> of making sure it's all good for the 2400.

Will do.

>
>> > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
>> > > +                            struct platform_device *pdev)
>> > > +{
>> > > +     u32 clk_freq, divisor;
>> > > +     struct clk *pclk;
>> > > +     int ret;
>> > > +
>> > > +     pclk = devm_clk_get(&pdev->dev, NULL);
>> > > +     if (IS_ERR(pclk)) {
>> > > +             dev_err(&pdev->dev, "clk_get failed\n");
>> > > +             return PTR_ERR(pclk);
>> > > +     }
>> > > +     ret = of_property_read_u32(pdev->dev.of_node,
>> > > +                                "clock-frequency", &clk_freq);
>> >
>> > See my previous comment about calling that 'bus-frequency' rather
>> > than 'clock-frequency'.
>> >
>> > > +     if (ret < 0) {
>> > > +             dev_err(&pdev->dev,
>> > > +                     "Could not read clock-frequency
>> > > property\n");
>> > > +             clk_freq = 100000;
>> > > +     }
>> > > +     divisor = clk_get_rate(pclk) / clk_freq;
>> > > +     /* We just need the clock rate, we don't actually use the
>> > > clk object. */
>> > > +     devm_clk_put(&pdev->dev, pclk);
>> > > +
>> > > +     /* Set AC Timing */
>> > > +     if (clk_freq / 1000 > 1000) {
>> > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
>> > > +                                                   ASPEED_I2C_FU
>> > > N_CTRL_REG) |
>> > > +                             ASPEED_I2CD_M_HIGH_SPEED_EN |
>> > > +                             ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
>> > > +                             ASPEED_I2CD_SDA_DRIVE_1T_EN,
>> > > +                             ASPEED_I2C_FUN_CTRL_REG);
>> > > +
>> > > +             aspeed_i2c_write(bus, 0x3,
>> > > ASPEED_I2C_AC_TIMING_REG2);
>> > > +             aspeed_i2c_write(bus,
>> > > aspeed_i2c_get_clk_reg_val(divisor),
>> > > +                              ASPEED_I2C_AC_TIMING_REG1);
>> >
>> > I already discussed by doubts about the above. I can try to scope
>> > it with the EVB if you don't get to it. For now I'd rather take the
>> > code out.
>> >
>> > We should ask aspeed from what frequency the "1T" stuff is useful.
>>
>> Will do, I will try to rope Ryan in on the next review; it will be
>> good for him to get used to working with upstream anyway.
>
> Yup. However, for the sake of getting something upstream (and in
> OpenBMC 4.10 kernel) asap, I would suggest just dropping support
> for those fast speeds for now, we can add them back later.

Alright, that's fine. Still, Ryan, could you provide some context on this?

>
>> >
>> > > +     } else {
>> > > +             aspeed_i2c_write(bus,
>> > > aspeed_i2c_get_clk_reg_val(divisor),
>> > > +                              ASPEED_I2C_AC_TIMING_REG1);
>> > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
>> > > +                              ASPEED_I2C_AC_TIMING_REG2);
>> > > +     }
>>
>> ...
>> > > +     spin_lock_init(&bus->lock);
>> > > +     init_completion(&bus->cmd_complete);
>> > > +     bus->adap.owner = THIS_MODULE;
>> > > +     bus->adap.retries = 0;
>> > > +     bus->adap.timeout = 5 * HZ;
>> > > +     bus->adap.algo = &aspeed_i2c_algo;
>> > > +     bus->adap.algo_data = bus;
>> > > +     bus->adap.dev.parent = &pdev->dev;
>> > > +     bus->adap.dev.of_node = pdev->dev.of_node;
>> > > +     snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed
>> > > i2c");
>> >
>> > Another trivial one, should we put some kind of bus number
>> > in that string ?
>>
>> Whoops, looks like I missed this one; I will get to it in the next
>> revision.
>
> Ok. I noticed you missed that in v7, so I assume you mean v8 :-)

Yep, I will get it in v8.

>
>> >
>> > > +     bus->dev = &pdev->dev;
>> > > +
>> > > +     /* reset device: disable master & slave functions */
>> > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
>>
>> ...
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello All,
		ASPEED_I2CD_M_SDA_DRIVE_1T_EN, ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage. 
		For example, if i2c bus is use on "high speed" and "single slave and master" and i2c bus is too long. It need drive SDA or SCL less lunacy. It would enable it. 
		Otherwise, don’t enable it. especially in multi-master. It can’t be enable. 

		  
	

Best Regards,
Ryan

信驊科技股份有限公司
ASPEED Technology Inc.
2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City 30077, Taiwan
Tel: 886-3-578-9568  #857
Fax: 886-3-578-9586
************* Email Confidentiality Notice ********************
DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.


-----Original Message-----
From: Brendan Higgins [mailto:brendanhiggins@google.com] 

Sent: Tuesday, April 25, 2017 4:32 PM
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>
Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

Adding Ryan.

On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:

>> > > +struct aspeed_i2c_bus {

>> > > +     struct i2c_adapter              adap;

>> > > +     struct device                   *dev;

>> > > +     void __iomem                    *base;

>> > > +     /* Synchronizes I/O mem access to base. */

>> > > +     spinlock_t                      lock;

>> >

>> > I am not entirely convinced we need that lock. The i2c core will 

>> > take a mutex protecting all operations on the bus. So we only need 

>> > to synchronize between our "xfer" code and our interrupt handler.

>>

>> You are right if both having slave and master active at the same time 

>> was not possible; however, it is.

>

> Right, I somewhat forgot about the slave case.

>

>   ...

>

>> > Some of those error states probably also warrant a reset of the 

>> > controller, I think aspeed does that in the SDK.

>>

>> For timeout and cmd_err, I do not see any argument against it; it 

>> sounds like we are in a very messed up, very unknown state, so full 

>> reset is probably the best last resort.

>

> Yup.

>

>> For SDA staying pulled down, I

>> think we can say with reasonable confidence that some device on our 

>> bus is behaving very badly and I am not convinced that resetting the 

>> controller is likely to do anything to help;

>

> Right. Hammering with STOPs and pray ...


I think sending recovery mode sends stops as a part of the recovery algorithm it executes.

>

>>  that being said, I really

>> do not have any good ideas to address that. So maybe praying and 

>> resetting the controller is *the most reasonable thing to do.* I 

>> would like to know what you think we should do in that case.

>

> Well, there's a (small ?) chance that it's a controller bug asserting 

> the line so ... but there's little we can do if not.


True.

>

>> While I was thinking about this I also realized that the SDA line 

>> check after recovery happens in the else branch, but SCL line check 

>> does not happen after we attempt to STOP if SCL is hung. If we decide 

>> to make special note SDA being hung by a device that won't let go, we 

>> might want to make a special note that SCL is hung by a device that 

>> won't let go. Just a thought.

>

> Maybe. Or just "unrecoverable error"... hopefully these don't happen 

> too often ... We had cases of a TPM misbehaving like that.


Yeah, definitely should print something out.

>

>> > > +out:

>>

>> ...

>> > What about I2C_M_NOSTART ?

>> >

>> > Not that I've ever seen it used... ;-)

>>

>> Right now I am not doing any of the protocol mangling options, but I 

>> can add them in if you think it is important for initial support.

>

> No, not important, we can add that later if it ever becomes useful.

>

>  ...

>

>> > In general, you always ACK all interrupts first. Then you handle 

>> > the bits you have harvested.

>> >

>>

>> The documentation says to ACK the interrupt after handling in the RX

>> case:

>>

>> <<<

>> S/W needs to clear this status bit to allow next data receiving.

>> > > >

>>

>> I will double check with Ryan to make sure TX works the same way.

>>

>> > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||

>> > > +         (!bus->msgs && bus->master_state !=

>> > > ASPEED_I2C_MASTER_STOP)) {

>>

>> ...

>> >

>> > I would set master_state to "RECOVERY" (new state ?) and ensure 

>> > those things are caught if they happen outside of a recovery.

>

> I replied privately ... as long as we ack before we start a new 

> command we should be ok but we shouldn't ack after.

>

> Your latest patch still does that. It will do things like start a STOP 

> command *then* ack the status bits. I'm pretty sure that's bogus.

>

> That way it's a lot simpler to simply move the

>

>         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);

>

> To either right after the readl of the status reg at the beginning of 

> aspeed_i2c_master_irq().

>

> I would be very surprised if that didn't work properly and wasn't much 

> safer than what you are currently doing.


I think I tried your way and it worked. In anycase, Ryan will be able to clarify for us.

>

>> Let me know if you still think we need a "RECOVERY" state.

>

> The way you just switch to stop state and store the error for later 

> should work I think.

>

>> >

>> > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {

>>

>> ...

>> >

>> > > +                     dev_dbg(bus->dev,

>> > > +                             "no slave present at %02x", msg-

>> > > >addr);

>> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;

>> > > +                     bus->cmd_err = -EIO;

>> > > +                     do_stop(bus);

>> > > +                     goto out_no_complete;

>> > > +             } else {

>> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;

>> > > +                     if (msg->flags & I2C_M_RD)

>> > > +                             bus->master_state =

>> > > ASPEED_I2C_MASTER_RX;

>> > > +                     else

>> > > +                             bus->master_state =

>> > > ASPEED_I2C_MASTER_TX_FIRST;

>> >

>> > What about the SMBUS_QUICK case ? (0-len transfer). Do we need to 

>> > handle this here ? A quick look at the TX_FIRST case makes me think 

>> > we are ok there but I'm not sure about the RX case.

>>

>> I did not think that there is an SMBUS_QUICK RX. Could you point me 

>> to an example?

>

> Not so much an RX, it's more like you are sending a 1-bit data in the 

> place of the Rd/Wr bit. So you have a read with a lenght of 0, I don't 

> think in that case you should set ASPEED_I2CD_M_RX_CMD in 

> __aspeed_i2c_do_start


Forget what I said, I was just not thinking about the fact that SMBus emulation causes the data bit to be encoded as the R/W flag. I see what you are saying; you are correct.

>

>> > I'm not sure the RX case is tight also. What completion does the HW 

>> > give you for the address cycle ? Won't you get that before it has 

>> > received the first character ? IE. You fall through to the read 

>> > case of the state machine with the read potentially not complete 

>> > yet no ?

>>

>> ...

>> > > +     case ASPEED_I2C_MASTER_RX:

>> > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {

>> > > +                     dev_err(bus->dev, "master failed to RX");

>> > > +                     goto out_complete;

>> > > +             }

>> >

>> > See my comment above for a bog standard i2c_read. Aren't you 

>> > getting the completion for the address before the read is even 

>> > started ?

>>

>> In practice no, but it is probably best to be safe :-)

>

> Yup :)

>> >

>> > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;

>> > > +

>> > > +             recv_byte = aspeed_i2c_read(bus,

>> > > ASPEED_I2C_BYTE_BUF_REG) >> 8;

>> > > +             msg->buf[bus->buf_index++] = recv_byte;

>> > > +

>> > > +             if (msg->flags & I2C_M_RECV_LEN &&

>> > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {

>> > > +                     msg->len = recv_byte +

>> > > +                                     ((msg->flags &

>> > > I2C_CLIENT_PEC) ? 2 : 1);

>>

>> ...

>> > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)

>> > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)

>> > > +                     | ((clk_low <<

>> > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)

>> > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)

>> > > +                     | (base_clk &

>> > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);

>> > > +}

>> >

>> > As I think I mentioned earlier, the AST2500 has a slightly 

>> > different register layout which support larger values for high and 

>> > low, thus allowing a finer granularity.

>>

>> I am developing against the 2500.

>

> Yes but we'd like the driver to work with both :-)


Right, I thought you were making an assertion about the 2500, if you are making an assertion about the 2400, I do not know and do not have one handy.

>

>> > BTW. In case you haven't, I would suggest you copy/paste the above 

>> > in a userspace app and run it for all frequency divisors and see if 

>> > your results match the aspeed table :)

>>

>> Good call.

>

> If you end up doing that, can you shoot it my way ? I can take care of 

> making sure it's all good for the 2400.


Will do.

>

>> > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,

>> > > +                            struct platform_device *pdev) {

>> > > +     u32 clk_freq, divisor;

>> > > +     struct clk *pclk;

>> > > +     int ret;

>> > > +

>> > > +     pclk = devm_clk_get(&pdev->dev, NULL);

>> > > +     if (IS_ERR(pclk)) {

>> > > +             dev_err(&pdev->dev, "clk_get failed\n");

>> > > +             return PTR_ERR(pclk);

>> > > +     }

>> > > +     ret = of_property_read_u32(pdev->dev.of_node,

>> > > +                                "clock-frequency", &clk_freq);

>> >

>> > See my previous comment about calling that 'bus-frequency' rather 

>> > than 'clock-frequency'.

>> >

>> > > +     if (ret < 0) {

>> > > +             dev_err(&pdev->dev,

>> > > +                     "Could not read clock-frequency

>> > > property\n");

>> > > +             clk_freq = 100000;

>> > > +     }

>> > > +     divisor = clk_get_rate(pclk) / clk_freq;

>> > > +     /* We just need the clock rate, we don't actually use the

>> > > clk object. */

>> > > +     devm_clk_put(&pdev->dev, pclk);

>> > > +

>> > > +     /* Set AC Timing */

>> > > +     if (clk_freq / 1000 > 1000) {

>> > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,

>> > > +                                                   ASPEED_I2C_FU

>> > > N_CTRL_REG) |

>> > > +                             ASPEED_I2CD_M_HIGH_SPEED_EN |

>> > > +                             ASPEED_I2CD_M_SDA_DRIVE_1T_EN |

>> > > +                             ASPEED_I2CD_SDA_DRIVE_1T_EN,

>> > > +                             ASPEED_I2C_FUN_CTRL_REG);

>> > > +

>> > > +             aspeed_i2c_write(bus, 0x3,

>> > > ASPEED_I2C_AC_TIMING_REG2);

>> > > +             aspeed_i2c_write(bus,

>> > > aspeed_i2c_get_clk_reg_val(divisor),

>> > > +                              ASPEED_I2C_AC_TIMING_REG1);

>> >

>> > I already discussed by doubts about the above. I can try to scope 

>> > it with the EVB if you don't get to it. For now I'd rather take the 

>> > code out.

>> >

>> > We should ask aspeed from what frequency the "1T" stuff is useful.

>>

>> Will do, I will try to rope Ryan in on the next review; it will be 

>> good for him to get used to working with upstream anyway.

>

> Yup. However, for the sake of getting something upstream (and in 

> OpenBMC 4.10 kernel) asap, I would suggest just dropping support for 

> those fast speeds for now, we can add them back later.


Alright, that's fine. Still, Ryan, could you provide some context on this?

>

>> >

>> > > +     } else {

>> > > +             aspeed_i2c_write(bus,

>> > > aspeed_i2c_get_clk_reg_val(divisor),

>> > > +                              ASPEED_I2C_AC_TIMING_REG1);

>> > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,

>> > > +                              ASPEED_I2C_AC_TIMING_REG2);

>> > > +     }

>>

>> ...

>> > > +     spin_lock_init(&bus->lock);

>> > > +     init_completion(&bus->cmd_complete);

>> > > +     bus->adap.owner = THIS_MODULE;

>> > > +     bus->adap.retries = 0;

>> > > +     bus->adap.timeout = 5 * HZ;

>> > > +     bus->adap.algo = &aspeed_i2c_algo;

>> > > +     bus->adap.algo_data = bus;

>> > > +     bus->adap.dev.parent = &pdev->dev;

>> > > +     bus->adap.dev.of_node = pdev->dev.of_node;

>> > > +     snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed

>> > > i2c");

>> >

>> > Another trivial one, should we put some kind of bus number in that 

>> > string ?

>>

>> Whoops, looks like I missed this one; I will get to it in the next 

>> revision.

>

> Ok. I noticed you missed that in v7, so I assume you mean v8 :-)


Yep, I will get it in v8.

>

>> >

>> > > +     bus->dev = &pdev->dev;

>> > > +

>> > > +     /* reset device: disable master & slave functions */

>> > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);

>>

>> ...

>> --

>> To unsubscribe from this list: send the line "unsubscribe devicetree"

>> in

>> the body of a message to majordomo@vger.kernel.org More majordomo 

>> info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
> Hello All,
> 		ASPEED_I2CD_M_SDA_DRIVE_1T_EN,
> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage. 
> 		For example, if i2c bus is use on "high speed" and
> "single slave and master" and i2c bus is too long. It need drive SDA
> or SCL less lunacy. It would enable it. 
> 		Otherwise, don’t enable it. especially in multi-master. 
> It can’t be enable. 

That smells like a specific enough use case that we should probably
cover with a device-tree property, something like an empty
"sda-extra-drive" property (empty properties are typically used
for booleans, their presence means "true").

Thanks Ryan. Can you shed some light on the meaning of the high-speed
bit as well please ? Does it force to a specific speed (ignoring the
divisor) or we can still play with the clock high/low counts ?

Cheers,
Ben.

> 		  
> 	
> 
> Best Regards,
> Ryan
> 
> 信驊科技股份有限公司
> ASPEED Technology Inc.
> 2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City
> 30077, Taiwan
> Tel: 886-3-578-9568  #857
> Fax: 886-3-578-9586
> ************* Email Confidentiality Notice ********************
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged
> and/or other confidential information. If you have received it in
> error, please notify the sender by reply e-mail and immediately
> delete the e-mail and any attachments without copying or disclosing
> the contents. Thank you.
> 
> 
> -----Original Message-----
> From: Brendan Higgins [mailto:brendanhiggins@google.com] 
> Sent: Tuesday, April 25, 2017 4:32 PM
> To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org
> >; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutro
> nix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyng
> ier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@m
> leia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod
> .org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist
> <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>
> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
> 
> Adding Ryan.
> 
> On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.
> crashing.org> wrote:
> > On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
> > > > > +struct aspeed_i2c_bus {
> > > > > +     struct i2c_adapter              adap;
> > > > > +     struct device                   *dev;
> > > > > +     void __iomem                    *base;
> > > > > +     /* Synchronizes I/O mem access to base. */
> > > > > +     spinlock_t                      lock;
> > > > 
> > > > I am not entirely convinced we need that lock. The i2c core
> > > > will 
> > > > take a mutex protecting all operations on the bus. So we only
> > > > need 
> > > > to synchronize between our "xfer" code and our interrupt
> > > > handler.
> > > 
> > > You are right if both having slave and master active at the same
> > > time 
> > > was not possible; however, it is.
> > 
> > Right, I somewhat forgot about the slave case.
> > 
> >   ...
> > 
> > > > Some of those error states probably also warrant a reset of
> > > > the 
> > > > controller, I think aspeed does that in the SDK.
> > > 
> > > For timeout and cmd_err, I do not see any argument against it;
> > > it 
> > > sounds like we are in a very messed up, very unknown state, so
> > > full 
> > > reset is probably the best last resort.
> > 
> > Yup.
> > 
> > > For SDA staying pulled down, I
> > > think we can say with reasonable confidence that some device on
> > > our 
> > > bus is behaving very badly and I am not convinced that resetting
> > > the 
> > > controller is likely to do anything to help;
> > 
> > Right. Hammering with STOPs and pray ...
> 
> I think sending recovery mode sends stops as a part of the recovery
> algorithm it executes.
> 
> > 
> > >  that being said, I really
> > > do not have any good ideas to address that. So maybe praying and 
> > > resetting the controller is *the most reasonable thing to do.* I 
> > > would like to know what you think we should do in that case.
> > 
> > Well, there's a (small ?) chance that it's a controller bug
> > asserting 
> > the line so ... but there's little we can do if not.
> 
> True.
> 
> > 
> > > While I was thinking about this I also realized that the SDA
> > > line 
> > > check after recovery happens in the else branch, but SCL line
> > > check 
> > > does not happen after we attempt to STOP if SCL is hung. If we
> > > decide 
> > > to make special note SDA being hung by a device that won't let
> > > go, we 
> > > might want to make a special note that SCL is hung by a device
> > > that 
> > > won't let go. Just a thought.
> > 
> > Maybe. Or just "unrecoverable error"... hopefully these don't
> > happen 
> > too often ... We had cases of a TPM misbehaving like that.
> 
> Yeah, definitely should print something out.
> 
> > 
> > > > > +out:
> > > 
> > > ...
> > > > What about I2C_M_NOSTART ?
> > > > 
> > > > Not that I've ever seen it used... ;-)
> > > 
> > > Right now I am not doing any of the protocol mangling options,
> > > but I 
> > > can add them in if you think it is important for initial support.
> > 
> > No, not important, we can add that later if it ever becomes useful.
> > 
> >  ...
> > 
> > > > In general, you always ACK all interrupts first. Then you
> > > > handle 
> > > > the bits you have harvested.
> > > > 
> > > 
> > > The documentation says to ACK the interrupt after handling in the
> > > RX
> > > case:
> > > 
> > > <<<
> > > S/W needs to clear this status bit to allow next data receiving.
> > > > > > 
> > > 
> > > I will double check with Ryan to make sure TX works the same way.
> > > 
> > > > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> > > > > +         (!bus->msgs && bus->master_state !=
> > > > > ASPEED_I2C_MASTER_STOP)) {
> > > 
> > > ...
> > > > 
> > > > I would set master_state to "RECOVERY" (new state ?) and
> > > > ensure 
> > > > those things are caught if they happen outside of a recovery.
> > 
> > I replied privately ... as long as we ack before we start a new 
> > command we should be ok but we shouldn't ack after.
> > 
> > Your latest patch still does that. It will do things like start a
> > STOP 
> > command *then* ack the status bits. I'm pretty sure that's bogus.
> > 
> > That way it's a lot simpler to simply move the
> > 
> >         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> > 
> > To either right after the readl of the status reg at the beginning
> > of 
> > aspeed_i2c_master_irq().
> > 
> > I would be very surprised if that didn't work properly and wasn't
> > much 
> > safer than what you are currently doing.
> 
> I think I tried your way and it worked. In anycase, Ryan will be able
> to clarify for us.
> 
> > 
> > > Let me know if you still think we need a "RECOVERY" state.
> > 
> > The way you just switch to stop state and store the error for
> > later 
> > should work I think.
> > 
> > > > 
> > > > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
> > > 
> > > ...
> > > > 
> > > > > +                     dev_dbg(bus->dev,
> > > > > +                             "no slave present at %02x",
> > > > > msg-
> > > > > > addr);
> > > > > 
> > > > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> > > > > +                     bus->cmd_err = -EIO;
> > > > > +                     do_stop(bus);
> > > > > +                     goto out_no_complete;
> > > > > +             } else {
> > > > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> > > > > +                     if (msg->flags & I2C_M_RD)
> > > > > +                             bus->master_state =
> > > > > ASPEED_I2C_MASTER_RX;
> > > > > +                     else
> > > > > +                             bus->master_state =
> > > > > ASPEED_I2C_MASTER_TX_FIRST;
> > > > 
> > > > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
> > > > to 
> > > > handle this here ? A quick look at the TX_FIRST case makes me
> > > > think 
> > > > we are ok there but I'm not sure about the RX case.
> > > 
> > > I did not think that there is an SMBUS_QUICK RX. Could you point
> > > me 
> > > to an example?
> > 
> > Not so much an RX, it's more like you are sending a 1-bit data in
> > the 
> > place of the Rd/Wr bit. So you have a read with a lenght of 0, I
> > don't 
> > think in that case you should set ASPEED_I2CD_M_RX_CMD in 
> > __aspeed_i2c_do_start
> 
> Forget what I said, I was just not thinking about the fact that SMBus
> emulation causes the data bit to be encoded as the R/W flag. I see
> what you are saying; you are correct.
> 
> > 
> > > > I'm not sure the RX case is tight also. What completion does
> > > > the HW 
> > > > give you for the address cycle ? Won't you get that before it
> > > > has 
> > > > received the first character ? IE. You fall through to the
> > > > read 
> > > > case of the state machine with the read potentially not
> > > > complete 
> > > > yet no ?
> > > 
> > > ...
> > > > > +     case ASPEED_I2C_MASTER_RX:
> > > > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> > > > > +                     dev_err(bus->dev, "master failed to
> > > > > RX");
> > > > > +                     goto out_complete;
> > > > > +             }
> > > > 
> > > > See my comment above for a bog standard i2c_read. Aren't you 
> > > > getting the completion for the address before the read is even 
> > > > started ?
> > > 
> > > In practice no, but it is probably best to be safe :-)
> > 
> > Yup :)
> > > > 
> > > > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> > > > > +
> > > > > +             recv_byte = aspeed_i2c_read(bus,
> > > > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
> > > > > +             msg->buf[bus->buf_index++] = recv_byte;
> > > > > +
> > > > > +             if (msg->flags & I2C_M_RECV_LEN &&
> > > > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> > > > > +                     msg->len = recv_byte +
> > > > > +                                     ((msg->flags &
> > > > > I2C_CLIENT_PEC) ? 2 : 1);
> > > 
> > > ...
> > > > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> > > > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> > > > > +                     | ((clk_low <<
> > > > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> > > > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> > > > > +                     | (base_clk &
> > > > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> > > > > +}
> > > > 
> > > > As I think I mentioned earlier, the AST2500 has a slightly 
> > > > different register layout which support larger values for high
> > > > and 
> > > > low, thus allowing a finer granularity.
> > > 
> > > I am developing against the 2500.
> > 
> > Yes but we'd like the driver to work with both :-)
> 
> Right, I thought you were making an assertion about the 2500, if you
> are making an assertion about the 2400, I do not know and do not have
> one handy.
> 
> > 
> > > > BTW. In case you haven't, I would suggest you copy/paste the
> > > > above 
> > > > in a userspace app and run it for all frequency divisors and
> > > > see if 
> > > > your results match the aspeed table :)
> > > 
> > > Good call.
> > 
> > If you end up doing that, can you shoot it my way ? I can take care
> > of 
> > making sure it's all good for the 2400.
> 
> Will do.
> 
> > 
> > > > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> > > > > +                            struct platform_device *pdev) {
> > > > > +     u32 clk_freq, divisor;
> > > > > +     struct clk *pclk;
> > > > > +     int ret;
> > > > > +
> > > > > +     pclk = devm_clk_get(&pdev->dev, NULL);
> > > > > +     if (IS_ERR(pclk)) {
> > > > > +             dev_err(&pdev->dev, "clk_get failed\n");
> > > > > +             return PTR_ERR(pclk);
> > > > > +     }
> > > > > +     ret = of_property_read_u32(pdev->dev.of_node,
> > > > > +                                "clock-frequency",
> > > > > &clk_freq);
> > > > 
> > > > See my previous comment about calling that 'bus-frequency'
> > > > rather 
> > > > than 'clock-frequency'.
> > > > 
> > > > > +     if (ret < 0) {
> > > > > +             dev_err(&pdev->dev,
> > > > > +                     "Could not read clock-frequency
> > > > > property\n");
> > > > > +             clk_freq = 100000;
> > > > > +     }
> > > > > +     divisor = clk_get_rate(pclk) / clk_freq;
> > > > > +     /* We just need the clock rate, we don't actually use
> > > > > the
> > > > > clk object. */
> > > > > +     devm_clk_put(&pdev->dev, pclk);
> > > > > +
> > > > > +     /* Set AC Timing */
> > > > > +     if (clk_freq / 1000 > 1000) {
> > > > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> > > > > +                                                   ASPEED_I2
> > > > > C_FU
> > > > > N_CTRL_REG) |
> > > > > +                             ASPEED_I2CD_M_HIGH_SPEED_EN |
> > > > > +                             ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
> > > > > +                             ASPEED_I2CD_SDA_DRIVE_1T_EN,
> > > > > +                             ASPEED_I2C_FUN_CTRL_REG);
> > > > > +
> > > > > +             aspeed_i2c_write(bus, 0x3,
> > > > > ASPEED_I2C_AC_TIMING_REG2);
> > > > > +             aspeed_i2c_write(bus,
> > > > > aspeed_i2c_get_clk_reg_val(divisor),
> > > > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > > > 
> > > > I already discussed by doubts about the above. I can try to
> > > > scope 
> > > > it with the EVB if you don't get to it. For now I'd rather take
> > > > the 
> > > > code out.
> > > > 
> > > > We should ask aspeed from what frequency the "1T" stuff is
> > > > useful.
> > > 
> > > Will do, I will try to rope Ryan in on the next review; it will
> > > be 
> > > good for him to get used to working with upstream anyway.
> > 
> > Yup. However, for the sake of getting something upstream (and in 
> > OpenBMC 4.10 kernel) asap, I would suggest just dropping support
> > for 
> > those fast speeds for now, we can add them back later.
> 
> Alright, that's fine. Still, Ryan, could you provide some context on
> this?
> 
> > 
> > > > 
> > > > > +     } else {
> > > > > +             aspeed_i2c_write(bus,
> > > > > aspeed_i2c_get_clk_reg_val(divisor),
> > > > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > > > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> > > > > +                              ASPEED_I2C_AC_TIMING_REG2);
> > > > > +     }
> > > 
> > > ...
> > > > > +     spin_lock_init(&bus->lock);
> > > > > +     init_completion(&bus->cmd_complete);
> > > > > +     bus->adap.owner = THIS_MODULE;
> > > > > +     bus->adap.retries = 0;
> > > > > +     bus->adap.timeout = 5 * HZ;
> > > > > +     bus->adap.algo = &aspeed_i2c_algo;
> > > > > +     bus->adap.algo_data = bus;
> > > > > +     bus->adap.dev.parent = &pdev->dev;
> > > > > +     bus->adap.dev.of_node = pdev->dev.of_node;
> > > > > +     snprintf(bus->adap.name, sizeof(bus->adap.name),
> > > > > "Aspeed
> > > > > i2c");
> > > > 
> > > > Another trivial one, should we put some kind of bus number in
> > > > that 
> > > > string ?
> > > 
> > > Whoops, looks like I missed this one; I will get to it in the
> > > next 
> > > revision.
> > 
> > Ok. I noticed you missed that in v7, so I assume you mean v8 :-)
> 
> Yep, I will get it in v8.
> 
> > 
> > > > 
> > > > > +     bus->dev = &pdev->dev;
> > > > > +
> > > > > +     /* reset device: disable master & slave functions */
> > > > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> > > 
> > > ...
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
> > > devicetree"
> > > in
> > > the body of a message to majordomo@vger.kernel.org More
> > > majordomo 
> > > info at  http://vger.kernel.org/majordomo-info.html

Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ?

About ASPEED_I2CD_M_HIGH_SPEED_EN, it is support for I2C specification "High speed transfer". And also device need support it. 
If you just speed up the I2C bus clock, you don’t have to enable ASPEED_I2CD_M_HIGH_SPEED_EN, just change the clock is ok.


-----Original Message-----
From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] 

Sent: Tuesday, April 25, 2017 5:35 PM
To: Ryan Chen <ryan_chen@aspeedtech.com>; Brendan Higgins <brendanhiggins@google.com>
Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
> Hello All,

> 		ASPEED_I2CD_M_SDA_DRIVE_1T_EN,

> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage. 

> 		For example, if i2c bus is use on "high speed" and "single slave and 

> master" and i2c bus is too long. It need drive SDA or SCL less lunacy. 

> It would enable it.

> 		Otherwise, don’t enable it. especially in multi-master. 

> It can’t be enable.


That smells like a specific enough use case that we should probably cover with a device-tree property, something like an empty "sda-extra-drive" property (empty properties are typically used for booleans, their presence means "true").

Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ? Does it force to a specific speed (ignoring the
divisor) or we can still play with the clock high/low counts ?

Cheers,
Ben.

> 		  

> 	

> 

> Best Regards,

> Ryan

> 

> 信驊科技股份有限公司

> ASPEED Technology Inc.

> 2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City 

> 30077, Taiwan

> Tel: 886-3-578-9568  #857

> Fax: 886-3-578-9586

> ************* Email Confidentiality Notice ********************

> DISCLAIMER:

> This message (and any attachments) may contain legally privileged 

> and/or other confidential information. If you have received it in 

> error, please notify the sender by reply e-mail and immediately delete 

> the e-mail and any attachments without copying or disclosing the 

> contents. Thank you.

> 

> 

> -----Original Message-----

> From: Brendan Higgins [mailto:brendanhiggins@google.com]

> Sent: Tuesday, April 25, 2017 4:32 PM

> To: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org

> >; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutro

> nix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyng 

> ier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@m 

> leia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod 

> .org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux 

> Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist 

> <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>

> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

> 

> Adding Ryan.

> 

> On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.

> crashing.org> wrote:

> > On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:

> > > > > +struct aspeed_i2c_bus {

> > > > > +     struct i2c_adapter              adap;

> > > > > +     struct device                   *dev;

> > > > > +     void __iomem                    *base;

> > > > > +     /* Synchronizes I/O mem access to base. */

> > > > > +     spinlock_t                      lock;

> > > > 

> > > > I am not entirely convinced we need that lock. The i2c core will 

> > > > take a mutex protecting all operations on the bus. So we only 

> > > > need to synchronize between our "xfer" code and our interrupt 

> > > > handler.

> > > 

> > > You are right if both having slave and master active at the same 

> > > time was not possible; however, it is.

> > 

> > Right, I somewhat forgot about the slave case.

> > 

> >   ...

> > 

> > > > Some of those error states probably also warrant a reset of the 

> > > > controller, I think aspeed does that in the SDK.

> > > 

> > > For timeout and cmd_err, I do not see any argument against it; it 

> > > sounds like we are in a very messed up, very unknown state, so 

> > > full reset is probably the best last resort.

> > 

> > Yup.

> > 

> > > For SDA staying pulled down, I

> > > think we can say with reasonable confidence that some device on 

> > > our bus is behaving very badly and I am not convinced that 

> > > resetting the controller is likely to do anything to help;

> > 

> > Right. Hammering with STOPs and pray ...

> 

> I think sending recovery mode sends stops as a part of the recovery 

> algorithm it executes.

> 

> > 

> > >  that being said, I really

> > > do not have any good ideas to address that. So maybe praying and 

> > > resetting the controller is *the most reasonable thing to do.* I 

> > > would like to know what you think we should do in that case.

> > 

> > Well, there's a (small ?) chance that it's a controller bug 

> > asserting the line so ... but there's little we can do if not.

> 

> True.

> 

> > 

> > > While I was thinking about this I also realized that the SDA line 

> > > check after recovery happens in the else branch, but SCL line 

> > > check does not happen after we attempt to STOP if SCL is hung. If 

> > > we decide to make special note SDA being hung by a device that 

> > > won't let go, we might want to make a special note that SCL is 

> > > hung by a device that won't let go. Just a thought.

> > 

> > Maybe. Or just "unrecoverable error"... hopefully these don't happen 

> > too often ... We had cases of a TPM misbehaving like that.

> 

> Yeah, definitely should print something out.

> 

> > 

> > > > > +out:

> > > 

> > > ...

> > > > What about I2C_M_NOSTART ?

> > > > 

> > > > Not that I've ever seen it used... ;-)

> > > 

> > > Right now I am not doing any of the protocol mangling options, but 

> > > I can add them in if you think it is important for initial 

> > > support.

> > 

> > No, not important, we can add that later if it ever becomes useful.

> > 

> >  ...

> > 

> > > > In general, you always ACK all interrupts first. Then you handle 

> > > > the bits you have harvested.

> > > > 

> > > 

> > > The documentation says to ACK the interrupt after handling in the 

> > > RX

> > > case:

> > > 

> > > <<<

> > > S/W needs to clear this status bit to allow next data receiving.

> > > > > > 

> > > 

> > > I will double check with Ryan to make sure TX works the same way.

> > > 

> > > > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||

> > > > > +         (!bus->msgs && bus->master_state !=

> > > > > ASPEED_I2C_MASTER_STOP)) {

> > > 

> > > ...

> > > > 

> > > > I would set master_state to "RECOVERY" (new state ?) and ensure 

> > > > those things are caught if they happen outside of a recovery.

> > 

> > I replied privately ... as long as we ack before we start a new 

> > command we should be ok but we shouldn't ack after.

> > 

> > Your latest patch still does that. It will do things like start a 

> > STOP command *then* ack the status bits. I'm pretty sure that's 

> > bogus.

> > 

> > That way it's a lot simpler to simply move the

> > 

> >         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);

> > 

> > To either right after the readl of the status reg at the beginning 

> > of aspeed_i2c_master_irq().

> > 

> > I would be very surprised if that didn't work properly and wasn't 

> > much safer than what you are currently doing.

> 

> I think I tried your way and it worked. In anycase, Ryan will be able 

> to clarify for us.

> 

> > 

> > > Let me know if you still think we need a "RECOVERY" state.

> > 

> > The way you just switch to stop state and store the error for later 

> > should work I think.

> > 

> > > > 

> > > > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {

> > > 

> > > ...

> > > > 

> > > > > +                     dev_dbg(bus->dev,

> > > > > +                             "no slave present at %02x",

> > > > > msg-

> > > > > > addr);

> > > > > 

> > > > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;

> > > > > +                     bus->cmd_err = -EIO;

> > > > > +                     do_stop(bus);

> > > > > +                     goto out_no_complete;

> > > > > +             } else {

> > > > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;

> > > > > +                     if (msg->flags & I2C_M_RD)

> > > > > +                             bus->master_state =

> > > > > ASPEED_I2C_MASTER_RX;

> > > > > +                     else

> > > > > +                             bus->master_state =

> > > > > ASPEED_I2C_MASTER_TX_FIRST;

> > > > 

> > > > What about the SMBUS_QUICK case ? (0-len transfer). Do we need 

> > > > to handle this here ? A quick look at the TX_FIRST case makes me 

> > > > think we are ok there but I'm not sure about the RX case.

> > > 

> > > I did not think that there is an SMBUS_QUICK RX. Could you point 

> > > me to an example?

> > 

> > Not so much an RX, it's more like you are sending a 1-bit data in 

> > the place of the Rd/Wr bit. So you have a read with a lenght of 0, I 

> > don't think in that case you should set ASPEED_I2CD_M_RX_CMD in 

> > __aspeed_i2c_do_start

> 

> Forget what I said, I was just not thinking about the fact that SMBus 

> emulation causes the data bit to be encoded as the R/W flag. I see 

> what you are saying; you are correct.

> 

> > 

> > > > I'm not sure the RX case is tight also. What completion does the 

> > > > HW give you for the address cycle ? Won't you get that before it 

> > > > has received the first character ? IE. You fall through to the 

> > > > read case of the state machine with the read potentially not 

> > > > complete yet no ?

> > > 

> > > ...

> > > > > +     case ASPEED_I2C_MASTER_RX:

> > > > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {

> > > > > +                     dev_err(bus->dev, "master failed to

> > > > > RX");

> > > > > +                     goto out_complete;

> > > > > +             }

> > > > 

> > > > See my comment above for a bog standard i2c_read. Aren't you 

> > > > getting the completion for the address before the read is even 

> > > > started ?

> > > 

> > > In practice no, but it is probably best to be safe :-)

> > 

> > Yup :)

> > > > 

> > > > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;

> > > > > +

> > > > > +             recv_byte = aspeed_i2c_read(bus,

> > > > > ASPEED_I2C_BYTE_BUF_REG) >> 8;

> > > > > +             msg->buf[bus->buf_index++] = recv_byte;

> > > > > +

> > > > > +             if (msg->flags & I2C_M_RECV_LEN &&

> > > > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {

> > > > > +                     msg->len = recv_byte +

> > > > > +                                     ((msg->flags &

> > > > > I2C_CLIENT_PEC) ? 2 : 1);

> > > 

> > > ...

> > > > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)

> > > > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)

> > > > > +                     | ((clk_low <<

> > > > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)

> > > > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)

> > > > > +                     | (base_clk &

> > > > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);

> > > > > +}

> > > > 

> > > > As I think I mentioned earlier, the AST2500 has a slightly 

> > > > different register layout which support larger values for high 

> > > > and low, thus allowing a finer granularity.

> > > 

> > > I am developing against the 2500.

> > 

> > Yes but we'd like the driver to work with both :-)

> 

> Right, I thought you were making an assertion about the 2500, if you 

> are making an assertion about the 2400, I do not know and do not have 

> one handy.

> 

> > 

> > > > BTW. In case you haven't, I would suggest you copy/paste the 

> > > > above in a userspace app and run it for all frequency divisors 

> > > > and see if your results match the aspeed table :)

> > > 

> > > Good call.

> > 

> > If you end up doing that, can you shoot it my way ? I can take care 

> > of making sure it's all good for the 2400.

> 

> Will do.

> 

> > 

> > > > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,

> > > > > +                            struct platform_device *pdev) {

> > > > > +     u32 clk_freq, divisor;

> > > > > +     struct clk *pclk;

> > > > > +     int ret;

> > > > > +

> > > > > +     pclk = devm_clk_get(&pdev->dev, NULL);

> > > > > +     if (IS_ERR(pclk)) {

> > > > > +             dev_err(&pdev->dev, "clk_get failed\n");

> > > > > +             return PTR_ERR(pclk);

> > > > > +     }

> > > > > +     ret = of_property_read_u32(pdev->dev.of_node,

> > > > > +                                "clock-frequency",

> > > > > &clk_freq);

> > > > 

> > > > See my previous comment about calling that 'bus-frequency'

> > > > rather

> > > > than 'clock-frequency'.

> > > > 

> > > > > +     if (ret < 0) {

> > > > > +             dev_err(&pdev->dev,

> > > > > +                     "Could not read clock-frequency

> > > > > property\n");

> > > > > +             clk_freq = 100000;

> > > > > +     }

> > > > > +     divisor = clk_get_rate(pclk) / clk_freq;

> > > > > +     /* We just need the clock rate, we don't actually use

> > > > > the

> > > > > clk object. */

> > > > > +     devm_clk_put(&pdev->dev, pclk);

> > > > > +

> > > > > +     /* Set AC Timing */

> > > > > +     if (clk_freq / 1000 > 1000) {

> > > > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,

> > > > > +                                                   ASPEED_I2

> > > > > C_FU

> > > > > N_CTRL_REG) |

> > > > > +                             ASPEED_I2CD_M_HIGH_SPEED_EN |

> > > > > +                             ASPEED_I2CD_M_SDA_DRIVE_1T_EN |

> > > > > +                             ASPEED_I2CD_SDA_DRIVE_1T_EN,

> > > > > +                             ASPEED_I2C_FUN_CTRL_REG);

> > > > > +

> > > > > +             aspeed_i2c_write(bus, 0x3,

> > > > > ASPEED_I2C_AC_TIMING_REG2);

> > > > > +             aspeed_i2c_write(bus,

> > > > > aspeed_i2c_get_clk_reg_val(divisor),

> > > > > +                              ASPEED_I2C_AC_TIMING_REG1);

> > > > 

> > > > I already discussed by doubts about the above. I can try to 

> > > > scope it with the EVB if you don't get to it. For now I'd rather 

> > > > take the code out.

> > > > 

> > > > We should ask aspeed from what frequency the "1T" stuff is 

> > > > useful.

> > > 

> > > Will do, I will try to rope Ryan in on the next review; it will be 

> > > good for him to get used to working with upstream anyway.

> > 

> > Yup. However, for the sake of getting something upstream (and in 

> > OpenBMC 4.10 kernel) asap, I would suggest just dropping support for 

> > those fast speeds for now, we can add them back later.

> 

> Alright, that's fine. Still, Ryan, could you provide some context on 

> this?

> 

> > 

> > > > 

> > > > > +     } else {

> > > > > +             aspeed_i2c_write(bus,

> > > > > aspeed_i2c_get_clk_reg_val(divisor),

> > > > > +                              ASPEED_I2C_AC_TIMING_REG1);

> > > > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,

> > > > > +                              ASPEED_I2C_AC_TIMING_REG2);

> > > > > +     }

> > > 

> > > ...

> > > > > +     spin_lock_init(&bus->lock);

> > > > > +     init_completion(&bus->cmd_complete);

> > > > > +     bus->adap.owner = THIS_MODULE;

> > > > > +     bus->adap.retries = 0;

> > > > > +     bus->adap.timeout = 5 * HZ;

> > > > > +     bus->adap.algo = &aspeed_i2c_algo;

> > > > > +     bus->adap.algo_data = bus;

> > > > > +     bus->adap.dev.parent = &pdev->dev;

> > > > > +     bus->adap.dev.of_node = pdev->dev.of_node;

> > > > > +     snprintf(bus->adap.name, sizeof(bus->adap.name),

> > > > > "Aspeed

> > > > > i2c");

> > > > 

> > > > Another trivial one, should we put some kind of bus number in 

> > > > that string ?

> > > 

> > > Whoops, looks like I missed this one; I will get to it in the next 

> > > revision.

> > 

> > Ok. I noticed you missed that in v7, so I assume you mean v8 :-)

> 

> Yep, I will get it in v8.

> 

> > 

> > > > 

> > > > > +     bus->dev = &pdev->dev;

> > > > > +

> > > > > +     /* reset device: disable master & slave functions */

> > > > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);

> > > 

> > > ...

> > > --

> > > To unsubscribe from this list: send the line "unsubscribe 

> > > devicetree"

> > > in

> > > the body of a message to majordomo@vger.kernel.org More majordomo 

> > > info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 25, 2017 at 2:47 AM, Ryan Chen <ryan_chen@aspeedtech.com> wrote:
> Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ?
>
> About ASPEED_I2CD_M_HIGH_SPEED_EN, it is support for I2C specification "High speed transfer". And also device need support it.
> If you just speed up the I2C bus clock, you don’t have to enable ASPEED_I2CD_M_HIGH_SPEED_EN, just change the clock is ok.
>

Interesting, I thought that ASPEED_I2CD_M_SDA_DRIVE_1T_EN and its
counterpart would be used for fast mode or fast mode plus and
ASPEED_I2CD_M_HIGH_SPEED_EN would be used for fast mode plus or high
speed mode and that they work by driving the SDA and SCL signals to
improve rise times. It made sense to me because the lowest SCL you can
get with base clock set to zero is about ~1.5MHz which is in between
fast mode plus (1MHz) and high speed mode (3.4MHz).

But from what you are saying, ASPEED_I2CD_M_SDA_DRIVE_1T_EN and its
counterpart are totally orthogonal to the selected speed and
ASPEED_I2CD_M_HIGH_SPEED_EN exists as a matter of convenience to set
all of the divider registers to their smallest possible values. Is my
understanding correct?

>
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Tuesday, April 25, 2017 5:35 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Brendan Higgins <brendanhiggins@google.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>
> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
>
> On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
>> Hello All,
>>               ASPEED_I2CD_M_SDA_DRIVE_1T_EN,
>> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage.
>>               For example, if i2c bus is use on "high speed" and "single slave and
>> master" and i2c bus is too long. It need drive SDA or SCL less lunacy.
>> It would enable it.
>>               Otherwise, don’t enable it. especially in multi-master.
>> It can’t be enable.
>
> That smells like a specific enough use case that we should probably cover with a device-tree property, something like an empty "sda-extra-drive" property (empty properties are typically used for booleans, their presence means "true").
>
> Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ? Does it force to a specific speed (ignoring the
> divisor) or we can still play with the clock high/low counts ?
>
...
>> > Your latest patch still does that. It will do things like start a
>> > STOP command *then* ack the status bits. I'm pretty sure that's
>> > bogus.
>> >
>> > That way it's a lot simpler to simply move the
>> >
>> >         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> >
>> > To either right after the readl of the status reg at the beginning
>> > of aspeed_i2c_master_irq().
>> >
>> > I would be very surprised if that didn't work properly and wasn't
>> > much safer than what you are currently doing.
>>
>> I think I tried your way and it worked. In anycase, Ryan will be able
>> to clarify for us.

After thinking about this more, I think Ben is right. It would be
unusual for such a common convention to be broken and even if it is, I
do not see how a command could take effect until it is actually
issued. Nevertheless, it would make me feel better if you, Ryan, could
comment on this.

>>
>> >
>> > > Let me know if you still think we need a "RECOVERY" state.
>> >
...
I feel pretty good about this; it does not look like there will be a
lot of changes going into v8; hopefully, that version will be good
enough to get merged.

> Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ?

>

> About ASPEED_I2CD_M_HIGH_SPEED_EN, it is support for I2C specification "High speed transfer". And also device need support it.

> If you just speed up the I2C bus clock, you don’t have to enable ASPEED_I2CD_M_HIGH_SPEED_EN, just change the clock is ok.

>


>Interesting, I thought that ASPEED_I2CD_M_SDA_DRIVE_1T_EN and its counterpart would be used for fast mode or fast mode plus and ASPEED_I2CD_M_HIGH_SPEED_EN would be used for fast mode plus or high speed mode and that they work by driving the SDA and SCL signals to >improve rise times. It made sense to me because the lowest SCL you can get with base clock set to zero is about ~1.5MHz which is in between fast mode plus (1MHz) and high speed mode (3.4MHz).


>But from what you are saying, ASPEED_I2CD_M_SDA_DRIVE_1T_EN and its counterpart are totally orthogonal to the selected speed and ASPEED_I2CD_M_HIGH_SPEED_EN exists as a matter of convenience to set all of the divider registers to their smallest possible values. Is my >

>understanding correct?



In I2c specification[http://www.csd.uoc.gr/~hy428/reading/i2c_spec.pdf] there have a chapter about high speed transfer. It will start from specific command (00001XXX) and after that can transfer to high speed mode. 
The following is our high speed mode programming guide. That also have description at AST2400 datasheet. 40.7.12



>

> -----Original Message-----

> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]

> Sent: Tuesday, April 25, 2017 5:35 PM

> To: Ryan Chen <ryan_chen@aspeedtech.com>; Brendan Higgins 

> <brendanhiggins@google.com>

> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring 

> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas 

> Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; 

> Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; 

> Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; 

> Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; 

> devicetree@vger.kernel.org; Linux Kernel Mailing List 

> <linux-kernel@vger.kernel.org>; OpenBMC Maillist 

> <openbmc@lists.ozlabs.org>

> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

>

> On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:

>> Hello All,

>>               ASPEED_I2CD_M_SDA_DRIVE_1T_EN, 

>> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage.

>>               For example, if i2c bus is use on "high speed" and 

>> "single slave and master" and i2c bus is too long. It need drive SDA or SCL less lunacy.

>> It would enable it.

>>               Otherwise, don’t enable it. especially in multi-master.

>> It can’t be enable.

>

> That smells like a specific enough use case that we should probably cover with a device-tree property, something like an empty "sda-extra-drive" property (empty properties are typically used for booleans, their presence means "true").

>

> Thanks Ryan. Can you shed some light on the meaning of the high-speed 

> bit as well please ? Does it force to a specific speed (ignoring the

> divisor) or we can still play with the clock high/low counts ?

>

...
>> > Your latest patch still does that. It will do things like start a 

>> > STOP command *then* ack the status bits. I'm pretty sure that's 

>> > bogus.

>> >

>> > That way it's a lot simpler to simply move the

>> >

>> >         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);

>> >

>> > To either right after the readl of the status reg at the beginning 

>> > of aspeed_i2c_master_irq().

>> >

>> > I would be very surprised if that didn't work properly and wasn't 

>> > much safer than what you are currently doing.

>>

>> I think I tried your way and it worked. In anycase, Ryan will be able 

>> to clarify for us.


After thinking about this more, I think Ben is right. It would be unusual for such a common convention to be broken and even if it is, I do not see how a command could take effect until it is actually issued. Nevertheless, it would make me feel better if you, Ryan, could comment on this.

>>

>> >

>> > > Let me know if you still think we need a "RECOVERY" state.

>> >

...
I feel pretty good about this; it does not look like there will be a lot of changes going into v8; hopefully, that version will be good enough to get merged.