[PATCH 1/4] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates

[Frank Li]

On Fri, Mar 07, 2025 at 11:14:06AM +0200, Jarkko Nikula wrote:
> Hi Frank
>
> Sorry the delay, somehow I missed your comments to this patch :-|
>
> On 2/28/25 6:00 PM, Frank Li wrote:
> > On Fri, Feb 28, 2025 at 04:17:59PM +0200, Jarkko Nikula wrote:
> > > Since MIPI I3C HCI specification version v0.8 INTR_STATUS bits 9:0 are
> > > reserved. Version v0.5 has bits 9 and 5:0 in use but not handled by the
> > > current driver code and not needed in DMA transfers.
> > >
> > > PIO transfers with v0.5 would require changes to both
> > > core.c: i3c_hci_irq_handler() and pio.c: hci_pio_irq_handler() though.
> >
> > If reasonable, why not change core.c and pio.c?
> >
> I had two reasons for that. The v0.5 compatible IP I use in testing doesn't
> support PIO transfers and thus didn't want to add code that is not tested.
>
> Then I believe perhaps nobody will release HW with this old IP and thus it
> would be dead code anyway.
>
> > >
> > > For these reasons don't enable signal updates from INTR_STATUS bits 9:0.
> > >
> > > This change is a no-op for specification versions v0.8 and beyond but
> > > gets rid of "unexpected INTR_STATUS" errors if somebody (read me) wants
> > > to run code on old v0.5 IP version.
> >
> > I think, simple said
> > "Get rid of "unexpected INTR_STATUS" errors at old v0.5 IP version and
> > no-op for version above v0.8."
> >
> Makes sense, will change.
>
> > >
> > > Signed-off-by: Jarkko Nikula <jarkko.nikula at linux.intel.com>
> > > ---
> > >   drivers/i3c/master/mipi-i3c-hci/core.c | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> > > index a71226d7ca59..e139d7e4d252 100644
> > > --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> > > +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> > > @@ -699,9 +699,10 @@ static int i3c_hci_init(struct i3c_hci *hci)
> > >   	if (ret)
> > >   		return -ENXIO;
> > >
> > > -	/* Disable all interrupts and allow all signal updates */
> > > +	/* Disable all interrupts */
> > >   	reg_write(INTR_SIGNAL_ENABLE, 0x0);
> > > -	reg_write(INTR_STATUS_ENABLE, 0xffffffff);
> > > +	/* Allow signal updates relevant to IP versions 0.8 and beyond */
> > > +	reg_write(INTR_STATUS_ENABLE, GENMASK(31, 10));
> >
> > Generally, just enable needed IRQ in driver. Define some BITS for
> > difference type IRQs.
> >
> I was thinking too these need changes too but decided those are better to do
> in another patch(es).
>
> Currently all of these generic interrupt sources are disabled so they won't
> generate interrupt but their status will be printed when handler is called
> due to DMA/PIO interrupt. Known bits 10 INTR_HC_INTERNAL_ERR and 11
> INTR_HC_SEQ_CANCEL will print their message and unknowns just "unexpected
> INTR_STATUS" with hex of unhandled status bits.
>
> Bits 12:14 can be defined since v1.2 describes them. I'm open for ideas does
> it make more sense to allow signal updates only from known sources or keep
> all bits enabled so that in future IP versions "unexpected INTR_STATUS" can
> be seen from user reports if something odd happens and driver is not yet
> handling that.

I think we can defer it until someone really complain it. I am fine for
your current change.

Frank
>
> Another change is should known generic interrupt sources be enabled so
> handler will be called immediately when that condition occurs.
>
> --
> linux-i3c mailing list
> linux-i3c at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c