| Message ID | 20170328051226.21677-5-brendanhiggins@google.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
>> + 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
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
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
>> +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
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
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
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
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
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
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
> 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.
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");
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