Re: [PATCH] esp_scsi: Add support for FSC chip

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Finn,

On 30/10/19 1:31 PM, Finn Thain wrote:
> Nice fix!
>
> As Michael mentioned already, you'll need to send this to all the relevant
> recipients:
>
> $ scripts/get_maintainer.pl add-support-for-fsc-chip.patch
> "James E.J. Bottomley" <jejb@linux.ibm.com> (maintainer:SCSI SUBSYSTEM)
> "Martin K. Petersen" <martin.petersen@oracle.com> (maintainer:SCSI SUBSYSTEM)
> linux-scsi@vger.kernel.org (open list:SCSI SUBSYSTEM)
> linux-kernel@vger.kernel.org (open list)
>
> A few more suggestions follow.
>
> On Tue, 29 Oct 2019, Kars de Jong wrote:
>
>> The FSC (NCR53CF9x-2 / SYM53CF9x-2) has a different family code than QLogic
>> or Emulex parts. This caused it to be detected as a FAS100A.
>>
>> Unforunately, this meant the configuration of the CONFIG3 register was
>> incorrect. This causes data transfer issues.
>>
>> The FSC also has a feature called Active Negation which should always be
>> enabled according to the data manual. This is done using the CONFIG4
>> register.
>>
>> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
>> ---
>>   drivers/scsi/esp_scsi.c | 10 ++++++++--
>>   drivers/scsi/esp_scsi.h | 10 ++++++----
>>   2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
>> index bb88995a12c7..6b34a5764de5 100644
>> --- a/drivers/scsi/esp_scsi.c
>> +++ b/drivers/scsi/esp_scsi.c
>> @@ -263,7 +263,11 @@ static void esp_reset_esp(struct esp *esp)
>>   			esp->rev = FAS236;
>>   		else if (family_code == 0x0a)
>>   			esp->rev = FASHME; /* Version is usually '5'. */
>> -		else
>> +		else if (family_code == 0x14) {
>> +			esp->rev = FSC;
>> +			/* Enable Active Negation */
>> +			esp_write8(ESP_CONFIG4_RADE, ESP_CFG4);
> There is a comment in esp_scsi.h which seems to contradict the above
> logic, "ESP config register 4 read-write, found only on am53c974 chips."
> I think the comment should be corrected now.
>
>> +		} else
>>   			esp->rev = FAS100A;
>>   		esp->min_period = ((4 * esp->ccycle) / 1000);
>>   	} else {
>> @@ -308,7 +312,8 @@ static void esp_reset_esp(struct esp *esp)
>>   
>>   	case FAS236:
>>   	case PCSCSI:
>> -		/* Fast 236, AM53c974 or HME */
>> +	case FSC:
>> +		/* Fast 236, AM53c974, FSC or HME */
>>   		esp_write8(esp->config2, ESP_CFG2);
>>   		if (esp->rev == FASHME) {
>>   			u8 cfg3 = esp->target[0].esp_config3;
>> @@ -2373,6 +2378,7 @@ static const char *esp_chip_names[] = {
>>   	"ESP100A",
>>   	"ESP236",
>>   	"FAS236",
>> +	"FSC",
>>   	"FAS100A",
>>   	"FAST",
>>   	"FASHME",
>> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
>> index 91b32f2a1a1b..b60ea3e5e0eb 100644
>> --- a/drivers/scsi/esp_scsi.h
>> +++ b/drivers/scsi/esp_scsi.h
>> @@ -211,6 +211,7 @@
>>   /* ESP unique ID register read-only, found on fas236+fas100a only */
>>   #define ESP_UID_F100A         0x00     /* ESP FAS100A  */
>>   #define ESP_UID_F236          0x02     /* ESP FAS236   */
>> +#define ESP_UID_FSC           0x14     /* NCR/Symbios Logic FSC */
>>   #define ESP_UID_REV           0x07     /* ESP revision */
>>   #define ESP_UID_FAM           0xf8     /* ESP family   */
>>   
> I think these should be in numerical order.
The latter two are bitmasks, so the order makes sense to me.
>
>> @@ -262,10 +263,11 @@ enum esp_rev {
>>   	ESP100A    = 0x01,  /* NCR53C90A */
>>   	ESP236     = 0x02,
>>   	FAS236     = 0x03,
>> -	FAS100A    = 0x04,
>> -	FAST       = 0x05,
>> -	FASHME     = 0x06,
>> -	PCSCSI     = 0x07,  /* AM53c974 */
>> +	FSC        = 0x04,  /* NCR/Symbios Logic FSC */
>> +	FAS100A    = 0x05,
>> +	FAST       = 0x06,
>> +	FASHME     = 0x07,
>> +	PCSCSI     = 0x08,  /* AM53c974 */
> I'm guessing that you've placed it here because of the esp->rev >= FAS236
> tests that appear in a couple of places relating to sync transfer period.
> Might be worth mentioning that in the commit log.

Good catch. I think these tests would need to be changed to esp->rev 
 >=FSC to reflect the fact that the FSC has the same CONFIG3 settings as 
the FAS236.

Cheers,

     Michael

>
> (No idea why this list uses hexadecimal, it's not a value from a chip
> register. It would be more readable in decimal, IMHO.)
>