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