Message-ID: <20241024-snagged-elated-d168d0d6bf35@spud>
Date: Thu, 24 Oct 2024 10:36:33 +0100
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
Subject: Re: [PATCH v1] i2c: microchip-core: actually use repeated sends

Yo,

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.

Thanks,
Conor.

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

Powered by blists - more mailing lists