On 2023/08/18 6:41, Igor Pylypiv wrote:
> By default PM80xx HBAs return FIS only when a drive reports an error.
s/FIS/SDB FIS or even better "Set Device Bits (SDB) FIS" to be clear.
> The RETFIS bit forces the controller to populate FIS even when a drive
> reports no error.
And here s/FIS/SDB FIS
>
> Signed-off-by: Igor Pylypiv <ipylypiv@xxxxxxxxxx>
> ---
> drivers/scsi/pm8001/pm8001_hwi.c | 8 +++++---
> drivers/scsi/pm8001/pm8001_hwi.h | 2 +-
> drivers/scsi/pm8001/pm80xx_hwi.c | 11 ++++++-----
> drivers/scsi/pm8001/pm80xx_hwi.h | 2 +-
> 4 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
> index 73cd25f30ca5..255553dcadb9 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -4095,7 +4095,7 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> u32 hdr_tag, ncg_tag = 0;
> u64 phys_addr;
> u32 ATAP = 0x0;
> - u32 dir;
> + u32 dir, retfis;
> u32 opc = OPC_INB_SATA_HOST_OPSTART;
>
> memset(&sata_cmd, 0, sizeof(sata_cmd));
> @@ -4124,8 +4124,10 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> sata_cmd.tag = cpu_to_le32(tag);
> sata_cmd.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
> sata_cmd.data_len = cpu_to_le32(task->total_xfer_len);
> - sata_cmd.ncqtag_atap_dir_m =
> - cpu_to_le32(((ncg_tag & 0xff)<<16)|((ATAP & 0x3f) << 10) | dir);
> + retfis = task->ata_task.return_fis_on_success;
While I think this should be OK, I think it would be safer to do:
u32 dir, retfis = 0;
...
if (task->ata_task.return_fis_on_success)
retfis = 1;
to avoid issues with funky compilers doing some tricky handling of single bit
fields.
> + sata_cmd.retfis_ncqtag_atap_dir_m =
> + cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> + ((ATAP & 0x3f) << 10) | dir);
Please align this line with "(retfis << 24)" above.
> sata_cmd.sata_fis = task->ata_task.fis;
> if (likely(!task->ata_task.device_control_reg_update))
> sata_cmd.sata_fis.flags |= 0x80;/* C=1: update ATA cmd reg */
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h
> index 961d0465b923..fc2127dcb58d 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.h
> +++ b/drivers/scsi/pm8001/pm8001_hwi.h
> @@ -515,7 +515,7 @@ struct sata_start_req {
> __le32 tag;
> __le32 device_id;
> __le32 data_len;
> - __le32 ncqtag_atap_dir_m;
> + __le32 retfis_ncqtag_atap_dir_m;
Naming this field from what is set in it is unusual... Not sure how the
controller spce named this field, but we should use that and stop changing it's
name whenever we change the bits that are set.
> struct host_to_dev_fis sata_fis;
> u32 reserved1;
> u32 reserved2;
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 39a12ee94a72..e839fb53f0e3 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -4457,7 +4457,7 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> u64 phys_addr, end_addr;
> u32 end_addr_high, end_addr_low;
> u32 ATAP = 0x0;
> - u32 dir;
> + u32 dir, retfis;
> u32 opc = OPC_INB_SATA_HOST_OPSTART;
> memset(&sata_cmd, 0, sizeof(sata_cmd));
>
> @@ -4487,6 +4487,7 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> sata_cmd.tag = cpu_to_le32(tag);
> sata_cmd.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
> sata_cmd.data_len = cpu_to_le32(task->total_xfer_len);
> + retfis = task->ata_task.return_fis_on_success;
>
> sata_cmd.sata_fis = task->ata_task.fis;
> if (likely(!task->ata_task.device_control_reg_update))
> @@ -4502,8 +4503,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> opc = OPC_INB_SATA_DIF_ENC_IO;
>
> /* set encryption bit */
> - sata_cmd.ncqtag_atap_dir_m_dad =
> - cpu_to_le32(((ncg_tag & 0xff)<<16)|
> + sata_cmd.retfis_ncqtag_atap_dir_m_dad =
> + cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> ((ATAP & 0x3f) << 10) | 0x20 | dir);
Same comments here.
> /* dad (bit 0-1) is 0 */
> /* fill in PRD (scatter/gather) table, if any */
> @@ -4569,8 +4570,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> "Sending Normal SATA command 0x%x inb %x\n",
> sata_cmd.sata_fis.command, q_index);
> /* dad (bit 0-1) is 0 */
> - sata_cmd.ncqtag_atap_dir_m_dad =
> - cpu_to_le32(((ncg_tag & 0xff)<<16) |
> + sata_cmd.retfis_ncqtag_atap_dir_m_dad =
> + cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> ((ATAP & 0x3f) << 10) | dir);
>
> /* fill in PRD (scatter/gather) table, if any */
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.h b/drivers/scsi/pm8001/pm80xx_hwi.h
> index acf6e3005b84..eb8fd37b2066 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.h
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.h
> @@ -731,7 +731,7 @@ struct sata_start_req {
> __le32 tag;
> __le32 device_id;
> __le32 data_len;
> - __le32 ncqtag_atap_dir_m_dad;
> + __le32 retfis_ncqtag_atap_dir_m_dad;
> struct host_to_dev_fis sata_fis;
> u32 reserved1;
> u32 reserved2; /* dword 11. rsvd for normal I/O. */
--
Damien Le Moal
Western Digital Research
