[PATCH v2 0/2] pm80xx updates

[PATCH v2 0/2] pm80xx updates

[Ajish Koshy]

This patchset includes bugfixes for pm80xx driver

Changes from v1 to v2:
	- For upper interrupt vectors patch
		- Removed unrequired casts u32
		- Removed '& 0xFFFFFFFF' operation
		- Removed 'vec_u' variable
		- Added 'Fixes' tag.
	- For upper inbound outbound queues patch
		- Removed brackets
		- Removed comments about msleep
		- Added 'Fixes' tag.

Ajish Koshy (2):
  scsi: pm80xx: mask and unmask upper interrupt vectors 32-63
  scsi: pm80xx: enable upper inbound, outbound queues

 drivers/scsi/pm8001/pm80xx_hwi.c | 37 ++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63

[Ajish Koshy]

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;
+		/*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);
-- 
2.31.1

[PATCH v2 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Ajish Koshy]

Executing driver on servers with more than 32 CPUs were faced with command
timeouts. This is because we were not geting completions for commands
submitted on IQ32 - IQ63.

Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in the
MPI main configuration table.

Added 500ms delay after successful MPI initialization as mentioned in
controller datasheet.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 3e6413e21bfe..c41c24a4b906 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -766,6 +766,10 @@ static void init_default_table_values(struct pm8001_hba_info *pm8001_ha)
 	pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity	= 0x01;
 	pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt		= 0x01;
 
+	/* Enable higher IQs and OQs, 32 to 63, bit 16*/
+	if (pm8001_ha->max_q_num > 32)
+		pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
+							1 << 16;
 	/* Disable end to end CRC checking */
 	pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 << 16);
 
@@ -1027,6 +1031,8 @@ static int mpi_init_check(struct pm8001_hba_info *pm8001_ha)
 	if (0x0000 != gst_len_mpistate)
 		return -EBUSY;
 
+	msleep(500);
+
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH v2 1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63

[Damien Le Moal]

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);


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Damien Le Moal]

On 4/5/22 18:28, Ajish Koshy wrote:
> Executing driver on servers with more than 32 CPUs were faced with command
> timeouts. This is because we were not geting completions for commands
> submitted on IQ32 - IQ63.
> 
> Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in the
> MPI main configuration table.
> 
> Added 500ms delay after successful MPI initialization as mentioned in
> controller datasheet.
> 
> 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 | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 3e6413e21bfe..c41c24a4b906 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -766,6 +766,10 @@ static void init_default_table_values(struct pm8001_hba_info *pm8001_ha)
>   	pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity	= 0x01;
>   	pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt		= 0x01;
>   
> +	/* Enable higher IQs and OQs, 32 to 63, bit 16*/

Nit: space missing before "*/"

> +	if (pm8001_ha->max_q_num > 32)
> +		pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
> +							1 << 16;
>   	/* Disable end to end CRC checking */
>   	pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 << 16);
>   
> @@ -1027,6 +1031,8 @@ static int mpi_init_check(struct pm8001_hba_info *pm8001_ha)
>   	if (0x0000 != gst_len_mpistate)
>   		return -EBUSY;
>   
> +	msleep(500);
> +
>   	return 0;
>   }
>   

Otherwise, looks OK.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Note: My test box with the pm80xx HBA has 16 cores/32 threads only so I 
cannot really test this.

-- 
Damien Le Moal
Western Digital Research

RE: [PATCH v2 2/2] scsi: pm80xx: enable upper inbound, outbound queues

[Ajish.Koshy]

Thanks Damien for your review, will make the changes in v3.

> On 4/5/22 18:28, Ajish Koshy wrote:
> > Executing driver on servers with more than 32 CPUs were faced with
> > command timeouts. This is because we were not geting completions for
> > commands submitted on IQ32 - IQ63.
> >
> > Set E64Q bit to enable upper inbound and outbound queues 32 to 63 in
> > the MPI main configuration table.
> >
> > Added 500ms delay after successful MPI initialization as mentioned in
> > controller datasheet.
> >
> > 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 | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c
> > b/drivers/scsi/pm8001/pm80xx_hwi.c
> > index 3e6413e21bfe..c41c24a4b906 100644
> > --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> > @@ -766,6 +766,10 @@ static void init_default_table_values(struct
> pm8001_hba_info *pm8001_ha)
> >       pm8001_ha->main_cfg_tbl.pm80xx_tbl.pcs_event_log_severity       =
> 0x01;
> >       pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt          = 0x01;
> >
> > +     /* Enable higher IQs and OQs, 32 to 63, bit 16*/
> 
> Nit: space missing before "*/"

OK. Will take care in v3

> 
> > +     if (pm8001_ha->max_q_num > 32)
> > +             pm8001_ha->main_cfg_tbl.pm80xx_tbl.fatal_err_interrupt |=
> > +                                                     1 << 16;
> >       /* Disable end to end CRC checking */
> >       pm8001_ha->main_cfg_tbl.pm80xx_tbl.crc_core_dump = (0x1 << 16);
> >
> > @@ -1027,6 +1031,8 @@ static int mpi_init_check(struct
> pm8001_hba_info *pm8001_ha)
> >       if (0x0000 != gst_len_mpistate)
> >               return -EBUSY;
> >
> > +     msleep(500);
> > +
> >       return 0;
> >   }
> >
> 
> Otherwise, looks OK.
> 
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
> Note: My test box with the pm80xx HBA has 16 cores/32 threads only so I
> cannot really test this.
> 
> --
> Damien Le Moal
> Western Digital Research

RE: [PATCH v2 1/2] scsi: pm80xx: mask and unmask upper interrupt vectors 32-63

[Ajish.Koshy]

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