Message-ID: <20241217-probe-snowdrift-9c4b9521a2e9@spud>
Date: Tue, 17 Dec 2024 18:35:50 +0000
From: Conor Dooley <conor@...nel.org>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc: linux-i2c@...r.kernel.org, Conor Dooley <conor.dooley@...rochip.com>,
	Daire McNamara <daire.mcnamara@...rochip.com>,
	Andi Shyti <andi.shyti@...nel.org>, Wolfram Sang <wsa@...nel.org>,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	valentina.fernandezalanis@...rochip.com
Subject: Re: [PATCH v1] i2c: microchip-core: actually use repeated sends

Andi, Wolfram,

I'm just going to resend this tomorrow, it's 2.5 months since this
conversation started and I'd really like to get this fix included.

Cheers,
conor.

On Fri, Dec 06, 2024 at 11:48:07AM +0000, Conor Dooley wrote:
> On Thu, Oct 24, 2024 at 10:36:33AM +0100, Conor Dooley wrote:
> > On Tue, Oct 01, 2024 at 11:16:24AM +0100, Conor Dooley wrote:
> > > 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 :)
> > 
> > I'd like to get this fix in, and Andi only had some minor comments that
> > didn't require a respin. I don't want to respin or resend while this
> > conversation remains unresolved.
> 
> Could you please respond to this thread? I don't want to respin without
> resolving this conversation since I feel like we'd just end up having it
> all over again.
> 
> Thanks,
> Conor.



Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists