Re: [PATCH] scsi:stex.c Support Pegasus 3 product

2016-06-13 Thread Julian Calaby 
Hi Charles,

On Mon, Jun 13, 2016 at 9:40 PM, Charles Chiou  wrote:
> Hi Julian,
>
>
> On 06/10/2016 08:10 AM, Julian Calaby wrote:
>>
>> Hi Charles,
>>
>> On Mon, Jun 6, 2016 at 5:53 PM, Charles Chiou 
>> wrote:
>>>
>>> From: Charles 
>>>
>>> Pegasus series is a RAID support product by using Thunderbolt technology.
>>>
>>> The newest product, Pegasus 3 is support Thunderbolt 3 technology with
>>> another chip.
>>>
>>> 1.Change driver version.
>>>
>>> 2.Add Pegasus 3 VID, DID and define it's device address.
>>>
>>> 3.Pegasus 3 use msi interrupt, so stex_request_irq P3 type enable msi.
>>>
>>> 4.For hibernation, use msi_lock in stex_ss_handshake to prevent msi
>>> register write again when handshaking.
>>>
>>> 5.Pegasus 3 don't need read() as flush.
>>>
>>> 6.In stex_ss_intr & stex_abort, P3 only clear interrupt register when
>>> getting vendor defined interrupt.
>>>
>>> 7.Add reboot notifier and register it in stex_probe for all supported
>>> device.
>>>
>>> 8.For all supported device in restart flow, we get a callback from
>>> notifier and set S6flag for stex_shutdown & stex_hba_stop to send restart
>>> command to FW.
>>>
>>> Signed-off-by: Charles 
>>> Signed-off-by: Paul 
>>> ---
>>>   drivers/scsi/stex.c | 282
>>> +++-
>>>   1 file changed, 214 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
>>> index 5b23175..9de2de2 100644
>>> --- a/drivers/scsi/stex.c
>>> +++ b/drivers/scsi/stex.c
>>> @@ -87,7 +95,7 @@ enum {
>>>  MU_STATE_STOP   = 5,
>>>  MU_STATE_NOCONNECT  = 6,
>>>
>>> -   MU_MAX_DELAY= 120,
>>> +   MU_MAX_DELAY= 50,
>>
>>
>> This won't cause problems for older adapters, right?
>
>
> Correct.
>
>>
>>>  MU_HANDSHAKE_SIGNATURE  = 0x5555,
>>>  MU_HANDSHAKE_SIGNATURE_HALF = 0x5a5a,
>>>  MU_HARD_RESET_WAIT  = 3,
>>> @@ -540,11 +556,15 @@ stex_ss_send_cmd(struct st_hba *hba, struct req_msg
>>> *req, u16 tag)
>>>
>>>  ++hba->req_head;
>>>  hba->req_head %= hba->rq_count+1;
>>> -
>>> -   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
>>> -   readl(hba->mmio_base + YH2I_REQ_HI); /* flush */
>>> -   writel(addr, hba->mmio_base + YH2I_REQ);
>>> -   readl(hba->mmio_base + YH2I_REQ); /* flush */
>>> +   if (hba->cardtype == st_P3) {
>>> +   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
>>> +   writel(addr, hba->mmio_base + YH2I_REQ);
>>> +   } else {
>>> +   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
>>> +   readl(hba->mmio_base + YH2I_REQ_HI); /* flush */
>>> +   writel(addr, hba->mmio_base + YH2I_REQ);
>>> +   readl(hba->mmio_base + YH2I_REQ); /* flush */
>>> +   }
>>
>>
>> The first writel() lines in each branch of the if statement are
>> identical, so they could be outside of it.
>
>
> I'll revise it in next patch.

On second thought, don't worry about doing this, keep the two
(slightly) different sets of code separate.

>>
>> Would it make sense to add a helper that does the readl() flush only
>> for non-st_P3? This could be a function pointer in the hba structure
>> which shouldn't slow stuff down.
>>
>
> Would you means to register another function pointer in struct "struct
> st_card_info" then point to hba strucrure for non-st_P3?
>
> struct st_card_info {
> struct req_msg * (*alloc_rq) (struct st_hba *);
> int (*map_sg)(struct st_hba *, struct req_msg *, struct st_ccb *);
> void (*send) (struct st_hba *, struct req_msg *, u16);
> unsigned int max_id;
> unsigned int max_lun;
> unsigned int max_channel;
> u16 rq_count;
> u16 rq_size;
> u16 sts_count;
> };

Again, on second thought, don't worry about it.

>>>   }
>>>
>>>   static void return_abnormal_state(struct st_hba *hba, int status)
>>> @@ -,30 +1160,63 @@ static int stex_ss_handshake(struct st_hba *hba)
>>>  scratch_size = (hba->sts_count+1)*sizeof(u32);
>>>  h->scratch_size = cpu_to_le32(scratch_size);
>>>
>>> -   data = readl(base + YINT_EN);
>>> -   data &= ~4;
>>> -   writel(data, base + YINT_EN);
>>> -   writel((hba->dma_handle >> 16) >> 16, base + YH2I_REQ_HI);
>>> -   readl(base + YH2I_REQ_HI);
>>> -   writel(hba->dma_handle, base + YH2I_REQ);
>>> -   readl(base + YH2I_REQ); /* flush */
>>> +   if (hba->cardtype == st_yel) {
>>
>>
>> Same question again.
>
>
> I'll revise it in next patch.
>
>>
>>> +   data = readl(base + YINT_EN);
>>> +   data &= ~4;
>>> +   writel(data, base + YINT_EN);
>>> +

Re: [PATCH] scsi:stex.c Support Pegasus 3 product

2016-06-13 Thread Charles Chiou 

Hi Julian,

On 06/10/2016 08:10 AM, Julian Calaby wrote:

Hi Charles,

On Mon, Jun 6, 2016 at 5:53 PM, Charles Chiou  wrote:

From: Charles 

Pegasus series is a RAID support product by using Thunderbolt technology.

The newest product, Pegasus 3 is support Thunderbolt 3 technology with another 
chip.

1.Change driver version.

2.Add Pegasus 3 VID, DID and define it's device address.

3.Pegasus 3 use msi interrupt, so stex_request_irq P3 type enable msi.

4.For hibernation, use msi_lock in stex_ss_handshake to prevent msi register 
write again when handshaking.

5.Pegasus 3 don't need read() as flush.

6.In stex_ss_intr & stex_abort, P3 only clear interrupt register when getting 
vendor defined interrupt.

7.Add reboot notifier and register it in stex_probe for all supported device.

8.For all supported device in restart flow, we get a callback from notifier and set 
S6flag for stex_shutdown & stex_hba_stop to send restart command to FW.

Signed-off-by: Charles 
Signed-off-by: Paul 
---
  drivers/scsi/stex.c | 282 +++-
  1 file changed, 214 insertions(+), 68 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index 5b23175..9de2de2 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -87,7 +95,7 @@ enum {
 MU_STATE_STOP   = 5,
 MU_STATE_NOCONNECT  = 6,

-   MU_MAX_DELAY= 120,
+   MU_MAX_DELAY= 50,


This won't cause problems for older adapters, right?


Correct.




 MU_HANDSHAKE_SIGNATURE  = 0x5555,
 MU_HANDSHAKE_SIGNATURE_HALF = 0x5a5a,
 MU_HARD_RESET_WAIT  = 3,
@@ -540,11 +556,15 @@ stex_ss_send_cmd(struct st_hba *hba, struct req_msg *req, 
u16 tag)

 ++hba->req_head;
 hba->req_head %= hba->rq_count+1;
-
-   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
-   readl(hba->mmio_base + YH2I_REQ_HI); /* flush */
-   writel(addr, hba->mmio_base + YH2I_REQ);
-   readl(hba->mmio_base + YH2I_REQ); /* flush */
+   if (hba->cardtype == st_P3) {
+   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
+   writel(addr, hba->mmio_base + YH2I_REQ);
+   } else {
+   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
+   readl(hba->mmio_base + YH2I_REQ_HI); /* flush */
+   writel(addr, hba->mmio_base + YH2I_REQ);
+   readl(hba->mmio_base + YH2I_REQ); /* flush */
+   }


The first writel() lines in each branch of the if statement are
identical, so they could be outside of it.


I'll revise it in next patch.



Would it make sense to add a helper that does the readl() flush only
for non-st_P3? This could be a function pointer in the hba structure
which shouldn't slow stuff down.



Would you means to register another function pointer in struct "struct 
st_card_info" then point to hba strucrure for non-st_P3?


struct st_card_info {
struct req_msg * (*alloc_rq) (struct st_hba *);
int (*map_sg)(struct st_hba *, struct req_msg *, struct st_ccb *);
void (*send) (struct st_hba *, struct req_msg *, u16);
unsigned int max_id;
unsigned int max_lun;
unsigned int max_channel;
u16 rq_count;
u16 rq_size;
u16 sts_count;
};


  }

  static void return_abnormal_state(struct st_hba *hba, int status)
@@ -974,15 +994,31 @@ static irqreturn_t stex_ss_intr(int irq, void *__hba)

 spin_lock_irqsave(hba->host->host_lock, flags);

-   data = readl(base + YI2H_INT);
-   if (data && data != 0x) {
-   /* clear the interrupt */
-   writel(data, base + YI2H_INT_C);
-   stex_ss_mu_intr(hba);
-   spin_unlock_irqrestore(hba->host->host_lock, flags);
-   if (unlikely(data & SS_I2H_REQUEST_RESET))
-   queue_work(hba->work_q, >reset_work);
-   return IRQ_HANDLED;
+   if (hba->cardtype == st_yel) {


I note that there's a few different card types beyond sd_yel and
st_P3. Does this function only get called for st_yel and st_P3?



This function only for st_yel & st_P3.


+   data = readl(base + YI2H_INT);
+   if (data && data != 0x) {
+   /* clear the interrupt */
+   writel(data, base + YI2H_INT_C);
+   stex_ss_mu_intr(hba);
+   spin_unlock_irqrestore(hba->host->host_lock, flags);
+   if (unlikely(data & SS_I2H_REQUEST_RESET))
+   queue_work(hba->work_q, >reset_work);
+   return IRQ_HANDLED;
+   }
+   } else {
+   data = readl(base +

Re: [PATCH] scsi:stex.c Support Pegasus 3 product

2016-06-09 Thread Julian Calaby 
Hi Charles,

On Mon, Jun 6, 2016 at 5:53 PM, Charles Chiou  wrote:
> From: Charles 
>
> Pegasus series is a RAID support product by using Thunderbolt technology.
>
> The newest product, Pegasus 3 is support Thunderbolt 3 technology with 
> another chip.
>
> 1.Change driver version.
>
> 2.Add Pegasus 3 VID, DID and define it's device address.
>
> 3.Pegasus 3 use msi interrupt, so stex_request_irq P3 type enable msi.
>
> 4.For hibernation, use msi_lock in stex_ss_handshake to prevent msi register 
> write again when handshaking.
>
> 5.Pegasus 3 don't need read() as flush.
>
> 6.In stex_ss_intr & stex_abort, P3 only clear interrupt register when getting 
> vendor defined interrupt.
>
> 7.Add reboot notifier and register it in stex_probe for all supported device.
>
> 8.For all supported device in restart flow, we get a callback from notifier 
> and set S6flag for stex_shutdown & stex_hba_stop to send restart command to 
> FW.
>
> Signed-off-by: Charles 
> Signed-off-by: Paul 
> ---
>  drivers/scsi/stex.c | 282 
> +++-
>  1 file changed, 214 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> index 5b23175..9de2de2 100644
> --- a/drivers/scsi/stex.c
> +++ b/drivers/scsi/stex.c
> @@ -87,7 +95,7 @@ enum {
> MU_STATE_STOP   = 5,
> MU_STATE_NOCONNECT  = 6,
>
> -   MU_MAX_DELAY= 120,
> +   MU_MAX_DELAY= 50,

This won't cause problems for older adapters, right?

> MU_HANDSHAKE_SIGNATURE  = 0x5555,
> MU_HANDSHAKE_SIGNATURE_HALF = 0x5a5a,
> MU_HARD_RESET_WAIT  = 3,
> @@ -540,11 +556,15 @@ stex_ss_send_cmd(struct st_hba *hba, struct req_msg 
> *req, u16 tag)
>
> ++hba->req_head;
> hba->req_head %= hba->rq_count+1;
> -
> -   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
> -   readl(hba->mmio_base + YH2I_REQ_HI); /* flush */
> -   writel(addr, hba->mmio_base + YH2I_REQ);
> -   readl(hba->mmio_base + YH2I_REQ); /* flush */
> +   if (hba->cardtype == st_P3) {
> +   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
> +   writel(addr, hba->mmio_base + YH2I_REQ);
> +   } else {
> +   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
> +   readl(hba->mmio_base + YH2I_REQ_HI); /* flush */
> +   writel(addr, hba->mmio_base + YH2I_REQ);
> +   readl(hba->mmio_base + YH2I_REQ); /* flush */
> +   }

The first writel() lines in each branch of the if statement are
identical, so they could be outside of it.

Would it make sense to add a helper that does the readl() flush only
for non-st_P3? This could be a function pointer in the hba structure
which shouldn't slow stuff down.

>  }
>
>  static void return_abnormal_state(struct st_hba *hba, int status)
> @@ -974,15 +994,31 @@ static irqreturn_t stex_ss_intr(int irq, void *__hba)
>
> spin_lock_irqsave(hba->host->host_lock, flags);
>
> -   data = readl(base + YI2H_INT);
> -   if (data && data != 0x) {
> -   /* clear the interrupt */
> -   writel(data, base + YI2H_INT_C);
> -   stex_ss_mu_intr(hba);
> -   spin_unlock_irqrestore(hba->host->host_lock, flags);
> -   if (unlikely(data & SS_I2H_REQUEST_RESET))
> -   queue_work(hba->work_q, >reset_work);
> -   return IRQ_HANDLED;
> +   if (hba->cardtype == st_yel) {

I note that there's a few different card types beyond sd_yel and
st_P3. Does this function only get called for st_yel and st_P3?

> +   data = readl(base + YI2H_INT);
> +   if (data && data != 0x) {
> +   /* clear the interrupt */
> +   writel(data, base + YI2H_INT_C);
> +   stex_ss_mu_intr(hba);
> +   spin_unlock_irqrestore(hba->host->host_lock, flags);
> +   if (unlikely(data & SS_I2H_REQUEST_RESET))
> +   queue_work(hba->work_q, >reset_work);
> +   return IRQ_HANDLED;
> +   }
> +   } else {
> +   data = readl(base + PSCRATCH4);
> +   if (data != 0x) {
> +   if (data != 0) {
> +   /* clear the interrupt */
> +   writel(data, base + PSCRATCH1);
> +   writel((1 << 22), base + YH2I_INT);
> +   }
> +   stex_ss_mu_intr(hba);
> +   spin_unlock_irqrestore(hba->host->host_lock, flags);
> +   if (unlikely(data &