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

[Jarkko Nikula]

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.

Another change is should known generic interrupt sources be enabled so 
handler will be called immediately when that condition occurs.