Re: [PATCH] i3c: dw: Fix IBI intr signal programming

From: Jeremy Kerr <jk@codeconstruct.com.au>
To: "Aniket ." <aniketmaurya@google.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Joel Stanley <joel@jms.id.au>,
	Billy Tsai <billy_tsai@aspeedtech.com>,
	 linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i3c: dw: Fix IBI intr signal programming
Date: Fri, 07 Jun 2024 13:11:16 +0800	[thread overview]
Message-ID: <3decb5293cb8fb2e8725600a8c5c930c32178602.camel@codeconstruct.com.au> (raw)
In-Reply-To: <CAMmmMt3u=DB5onXdayMN5ZHvCmdnam4wOo0hKizve4K0LnZLZQ@mail.gmail.com>

Hi Aniket,

> > I think we're OK in this case (just not reading the value out of the
> > SIR_REQ_REJECT register), but any thoughts on adding corresponding
> > switches in the driver so we can support those configurations? These
> > would be represented as DT config of the specific hardware instance - at
> > the most granular, just by the specific compatible string.
> 
> We can go with some DT quirk, but I don't see the strong need to do this
> here.

Oh definitely - the behaviour here doesn't need any special handling
that would warrant a quirk/etc.

This is more for handling IP configuration options we may see in future.
For example, I believe support for target/secondary mode is entirely
optional too.

> > Could we use the SIR mask for this, but just read it from a field in the
> > struct dw_i3c_master, instead of IBI_SIR_REQ_REJECT?
> > 
> > This would mean that there's no possibility of the counter going out of
> > sync from the SIR settings - say, on underflow if we get a spurious
> > disable.
> 
> Yes, we can keep a SW SIR mask instead of a counter. It would replace
> all the places where we read IBI_SIR_REQ_REJECT.
> Both methods are okay, but if you think the mask might come in handy in
> some situations rather than just the count, we can go with that.
> Let me know your thoughts on this.

I think keeping the mask value locally would be best. this means we

 1) cannot get the counter and mask out of sync; and
 2) don't need to do a read-modify-write on a register that is only
    updated by the driver.

Cheers,

Jeremy

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c