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

[Benjamin Herrenschmidt]

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@xxxxxxxxxx]Â;
> Sent: Tuesday, April 25, 2017 4:32 PM
> To: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx
> >; Mark Rutland <mark.rutland@xxxxxxx>; Thomas Gleixner <tglx@linutro
> nix.de>; Jason Cooper <jason@xxxxxxxxxxxxxx>; Marc Zyngier <marc.zyng
> ier@xxxxxxx>; Joel Stanley <joel@xxxxxxxxx>; Vladimir Zapolskiy <vz@m
> leia.com>; Kachalov Anton <mouse@xxxxxxx>; CÃdric Le Goater <clg@kaod
> .org>; linux-i2c@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Linux
> Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; OpenBMC Maillist
> <openbmc@xxxxxxxxxxxxxxxx>; Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx More
> > > majordomoÂ
> > > info atÂÂhttp://vger.kernel.org/majordomo-info.html