mbox series

[v3,0/8] hisi_sas: support x6000 board and some misc changes

Message ID 1520261330-204596-1-git-send-email-john.garry@huawei.com
Headers show
Series hisi_sas: support x6000 board and some misc changes | expand

Message

John Garry March 5, 2018, 2:48 p.m. UTC 
This patchset primarily adds support for the Huawei x6000 board,
which includes hip07 chipset. Unfortunately, due to some board
layout differences with our development board, we need to set
a PHY-related register differently for optimal signal quality. As
such, a signal attenuation property is added to describe the
differences in the boards and allow the PHY register to be set
appropriately.

In addition to this above feature, some misc changes are added for:
- PHY linkrate sysfs interface
- linkrate set function
- internal abort timer timeout increase

Differences to v2:
- rename dt binding property name to "hisilicon,signal-attenuation"

Differences to v1:
- rename dt binding property name to include "hisi-" prefix

Xiang Chen (2):
  scsi: hisi_sas: remove unused variable hisi_sas_devices.running_req
  scsi: hisi_sas: Code cleanup and minor bug fixes

Xiaofei Tan (6):
  dt-bindings: scsi: hisi_sas: add an property of signal attenuation
  scsi: hisi_sas: support the property of signal attenuation for v2 hw
  scsi: hisi_sas: fix the issue of link rate inconsistency
  scsi: hisi_sas: fix the issue of setting linkrate register
  scsi: hisi_sas: increase timer expire of internal abort task
  scsi: hisi_sas: fix return value of hisi_sas_task_prep()

 .../devicetree/bindings/scsi/hisilicon-sas.txt     |  7 +++
 drivers/scsi/hisi_sas/hisi_sas.h                   |  1 -
 drivers/scsi/hisi_sas/hisi_sas_main.c              | 34 +++++-------
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c             | 13 +++--
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c             | 62 +++++++++++++++++-----
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c             | 34 +++++-------
 6 files changed, 88 insertions(+), 63 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Hannes Reinecke March 6, 2018, 11:34 a.m. UTC | #1 
On 03/05/2018 03:48 PM, John Garry wrote:
> From: Xiang Chen <chenxiang66@hisilicon.com>

> 

> The patch does some code cleanup and fixes some small bugs:

> - Correct return status of phy_up_v3_hw()

> - Add static for function phy_get_max_linkrate_v3_hw()

> - Change exception return status when no reset method

> - Change magic value to ts->stat in slot_complete_vx_hw()

> - Remove unnecessary check for dev_is_sata()

> - Fix some issues of alignment and indents (Authored by

>   Xiaofei Tan in another patch, but added here to be

>   practical)

> 

> Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com>

> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>

> Signed-off-by: John Garry <john.garry@huawei.com>

> ---

>  drivers/scsi/hisi_sas/hisi_sas_main.c  | 14 +++++++-------

>  drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  4 +++-

>  drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 10 ++++++----

>  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 16 +++++++++-------

>  4 files changed, 25 insertions(+), 19 deletions(-)

> 

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c

> index dff9723..49c1fa6 100644

> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c

> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c

> @@ -33,7 +33,7 @@ u8 hisi_sas_get_ata_protocol(struct host_to_dev_fis *fis, int direction)

>  	case ATA_CMD_FPDMA_RECV:

>  	case ATA_CMD_FPDMA_SEND:

>  	case ATA_CMD_NCQ_NON_DATA:

> -	return HISI_SAS_SATA_PROTOCOL_FPDMA;

> +		return HISI_SAS_SATA_PROTOCOL_FPDMA;

>  

>  	case ATA_CMD_DOWNLOAD_MICRO:

>  	case ATA_CMD_ID_ATA:

> @@ -45,7 +45,7 @@ u8 hisi_sas_get_ata_protocol(struct host_to_dev_fis *fis, int direction)

>  	case ATA_CMD_WRITE_LOG_EXT:

>  	case ATA_CMD_PIO_WRITE:

>  	case ATA_CMD_PIO_WRITE_EXT:

> -	return HISI_SAS_SATA_PROTOCOL_PIO;

> +		return HISI_SAS_SATA_PROTOCOL_PIO;

>  

>  	case ATA_CMD_DSM:

>  	case ATA_CMD_DOWNLOAD_MICRO_DMA:

> @@ -64,7 +64,7 @@ u8 hisi_sas_get_ata_protocol(struct host_to_dev_fis *fis, int direction)

>  	case ATA_CMD_WRITE_LOG_DMA_EXT:

>  	case ATA_CMD_WRITE_STREAM_DMA_EXT:

>  	case ATA_CMD_ZAC_MGMT_IN:

> -	return HISI_SAS_SATA_PROTOCOL_DMA;

> +		return HISI_SAS_SATA_PROTOCOL_DMA;

>  

>  	case ATA_CMD_CHK_POWER:

>  	case ATA_CMD_DEV_RESET:

> @@ -77,21 +77,21 @@ u8 hisi_sas_get_ata_protocol(struct host_to_dev_fis *fis, int direction)

>  	case ATA_CMD_STANDBY:

>  	case ATA_CMD_STANDBYNOW1:

>  	case ATA_CMD_ZAC_MGMT_OUT:

> -	return HISI_SAS_SATA_PROTOCOL_NONDATA;

> +		return HISI_SAS_SATA_PROTOCOL_NONDATA;

>  	default:

>  	{

>  		if (fis->command == ATA_CMD_SET_MAX) {

>  			switch (fis->features) {

>  			case ATA_SET_MAX_PASSWD:

>  			case ATA_SET_MAX_LOCK:

> -			return HISI_SAS_SATA_PROTOCOL_PIO;

> +				return HISI_SAS_SATA_PROTOCOL_PIO;

>  

>  			case ATA_SET_MAX_PASSWD_DMA:

>  			case ATA_SET_MAX_UNLOCK_DMA:

> -			return HISI_SAS_SATA_PROTOCOL_DMA;

> +				return HISI_SAS_SATA_PROTOCOL_DMA;

>  

>  			default:

> -			return HISI_SAS_SATA_PROTOCOL_NONDATA;

> +				return HISI_SAS_SATA_PROTOCOL_NONDATA;

>  			}

>  		}

>  		if (direction == DMA_NONE)

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c

> index 8dd0e6a6..520ba69 100644

> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c

> +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c

> @@ -651,8 +651,10 @@ static int reset_hw_v1_hw(struct hisi_hba *hisi_hba)

>  			dev_err(dev, "De-reset failed\n");

>  			return -EIO;

>  		}

> -	} else

> +	} else {

>  		dev_warn(dev, "no reset method\n");

> +		return -EIO;

> +	}

>  

return -EINVAL?

>  	return 0;

>  }

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c

> index bd1a48a..69c4dd1 100644

> --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c

> +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c

> @@ -1095,8 +1095,10 @@ static int reset_hw_v2_hw(struct hisi_hba *hisi_hba)

>  			dev_err(dev, "SAS de-reset fail.\n");

>  			return -EIO;

>  		}

> -	} else

> -		dev_warn(dev, "no reset method\n");

> +	} else {

> +		dev_err(dev, "no reset method\n");

> +		return -EIO;

> +	}

>  

>  	return 0;

>  }

return -EINVAL?

> @@ -2408,7 +2410,7 @@ static void slot_err_v2_hw(struct hisi_hba *hisi_hba,

>  		spin_lock_irqsave(&hisi_hba->lock, flags);

>  		hisi_sas_slot_task_free(hisi_hba, task, slot);

>  		spin_unlock_irqrestore(&hisi_hba->lock, flags);

> -		return -1;

> +		return ts->stat;

>  	}

>  

>  	if (unlikely(!sas_dev)) {> @@ -2667,7 +2669,7 @@ static int prep_abort_v2_hw(struct hisi_hba

*hisi_hba,
>  	/* dw0 */

>  	hdr->dw0 = cpu_to_le32((5 << CMD_HDR_CMD_OFF) | /*abort*/

>  			       (port->id << CMD_HDR_PORT_OFF) |

> -			       ((dev_is_sata(dev) ? 1:0) <<

> +			       (dev_is_sata(dev) <<

>  				CMD_HDR_ABORT_DEVICE_TYPE_OFF) |

>  			       (abort_flag << CMD_HDR_ABORT_FLAG_OFF));

>  

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c

> index 8da9de7..b9b5d9f 100644

> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c

> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c

> @@ -670,8 +670,10 @@ static int reset_hw_v3_hw(struct hisi_hba *hisi_hba)

>  			dev_err(dev, "Reset failed\n");

>  			return -EIO;

>  		}

> -	} else

> +	} else {

>  		dev_err(dev, "no reset method!\n");

> +		return -EIO;

> +	}

>  

>  	return 0;

>  }

return -EINVAL?

> @@ -731,7 +733,7 @@ static void phy_hard_reset_v3_hw(struct hisi_hba *hisi_hba, int phy_no)

>  	start_phy_v3_hw(hisi_hba, phy_no);

>  }

>  

> -enum sas_linkrate phy_get_max_linkrate_v3_hw(void)

> +static enum sas_linkrate phy_get_max_linkrate_v3_hw(void)

>  {

>  	return SAS_LINK_RATE_12_0_GBPS;

>  }

> @@ -1096,7 +1098,7 @@ static int prep_abort_v3_hw(struct hisi_hba *hisi_hba,

>  	/* dw0 */

>  	hdr->dw0 = cpu_to_le32((5 << CMD_HDR_CMD_OFF) | /*abort*/

>  			       (port->id << CMD_HDR_PORT_OFF) |

> -				   ((dev_is_sata(dev) ? 1:0)

> +				   (dev_is_sata(dev)

>  					<< CMD_HDR_ABORT_DEVICE_TYPE_OFF) |

>  					(abort_flag

>  					 << CMD_HDR_ABORT_FLAG_OFF));

> @@ -1114,7 +1116,7 @@ static int prep_abort_v3_hw(struct hisi_hba *hisi_hba,

>  

>  static int phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)

>  {

> -	int i, res = 0;

> +	int i, res;

>  	u32 context, port_id, link_rate;

>  	struct hisi_sas_phy *phy = &hisi_hba->phy[phy_no];

>  	struct asd_sas_phy *sas_phy = &phy->sas_phy;

> @@ -1186,7 +1188,7 @@ static int phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)

>  	phy->port_id = port_id;

>  	phy->phy_attached = 1;

>  	hisi_sas_notify_phy_event(phy, HISI_PHYE_PHY_UP);

> -

> +	res = IRQ_HANDLED;

>  end:

>  	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT0,

>  			     CHL_INT0_SL_PHY_ENABLE_MSK);

> @@ -1217,7 +1219,7 @@ static int phy_down_v3_hw(int phy_no, struct hisi_hba *hisi_hba)

>  	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT0, CHL_INT0_NOT_RDY_MSK);

>  	hisi_sas_phy_write32(hisi_hba, phy_no, PHYCTRL_NOT_RDY_MSK, 0);

>  

> -	return 0;

> +	return IRQ_HANDLED;

>  }

>  

>  static void phy_bcast_v3_hw(int phy_no, struct hisi_hba *hisi_hba)


If we're returning IRQ_HANDLED, shouldn't the function have the return
type irqreturn_t ?
But as this isn't an interrupt handler, shouldn't we rather fixup the
caller to check for the correct return values?

> @@ -1573,7 +1575,7 @@ static irqreturn_t fatal_axi_int_v3_hw(int irq_no, void *p)

>  		spin_lock_irqsave(&hisi_hba->lock, flags);

>  		hisi_sas_slot_task_free(hisi_hba, task, slot);

>  		spin_unlock_irqrestore(&hisi_hba->lock, flags);

> -		return -1;

> +		return ts->stat;

>  	}

>  

>  	if (unlikely(!sas_dev)) {

> 


Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Garry March 6, 2018, 12:26 p.m. UTC | #2 
Hi Hannes,

Thanks for checking this.

>> > +	}

>> >

> return -EINVAL?

>

>> >  	return 0;

>> >  }


[ ... ]

>> > +	}

>> >

>> >  	return 0;

>> >  }

> return -EINVAL?

>

>> > @@ -2408,7 +2410,7 @@ static void slot_err_v2_hw(struct hisi_hba *hisi_hba,

>> >  		spin_lock_irqsave(&hisi_hba->lock, flags);

>> >  		hisi_sas_slot_task_free(hisi_hba, task, slot);

>> >  		spin_unlock_irqrestore(&hisi_hba->lock, flags);


[ ... ]

>> >  		}

>> > -	} else

>> > +	} else {

>> >  		dev_err(dev, "no reset method!\n");

>> > +		return -EIO;

>> > +	}

>> >

>> >  	return 0;

>> >  }

> return -EINVAL?

>


These 3 changes you suggest are accepted.

>> > @@ -731,7 +733,7 @@ static void phy_hard_reset_v3_hw(struct hisi_hba *hisi_hba, int phy_no)

>> >  	start_phy_v3_hw(hisi_hba, phy_no);

>> >  }

>> >

>> > -enum sas_linkrate phy_get_max_linkrate_v3_hw(void)

>> > +static enum sas_linkrate phy_get_max_linkrate_v3_hw(void)

>> >  {

>> >  	return SAS_LINK_RATE_12_0_GBPS;

>> >  }

>> > @@ -1096,7 +1098,7 @@ static int prep_abort_v3_hw(struct hisi_hba *hisi_hba,

>> >  	/* dw0 */

>> >  	hdr->dw0 = cpu_to_le32((5 << CMD_HDR_CMD_OFF) | /*abort*/

>> >  			       (port->id << CMD_HDR_PORT_OFF) |

>> > -				   ((dev_is_sata(dev) ? 1:0)

>> > +				   (dev_is_sata(dev)

>> >  					<< CMD_HDR_ABORT_DEVICE_TYPE_OFF) |

>> >  					(abort_flag

>> >  					 << CMD_HDR_ABORT_FLAG_OFF));

>> > @@ -1114,7 +1116,7 @@ static int prep_abort_v3_hw(struct hisi_hba *hisi_hba,

>> >

>> >  static int phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)

>> >  {

>> > -	int i, res = 0;

>> > +	int i, res;

>> >  	u32 context, port_id, link_rate;

>> >  	struct hisi_sas_phy *phy = &hisi_hba->phy[phy_no];

>> >  	struct asd_sas_phy *sas_phy = &phy->sas_phy;

>> > @@ -1186,7 +1188,7 @@ static int phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)

>> >  	phy->port_id = port_id;

>> >  	phy->phy_attached = 1;

>> >  	hisi_sas_notify_phy_event(phy, HISI_PHYE_PHY_UP);

>> > -

>> > +	res = IRQ_HANDLED;

>> >  end:

>> >  	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT0,

>> >  			     CHL_INT0_SL_PHY_ENABLE_MSK);

>> > @@ -1217,7 +1219,7 @@ static int phy_down_v3_hw(int phy_no, struct hisi_hba *hisi_hba)

>> >  	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT0, CHL_INT0_NOT_RDY_MSK);

>> >  	hisi_sas_phy_write32(hisi_hba, phy_no, PHYCTRL_NOT_RDY_MSK, 0);

>> >

>> > -	return 0;

>> > +	return IRQ_HANDLED;

>> >  }

>> >

>> >  static void phy_bcast_v3_hw(int phy_no, struct hisi_hba *hisi_hba)

> If we're returning IRQ_HANDLED, shouldn't the function have the return

> type irqreturn_t ?

> But as this isn't an interrupt handler, shouldn't we rather fixup the

> caller to check for the correct return values?


Since function phy_bcast_v3_hw() does no checking and would always 
return IRQ_HANDLED, we saw not point in having a return code and 
checking it. However, I did notice that we don't set res = IRQ_HANDLED 
after calling this function:
                 if (irq_value & CHL_INT0_SL_RX_BCST_ACK_MSK)
                     /* phy bcast */
                     phy_bcast_v3_hw(phy_no, hisi_hba);
             } else {
                 if (irq_value & CHL_INT0_NOT_RDY_MSK)
                     /* phy down */
                     if (phy_down_v3_hw(phy_no, hisi_hba)
                             == IRQ_HANDLED)
                         res = IRQ_HANDLED;
             }
         }
         irq_msk >>= 4;
         phy_no++;
     }

     return res;
}

So this needs to be remedied.

>

>> > @@ -1573,7 +1575,7 @@ static irqreturn_t fatal_axi_int_v3_hw(int irq_no, void *p)

>> >  		spin_lock_irqsave(&hisi_hba->lock, flags);

>> >  		hisi_sas_slot_task_free(hisi_hba, task, slot);

>> >  		spin_unlock_irqrestore(&hisi_hba->lock, flags);

>> > -		return -1;

>> > +		return ts->stat;

>> >  	}

>> >

>> >  	if (unlikely(!sas_dev)) {

>> >


Thanks,
John

> Cheers,

>

> Hannes

> -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@suse.de	+49

> 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F.

> Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG

> Nürnberg) .



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Garry March 6, 2018, 12:30 p.m. UTC | #3 
On 06/03/2018 11:23, Hannes Reinecke wrote:
> On 03/05/2018 03:48 PM, John Garry wrote:

>> From: Xiaofei Tan <tanxiaofei@huawei.com>

>>

>> The current 110ms expiry time is not long enough for the internal

>> abort task.

>>

>> The reason is that the internal abort task could be blocked in HW

>> if the HW is retrying to set up link. The internal abort task will

>> be executed only when the retry process finished.

>>

> Hmm. That sounds weird.

> I would have expected that a link retrain will force a device reset,

> after which no tasks should be active on the target.

> Consequently the succeeding abort task will be a no-op.

> Care to clarify?

>


Hi Hannes,

It sounds like you're talking about TMF task abort, right? This patch is 
related to controller internal abort function.

Our HW supports internal abort, where any pending queued commands in the 
controller can be aborted, so they will not be executed. When a disk is 
removed or a nexus reset are times when we issue such a command.

Thanks very much,
John

> Cheers,

>

> Hannes

>



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html