| Message ID | 20220405092833.83335-2-Ajish.Koshy@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63 | expand |
On 4/5/22 18:28, Ajish Koshy wrote: > When upper inbound and outbound queues 32-63 are enabled, we see upper > vectors 32-63 in interrupt service routine. We need corresponding > registers to handle masking and unmasking of these upper interrupts. > > To achieve this, we use registers MSGU_ODMR_U(0x34) to mask and > MSGU_ODMR_CLR_U(0x3C) to unmask the interrupts. In these registers bit > 0-31 represents interrupt vectors 32-63. > > Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com> > Signed-off-by: Viswas G <Viswas.G@microchip.com> > Fixes: 05c6c029a44d ("scsi: pm80xx: Increase number of supported queues") > --- > drivers/scsi/pm8001/pm80xx_hwi.c | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c > index 9bb31f66db85..3e6413e21bfe 100644 > --- a/drivers/scsi/pm8001/pm80xx_hwi.c > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c > @@ -1728,9 +1728,17 @@ pm80xx_chip_interrupt_enable(struct pm8001_hba_info *pm8001_ha, u8 vec) > { > #ifdef PM8001_USE_MSIX > u32 mask; > - mask = (u32)(1 << vec); > > - pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, (u32)(mask & 0xFFFFFFFF)); > + if (vec < 32) { > + mask = 1U << vec; Nit: Drop this... > + /*vectors 0 - 31*/ > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, mask); ...and do: pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, 1U << vec); > + } else { > + vec = vec - 32; > + mask = 1U << vec; > + /*vectors 32 - 63*/ > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U, mask); And here: pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U, 1U << (vec - 32)); Then you do not need the mask variable. > + } > return; > #endif > pm80xx_chip_intx_interrupt_enable(pm8001_ha); > @@ -1747,11 +1755,22 @@ pm80xx_chip_interrupt_disable(struct pm8001_hba_info *pm8001_ha, u8 vec) > { > #ifdef PM8001_USE_MSIX > u32 mask; > - if (vec == 0xFF) > + > + if (vec == 0xFF) { This is not symmetric with pm80xx_chip_interrupt_enable(). Does pm80xx_chip_interrupt_enable() need the same case too ? > mask = 0xFFFFFFFF; > - else > - mask = (u32)(1 << vec); > - pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, (u32)(mask & 0xFFFFFFFF)); > + /* disable all vectors 0-31, 32-63*/ > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask); > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask); Similar here, no need for the mask variable. pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, 0xFFFFFFFF); pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, 0xFFFFFFFF); > + } else if (vec < 32) { > + mask = 1U << vec; > + /*vectors 0 - 31*/ > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask); And here. pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, 1U << vec); > + } else { > + vec = vec - 32; > + mask = 1U << vec; > + /*vectors 32 - 63*/ > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask); And here. pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, 1U << (vec - 32)); > + } > return; > #endif > pm80xx_chip_intx_interrupt_disable(pm8001_ha);
Thank you Damien for your comments below. Will make changes in v3. > On 4/5/22 18:28, Ajish Koshy wrote: > > When upper inbound and outbound queues 32-63 are enabled, we see > upper > > vectors 32-63 in interrupt service routine. We need corresponding > > registers to handle masking and unmasking of these upper interrupts. > > > > To achieve this, we use registers MSGU_ODMR_U(0x34) to mask and > > MSGU_ODMR_CLR_U(0x3C) to unmask the interrupts. In these registers bit > > 0-31 represents interrupt vectors 32-63. > > > > Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com> > > Signed-off-by: Viswas G <Viswas.G@microchip.com> > > Fixes: 05c6c029a44d ("scsi: pm80xx: Increase number of supported > > queues") > > --- > > drivers/scsi/pm8001/pm80xx_hwi.c | 31 +++++++++++++++++++++++++---- > -- > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c > > b/drivers/scsi/pm8001/pm80xx_hwi.c > > index 9bb31f66db85..3e6413e21bfe 100644 > > --- a/drivers/scsi/pm8001/pm80xx_hwi.c > > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c > > @@ -1728,9 +1728,17 @@ pm80xx_chip_interrupt_enable(struct > pm8001_hba_info *pm8001_ha, u8 vec) > > { > > #ifdef PM8001_USE_MSIX > > u32 mask; > > - mask = (u32)(1 << vec); > > > > - pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, (u32)(mask & > 0xFFFFFFFF)); > > + if (vec < 32) { > > + mask = 1U << vec; > > Nit: Drop this... > > > + /*vectors 0 - 31*/ > > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, mask); > > ...and do: > > pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, 1U << vec); OK > > > + } else { > > + vec = vec - 32; > > + mask = 1U << vec; > > + /*vectors 32 - 63*/ > > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U, mask); > > And here: > > pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U, > 1U << (vec - 32)); > > Then you do not need the mask variable. OK > > > + } > > return; > > #endif > > pm80xx_chip_intx_interrupt_enable(pm8001_ha); > > @@ -1747,11 +1755,22 @@ pm80xx_chip_interrupt_disable(struct > pm8001_hba_info *pm8001_ha, u8 vec) > > { > > #ifdef PM8001_USE_MSIX > > u32 mask; > > - if (vec == 0xFF) > > + > > + if (vec == 0xFF) { > > This is not symmetric with pm80xx_chip_interrupt_enable(). Does > pm80xx_chip_interrupt_enable() need the same case too ? I believe it's not needed right now. For interrupt disable I could to see the following entry point PM8001_CHIP_DISP->interrupt_disable(pm8001_ha, 0xFF) is used in pm8001_pci_remove, pm8001_pci_suspend, pm8001_pci_resume But same is not the case with interrupt enable function. I don't see interrupt enable being called with 0xFF. > > > mask = 0xFFFFFFFF; > > - else > > - mask = (u32)(1 << vec); > > - pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, (u32)(mask & > 0xFFFFFFFF)); > > + /* disable all vectors 0-31, 32-63*/ > > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask); > > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask); > > Similar here, no need for the mask variable. OK. > > pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, 0xFFFFFFFF); > pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, 0xFFFFFFFF); > > > + } else if (vec < 32) { > > + mask = 1U << vec; > > + /*vectors 0 - 31*/ > > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask); > > And here. > pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, 1U << vec); OK > > > + } else { > > + vec = vec - 32; > > + mask = 1U << vec; > > + /*vectors 32 - 63*/ > > + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask); > > And here. > pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, > 1U << (vec - 32)); OK > > > + } > > return; > > #endif > > pm80xx_chip_intx_interrupt_disable(pm8001_ha); > > > -- > Damien Le Moal > Western Digital Research
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c index 9bb31f66db85..3e6413e21bfe 100644 --- a/drivers/scsi/pm8001/pm80xx_hwi.c +++ b/drivers/scsi/pm8001/pm80xx_hwi.c @@ -1728,9 +1728,17 @@ pm80xx_chip_interrupt_enable(struct pm8001_hba_info *pm8001_ha, u8 vec) { #ifdef PM8001_USE_MSIX u32 mask; - mask = (u32)(1 << vec); - pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, (u32)(mask & 0xFFFFFFFF)); + if (vec < 32) { + mask = 1U << vec; + /*vectors 0 - 31*/ + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR, mask); + } else { + vec = vec - 32; + mask = 1U << vec; + /*vectors 32 - 63*/ + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_CLR_U, mask); + } return; #endif pm80xx_chip_intx_interrupt_enable(pm8001_ha); @@ -1747,11 +1755,22 @@ pm80xx_chip_interrupt_disable(struct pm8001_hba_info *pm8001_ha, u8 vec) { #ifdef PM8001_USE_MSIX u32 mask; - if (vec == 0xFF) + + if (vec == 0xFF) { mask = 0xFFFFFFFF; - else - mask = (u32)(1 << vec); - pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, (u32)(mask & 0xFFFFFFFF)); + /* disable all vectors 0-31, 32-63*/ + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask); + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask); + } else if (vec < 32) { + mask = 1U << vec; + /*vectors 0 - 31*/ + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR, mask); + } else { + vec = vec - 32; + mask = 1U << vec; + /*vectors 32 - 63*/ + pm8001_cw32(pm8001_ha, 0, MSGU_ODMR_U, mask); + } return; #endif pm80xx_chip_intx_interrupt_disable(pm8001_ha);