| Message ID | 20250228141802.1344453-1-jarkko.nikula@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates | expand |
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? > > 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." > > Signed-off-by: Jarkko Nikula <jarkko.nikula@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. Frank > > /* Make sure our data ordering fits the host's */ > regval = reg_read(HC_CONTROL); > -- > 2.47.2 > > > -- > linux-i3c mailing list > linux-i3c@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-i3c
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@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.
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@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-i3c
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)); /* Make sure our data ordering fits the host's */ regval = reg_read(HC_CONTROL);
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. 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. Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- drivers/i3c/master/mipi-i3c-hci/core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)