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

Submitted by Brendan Higgins on March 28, 2017, 5:12 a.m.

Details

Message ID 20170328051226.21677-5-brendanhiggins@google.com
State New
Headers show

Commit Message

Brendan Higgins March 28, 2017, 5:12 a.m.
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

Comments

Benjamin Herrenschmidt March 28, 2017, 8:57 a.m.
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt March 28, 2017, 9:09 a.m.
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brendan Higgins March 29, 2017, 10:23 a.m.
>> +                               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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joel Stanley March 31, 2017, 12:33 a.m.
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt March 31, 2017, 7:33 a.m.
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");
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brendan Higgins April 24, 2017, 6:56 p.m.
>> +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);
...
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt April 25, 2017, 2:19 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brendan Higgins April 25, 2017, 8:32 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ryan Chen April 25, 2017, 8:50 a.m.
SGVsbG8gQWxsLA0KCQlBU1BFRURfSTJDRF9NX1NEQV9EUklWRV8xVF9FTiwgQVNQRUVEX0kyQ0Rf
U0RBX0RSSVZFXzFUX0VOIGlzIHNwZWNpZmljIGZvciBzb21lIGNhc2UgdXNhZ2UuIA0KCQlGb3Ig
ZXhhbXBsZSwgaWYgaTJjIGJ1cyBpcyB1c2Ugb24gImhpZ2ggc3BlZWQiIGFuZCAic2luZ2xlIHNs
YXZlIGFuZCBtYXN0ZXIiIGFuZCBpMmMgYnVzIGlzIHRvbyBsb25nLiBJdCBuZWVkIGRyaXZlIFNE
QSBvciBTQ0wgbGVzcyBsdW5hY3kuIEl0IHdvdWxkIGVuYWJsZSBpdC4gDQoJCU90aGVyd2lzZSwg
ZG9u4oCZdCBlbmFibGUgaXQuIGVzcGVjaWFsbHkgaW4gbXVsdGktbWFzdGVyLiBJdCBjYW7igJl0
IGJlIGVuYWJsZS4gDQoNCgkJICANCgkNCg0KQmVzdCBSZWdhcmRzLA0KUnlhbg0KDQrkv6HpqYrn
p5HmioDogqHku73mnInpmZDlhazlj7gNCkFTUEVFRCBUZWNobm9sb2d5IEluYy4NCjJGLE5vLjE1
LEluZHVzdHJ5IEVhc3QgUm9hZCA0LixIc2luY2h1IFNjaWVuY2UgUGFyaywgSHNpbmNodSBDaXR5
IDMwMDc3LCBUYWl3YW4NClRlbDogODg2LTMtNTc4LTk1NjjCoCAjODU3DQpGYXg6IDg4Ni0zLTU3
OC05NTg2DQoqKioqKioqKioqKioqIEVtYWlsIENvbmZpZGVudGlhbGl0eSBOb3RpY2UgKioqKioq
KioqKioqKioqKioqKioNCkRJU0NMQUlNRVI6DQpUaGlzIG1lc3NhZ2UgKGFuZCBhbnkgYXR0YWNo
bWVudHMpIG1heSBjb250YWluIGxlZ2FsbHkgcHJpdmlsZWdlZCBhbmQvb3Igb3RoZXIgY29uZmlk
ZW50aWFsIGluZm9ybWF0aW9uLiBJZiB5b3UgaGF2ZSByZWNlaXZlZCBpdCBpbiBlcnJvciwgcGxl
YXNlIG5vdGlmeSB0aGUgc2VuZGVyIGJ5IHJlcGx5IGUtbWFpbCBhbmQgaW1tZWRpYXRlbHkgZGVs
ZXRlIHRoZSBlLW1haWwgYW5kIGFueSBhdHRhY2htZW50cyB3aXRob3V0IGNvcHlpbmcgb3IgZGlz
Y2xvc2luZyB0aGUgY29udGVudHMuIFRoYW5rIHlvdS4NCg0KDQotLS0tLU9yaWdpbmFsIE1lc3Nh
Z2UtLS0tLQ0KRnJvbTogQnJlbmRhbiBIaWdnaW5zIFttYWlsdG86YnJlbmRhbmhpZ2dpbnNAZ29v
Z2xlLmNvbV0gDQpTZW50OiBUdWVzZGF5LCBBcHJpbCAyNSwgMjAxNyA0OjMyIFBNDQpUbzogQmVu
amFtaW4gSGVycmVuc2NobWlkdCA8YmVuaEBrZXJuZWwuY3Jhc2hpbmcub3JnPg0KQ2M6IFdvbGZy
YW0gU2FuZyA8d3NhQHRoZS1kcmVhbXMuZGU+OyBSb2IgSGVycmluZyA8cm9iaCtkdEBrZXJuZWwu
b3JnPjsgTWFyayBSdXRsYW5kIDxtYXJrLnJ1dGxhbmRAYXJtLmNvbT47IFRob21hcyBHbGVpeG5l
ciA8dGdseEBsaW51dHJvbml4LmRlPjsgSmFzb24gQ29vcGVyIDxqYXNvbkBsYWtlZGFlbW9uLm5l
dD47IE1hcmMgWnluZ2llciA8bWFyYy56eW5naWVyQGFybS5jb20+OyBKb2VsIFN0YW5sZXkgPGpv
ZWxAam1zLmlkLmF1PjsgVmxhZGltaXIgWmFwb2xza2l5IDx2ekBtbGVpYS5jb20+OyBLYWNoYWxv
diBBbnRvbiA8bW91c2VAbWF5Yy5ydT47IEPDqWRyaWMgTGUgR29hdGVyIDxjbGdAa2FvZC5vcmc+
OyBsaW51eC1pMmNAdmdlci5rZXJuZWwub3JnOyBkZXZpY2V0cmVlQHZnZXIua2VybmVsLm9yZzsg
TGludXggS2VybmVsIE1haWxpbmcgTGlzdCA8bGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZz47
IE9wZW5CTUMgTWFpbGxpc3QgPG9wZW5ibWNAbGlzdHMub3psYWJzLm9yZz47IFJ5YW4gQ2hlbiA8
cnlhbl9jaGVuQGFzcGVlZHRlY2guY29tPg0KU3ViamVjdDogUmU6IFtQQVRDSCB2NiA0LzVdIGky
YzogYXNwZWVkOiBhZGRlZCBkcml2ZXIgZm9yIEFzcGVlZCBJMkMNCg0KQWRkaW5nIFJ5YW4uDQoN
Ck9uIE1vbiwgQXByIDI0LCAyMDE3IGF0IDc6MTkgUE0sIEJlbmphbWluIEhlcnJlbnNjaG1pZHQg
PGJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZz4gd3JvdGU6DQo+IE9uIE1vbiwgMjAxNy0wNC0yNCBh
dCAxMTo1NiAtMDcwMCwgQnJlbmRhbiBIaWdnaW5zIHdyb3RlOg0KPj4gPiA+ICtzdHJ1Y3QgYXNw
ZWVkX2kyY19idXMgew0KPj4gPiA+ICsgICAgIHN0cnVjdCBpMmNfYWRhcHRlciAgICAgICAgICAg
ICAgYWRhcDsNCj4+ID4gPiArICAgICBzdHJ1Y3QgZGV2aWNlICAgICAgICAgICAgICAgICAgICpk
ZXY7DQo+PiA+ID4gKyAgICAgdm9pZCBfX2lvbWVtICAgICAgICAgICAgICAgICAgICAqYmFzZTsN
Cj4+ID4gPiArICAgICAvKiBTeW5jaHJvbml6ZXMgSS9PIG1lbSBhY2Nlc3MgdG8gYmFzZS4gKi8N
Cj4+ID4gPiArICAgICBzcGlubG9ja190ICAgICAgICAgICAgICAgICAgICAgIGxvY2s7DQo+PiA+
DQo+PiA+IEkgYW0gbm90IGVudGlyZWx5IGNvbnZpbmNlZCB3ZSBuZWVkIHRoYXQgbG9jay4gVGhl
IGkyYyBjb3JlIHdpbGwgDQo+PiA+IHRha2UgYSBtdXRleCBwcm90ZWN0aW5nIGFsbCBvcGVyYXRp
b25zIG9uIHRoZSBidXMuIFNvIHdlIG9ubHkgbmVlZCANCj4+ID4gdG8gc3luY2hyb25pemUgYmV0
d2VlbiBvdXIgInhmZXIiIGNvZGUgYW5kIG91ciBpbnRlcnJ1cHQgaGFuZGxlci4NCj4+DQo+PiBZ
b3UgYXJlIHJpZ2h0IGlmIGJvdGggaGF2aW5nIHNsYXZlIGFuZCBtYXN0ZXIgYWN0aXZlIGF0IHRo
ZSBzYW1lIHRpbWUgDQo+PiB3YXMgbm90IHBvc3NpYmxlOyBob3dldmVyLCBpdCBpcy4NCj4NCj4g
UmlnaHQsIEkgc29tZXdoYXQgZm9yZ290IGFib3V0IHRoZSBzbGF2ZSBjYXNlLg0KPg0KPiAgIC4u
Lg0KPg0KPj4gPiBTb21lIG9mIHRob3NlIGVycm9yIHN0YXRlcyBwcm9iYWJseSBhbHNvIHdhcnJh
bnQgYSByZXNldCBvZiB0aGUgDQo+PiA+IGNvbnRyb2xsZXIsIEkgdGhpbmsgYXNwZWVkIGRvZXMg
dGhhdCBpbiB0aGUgU0RLLg0KPj4NCj4+IEZvciB0aW1lb3V0IGFuZCBjbWRfZXJyLCBJIGRvIG5v
dCBzZWUgYW55IGFyZ3VtZW50IGFnYWluc3QgaXQ7IGl0IA0KPj4gc291bmRzIGxpa2Ugd2UgYXJl
IGluIGEgdmVyeSBtZXNzZWQgdXAsIHZlcnkgdW5rbm93biBzdGF0ZSwgc28gZnVsbCANCj4+IHJl
c2V0IGlzIHByb2JhYmx5IHRoZSBiZXN0IGxhc3QgcmVzb3J0Lg0KPg0KPiBZdXAuDQo+DQo+PiBG
b3IgU0RBIHN0YXlpbmcgcHVsbGVkIGRvd24sIEkNCj4+IHRoaW5rIHdlIGNhbiBzYXkgd2l0aCBy
ZWFzb25hYmxlIGNvbmZpZGVuY2UgdGhhdCBzb21lIGRldmljZSBvbiBvdXIgDQo+PiBidXMgaXMg
YmVoYXZpbmcgdmVyeSBiYWRseSBhbmQgSSBhbSBub3QgY29udmluY2VkIHRoYXQgcmVzZXR0aW5n
IHRoZSANCj4+IGNvbnRyb2xsZXIgaXMgbGlrZWx5IHRvIGRvIGFueXRoaW5nIHRvIGhlbHA7DQo+
DQo+IFJpZ2h0LiBIYW1tZXJpbmcgd2l0aCBTVE9QcyBhbmQgcHJheSAuLi4NCg0KSSB0aGluayBz
ZW5kaW5nIHJlY292ZXJ5IG1vZGUgc2VuZHMgc3RvcHMgYXMgYSBwYXJ0IG9mIHRoZSByZWNvdmVy
eSBhbGdvcml0aG0gaXQgZXhlY3V0ZXMuDQoNCj4NCj4+ICB0aGF0IGJlaW5nIHNhaWQsIEkgcmVh
bGx5DQo+PiBkbyBub3QgaGF2ZSBhbnkgZ29vZCBpZGVhcyB0byBhZGRyZXNzIHRoYXQuIFNvIG1h
eWJlIHByYXlpbmcgYW5kIA0KPj4gcmVzZXR0aW5nIHRoZSBjb250cm9sbGVyIGlzICp0aGUgbW9z
dCByZWFzb25hYmxlIHRoaW5nIHRvIGRvLiogSSANCj4+IHdvdWxkIGxpa2UgdG8ga25vdyB3aGF0
IHlvdSB0aGluayB3ZSBzaG91bGQgZG8gaW4gdGhhdCBjYXNlLg0KPg0KPiBXZWxsLCB0aGVyZSdz
IGEgKHNtYWxsID8pIGNoYW5jZSB0aGF0IGl0J3MgYSBjb250cm9sbGVyIGJ1ZyBhc3NlcnRpbmcg
DQo+IHRoZSBsaW5lIHNvIC4uLiBidXQgdGhlcmUncyBsaXR0bGUgd2UgY2FuIGRvIGlmIG5vdC4N
Cg0KVHJ1ZS4NCg0KPg0KPj4gV2hpbGUgSSB3YXMgdGhpbmtpbmcgYWJvdXQgdGhpcyBJIGFsc28g
cmVhbGl6ZWQgdGhhdCB0aGUgU0RBIGxpbmUgDQo+PiBjaGVjayBhZnRlciByZWNvdmVyeSBoYXBw
ZW5zIGluIHRoZSBlbHNlIGJyYW5jaCwgYnV0IFNDTCBsaW5lIGNoZWNrIA0KPj4gZG9lcyBub3Qg
aGFwcGVuIGFmdGVyIHdlIGF0dGVtcHQgdG8gU1RPUCBpZiBTQ0wgaXMgaHVuZy4gSWYgd2UgZGVj
aWRlIA0KPj4gdG8gbWFrZSBzcGVjaWFsIG5vdGUgU0RBIGJlaW5nIGh1bmcgYnkgYSBkZXZpY2Ug
dGhhdCB3b24ndCBsZXQgZ28sIHdlIA0KPj4gbWlnaHQgd2FudCB0byBtYWtlIGEgc3BlY2lhbCBu
b3RlIHRoYXQgU0NMIGlzIGh1bmcgYnkgYSBkZXZpY2UgdGhhdCANCj4+IHdvbid0IGxldCBnby4g
SnVzdCBhIHRob3VnaHQuDQo+DQo+IE1heWJlLiBPciBqdXN0ICJ1bnJlY292ZXJhYmxlIGVycm9y
Ii4uLiBob3BlZnVsbHkgdGhlc2UgZG9uJ3QgaGFwcGVuIA0KPiB0b28gb2Z0ZW4gLi4uIFdlIGhh
ZCBjYXNlcyBvZiBhIFRQTSBtaXNiZWhhdmluZyBsaWtlIHRoYXQuDQoNClllYWgsIGRlZmluaXRl
bHkgc2hvdWxkIHByaW50IHNvbWV0aGluZyBvdXQuDQoNCj4NCj4+ID4gPiArb3V0Og0KPj4NCj4+
IC4uLg0KPj4gPiBXaGF0IGFib3V0IEkyQ19NX05PU1RBUlQgPw0KPj4gPg0KPj4gPiBOb3QgdGhh
dCBJJ3ZlIGV2ZXIgc2VlbiBpdCB1c2VkLi4uIDstKQ0KPj4NCj4+IFJpZ2h0IG5vdyBJIGFtIG5v
dCBkb2luZyBhbnkgb2YgdGhlIHByb3RvY29sIG1hbmdsaW5nIG9wdGlvbnMsIGJ1dCBJIA0KPj4g
Y2FuIGFkZCB0aGVtIGluIGlmIHlvdSB0aGluayBpdCBpcyBpbXBvcnRhbnQgZm9yIGluaXRpYWwg
c3VwcG9ydC4NCj4NCj4gTm8sIG5vdCBpbXBvcnRhbnQsIHdlIGNhbiBhZGQgdGhhdCBsYXRlciBp
ZiBpdCBldmVyIGJlY29tZXMgdXNlZnVsLg0KPg0KPiAgLi4uDQo+DQo+PiA+IEluIGdlbmVyYWws
IHlvdSBhbHdheXMgQUNLIGFsbCBpbnRlcnJ1cHRzIGZpcnN0LiBUaGVuIHlvdSBoYW5kbGUgDQo+
PiA+IHRoZSBiaXRzIHlvdSBoYXZlIGhhcnZlc3RlZC4NCj4+ID4NCj4+DQo+PiBUaGUgZG9jdW1l
bnRhdGlvbiBzYXlzIHRvIEFDSyB0aGUgaW50ZXJydXB0IGFmdGVyIGhhbmRsaW5nIGluIHRoZSBS
WA0KPj4gY2FzZToNCj4+DQo+PiA8PDwNCj4+IFMvVyBuZWVkcyB0byBjbGVhciB0aGlzIHN0YXR1
cyBiaXQgdG8gYWxsb3cgbmV4dCBkYXRhIHJlY2VpdmluZy4NCj4+ID4gPiA+DQo+Pg0KPj4gSSB3
aWxsIGRvdWJsZSBjaGVjayB3aXRoIFJ5YW4gdG8gbWFrZSBzdXJlIFRYIHdvcmtzIHRoZSBzYW1l
IHdheS4NCj4+DQo+PiA+ID4gKyAgICAgaWYgKGlycV9zdGF0dXMgJiBBU1BFRURfSTJDRF9JTlRS
X0VSUk9SIHx8DQo+PiA+ID4gKyAgICAgICAgICghYnVzLT5tc2dzICYmIGJ1cy0+bWFzdGVyX3N0
YXRlICE9DQo+PiA+ID4gQVNQRUVEX0kyQ19NQVNURVJfU1RPUCkpIHsNCj4+DQo+PiAuLi4NCj4+
ID4NCj4+ID4gSSB3b3VsZCBzZXQgbWFzdGVyX3N0YXRlIHRvICJSRUNPVkVSWSIgKG5ldyBzdGF0
ZSA/KSBhbmQgZW5zdXJlIA0KPj4gPiB0aG9zZSB0aGluZ3MgYXJlIGNhdWdodCBpZiB0aGV5IGhh
cHBlbiBvdXRzaWRlIG9mIGEgcmVjb3ZlcnkuDQo+DQo+IEkgcmVwbGllZCBwcml2YXRlbHkgLi4u
IGFzIGxvbmcgYXMgd2UgYWNrIGJlZm9yZSB3ZSBzdGFydCBhIG5ldyANCj4gY29tbWFuZCB3ZSBz
aG91bGQgYmUgb2sgYnV0IHdlIHNob3VsZG4ndCBhY2sgYWZ0ZXIuDQo+DQo+IFlvdXIgbGF0ZXN0
IHBhdGNoIHN0aWxsIGRvZXMgdGhhdC4gSXQgd2lsbCBkbyB0aGluZ3MgbGlrZSBzdGFydCBhIFNU
T1AgDQo+IGNvbW1hbmQgKnRoZW4qIGFjayB0aGUgc3RhdHVzIGJpdHMuIEknbSBwcmV0dHkgc3Vy
ZSB0aGF0J3MgYm9ndXMuDQo+DQo+IFRoYXQgd2F5IGl0J3MgYSBsb3Qgc2ltcGxlciB0byBzaW1w
bHkgbW92ZSB0aGUNCj4NCj4gICAgICAgICB3cml0ZWwoaXJxX3N0YXR1cywgYnVzLT5iYXNlICsg
QVNQRUVEX0kyQ19JTlRSX1NUU19SRUcpOw0KPg0KPiBUbyBlaXRoZXIgcmlnaHQgYWZ0ZXIgdGhl
IHJlYWRsIG9mIHRoZSBzdGF0dXMgcmVnIGF0IHRoZSBiZWdpbm5pbmcgb2YgDQo+IGFzcGVlZF9p
MmNfbWFzdGVyX2lycSgpLg0KPg0KPiBJIHdvdWxkIGJlIHZlcnkgc3VycHJpc2VkIGlmIHRoYXQg
ZGlkbid0IHdvcmsgcHJvcGVybHkgYW5kIHdhc24ndCBtdWNoIA0KPiBzYWZlciB0aGFuIHdoYXQg
eW91IGFyZSBjdXJyZW50bHkgZG9pbmcuDQoNCkkgdGhpbmsgSSB0cmllZCB5b3VyIHdheSBhbmQg
aXQgd29ya2VkLiBJbiBhbnljYXNlLCBSeWFuIHdpbGwgYmUgYWJsZSB0byBjbGFyaWZ5IGZvciB1
cy4NCg0KPg0KPj4gTGV0IG1lIGtub3cgaWYgeW91IHN0aWxsIHRoaW5rIHdlIG5lZWQgYSAiUkVD
T1ZFUlkiIHN0YXRlLg0KPg0KPiBUaGUgd2F5IHlvdSBqdXN0IHN3aXRjaCB0byBzdG9wIHN0YXRl
IGFuZCBzdG9yZSB0aGUgZXJyb3IgZm9yIGxhdGVyIA0KPiBzaG91bGQgd29yayBJIHRoaW5rLg0K
Pg0KPj4gPg0KPj4gPiA+ICsgICAgIGlmIChidXMtPm1hc3Rlcl9zdGF0ZSA9PSBBU1BFRURfSTJD
X01BU1RFUl9TVEFSVCkgew0KPj4NCj4+IC4uLg0KPj4gPg0KPj4gPiA+ICsgICAgICAgICAgICAg
ICAgICAgICBkZXZfZGJnKGJ1cy0+ZGV2LA0KPj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICJubyBzbGF2ZSBwcmVzZW50IGF0ICUwMngiLCBtc2ctDQo+PiA+ID4gPmFkZHIpOw0K
Pj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICBzdGF0dXNfYWNrIHw9IEFTUEVFRF9JMkNEX0lO
VFJfVFhfTkFLOw0KPj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICBidXMtPmNtZF9lcnIgPSAt
RUlPOw0KPj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICBkb19zdG9wKGJ1cyk7DQo+PiA+ID4g
KyAgICAgICAgICAgICAgICAgICAgIGdvdG8gb3V0X25vX2NvbXBsZXRlOw0KPj4gPiA+ICsgICAg
ICAgICAgICAgfSBlbHNlIHsNCj4+ID4gPiArICAgICAgICAgICAgICAgICAgICAgc3RhdHVzX2Fj
ayB8PSBBU1BFRURfSTJDRF9JTlRSX1RYX0FDSzsNCj4+ID4gPiArICAgICAgICAgICAgICAgICAg
ICAgaWYgKG1zZy0+ZmxhZ3MgJiBJMkNfTV9SRCkNCj4+ID4gPiArICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICBidXMtPm1hc3Rlcl9zdGF0ZSA9DQo+PiA+ID4gQVNQRUVEX0kyQ19NQVNURVJf
Ulg7DQo+PiA+ID4gKyAgICAgICAgICAgICAgICAgICAgIGVsc2UNCj4+ID4gPiArICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICBidXMtPm1hc3Rlcl9zdGF0ZSA9DQo+PiA+ID4gQVNQRUVEX0ky
Q19NQVNURVJfVFhfRklSU1Q7DQo+PiA+DQo+PiA+IFdoYXQgYWJvdXQgdGhlIFNNQlVTX1FVSUNL
IGNhc2UgPyAoMC1sZW4gdHJhbnNmZXIpLiBEbyB3ZSBuZWVkIHRvIA0KPj4gPiBoYW5kbGUgdGhp
cyBoZXJlID8gQSBxdWljayBsb29rIGF0IHRoZSBUWF9GSVJTVCBjYXNlIG1ha2VzIG1lIHRoaW5r
IA0KPj4gPiB3ZSBhcmUgb2sgdGhlcmUgYnV0IEknbSBub3Qgc3VyZSBhYm91dCB0aGUgUlggY2Fz
ZS4NCj4+DQo+PiBJIGRpZCBub3QgdGhpbmsgdGhhdCB0aGVyZSBpcyBhbiBTTUJVU19RVUlDSyBS
WC4gQ291bGQgeW91IHBvaW50IG1lIA0KPj4gdG8gYW4gZXhhbXBsZT8NCj4NCj4gTm90IHNvIG11
Y2ggYW4gUlgsIGl0J3MgbW9yZSBsaWtlIHlvdSBhcmUgc2VuZGluZyBhIDEtYml0IGRhdGEgaW4g
dGhlIA0KPiBwbGFjZSBvZiB0aGUgUmQvV3IgYml0LiBTbyB5b3UgaGF2ZSBhIHJlYWQgd2l0aCBh
IGxlbmdodCBvZiAwLCBJIGRvbid0IA0KPiB0aGluayBpbiB0aGF0IGNhc2UgeW91IHNob3VsZCBz
ZXQgQVNQRUVEX0kyQ0RfTV9SWF9DTUQgaW4gDQo+IF9fYXNwZWVkX2kyY19kb19zdGFydA0KDQpG
b3JnZXQgd2hhdCBJIHNhaWQsIEkgd2FzIGp1c3Qgbm90IHRoaW5raW5nIGFib3V0IHRoZSBmYWN0
IHRoYXQgU01CdXMgZW11bGF0aW9uIGNhdXNlcyB0aGUgZGF0YSBiaXQgdG8gYmUgZW5jb2RlZCBh
cyB0aGUgUi9XIGZsYWcuIEkgc2VlIHdoYXQgeW91IGFyZSBzYXlpbmc7IHlvdSBhcmUgY29ycmVj
dC4NCg0KPg0KPj4gPiBJJ20gbm90IHN1cmUgdGhlIFJYIGNhc2UgaXMgdGlnaHQgYWxzby4gV2hh
dCBjb21wbGV0aW9uIGRvZXMgdGhlIEhXIA0KPj4gPiBnaXZlIHlvdSBmb3IgdGhlIGFkZHJlc3Mg
Y3ljbGUgPyBXb24ndCB5b3UgZ2V0IHRoYXQgYmVmb3JlIGl0IGhhcyANCj4+ID4gcmVjZWl2ZWQg
dGhlIGZpcnN0IGNoYXJhY3RlciA/IElFLiBZb3UgZmFsbCB0aHJvdWdoIHRvIHRoZSByZWFkIA0K
Pj4gPiBjYXNlIG9mIHRoZSBzdGF0ZSBtYWNoaW5lIHdpdGggdGhlIHJlYWQgcG90ZW50aWFsbHkg
bm90IGNvbXBsZXRlIA0KPj4gPiB5ZXQgbm8gPw0KPj4NCj4+IC4uLg0KPj4gPiA+ICsgICAgIGNh
c2UgQVNQRUVEX0kyQ19NQVNURVJfUlg6DQo+PiA+ID4gKyAgICAgICAgICAgICBpZiAoIShpcnFf
c3RhdHVzICYgQVNQRUVEX0kyQ0RfSU5UUl9SWF9ET05FKSkgew0KPj4gPiA+ICsgICAgICAgICAg
ICAgICAgICAgICBkZXZfZXJyKGJ1cy0+ZGV2LCAibWFzdGVyIGZhaWxlZCB0byBSWCIpOw0KPj4g
PiA+ICsgICAgICAgICAgICAgICAgICAgICBnb3RvIG91dF9jb21wbGV0ZTsNCj4+ID4gPiArICAg
ICAgICAgICAgIH0NCj4+ID4NCj4+ID4gU2VlIG15IGNvbW1lbnQgYWJvdmUgZm9yIGEgYm9nIHN0
YW5kYXJkIGkyY19yZWFkLiBBcmVuJ3QgeW91IA0KPj4gPiBnZXR0aW5nIHRoZSBjb21wbGV0aW9u
IGZvciB0aGUgYWRkcmVzcyBiZWZvcmUgdGhlIHJlYWQgaXMgZXZlbiANCj4+ID4gc3RhcnRlZCA/
DQo+Pg0KPj4gSW4gcHJhY3RpY2Ugbm8sIGJ1dCBpdCBpcyBwcm9iYWJseSBiZXN0IHRvIGJlIHNh
ZmUgOi0pDQo+DQo+IFl1cCA6KQ0KPj4gPg0KPj4gPiA+ICsgICAgICAgICAgICAgc3RhdHVzX2Fj
ayB8PSBBU1BFRURfSTJDRF9JTlRSX1JYX0RPTkU7DQo+PiA+ID4gKw0KPj4gPiA+ICsgICAgICAg
ICAgICAgcmVjdl9ieXRlID0gYXNwZWVkX2kyY19yZWFkKGJ1cywNCj4+ID4gPiBBU1BFRURfSTJD
X0JZVEVfQlVGX1JFRykgPj4gODsNCj4+ID4gPiArICAgICAgICAgICAgIG1zZy0+YnVmW2J1cy0+
YnVmX2luZGV4KytdID0gcmVjdl9ieXRlOw0KPj4gPiA+ICsNCj4+ID4gPiArICAgICAgICAgICAg
IGlmIChtc2ctPmZsYWdzICYgSTJDX01fUkVDVl9MRU4gJiYNCj4+ID4gPiArICAgICAgICAgICAg
ICAgICByZWN2X2J5dGUgPD0gSTJDX1NNQlVTX0JMT0NLX01BWCkgew0KPj4gPiA+ICsgICAgICAg
ICAgICAgICAgICAgICBtc2ctPmxlbiA9IHJlY3ZfYnl0ZSArDQo+PiA+ID4gKyAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAoKG1zZy0+ZmxhZ3MgJg0KPj4gPiA+IEkyQ19DTElF
TlRfUEVDKSA/IDIgOiAxKTsNCj4+DQo+PiAuLi4NCj4+ID4gPiArICAgICByZXR1cm4gKChjbGtf
aGlnaCA8PCBBU1BFRURfSTJDRF9USU1FX1NDTF9ISUdIX1NISUZUKQ0KPj4gPiA+ICsgICAgICAg
ICAgICAgJiBBU1BFRURfSTJDRF9USU1FX1NDTF9ISUdIX01BU0spDQo+PiA+ID4gKyAgICAgICAg
ICAgICAgICAgICAgIHwgKChjbGtfbG93IDw8DQo+PiA+ID4gQVNQRUVEX0kyQ0RfVElNRV9TQ0xf
TE9XX1NISUZUKQ0KPj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAmIEFTUEVFRF9JMkNE
X1RJTUVfU0NMX0xPV19NQVNLKQ0KPj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICB8IChiYXNl
X2NsayAmDQo+PiA+ID4gQVNQRUVEX0kyQ0RfVElNRV9CQVNFX0RJVklTT1JfTUFTSyk7DQo+PiA+
ID4gK30NCj4+ID4NCj4+ID4gQXMgSSB0aGluayBJIG1lbnRpb25lZCBlYXJsaWVyLCB0aGUgQVNU
MjUwMCBoYXMgYSBzbGlnaHRseSANCj4+ID4gZGlmZmVyZW50IHJlZ2lzdGVyIGxheW91dCB3aGlj
aCBzdXBwb3J0IGxhcmdlciB2YWx1ZXMgZm9yIGhpZ2ggYW5kIA0KPj4gPiBsb3csIHRodXMgYWxs
b3dpbmcgYSBmaW5lciBncmFudWxhcml0eS4NCj4+DQo+PiBJIGFtIGRldmVsb3BpbmcgYWdhaW5z
dCB0aGUgMjUwMC4NCj4NCj4gWWVzIGJ1dCB3ZSdkIGxpa2UgdGhlIGRyaXZlciB0byB3b3JrIHdp
dGggYm90aCA6LSkNCg0KUmlnaHQsIEkgdGhvdWdodCB5b3Ugd2VyZSBtYWtpbmcgYW4gYXNzZXJ0
aW9uIGFib3V0IHRoZSAyNTAwLCBpZiB5b3UgYXJlIG1ha2luZyBhbiBhc3NlcnRpb24gYWJvdXQg
dGhlIDI0MDAsIEkgZG8gbm90IGtub3cgYW5kIGRvIG5vdCBoYXZlIG9uZSBoYW5keS4NCg0KPg0K
Pj4gPiBCVFcuIEluIGNhc2UgeW91IGhhdmVuJ3QsIEkgd291bGQgc3VnZ2VzdCB5b3UgY29weS9w
YXN0ZSB0aGUgYWJvdmUgDQo+PiA+IGluIGEgdXNlcnNwYWNlIGFwcCBhbmQgcnVuIGl0IGZvciBh
bGwgZnJlcXVlbmN5IGRpdmlzb3JzIGFuZCBzZWUgaWYgDQo+PiA+IHlvdXIgcmVzdWx0cyBtYXRj
aCB0aGUgYXNwZWVkIHRhYmxlIDopDQo+Pg0KPj4gR29vZCBjYWxsLg0KPg0KPiBJZiB5b3UgZW5k
IHVwIGRvaW5nIHRoYXQsIGNhbiB5b3Ugc2hvb3QgaXQgbXkgd2F5ID8gSSBjYW4gdGFrZSBjYXJl
IG9mIA0KPiBtYWtpbmcgc3VyZSBpdCdzIGFsbCBnb29kIGZvciB0aGUgMjQwMC4NCg0KV2lsbCBk
by4NCg0KPg0KPj4gPiA+ICtzdGF0aWMgaW50IGFzcGVlZF9pMmNfaW5pdF9jbGsoc3RydWN0IGFz
cGVlZF9pMmNfYnVzICpidXMsDQo+PiA+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICBz
dHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KSB7DQo+PiA+ID4gKyAgICAgdTMyIGNsa19mcmVx
LCBkaXZpc29yOw0KPj4gPiA+ICsgICAgIHN0cnVjdCBjbGsgKnBjbGs7DQo+PiA+ID4gKyAgICAg
aW50IHJldDsNCj4+ID4gPiArDQo+PiA+ID4gKyAgICAgcGNsayA9IGRldm1fY2xrX2dldCgmcGRl
di0+ZGV2LCBOVUxMKTsNCj4+ID4gPiArICAgICBpZiAoSVNfRVJSKHBjbGspKSB7DQo+PiA+ID4g
KyAgICAgICAgICAgICBkZXZfZXJyKCZwZGV2LT5kZXYsICJjbGtfZ2V0IGZhaWxlZFxuIik7DQo+
PiA+ID4gKyAgICAgICAgICAgICByZXR1cm4gUFRSX0VSUihwY2xrKTsNCj4+ID4gPiArICAgICB9
DQo+PiA+ID4gKyAgICAgcmV0ID0gb2ZfcHJvcGVydHlfcmVhZF91MzIocGRldi0+ZGV2Lm9mX25v
ZGUsDQo+PiA+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgImNsb2NrLWZyZXF1
ZW5jeSIsICZjbGtfZnJlcSk7DQo+PiA+DQo+PiA+IFNlZSBteSBwcmV2aW91cyBjb21tZW50IGFi
b3V0IGNhbGxpbmcgdGhhdCAnYnVzLWZyZXF1ZW5jeScgcmF0aGVyIA0KPj4gPiB0aGFuICdjbG9j
ay1mcmVxdWVuY3knLg0KPj4gPg0KPj4gPiA+ICsgICAgIGlmIChyZXQgPCAwKSB7DQo+PiA+ID4g
KyAgICAgICAgICAgICBkZXZfZXJyKCZwZGV2LT5kZXYsDQo+PiA+ID4gKyAgICAgICAgICAgICAg
ICAgICAgICJDb3VsZCBub3QgcmVhZCBjbG9jay1mcmVxdWVuY3kNCj4+ID4gPiBwcm9wZXJ0eVxu
Iik7DQo+PiA+ID4gKyAgICAgICAgICAgICBjbGtfZnJlcSA9IDEwMDAwMDsNCj4+ID4gPiArICAg
ICB9DQo+PiA+ID4gKyAgICAgZGl2aXNvciA9IGNsa19nZXRfcmF0ZShwY2xrKSAvIGNsa19mcmVx
Ow0KPj4gPiA+ICsgICAgIC8qIFdlIGp1c3QgbmVlZCB0aGUgY2xvY2sgcmF0ZSwgd2UgZG9uJ3Qg
YWN0dWFsbHkgdXNlIHRoZQ0KPj4gPiA+IGNsayBvYmplY3QuICovDQo+PiA+ID4gKyAgICAgZGV2
bV9jbGtfcHV0KCZwZGV2LT5kZXYsIHBjbGspOw0KPj4gPiA+ICsNCj4+ID4gPiArICAgICAvKiBT
ZXQgQUMgVGltaW5nICovDQo+PiA+ID4gKyAgICAgaWYgKGNsa19mcmVxIC8gMTAwMCA+IDEwMDAp
IHsNCj4+ID4gPiArICAgICAgICAgICAgIGFzcGVlZF9pMmNfd3JpdGUoYnVzLCBhc3BlZWRfaTJj
X3JlYWQoYnVzLA0KPj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICBBU1BFRURfSTJDX0ZVDQo+PiA+ID4gTl9DVFJMX1JFRykgfA0KPj4gPiA+
ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgIEFTUEVFRF9JMkNEX01fSElHSF9TUEVFRF9F
TiB8DQo+PiA+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgQVNQRUVEX0kyQ0RfTV9T
REFfRFJJVkVfMVRfRU4gfA0KPj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgIEFT
UEVFRF9JMkNEX1NEQV9EUklWRV8xVF9FTiwNCj4+ID4gPiArICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICBBU1BFRURfSTJDX0ZVTl9DVFJMX1JFRyk7DQo+PiA+ID4gKw0KPj4gPiA+ICsgICAg
ICAgICAgICAgYXNwZWVkX2kyY193cml0ZShidXMsIDB4MywNCj4+ID4gPiBBU1BFRURfSTJDX0FD
X1RJTUlOR19SRUcyKTsNCj4+ID4gPiArICAgICAgICAgICAgIGFzcGVlZF9pMmNfd3JpdGUoYnVz
LA0KPj4gPiA+IGFzcGVlZF9pMmNfZ2V0X2Nsa19yZWdfdmFsKGRpdmlzb3IpLA0KPj4gPiA+ICsg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBBU1BFRURfSTJDX0FDX1RJTUlOR19SRUcxKTsN
Cj4+ID4NCj4+ID4gSSBhbHJlYWR5IGRpc2N1c3NlZCBieSBkb3VidHMgYWJvdXQgdGhlIGFib3Zl
LiBJIGNhbiB0cnkgdG8gc2NvcGUgDQo+PiA+IGl0IHdpdGggdGhlIEVWQiBpZiB5b3UgZG9uJ3Qg
Z2V0IHRvIGl0LiBGb3Igbm93IEknZCByYXRoZXIgdGFrZSB0aGUgDQo+PiA+IGNvZGUgb3V0Lg0K
Pj4gPg0KPj4gPiBXZSBzaG91bGQgYXNrIGFzcGVlZCBmcm9tIHdoYXQgZnJlcXVlbmN5IHRoZSAi
MVQiIHN0dWZmIGlzIHVzZWZ1bC4NCj4+DQo+PiBXaWxsIGRvLCBJIHdpbGwgdHJ5IHRvIHJvcGUg
UnlhbiBpbiBvbiB0aGUgbmV4dCByZXZpZXc7IGl0IHdpbGwgYmUgDQo+PiBnb29kIGZvciBoaW0g
dG8gZ2V0IHVzZWQgdG8gd29ya2luZyB3aXRoIHVwc3RyZWFtIGFueXdheS4NCj4NCj4gWXVwLiBI
b3dldmVyLCBmb3IgdGhlIHNha2Ugb2YgZ2V0dGluZyBzb21ldGhpbmcgdXBzdHJlYW0gKGFuZCBp
biANCj4gT3BlbkJNQyA0LjEwIGtlcm5lbCkgYXNhcCwgSSB3b3VsZCBzdWdnZXN0IGp1c3QgZHJv
cHBpbmcgc3VwcG9ydCBmb3IgDQo+IHRob3NlIGZhc3Qgc3BlZWRzIGZvciBub3csIHdlIGNhbiBh
ZGQgdGhlbSBiYWNrIGxhdGVyLg0KDQpBbHJpZ2h0LCB0aGF0J3MgZmluZS4gU3RpbGwsIFJ5YW4s
IGNvdWxkIHlvdSBwcm92aWRlIHNvbWUgY29udGV4dCBvbiB0aGlzPw0KDQo+DQo+PiA+DQo+PiA+
ID4gKyAgICAgfSBlbHNlIHsNCj4+ID4gPiArICAgICAgICAgICAgIGFzcGVlZF9pMmNfd3JpdGUo
YnVzLA0KPj4gPiA+IGFzcGVlZF9pMmNfZ2V0X2Nsa19yZWdfdmFsKGRpdmlzb3IpLA0KPj4gPiA+
ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBBU1BFRURfSTJDX0FDX1RJTUlOR19SRUcx
KTsNCj4+ID4gPiArICAgICAgICAgICAgIGFzcGVlZF9pMmNfd3JpdGUoYnVzLCBBU1BFRURfTk9f
VElNRU9VVF9DVFJMLA0KPj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBBU1BF
RURfSTJDX0FDX1RJTUlOR19SRUcyKTsNCj4+ID4gPiArICAgICB9DQo+Pg0KPj4gLi4uDQo+PiA+
ID4gKyAgICAgc3Bpbl9sb2NrX2luaXQoJmJ1cy0+bG9jayk7DQo+PiA+ID4gKyAgICAgaW5pdF9j
b21wbGV0aW9uKCZidXMtPmNtZF9jb21wbGV0ZSk7DQo+PiA+ID4gKyAgICAgYnVzLT5hZGFwLm93
bmVyID0gVEhJU19NT0RVTEU7DQo+PiA+ID4gKyAgICAgYnVzLT5hZGFwLnJldHJpZXMgPSAwOw0K
Pj4gPiA+ICsgICAgIGJ1cy0+YWRhcC50aW1lb3V0ID0gNSAqIEhaOw0KPj4gPiA+ICsgICAgIGJ1
cy0+YWRhcC5hbGdvID0gJmFzcGVlZF9pMmNfYWxnbzsNCj4+ID4gPiArICAgICBidXMtPmFkYXAu
YWxnb19kYXRhID0gYnVzOw0KPj4gPiA+ICsgICAgIGJ1cy0+YWRhcC5kZXYucGFyZW50ID0gJnBk
ZXYtPmRldjsNCj4+ID4gPiArICAgICBidXMtPmFkYXAuZGV2Lm9mX25vZGUgPSBwZGV2LT5kZXYu
b2Zfbm9kZTsNCj4+ID4gPiArICAgICBzbnByaW50ZihidXMtPmFkYXAubmFtZSwgc2l6ZW9mKGJ1
cy0+YWRhcC5uYW1lKSwgIkFzcGVlZA0KPj4gPiA+IGkyYyIpOw0KPj4gPg0KPj4gPiBBbm90aGVy
IHRyaXZpYWwgb25lLCBzaG91bGQgd2UgcHV0IHNvbWUga2luZCBvZiBidXMgbnVtYmVyIGluIHRo
YXQgDQo+PiA+IHN0cmluZyA/DQo+Pg0KPj4gV2hvb3BzLCBsb29rcyBsaWtlIEkgbWlzc2VkIHRo
aXMgb25lOyBJIHdpbGwgZ2V0IHRvIGl0IGluIHRoZSBuZXh0IA0KPj4gcmV2aXNpb24uDQo+DQo+
IE9rLiBJIG5vdGljZWQgeW91IG1pc3NlZCB0aGF0IGluIHY3LCBzbyBJIGFzc3VtZSB5b3UgbWVh
biB2OCA6LSkNCg0KWWVwLCBJIHdpbGwgZ2V0IGl0IGluIHY4Lg0KDQo+DQo+PiA+DQo+PiA+ID4g
KyAgICAgYnVzLT5kZXYgPSAmcGRldi0+ZGV2Ow0KPj4gPiA+ICsNCj4+ID4gPiArICAgICAvKiBy
ZXNldCBkZXZpY2U6IGRpc2FibGUgbWFzdGVyICYgc2xhdmUgZnVuY3Rpb25zICovDQo+PiA+ID4g
KyAgICAgYXNwZWVkX2kyY193cml0ZShidXMsIDAsIEFTUEVFRF9JMkNfRlVOX0NUUkxfUkVHKTsN
Cj4+DQo+PiAuLi4NCj4+IC0tDQo+PiBUbyB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2Vu
ZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgZGV2aWNldHJlZSINCj4+IGluDQo+PiB0aGUgYm9keSBv
ZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZyBNb3JlIG1ham9yZG9tbyAN
Cj4+IGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0K
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt April 25, 2017, 9:34 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ryan Chen April 25, 2017, 9:47 a.m.
VGhhbmtzIFJ5YW4uIENhbiB5b3Ugc2hlZCBzb21lIGxpZ2h0IG9uIHRoZSBtZWFuaW5nIG9mIHRo
ZSBoaWdoLXNwZWVkIGJpdCBhcyB3ZWxsIHBsZWFzZSA/DQoNCkFib3V0IEFTUEVFRF9JMkNEX01f
SElHSF9TUEVFRF9FTiwgaXQgaXMgc3VwcG9ydCBmb3IgSTJDIHNwZWNpZmljYXRpb24gIkhpZ2gg
c3BlZWQgdHJhbnNmZXIiLiBBbmQgYWxzbyBkZXZpY2UgbmVlZCBzdXBwb3J0IGl0LiANCklmIHlv
dSBqdXN0IHNwZWVkIHVwIHRoZSBJMkMgYnVzIGNsb2NrLCB5b3UgZG9u4oCZdCBoYXZlIHRvIGVu
YWJsZSBBU1BFRURfSTJDRF9NX0hJR0hfU1BFRURfRU4sIGp1c3QgY2hhbmdlIHRoZSBjbG9jayBp
cyBvay4NCg0KDQotLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KRnJvbTogQmVuamFtaW4gSGVy
cmVuc2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10gDQpTZW50OiBUdWVz
ZGF5LCBBcHJpbCAyNSwgMjAxNyA1OjM1IFBNDQpUbzogUnlhbiBDaGVuIDxyeWFuX2NoZW5AYXNw
ZWVkdGVjaC5jb20+OyBCcmVuZGFuIEhpZ2dpbnMgPGJyZW5kYW5oaWdnaW5zQGdvb2dsZS5jb20+
DQpDYzogV29sZnJhbSBTYW5nIDx3c2FAdGhlLWRyZWFtcy5kZT47IFJvYiBIZXJyaW5nIDxyb2Jo
K2R0QGtlcm5lbC5vcmc+OyBNYXJrIFJ1dGxhbmQgPG1hcmsucnV0bGFuZEBhcm0uY29tPjsgVGhv
bWFzIEdsZWl4bmVyIDx0Z2x4QGxpbnV0cm9uaXguZGU+OyBKYXNvbiBDb29wZXIgPGphc29uQGxh
a2VkYWVtb24ubmV0PjsgTWFyYyBaeW5naWVyIDxtYXJjLnp5bmdpZXJAYXJtLmNvbT47IEpvZWwg
U3RhbmxleSA8am9lbEBqbXMuaWQuYXU+OyBWbGFkaW1pciBaYXBvbHNraXkgPHZ6QG1sZWlhLmNv
bT47IEthY2hhbG92IEFudG9uIDxtb3VzZUBtYXljLnJ1PjsgQ8OpZHJpYyBMZSBHb2F0ZXIgPGNs
Z0BrYW9kLm9yZz47IGxpbnV4LWkyY0B2Z2VyLmtlcm5lbC5vcmc7IGRldmljZXRyZWVAdmdlci5r
ZXJuZWwub3JnOyBMaW51eCBLZXJuZWwgTWFpbGluZyBMaXN0IDxsaW51eC1rZXJuZWxAdmdlci5r
ZXJuZWwub3JnPjsgT3BlbkJNQyBNYWlsbGlzdCA8b3BlbmJtY0BsaXN0cy5vemxhYnMub3JnPg0K
U3ViamVjdDogUmU6IFtQQVRDSCB2NiA0LzVdIGkyYzogYXNwZWVkOiBhZGRlZCBkcml2ZXIgZm9y
IEFzcGVlZCBJMkMNCg0KT24gVHVlLCAyMDE3LTA0LTI1IGF0IDA4OjUwICswMDAwLCBSeWFuIENo
ZW4gd3JvdGU6DQo+IEhlbGxvIEFsbCwNCj4gCQlBU1BFRURfSTJDRF9NX1NEQV9EUklWRV8xVF9F
TiwNCj4gQVNQRUVEX0kyQ0RfU0RBX0RSSVZFXzFUX0VOIGlzIHNwZWNpZmljIGZvciBzb21lIGNh
c2UgdXNhZ2UuwqANCj4gCQlGb3IgZXhhbXBsZSwgaWYgaTJjIGJ1cyBpcyB1c2Ugb24gImhpZ2gg
c3BlZWQiIGFuZCAic2luZ2xlIHNsYXZlIGFuZCANCj4gbWFzdGVyIiBhbmQgaTJjIGJ1cyBpcyB0
b28gbG9uZy4gSXQgbmVlZCBkcml2ZSBTREEgb3IgU0NMIGxlc3MgbHVuYWN5LiANCj4gSXQgd291
bGQgZW5hYmxlIGl0Lg0KPiAJCU90aGVyd2lzZSwgZG9u4oCZdCBlbmFibGUgaXQuIGVzcGVjaWFs
bHkgaW4gbXVsdGktbWFzdGVyLiANCj4gSXQgY2Fu4oCZdCBiZSBlbmFibGUuDQoNClRoYXQgc21l
bGxzIGxpa2UgYSBzcGVjaWZpYyBlbm91Z2ggdXNlIGNhc2UgdGhhdCB3ZSBzaG91bGQgcHJvYmFi
bHkgY292ZXIgd2l0aCBhIGRldmljZS10cmVlIHByb3BlcnR5LCBzb21ldGhpbmcgbGlrZSBhbiBl
bXB0eSAic2RhLWV4dHJhLWRyaXZlIiBwcm9wZXJ0eSAoZW1wdHkgcHJvcGVydGllcyBhcmUgdHlw
aWNhbGx5IHVzZWQgZm9yIGJvb2xlYW5zLCB0aGVpciBwcmVzZW5jZSBtZWFucyAidHJ1ZSIpLg0K
DQpUaGFua3MgUnlhbi4gQ2FuIHlvdSBzaGVkIHNvbWUgbGlnaHQgb24gdGhlIG1lYW5pbmcgb2Yg
dGhlIGhpZ2gtc3BlZWQgYml0IGFzIHdlbGwgcGxlYXNlID8gRG9lcyBpdCBmb3JjZSB0byBhIHNw
ZWNpZmljIHNwZWVkIChpZ25vcmluZyB0aGUNCmRpdmlzb3IpIG9yIHdlIGNhbiBzdGlsbCBwbGF5
IHdpdGggdGhlIGNsb2NrIGhpZ2gvbG93IGNvdW50cyA/DQoNCkNoZWVycywNCkJlbi4NCg0KPiAJ
CcKgwqANCj4gCQ0KPiANCj4gQmVzdCBSZWdhcmRzLA0KPiBSeWFuDQo+IA0KPiDkv6HpqYrnp5Hm
ioDogqHku73mnInpmZDlhazlj7gNCj4gQVNQRUVEIFRlY2hub2xvZ3kgSW5jLg0KPiAyRixOby4x
NSxJbmR1c3RyeSBFYXN0IFJvYWQgNC4sSHNpbmNodSBTY2llbmNlIFBhcmssIEhzaW5jaHUgQ2l0
eSANCj4gMzAwNzcsIFRhaXdhbg0KPiBUZWw6IDg4Ni0zLTU3OC05NTY4wqAgIzg1Nw0KPiBGYXg6
IDg4Ni0zLTU3OC05NTg2DQo+ICoqKioqKioqKioqKiogRW1haWwgQ29uZmlkZW50aWFsaXR5IE5v
dGljZSAqKioqKioqKioqKioqKioqKioqKg0KPiBESVNDTEFJTUVSOg0KPiBUaGlzIG1lc3NhZ2Ug
KGFuZCBhbnkgYXR0YWNobWVudHMpIG1heSBjb250YWluIGxlZ2FsbHkgcHJpdmlsZWdlZCANCj4g
YW5kL29yIG90aGVyIGNvbmZpZGVudGlhbCBpbmZvcm1hdGlvbi4gSWYgeW91IGhhdmUgcmVjZWl2
ZWQgaXQgaW4gDQo+IGVycm9yLCBwbGVhc2Ugbm90aWZ5IHRoZSBzZW5kZXIgYnkgcmVwbHkgZS1t
YWlsIGFuZCBpbW1lZGlhdGVseSBkZWxldGUgDQo+IHRoZSBlLW1haWwgYW5kIGFueSBhdHRhY2ht
ZW50cyB3aXRob3V0IGNvcHlpbmcgb3IgZGlzY2xvc2luZyB0aGUgDQo+IGNvbnRlbnRzLiBUaGFu
ayB5b3UuDQo+IA0KPiANCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQnJl
bmRhbiBIaWdnaW5zIFttYWlsdG86YnJlbmRhbmhpZ2dpbnNAZ29vZ2xlLmNvbV0NCj4gU2VudDog
VHVlc2RheSwgQXByaWwgMjUsIDIwMTcgNDozMiBQTQ0KPiBUbzogQmVuamFtaW4gSGVycmVuc2No
bWlkdCA8YmVuaEBrZXJuZWwuY3Jhc2hpbmcub3JnPg0KPiBDYzogV29sZnJhbSBTYW5nIDx3c2FA
dGhlLWRyZWFtcy5kZT47IFJvYiBIZXJyaW5nIDxyb2JoK2R0QGtlcm5lbC5vcmcNCj4gPjsgTWFy
ayBSdXRsYW5kIDxtYXJrLnJ1dGxhbmRAYXJtLmNvbT47IFRob21hcyBHbGVpeG5lciA8dGdseEBs
aW51dHJvDQo+IG5peC5kZT47IEphc29uIENvb3BlciA8amFzb25AbGFrZWRhZW1vbi5uZXQ+OyBN
YXJjIFp5bmdpZXIgPG1hcmMuenluZyANCj4gaWVyQGFybS5jb20+OyBKb2VsIFN0YW5sZXkgPGpv
ZWxAam1zLmlkLmF1PjsgVmxhZGltaXIgWmFwb2xza2l5IDx2ekBtIA0KPiBsZWlhLmNvbT47IEth
Y2hhbG92IEFudG9uIDxtb3VzZUBtYXljLnJ1PjsgQ8OpZHJpYyBMZSBHb2F0ZXIgPGNsZ0BrYW9k
IA0KPiAub3JnPjsgbGludXgtaTJjQHZnZXIua2VybmVsLm9yZzsgZGV2aWNldHJlZUB2Z2VyLmtl
cm5lbC5vcmc7IExpbnV4IA0KPiBLZXJuZWwgTWFpbGluZyBMaXN0IDxsaW51eC1rZXJuZWxAdmdl
ci5rZXJuZWwub3JnPjsgT3BlbkJNQyBNYWlsbGlzdCANCj4gPG9wZW5ibWNAbGlzdHMub3psYWJz
Lm9yZz47IFJ5YW4gQ2hlbiA8cnlhbl9jaGVuQGFzcGVlZHRlY2guY29tPg0KPiBTdWJqZWN0OiBS
ZTogW1BBVENIIHY2IDQvNV0gaTJjOiBhc3BlZWQ6IGFkZGVkIGRyaXZlciBmb3IgQXNwZWVkIEky
Qw0KPiANCj4gQWRkaW5nIFJ5YW4uDQo+IA0KPiBPbiBNb24sIEFwciAyNCwgMjAxNyBhdCA3OjE5
IFBNLCBCZW5qYW1pbiBIZXJyZW5zY2htaWR0IDxiZW5oQGtlcm5lbC4NCj4gY3Jhc2hpbmcub3Jn
PiB3cm90ZToNCj4gPiBPbiBNb24sIDIwMTctMDQtMjQgYXQgMTE6NTYgLTA3MDAsIEJyZW5kYW4g
SGlnZ2lucyB3cm90ZToNCj4gPiA+ID4gPiArc3RydWN0IGFzcGVlZF9pMmNfYnVzIHsNCj4gPiA+
ID4gPiArwqDCoMKgwqDCoHN0cnVjdCBpMmNfYWRhcHRlcsKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqBhZGFwOw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgc3RydWN0IGRldmljZcKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgKmRldjsNCj4gPiA+ID4gPiArwqDCoMKgwqDCoHZv
aWQgX19pb21lbcKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAqYmFzZTsN
Cj4gPiA+ID4gPiArwqDCoMKgwqDCoC8qIFN5bmNocm9uaXplcyBJL08gbWVtIGFjY2VzcyB0byBi
YXNlLiAqLw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgc3BpbmxvY2tfdMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgbG9jazsNCj4gPiA+ID4gDQo+ID4gPiA+IEkgYW0g
bm90IGVudGlyZWx5IGNvbnZpbmNlZCB3ZSBuZWVkIHRoYXQgbG9jay4gVGhlIGkyYyBjb3JlIHdp
bGwgDQo+ID4gPiA+IHRha2UgYSBtdXRleCBwcm90ZWN0aW5nIGFsbCBvcGVyYXRpb25zIG9uIHRo
ZSBidXMuIFNvIHdlIG9ubHkgDQo+ID4gPiA+IG5lZWQgdG8gc3luY2hyb25pemUgYmV0d2VlbiBv
dXIgInhmZXIiIGNvZGUgYW5kIG91ciBpbnRlcnJ1cHQgDQo+ID4gPiA+IGhhbmRsZXIuDQo+ID4g
PiANCj4gPiA+IFlvdSBhcmUgcmlnaHQgaWYgYm90aCBoYXZpbmcgc2xhdmUgYW5kIG1hc3RlciBh
Y3RpdmUgYXQgdGhlIHNhbWUgDQo+ID4gPiB0aW1lIHdhcyBub3QgcG9zc2libGU7IGhvd2V2ZXIs
IGl0IGlzLg0KPiA+IA0KPiA+IFJpZ2h0LCBJIHNvbWV3aGF0IGZvcmdvdCBhYm91dCB0aGUgc2xh
dmUgY2FzZS4NCj4gPiANCj4gPiDCoCAuLi4NCj4gPiANCj4gPiA+ID4gU29tZSBvZiB0aG9zZSBl
cnJvciBzdGF0ZXMgcHJvYmFibHkgYWxzbyB3YXJyYW50IGEgcmVzZXQgb2YgdGhlIA0KPiA+ID4g
PiBjb250cm9sbGVyLCBJIHRoaW5rIGFzcGVlZCBkb2VzIHRoYXQgaW4gdGhlIFNESy4NCj4gPiA+
IA0KPiA+ID4gRm9yIHRpbWVvdXQgYW5kIGNtZF9lcnIsIEkgZG8gbm90IHNlZSBhbnkgYXJndW1l
bnQgYWdhaW5zdCBpdDsgaXQgDQo+ID4gPiBzb3VuZHMgbGlrZSB3ZSBhcmUgaW4gYSB2ZXJ5IG1l
c3NlZCB1cCwgdmVyeSB1bmtub3duIHN0YXRlLCBzbyANCj4gPiA+IGZ1bGwgcmVzZXQgaXMgcHJv
YmFibHkgdGhlIGJlc3QgbGFzdCByZXNvcnQuDQo+ID4gDQo+ID4gWXVwLg0KPiA+IA0KPiA+ID4g
Rm9yIFNEQSBzdGF5aW5nIHB1bGxlZCBkb3duLCBJDQo+ID4gPiB0aGluayB3ZSBjYW4gc2F5IHdp
dGggcmVhc29uYWJsZSBjb25maWRlbmNlIHRoYXQgc29tZSBkZXZpY2Ugb24gDQo+ID4gPiBvdXIg
YnVzIGlzIGJlaGF2aW5nIHZlcnkgYmFkbHkgYW5kIEkgYW0gbm90IGNvbnZpbmNlZCB0aGF0IA0K
PiA+ID4gcmVzZXR0aW5nIHRoZSBjb250cm9sbGVyIGlzIGxpa2VseSB0byBkbyBhbnl0aGluZyB0
byBoZWxwOw0KPiA+IA0KPiA+IFJpZ2h0LiBIYW1tZXJpbmcgd2l0aCBTVE9QcyBhbmQgcHJheSAu
Li4NCj4gDQo+IEkgdGhpbmsgc2VuZGluZyByZWNvdmVyeSBtb2RlIHNlbmRzIHN0b3BzIGFzIGEg
cGFydCBvZiB0aGUgcmVjb3ZlcnkgDQo+IGFsZ29yaXRobSBpdCBleGVjdXRlcy4NCj4gDQo+ID4g
DQo+ID4gPiDCoHRoYXQgYmVpbmcgc2FpZCwgSSByZWFsbHkNCj4gPiA+IGRvIG5vdCBoYXZlIGFu
eSBnb29kIGlkZWFzIHRvIGFkZHJlc3MgdGhhdC4gU28gbWF5YmUgcHJheWluZyBhbmQgDQo+ID4g
PiByZXNldHRpbmcgdGhlIGNvbnRyb2xsZXIgaXMgKnRoZSBtb3N0IHJlYXNvbmFibGUgdGhpbmcg
dG8gZG8uKiBJIA0KPiA+ID4gd291bGQgbGlrZSB0byBrbm93IHdoYXQgeW91IHRoaW5rIHdlIHNo
b3VsZCBkbyBpbiB0aGF0IGNhc2UuDQo+ID4gDQo+ID4gV2VsbCwgdGhlcmUncyBhIChzbWFsbCA/
KSBjaGFuY2UgdGhhdCBpdCdzIGEgY29udHJvbGxlciBidWcgDQo+ID4gYXNzZXJ0aW5nIHRoZSBs
aW5lIHNvIC4uLiBidXQgdGhlcmUncyBsaXR0bGUgd2UgY2FuIGRvIGlmIG5vdC4NCj4gDQo+IFRy
dWUuDQo+IA0KPiA+IA0KPiA+ID4gV2hpbGUgSSB3YXMgdGhpbmtpbmcgYWJvdXQgdGhpcyBJIGFs
c28gcmVhbGl6ZWQgdGhhdCB0aGUgU0RBIGxpbmUgDQo+ID4gPiBjaGVjayBhZnRlciByZWNvdmVy
eSBoYXBwZW5zIGluIHRoZSBlbHNlIGJyYW5jaCwgYnV0IFNDTCBsaW5lIA0KPiA+ID4gY2hlY2sg
ZG9lcyBub3QgaGFwcGVuIGFmdGVyIHdlIGF0dGVtcHQgdG8gU1RPUCBpZiBTQ0wgaXMgaHVuZy4g
SWYgDQo+ID4gPiB3ZSBkZWNpZGUgdG8gbWFrZSBzcGVjaWFsIG5vdGUgU0RBIGJlaW5nIGh1bmcg
YnkgYSBkZXZpY2UgdGhhdCANCj4gPiA+IHdvbid0IGxldCBnbywgd2UgbWlnaHQgd2FudCB0byBt
YWtlIGEgc3BlY2lhbCBub3RlIHRoYXQgU0NMIGlzIA0KPiA+ID4gaHVuZyBieSBhIGRldmljZSB0
aGF0IHdvbid0IGxldCBnby4gSnVzdCBhIHRob3VnaHQuDQo+ID4gDQo+ID4gTWF5YmUuIE9yIGp1
c3QgInVucmVjb3ZlcmFibGUgZXJyb3IiLi4uIGhvcGVmdWxseSB0aGVzZSBkb24ndCBoYXBwZW4g
DQo+ID4gdG9vIG9mdGVuIC4uLiBXZSBoYWQgY2FzZXMgb2YgYSBUUE0gbWlzYmVoYXZpbmcgbGlr
ZSB0aGF0Lg0KPiANCj4gWWVhaCwgZGVmaW5pdGVseSBzaG91bGQgcHJpbnQgc29tZXRoaW5nIG91
dC4NCj4gDQo+ID4gDQo+ID4gPiA+ID4gK291dDoNCj4gPiA+IA0KPiA+ID4gLi4uDQo+ID4gPiA+
IFdoYXQgYWJvdXQgSTJDX01fTk9TVEFSVCA/DQo+ID4gPiA+IA0KPiA+ID4gPiBOb3QgdGhhdCBJ
J3ZlIGV2ZXIgc2VlbiBpdCB1c2VkLi4uIDstKQ0KPiA+ID4gDQo+ID4gPiBSaWdodCBub3cgSSBh
bSBub3QgZG9pbmcgYW55IG9mIHRoZSBwcm90b2NvbCBtYW5nbGluZyBvcHRpb25zLCBidXQgDQo+
ID4gPiBJIGNhbiBhZGQgdGhlbSBpbiBpZiB5b3UgdGhpbmsgaXQgaXMgaW1wb3J0YW50IGZvciBp
bml0aWFsIA0KPiA+ID4gc3VwcG9ydC4NCj4gPiANCj4gPiBObywgbm90IGltcG9ydGFudCwgd2Ug
Y2FuIGFkZCB0aGF0IGxhdGVyIGlmIGl0IGV2ZXIgYmVjb21lcyB1c2VmdWwuDQo+ID4gDQo+ID4g
wqAuLi4NCj4gPiANCj4gPiA+ID4gSW4gZ2VuZXJhbCwgeW91IGFsd2F5cyBBQ0sgYWxsIGludGVy
cnVwdHMgZmlyc3QuIFRoZW4geW91IGhhbmRsZSANCj4gPiA+ID4gdGhlIGJpdHMgeW91IGhhdmUg
aGFydmVzdGVkLg0KPiA+ID4gPiANCj4gPiA+IA0KPiA+ID4gVGhlIGRvY3VtZW50YXRpb24gc2F5
cyB0byBBQ0sgdGhlIGludGVycnVwdCBhZnRlciBoYW5kbGluZyBpbiB0aGUgDQo+ID4gPiBSWA0K
PiA+ID4gY2FzZToNCj4gPiA+IA0KPiA+ID4gPDw8DQo+ID4gPiBTL1cgbmVlZHMgdG8gY2xlYXIg
dGhpcyBzdGF0dXMgYml0IHRvIGFsbG93IG5leHQgZGF0YSByZWNlaXZpbmcuDQo+ID4gPiA+ID4g
PiANCj4gPiA+IA0KPiA+ID4gSSB3aWxsIGRvdWJsZSBjaGVjayB3aXRoIFJ5YW4gdG8gbWFrZSBz
dXJlIFRYIHdvcmtzIHRoZSBzYW1lIHdheS4NCj4gPiA+IA0KPiA+ID4gPiA+ICvCoMKgwqDCoMKg
aWYgKGlycV9zdGF0dXMgJiBBU1BFRURfSTJDRF9JTlRSX0VSUk9SIHx8DQo+ID4gPiA+ID4gK8Kg
wqDCoMKgwqDCoMKgwqDCoCghYnVzLT5tc2dzICYmIGJ1cy0+bWFzdGVyX3N0YXRlICE9DQo+ID4g
PiA+ID4gQVNQRUVEX0kyQ19NQVNURVJfU1RPUCkpIHsNCj4gPiA+IA0KPiA+ID4gLi4uDQo+ID4g
PiA+IA0KPiA+ID4gPiBJIHdvdWxkIHNldCBtYXN0ZXJfc3RhdGUgdG8gIlJFQ09WRVJZIiAobmV3
IHN0YXRlID8pIGFuZCBlbnN1cmUgDQo+ID4gPiA+IHRob3NlIHRoaW5ncyBhcmUgY2F1Z2h0IGlm
IHRoZXkgaGFwcGVuIG91dHNpZGUgb2YgYSByZWNvdmVyeS4NCj4gPiANCj4gPiBJIHJlcGxpZWQg
cHJpdmF0ZWx5IC4uLiBhcyBsb25nIGFzIHdlIGFjayBiZWZvcmUgd2Ugc3RhcnQgYSBuZXcgDQo+
ID4gY29tbWFuZCB3ZSBzaG91bGQgYmUgb2sgYnV0IHdlIHNob3VsZG4ndCBhY2sgYWZ0ZXIuDQo+
ID4gDQo+ID4gWW91ciBsYXRlc3QgcGF0Y2ggc3RpbGwgZG9lcyB0aGF0LiBJdCB3aWxsIGRvIHRo
aW5ncyBsaWtlIHN0YXJ0IGEgDQo+ID4gU1RPUCBjb21tYW5kICp0aGVuKiBhY2sgdGhlIHN0YXR1
cyBiaXRzLiBJJ20gcHJldHR5IHN1cmUgdGhhdCdzIA0KPiA+IGJvZ3VzLg0KPiA+IA0KPiA+IFRo
YXQgd2F5IGl0J3MgYSBsb3Qgc2ltcGxlciB0byBzaW1wbHkgbW92ZSB0aGUNCj4gPiANCj4gPiDC
oMKgwqDCoMKgwqDCoMKgd3JpdGVsKGlycV9zdGF0dXMsIGJ1cy0+YmFzZSArIEFTUEVFRF9JMkNf
SU5UUl9TVFNfUkVHKTsNCj4gPiANCj4gPiBUbyBlaXRoZXIgcmlnaHQgYWZ0ZXIgdGhlIHJlYWRs
IG9mIHRoZSBzdGF0dXMgcmVnIGF0IHRoZSBiZWdpbm5pbmcgDQo+ID4gb2YgYXNwZWVkX2kyY19t
YXN0ZXJfaXJxKCkuDQo+ID4gDQo+ID4gSSB3b3VsZCBiZSB2ZXJ5IHN1cnByaXNlZCBpZiB0aGF0
IGRpZG4ndCB3b3JrIHByb3Blcmx5IGFuZCB3YXNuJ3QgDQo+ID4gbXVjaCBzYWZlciB0aGFuIHdo
YXQgeW91IGFyZSBjdXJyZW50bHkgZG9pbmcuDQo+IA0KPiBJIHRoaW5rIEkgdHJpZWQgeW91ciB3
YXkgYW5kIGl0IHdvcmtlZC4gSW4gYW55Y2FzZSwgUnlhbiB3aWxsIGJlIGFibGUgDQo+IHRvIGNs
YXJpZnkgZm9yIHVzLg0KPiANCj4gPiANCj4gPiA+IExldCBtZSBrbm93IGlmIHlvdSBzdGlsbCB0
aGluayB3ZSBuZWVkIGEgIlJFQ09WRVJZIiBzdGF0ZS4NCj4gPiANCj4gPiBUaGUgd2F5IHlvdSBq
dXN0IHN3aXRjaCB0byBzdG9wIHN0YXRlIGFuZCBzdG9yZSB0aGUgZXJyb3IgZm9yIGxhdGVyIA0K
PiA+IHNob3VsZCB3b3JrIEkgdGhpbmsuDQo+ID4gDQo+ID4gPiA+IA0KPiA+ID4gPiA+ICvCoMKg
wqDCoMKgaWYgKGJ1cy0+bWFzdGVyX3N0YXRlID09IEFTUEVFRF9JMkNfTUFTVEVSX1NUQVJUKSB7
DQo+ID4gPiANCj4gPiA+IC4uLg0KPiA+ID4gPiANCj4gPiA+ID4gPiArwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgZGV2X2RiZyhidXMtPmRldiwNCj4gPiA+ID4gPiAr
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oCJubyBzbGF2ZSBwcmVzZW50IGF0ICUwMngiLA0KPiA+ID4gPiA+IG1zZy0NCj4gPiA+ID4gPiA+
IGFkZHIpOw0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqBzdGF0dXNfYWNrIHw9IEFTUEVFRF9JMkNEX0lOVFJfVFhfTkFLOw0K
PiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBidXMt
PmNtZF9lcnIgPSAtRUlPOw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqBkb19zdG9wKGJ1cyk7DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGdvdG8gb3V0X25vX2NvbXBsZXRlOw0KPiA+ID4gPiA+
ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoH0gZWxzZSB7DQo+ID4gPiA+ID4gK8KgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHN0YXR1c19hY2sgfD0gQVNQRUVEX0ky
Q0RfSU5UUl9UWF9BQ0s7DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoGlmIChtc2ctPmZsYWdzICYgSTJDX01fUkQpDQo+ID4gPiA+ID4gK8KgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBidXMt
Pm1hc3Rlcl9zdGF0ZSA9DQo+ID4gPiA+ID4gQVNQRUVEX0kyQ19NQVNURVJfUlg7DQo+ID4gPiA+
ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGVsc2UNCj4gPiA+
ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoGJ1cy0+bWFzdGVyX3N0YXRlID0NCj4gPiA+ID4gPiBBU1BFRURfSTJDX01BU1RFUl9U
WF9GSVJTVDsNCj4gPiA+ID4gDQo+ID4gPiA+IFdoYXQgYWJvdXQgdGhlIFNNQlVTX1FVSUNLIGNh
c2UgPyAoMC1sZW4gdHJhbnNmZXIpLiBEbyB3ZSBuZWVkIA0KPiA+ID4gPiB0byBoYW5kbGUgdGhp
cyBoZXJlID8gQSBxdWljayBsb29rIGF0IHRoZSBUWF9GSVJTVCBjYXNlIG1ha2VzIG1lIA0KPiA+
ID4gPiB0aGluayB3ZSBhcmUgb2sgdGhlcmUgYnV0IEknbSBub3Qgc3VyZSBhYm91dCB0aGUgUlgg
Y2FzZS4NCj4gPiA+IA0KPiA+ID4gSSBkaWQgbm90IHRoaW5rIHRoYXQgdGhlcmUgaXMgYW4gU01C
VVNfUVVJQ0sgUlguIENvdWxkIHlvdSBwb2ludCANCj4gPiA+IG1lIHRvIGFuIGV4YW1wbGU/DQo+
ID4gDQo+ID4gTm90IHNvIG11Y2ggYW4gUlgsIGl0J3MgbW9yZSBsaWtlIHlvdSBhcmUgc2VuZGlu
ZyBhIDEtYml0IGRhdGEgaW4gDQo+ID4gdGhlIHBsYWNlIG9mIHRoZSBSZC9XciBiaXQuIFNvIHlv
dSBoYXZlIGEgcmVhZCB3aXRoIGEgbGVuZ2h0IG9mIDAsIEkgDQo+ID4gZG9uJ3QgdGhpbmsgaW4g
dGhhdCBjYXNlIHlvdSBzaG91bGQgc2V0IEFTUEVFRF9JMkNEX01fUlhfQ01EIGluIA0KPiA+IF9f
YXNwZWVkX2kyY19kb19zdGFydA0KPiANCj4gRm9yZ2V0IHdoYXQgSSBzYWlkLCBJIHdhcyBqdXN0
IG5vdCB0aGlua2luZyBhYm91dCB0aGUgZmFjdCB0aGF0IFNNQnVzIA0KPiBlbXVsYXRpb24gY2F1
c2VzIHRoZSBkYXRhIGJpdCB0byBiZSBlbmNvZGVkIGFzIHRoZSBSL1cgZmxhZy4gSSBzZWUgDQo+
IHdoYXQgeW91IGFyZSBzYXlpbmc7IHlvdSBhcmUgY29ycmVjdC4NCj4gDQo+ID4gDQo+ID4gPiA+
IEknbSBub3Qgc3VyZSB0aGUgUlggY2FzZSBpcyB0aWdodCBhbHNvLiBXaGF0IGNvbXBsZXRpb24g
ZG9lcyB0aGUgDQo+ID4gPiA+IEhXIGdpdmUgeW91IGZvciB0aGUgYWRkcmVzcyBjeWNsZSA/IFdv
bid0IHlvdSBnZXQgdGhhdCBiZWZvcmUgaXQgDQo+ID4gPiA+IGhhcyByZWNlaXZlZCB0aGUgZmly
c3QgY2hhcmFjdGVyID8gSUUuIFlvdSBmYWxsIHRocm91Z2ggdG8gdGhlIA0KPiA+ID4gPiByZWFk
IGNhc2Ugb2YgdGhlIHN0YXRlIG1hY2hpbmUgd2l0aCB0aGUgcmVhZCBwb3RlbnRpYWxseSBub3Qg
DQo+ID4gPiA+IGNvbXBsZXRlIHlldCBubyA/DQo+ID4gPiANCj4gPiA+IC4uLg0KPiA+ID4gPiA+
ICvCoMKgwqDCoMKgY2FzZSBBU1BFRURfSTJDX01BU1RFUl9SWDoNCj4gPiA+ID4gPiArwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqBpZiAoIShpcnFfc3RhdHVzICYgQVNQRUVEX0kyQ0RfSU5UUl9S
WF9ET05FKSkgew0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqBkZXZfZXJyKGJ1cy0+ZGV2LCAibWFzdGVyIGZhaWxlZCB0bw0KPiA+ID4gPiA+IFJY
Iik7DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oGdvdG8gb3V0X2NvbXBsZXRlOw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oH0NCj4gPiA+ID4gDQo+ID4gPiA+IFNlZSBteSBjb21tZW50IGFib3ZlIGZvciBhIGJvZyBzdGFu
ZGFyZCBpMmNfcmVhZC4gQXJlbid0IHlvdSANCj4gPiA+ID4gZ2V0dGluZyB0aGUgY29tcGxldGlv
biBmb3IgdGhlIGFkZHJlc3MgYmVmb3JlIHRoZSByZWFkIGlzIGV2ZW4gDQo+ID4gPiA+IHN0YXJ0
ZWQgPw0KPiA+ID4gDQo+ID4gPiBJbiBwcmFjdGljZSBubywgYnV0IGl0IGlzIHByb2JhYmx5IGJl
c3QgdG8gYmUgc2FmZSA6LSkNCj4gPiANCj4gPiBZdXAgOikNCj4gPiA+ID4gDQo+ID4gPiA+ID4g
K8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgc3RhdHVzX2FjayB8PSBBU1BFRURfSTJDRF9JTlRS
X1JYX0RPTkU7DQo+ID4gPiA+ID4gKw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoHJlY3ZfYnl0ZSA9IGFzcGVlZF9pMmNfcmVhZChidXMsDQo+ID4gPiA+ID4gQVNQRUVEX0ky
Q19CWVRFX0JVRl9SRUcpID4+IDg7DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgbXNnLT5idWZbYnVzLT5idWZfaW5kZXgrK10gPSByZWN2X2J5dGU7DQo+ID4gPiA+ID4gKw0K
PiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGlmIChtc2ctPmZsYWdzICYgSTJD
X01fUkVDVl9MRU4gJiYNCj4gPiA+ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoHJlY3ZfYnl0ZSA8PSBJMkNfU01CVVNfQkxPQ0tfTUFYKSB7DQo+ID4gPiA+ID4gK8KgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoG1zZy0+bGVuID0gcmVjdl9ieXRl
ICsNCj4gPiA+ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAoKG1zZy0+ZmxhZ3MgJg0KPiA+ID4gPiA+
IEkyQ19DTElFTlRfUEVDKSA/IDIgOiAxKTsNCj4gPiA+IA0KPiA+ID4gLi4uDQo+ID4gPiA+ID4g
K8KgwqDCoMKgwqByZXR1cm4gKChjbGtfaGlnaCA8PCBBU1BFRURfSTJDRF9USU1FX1NDTF9ISUdI
X1NISUZUKQ0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCYgQVNQRUVEX0ky
Q0RfVElNRV9TQ0xfSElHSF9NQVNLKQ0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqB8ICgoY2xrX2xvdyA8PA0KPiA+ID4gPiA+IEFTUEVFRF9JMkNE
X1RJTUVfU0NMX0xPV19TSElGVCkNCj4gPiA+ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgJiBBU1BFRURfSTJDRF9USU1FX1NDTF9MT1dfTUFTSykN
Cj4gPiA+ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgfCAo
YmFzZV9jbGsgJg0KPiA+ID4gPiA+IEFTUEVFRF9JMkNEX1RJTUVfQkFTRV9ESVZJU09SX01BU0sp
Ow0KPiA+ID4gPiA+ICt9DQo+ID4gPiA+IA0KPiA+ID4gPiBBcyBJIHRoaW5rIEkgbWVudGlvbmVk
IGVhcmxpZXIsIHRoZSBBU1QyNTAwIGhhcyBhIHNsaWdodGx5IA0KPiA+ID4gPiBkaWZmZXJlbnQg
cmVnaXN0ZXIgbGF5b3V0IHdoaWNoIHN1cHBvcnQgbGFyZ2VyIHZhbHVlcyBmb3IgaGlnaCANCj4g
PiA+ID4gYW5kIGxvdywgdGh1cyBhbGxvd2luZyBhIGZpbmVyIGdyYW51bGFyaXR5Lg0KPiA+ID4g
DQo+ID4gPiBJIGFtIGRldmVsb3BpbmcgYWdhaW5zdCB0aGUgMjUwMC4NCj4gPiANCj4gPiBZZXMg
YnV0IHdlJ2QgbGlrZSB0aGUgZHJpdmVyIHRvIHdvcmsgd2l0aCBib3RoIDotKQ0KPiANCj4gUmln
aHQsIEkgdGhvdWdodCB5b3Ugd2VyZSBtYWtpbmcgYW4gYXNzZXJ0aW9uIGFib3V0IHRoZSAyNTAw
LCBpZiB5b3UgDQo+IGFyZSBtYWtpbmcgYW4gYXNzZXJ0aW9uIGFib3V0IHRoZSAyNDAwLCBJIGRv
IG5vdCBrbm93IGFuZCBkbyBub3QgaGF2ZSANCj4gb25lIGhhbmR5Lg0KPiANCj4gPiANCj4gPiA+
ID4gQlRXLiBJbiBjYXNlIHlvdSBoYXZlbid0LCBJIHdvdWxkIHN1Z2dlc3QgeW91IGNvcHkvcGFz
dGUgdGhlIA0KPiA+ID4gPiBhYm92ZSBpbiBhIHVzZXJzcGFjZSBhcHAgYW5kIHJ1biBpdCBmb3Ig
YWxsIGZyZXF1ZW5jeSBkaXZpc29ycyANCj4gPiA+ID4gYW5kIHNlZSBpZiB5b3VyIHJlc3VsdHMg
bWF0Y2ggdGhlIGFzcGVlZCB0YWJsZSA6KQ0KPiA+ID4gDQo+ID4gPiBHb29kIGNhbGwuDQo+ID4g
DQo+ID4gSWYgeW91IGVuZCB1cCBkb2luZyB0aGF0LCBjYW4geW91IHNob290IGl0IG15IHdheSA/
IEkgY2FuIHRha2UgY2FyZSANCj4gPiBvZiBtYWtpbmcgc3VyZSBpdCdzIGFsbCBnb29kIGZvciB0
aGUgMjQwMC4NCj4gDQo+IFdpbGwgZG8uDQo+IA0KPiA+IA0KPiA+ID4gPiA+ICtzdGF0aWMgaW50
IGFzcGVlZF9pMmNfaW5pdF9jbGsoc3RydWN0IGFzcGVlZF9pMmNfYnVzICpidXMsDQo+ID4gPiA+
ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikgew0KPiA+ID4gPiA+ICvCoMKgwqDCoMKg
dTMyIGNsa19mcmVxLCBkaXZpc29yOw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgc3RydWN0IGNsayAq
cGNsazsNCj4gPiA+ID4gPiArwqDCoMKgwqDCoGludCByZXQ7DQo+ID4gPiA+ID4gKw0KPiA+ID4g
PiA+ICvCoMKgwqDCoMKgcGNsayA9IGRldm1fY2xrX2dldCgmcGRldi0+ZGV2LCBOVUxMKTsNCj4g
PiA+ID4gPiArwqDCoMKgwqDCoGlmIChJU19FUlIocGNsaykpIHsNCj4gPiA+ID4gPiArwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqBkZXZfZXJyKCZwZGV2LT5kZXYsICJjbGtfZ2V0IGZhaWxlZFxu
Iik7DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgcmV0dXJuIFBUUl9FUlIo
cGNsayk7DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqB9DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqByZXQg
PSBvZl9wcm9wZXJ0eV9yZWFkX3UzMihwZGV2LT5kZXYub2Zfbm9kZSwNCj4gPiA+ID4gPiArwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoCJjbG9jay1mcmVxdWVuY3kiLA0KPiA+ID4gPiA+ICZjbGtfZnJlcSk7DQo+ID4gPiA+IA0K
PiA+ID4gPiBTZWUgbXkgcHJldmlvdXMgY29tbWVudCBhYm91dCBjYWxsaW5nIHRoYXQgJ2J1cy1m
cmVxdWVuY3knDQo+ID4gPiA+IHJhdGhlcg0KPiA+ID4gPiB0aGFuICdjbG9jay1mcmVxdWVuY3kn
Lg0KPiA+ID4gPiANCj4gPiA+ID4gPiArwqDCoMKgwqDCoGlmIChyZXQgPCAwKSB7DQo+ID4gPiA+
ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgZGV2X2VycigmcGRldi0+ZGV2LA0KPiA+ID4g
PiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAiQ291bGQgbm90
IHJlYWQgY2xvY2stZnJlcXVlbmN5DQo+ID4gPiA+ID4gcHJvcGVydHlcbiIpOw0KPiA+ID4gPiA+
ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGNsa19mcmVxID0gMTAwMDAwOw0KPiA+ID4gPiA+
ICvCoMKgwqDCoMKgfQ0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgZGl2aXNvciA9IGNsa19nZXRfcmF0
ZShwY2xrKSAvIGNsa19mcmVxOw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgLyogV2UganVzdCBuZWVk
IHRoZSBjbG9jayByYXRlLCB3ZSBkb24ndCBhY3R1YWxseSB1c2UNCj4gPiA+ID4gPiB0aGUNCj4g
PiA+ID4gPiBjbGsgb2JqZWN0LiAqLw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgZGV2bV9jbGtfcHV0
KCZwZGV2LT5kZXYsIHBjbGspOw0KPiA+ID4gPiA+ICsNCj4gPiA+ID4gPiArwqDCoMKgwqDCoC8q
IFNldCBBQyBUaW1pbmcgKi8NCj4gPiA+ID4gPiArwqDCoMKgwqDCoGlmIChjbGtfZnJlcSAvIDEw
MDAgPiAxMDAwKSB7DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgYXNwZWVk
X2kyY193cml0ZShidXMsIGFzcGVlZF9pMmNfcmVhZChidXMsDQo+ID4gPiA+ID4gK8KgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoEFTUEVFRF9JMg0KPiA+ID4gPiA+
IENfRlUNCj4gPiA+ID4gPiBOX0NUUkxfUkVHKSB8DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBBU1BFRURfSTJDRF9N
X0hJR0hfU1BFRURfRU4gfA0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgQVNQRUVEX0kyQ0RfTV9TREFfRFJJVkVfMVRf
RU4gfA0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgQVNQRUVEX0kyQ0RfU0RBX0RSSVZFXzFUX0VOLA0KPiA+ID4gPiA+
ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgQVNQRUVEX0kyQ19GVU5fQ1RSTF9SRUcpOw0KPiA+ID4gPiA+ICsNCj4gPiA+ID4gPiArwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBhc3BlZWRfaTJjX3dyaXRlKGJ1cywgMHgzLA0KPiA+ID4g
PiA+IEFTUEVFRF9JMkNfQUNfVElNSU5HX1JFRzIpOw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoGFzcGVlZF9pMmNfd3JpdGUoYnVzLA0KPiA+ID4gPiA+IGFzcGVlZF9pMmNf
Z2V0X2Nsa19yZWdfdmFsKGRpdmlzb3IpLA0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBBU1BFRURfSTJDX0FDX1RJ
TUlOR19SRUcxKTsNCj4gPiA+ID4gDQo+ID4gPiA+IEkgYWxyZWFkeSBkaXNjdXNzZWQgYnkgZG91
YnRzIGFib3V0IHRoZSBhYm92ZS4gSSBjYW4gdHJ5IHRvIA0KPiA+ID4gPiBzY29wZSBpdCB3aXRo
IHRoZSBFVkIgaWYgeW91IGRvbid0IGdldCB0byBpdC4gRm9yIG5vdyBJJ2QgcmF0aGVyIA0KPiA+
ID4gPiB0YWtlIHRoZSBjb2RlIG91dC4NCj4gPiA+ID4gDQo+ID4gPiA+IFdlIHNob3VsZCBhc2sg
YXNwZWVkIGZyb20gd2hhdCBmcmVxdWVuY3kgdGhlICIxVCIgc3R1ZmYgaXMgDQo+ID4gPiA+IHVz
ZWZ1bC4NCj4gPiA+IA0KPiA+ID4gV2lsbCBkbywgSSB3aWxsIHRyeSB0byByb3BlIFJ5YW4gaW4g
b24gdGhlIG5leHQgcmV2aWV3OyBpdCB3aWxsIGJlIA0KPiA+ID4gZ29vZCBmb3IgaGltIHRvIGdl
dCB1c2VkIHRvIHdvcmtpbmcgd2l0aCB1cHN0cmVhbSBhbnl3YXkuDQo+ID4gDQo+ID4gWXVwLiBI
b3dldmVyLCBmb3IgdGhlIHNha2Ugb2YgZ2V0dGluZyBzb21ldGhpbmcgdXBzdHJlYW0gKGFuZCBp
biANCj4gPiBPcGVuQk1DIDQuMTAga2VybmVsKSBhc2FwLCBJIHdvdWxkIHN1Z2dlc3QganVzdCBk
cm9wcGluZyBzdXBwb3J0IGZvciANCj4gPiB0aG9zZSBmYXN0IHNwZWVkcyBmb3Igbm93LCB3ZSBj
YW4gYWRkIHRoZW0gYmFjayBsYXRlci4NCj4gDQo+IEFscmlnaHQsIHRoYXQncyBmaW5lLiBTdGls
bCwgUnlhbiwgY291bGQgeW91IHByb3ZpZGUgc29tZSBjb250ZXh0IG9uIA0KPiB0aGlzPw0KPiAN
Cj4gPiANCj4gPiA+ID4gDQo+ID4gPiA+ID4gK8KgwqDCoMKgwqB9IGVsc2Ugew0KPiA+ID4gPiA+
ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGFzcGVlZF9pMmNfd3JpdGUoYnVzLA0KPiA+ID4g
PiA+IGFzcGVlZF9pMmNfZ2V0X2Nsa19yZWdfdmFsKGRpdmlzb3IpLA0KPiA+ID4gPiA+ICvCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBB
U1BFRURfSTJDX0FDX1RJTUlOR19SRUcxKTsNCj4gPiA+ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqBhc3BlZWRfaTJjX3dyaXRlKGJ1cywgQVNQRUVEX05PX1RJTUVPVVRfQ1RSTCwNCj4g
PiA+ID4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgQVNQRUVEX0kyQ19BQ19USU1JTkdfUkVHMik7DQo+ID4gPiA+ID4gK8KgwqDC
oMKgwqB9DQo+ID4gPiANCj4gPiA+IC4uLg0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgc3Bpbl9sb2Nr
X2luaXQoJmJ1cy0+bG9jayk7DQo+ID4gPiA+ID4gK8KgwqDCoMKgwqBpbml0X2NvbXBsZXRpb24o
JmJ1cy0+Y21kX2NvbXBsZXRlKTsNCj4gPiA+ID4gPiArwqDCoMKgwqDCoGJ1cy0+YWRhcC5vd25l
ciA9IFRISVNfTU9EVUxFOw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgYnVzLT5hZGFwLnJldHJpZXMg
PSAwOw0KPiA+ID4gPiA+ICvCoMKgwqDCoMKgYnVzLT5hZGFwLnRpbWVvdXQgPSA1ICogSFo7DQo+
ID4gPiA+ID4gK8KgwqDCoMKgwqBidXMtPmFkYXAuYWxnbyA9ICZhc3BlZWRfaTJjX2FsZ287DQo+
ID4gPiA+ID4gK8KgwqDCoMKgwqBidXMtPmFkYXAuYWxnb19kYXRhID0gYnVzOw0KPiA+ID4gPiA+
ICvCoMKgwqDCoMKgYnVzLT5hZGFwLmRldi5wYXJlbnQgPSAmcGRldi0+ZGV2Ow0KPiA+ID4gPiA+
ICvCoMKgwqDCoMKgYnVzLT5hZGFwLmRldi5vZl9ub2RlID0gcGRldi0+ZGV2Lm9mX25vZGU7DQo+
ID4gPiA+ID4gK8KgwqDCoMKgwqBzbnByaW50ZihidXMtPmFkYXAubmFtZSwgc2l6ZW9mKGJ1cy0+
YWRhcC5uYW1lKSwNCj4gPiA+ID4gPiAiQXNwZWVkDQo+ID4gPiA+ID4gaTJjIik7DQo+ID4gPiA+
IA0KPiA+ID4gPiBBbm90aGVyIHRyaXZpYWwgb25lLCBzaG91bGQgd2UgcHV0IHNvbWUga2luZCBv
ZiBidXMgbnVtYmVyIGluIA0KPiA+ID4gPiB0aGF0IHN0cmluZyA/DQo+ID4gPiANCj4gPiA+IFdo
b29wcywgbG9va3MgbGlrZSBJIG1pc3NlZCB0aGlzIG9uZTsgSSB3aWxsIGdldCB0byBpdCBpbiB0
aGUgbmV4dCANCj4gPiA+IHJldmlzaW9uLg0KPiA+IA0KPiA+IE9rLiBJIG5vdGljZWQgeW91IG1p
c3NlZCB0aGF0IGluIHY3LCBzbyBJIGFzc3VtZSB5b3UgbWVhbiB2OCA6LSkNCj4gDQo+IFllcCwg
SSB3aWxsIGdldCBpdCBpbiB2OC4NCj4gDQo+ID4gDQo+ID4gPiA+IA0KPiA+ID4gPiA+ICvCoMKg
wqDCoMKgYnVzLT5kZXYgPSAmcGRldi0+ZGV2Ow0KPiA+ID4gPiA+ICsNCj4gPiA+ID4gPiArwqDC
oMKgwqDCoC8qIHJlc2V0IGRldmljZTogZGlzYWJsZSBtYXN0ZXIgJiBzbGF2ZSBmdW5jdGlvbnMg
Ki8NCj4gPiA+ID4gPiArwqDCoMKgwqDCoGFzcGVlZF9pMmNfd3JpdGUoYnVzLCAwLCBBU1BFRURf
STJDX0ZVTl9DVFJMX1JFRyk7DQo+ID4gPiANCj4gPiA+IC4uLg0KPiA+ID4gLS0NCj4gPiA+IFRv
IHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSAN
Cj4gPiA+IGRldmljZXRyZWUiDQo+ID4gPiBpbg0KPiA+ID4gdGhlIGJvZHkgb2YgYSBtZXNzYWdl
IHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcgTW9yZSBtYWpvcmRvbW8gDQo+ID4gPiBpbmZv
IGF0wqDCoGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0K
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brendan Higgins April 25, 2017, 7:50 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ryan Chen April 26, 2017, 12:52 a.m.
> 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.

Patch hide | download patch | download mbox

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"
+	depends on ARCH_ASPEED
+	help
+	  If you say yes to this option, support will be included for the
+	  Aspeed AST2xxx SoC I2C controller.
+
+	  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/Makefile b/drivers/i2c/busses/Makefile
index 30b60855fbcd..e84604b9bf3b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -29,6 +29,7 @@  obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 
 # Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_AXXIA)		+= i2c-axxia.o
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
@@ -0,0 +1,610 @@ 
+/*
+ *  Aspeed 24XX/25XX I2C Interrupt Controller.
+ *
+ *  Copyright (C) 2012-2017 ASPEED Technology Inc.
+ *  Copyright 2017 IBM Corporation
+ *  Copyright 2017 Google, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* I2C Register */
+#define ASPEED_I2C_FUN_CTRL_REG				0x00
+#define ASPEED_I2C_AC_TIMING_REG1			0x04
+#define ASPEED_I2C_AC_TIMING_REG2			0x08
+#define ASPEED_I2C_INTR_CTRL_REG			0x0c
+#define ASPEED_I2C_INTR_STS_REG				0x10
+#define ASPEED_I2C_CMD_REG				0x14
+#define ASPEED_I2C_DEV_ADDR_REG				0x18
+#define ASPEED_I2C_BYTE_BUF_REG				0x20
+
+/* Global Register Definition */
+/* 0x00 : I2C Interrupt Status Register  */
+/* 0x08 : I2C Interrupt Target Assignment  */
+
+/* Device Register Definition */
+/* 0x00 : I2CD Function Control Register  */
+#define ASPEED_I2CD_MULTI_MASTER_DIS			BIT(15)
+#define ASPEED_I2CD_SDA_DRIVE_1T_EN			BIT(8)
+#define ASPEED_I2CD_M_SDA_DRIVE_1T_EN			BIT(7)
+#define ASPEED_I2CD_M_HIGH_SPEED_EN			BIT(6)
+#define ASPEED_I2CD_MASTER_EN				BIT(0)
+
+/* 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
+
+/* 0x0c : I2CD Interrupt Control Register &
+ * 0x10 : I2CD Interrupt Status Register
+ *
+ * These share bit definitions, so use the same values for the enable &
+ * status bits.
+ */
+#define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT			BIT(14)
+#define ASPEED_I2CD_INTR_BUS_RECOVER_DONE		BIT(13)
+#define ASPEED_I2CD_INTR_SCL_TIMEOUT			BIT(6)
+#define ASPEED_I2CD_INTR_ABNORMAL			BIT(5)
+#define ASPEED_I2CD_INTR_NORMAL_STOP			BIT(4)
+#define ASPEED_I2CD_INTR_ARBIT_LOSS			BIT(3)
+#define ASPEED_I2CD_INTR_RX_DONE			BIT(2)
+#define ASPEED_I2CD_INTR_TX_NAK				BIT(1)
+#define ASPEED_I2CD_INTR_TX_ACK				BIT(0)
+#define ASPEED_I2CD_INTR_ERROR						       \
+		(ASPEED_I2CD_INTR_ARBIT_LOSS |				       \
+		 ASPEED_I2CD_INTR_ABNORMAL |				       \
+		 ASPEED_I2CD_INTR_SCL_TIMEOUT |				       \
+		 ASPEED_I2CD_INTR_SDA_DL_TIMEOUT)
+#define ASPEED_I2CD_INTR_ALL						       \
+		(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |			       \
+		 ASPEED_I2CD_INTR_BUS_RECOVER_DONE |			       \
+		 ASPEED_I2CD_INTR_SCL_TIMEOUT |				       \
+		 ASPEED_I2CD_INTR_ABNORMAL |				       \
+		 ASPEED_I2CD_INTR_NORMAL_STOP |				       \
+		 ASPEED_I2CD_INTR_ARBIT_LOSS |				       \
+		 ASPEED_I2CD_INTR_RX_DONE |				       \
+		 ASPEED_I2CD_INTR_TX_NAK |				       \
+		 ASPEED_I2CD_INTR_TX_ACK)
+
+/* 0x14 : I2CD Command/Status Register   */
+#define ASPEED_I2CD_SCL_LINE_STS			BIT(18)
+#define ASPEED_I2CD_SDA_LINE_STS			BIT(17)
+#define ASPEED_I2CD_BUS_BUSY_STS			BIT(16)
+#define ASPEED_I2CD_BUS_RECOVER_CMD			BIT(11)
+
+/* Command Bit */
+#define ASPEED_I2CD_M_STOP_CMD				BIT(5)
+#define ASPEED_I2CD_M_S_RX_CMD_LAST			BIT(4)
+#define ASPEED_I2CD_M_RX_CMD				BIT(3)
+#define ASPEED_I2CD_S_TX_CMD				BIT(2)
+#define ASPEED_I2CD_M_TX_CMD				BIT(1)
+#define ASPEED_I2CD_M_START_CMD				BIT(0)
+
+enum aspeed_i2c_master_state {
+	ASPEED_I2C_MASTER_START,
+	ASPEED_I2C_MASTER_TX_FIRST,
+	ASPEED_I2C_MASTER_TX,
+	ASPEED_I2C_MASTER_RX,
+	ASPEED_I2C_MASTER_STOP,
+	ASPEED_I2C_MASTER_INACTIVE,
+};
+
+struct aspeed_i2c_bus {
+	struct i2c_adapter		adap;
+	struct device			*dev;
+	void __iomem			*base;
+	/* Synchronizes I/O mem access to base. */
+	spinlock_t			lock;
+	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
+};
+
+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);
+}
+
+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);
+
+		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);
+		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;
+	}
+
+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;
+	}
+
+	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;
+
+	spin_lock(&bus->lock);
+	irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
+
+	if (irq_status & ASPEED_I2CD_INTR_ERROR ||
+	    (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP)) {
+		dev_dbg(bus->dev, "received error interrupt: 0x%08x",
+			irq_status);
+		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;
+	}
+
+	if (bus->master_state == ASPEED_I2C_MASTER_START) {
+		if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK)) {
+			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;
+		}
+	}
+
+	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;
+		} else if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK)) {
+			dev_err(bus->dev, "slave failed to ACK TX");
+			goto out_complete;
+		}
+		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;
+		}
+		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;
+		}
+
+		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);
+		}
+		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;
+}
+
+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);
+	bus->msgs = msgs;
+	bus->msgs_index = 0;
+	bus->msgs_size = num;
+
+	do_start(bus);
+	reinit_completion(&bus->cmd_complete);
+	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;
+	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);
+}
+
+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);
+	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);
+	} 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;
+	}
+
+	/* 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");
+
+	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);
+
+	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");