Re: [PATCH v1] i2c: microchip-core: actually use repeated sends

[Conor Dooley]

On Tue, Oct 01, 2024 at 10:50:56AM +0200, Wolfram Sang wrote:
> > At present, where repeated sends are intended to be used, the
> > i2c-microchip-core driver sends a stop followed by a start. Lots of i2c
> 
> Oh, this is wrong. Was this just overlooked or was maybe older hardware
> not able to generated correct repeated-starts?

Overlooked, because the devices that had been used until recently didn't
care about whether they got a repeated start or stop + start. The bare
metal driver upon which the Linux one was originally based had a trivial
time of supporting repeated starts because it only allows specific sorts
of transfers. I kinda doubt you care, but the bare metal implementation
is here:
https://github.com/polarfire-soc/polarfire-soc-bare-metal-library/blob/614a67abb3023ba47ea6d1b8d7b9a9997353e007/src/platform/drivers/mss/mss_i2c/mss_i2c.c#L737

It just must have been missed that the bare metal method was not replaced.

> > devices must not malfunction in the face of this behaviour, because the
> > driver has operated like this for years! Try to keep track of whether or
> > not a repeated send is required, and suppress sending a stop in these
> > cases.
> 
> ? I don't get that argument. If the driver is expected to do a repeated
> start, it should do a repeated start. If it didn't, it was a bug and you
> were lucky that the targets could handle this. Because most controllers
> can do repeated starts correctly, we can also argue that this works for
> most targets for years. In the unlikely event that a target fails after
> converting this driver to proper repeated starts, the target is buggy
> and needs fixing. It would not work with the majority of other
> controllers this way.
> 
> I didn't look at the code but reading "keeping track whether rep start
> is required" looks wrong from a high level perspective.

I think if you had looked at the code, you'd (hopefully) understand what
I meant w.r.t. tracking that.
The design of this IP is pretty old, and intended for use with other
logic implemented in FPGA fabric where each interrupt generated by
the core would be the stimulus for the state machine controlling it to
transition state. Cos of that, when controlling it from software, the
interrupt handler assumes the role of that state machine. When I talk
about tracking whether or not a repeated send is required, that's
whether or not a particular message in a transfer requires it, not
whether or not the target device requires them or not.

Currently the driver operates by iterating over a list of messages in a
transfer, and calling send() for each one, and then effectively "looping"
in the interrupt handler until the message has been sent. By looking at
the current code, you can see that the completion's "lifecycle" matches
that. Currently, at the end of each message being sent
	static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev *idev)
	{
	
		<snip>
	
		/* On the last byte to be transmitted, send STOP */
		if (last_byte)
			mchp_corei2c_stop(idev);
	
		if (last_byte || finished)
			complete(&idev->msg_complete);
	
		return IRQ_HANDLED;
	}
a stop is put on the bus, unless !last_byte, which is only true in error
cases. Clearly I don't need to explain why that is a problem to you...
You'd think that we could do something like moving the stop out of the
interrupt handler, and to the loop in mchp_corei2c_xfer(), where we have
access to the transfer's message list and can check if a stop should be
sent or not - that's not really possible with the hardware we have.

When the interrupt handler completes, it clears the interrupt bit in the
IP, as you might expect. The controller IP uses that as the trigger to
transition state in its state machine, which is detailed in
https://ww1.microchip.com/downloads/aemDocuments/documents/FPGA/ProductDocuments/UserGuides/ip_cores/directcores/CoreI2C_HB.pdf
On page 23, row 0x28, you can see the case that (IIRC) is the
problematic one. It is impossible to leave this state without triggering
some sort of action.
The only way that I could see to make this work correctly was to get the
driver track whether or not the next message required a repeated start or
not, so as to transition out of state 0x28 correctly.

Unfortunately, then the clearing of the interrupt bit causing state
transitions kicked in again - after sending a repeated start, it will
immediately attempt to act (see state 0x10 on page 23). Without
reworking the driver to send entire transfers "in one go" (where the
completion is that of the transfer rather than the message as it
currently is) the controller will re-send the last target address +
read/write command it sent, instead of the next one. That's why there's
so many changes outside of the interrupt handler and so many additional
members in the controller's private data structure.

I hope that that at least makes some sense..

> The driver
> should do repeated start when it should do repeated start.

Yup, that's what I'm trying to do here :)

Attachment: signature.asc
Description: PGP signature