> -----Original Message----- > From: John Garry [mailto:john.garry@xxxxxxxxxx] > Sent: Wednesday, August 5, 2020 9:56 PM > To: Deepak Ukey - I31172 <Deepak.Ukey@xxxxxxxxxxxxx>; > hch@xxxxxxxxxxxxx > Cc: linux-scsi@xxxxxxxxxxxxxxx; Vasanthalakshmi Tharmarajan - I30664 > <Vasanthalakshmi.Tharmarajan@xxxxxxxxxxxxx>; Viswas G - I30667 > <Viswas.G@xxxxxxxxxxxxx>; jinpu.wang@xxxxxxxxxxxxxxxx; > martin.petersen@xxxxxxxxxx; yuuzheng@xxxxxxxxxx; > auradkar@xxxxxxxxxx; vishakhavc@xxxxxxxxxx; bjashnani@xxxxxxxxxx; > radha@xxxxxxxxxx; akshatzen@xxxxxxxxxx; chenxiang66@xxxxxxxxxxxxx; > yanaijie@xxxxxxxxxx; luojiaxing@xxxxxxxxxx > Subject: Re: [PATCH V6 2/2] pm80xx : Staggered spin up support. > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On 04/08/2020 10:51, Deepak.Ukey@xxxxxxxxxxxxx wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know > > the content is safe > > > > On 04/08/2020 07:33,Deepak.Ukey@xxxxxxxxxxxxx wrote: > >> Hi Christoph, > >> > >> Yes, It is better to be implemented in libsas. Since the out of box pm80xx > driver has this support, we would like to push this for the time being. We will > see how this can be moved to libsas. > >> > > Other libsas users may like this feature. And libsas does already support SATA > spin-up hold events - as does pm8001 - but there's not really much to that in > libsas. > > > > Question: why have a module param to enable this feature? Why not solely > rely on the seeprom spin-up interval, whereby a value of 0 means no staggered > spin-up? > > - Setting spin-up interval may increase the time for device discovery. > > Customer who has a valid spin up interval > > Please use standard mailing list practices in replying. > > - configured can still turn of this using module parameter. Or else, they have > to reflash the seeprom. > > So another knob to control the driver. Anyway I think that it would be good for > libsas to support this feature to benefit all users. > > All the tunables from the LLDD can be fed to libsas, and libsas just needs to do > what you do in pm8001_spinup_timedout() to callback to the LLDD to enable > the SPINUP. > > BTW, out of interest, what is this change for: > > > void pm8001_scan_start(struct Scsi_Host *shost) > > { > > int i; > > + unsigned long lock_flags; > > struct pm8001_hba_info *pm8001_ha; > > struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost); > > + DECLARE_COMPLETION(comp); > > note: this should be the _ONSTACK() variant, and is it safe to even reuse in the > loop, below? > Thank you John for pointing this out. We will change this to ‘DECLARE_COMPLETION_ONSTACK’ and submit the updated patch. “ > phy start” request is sequential and driver submit the new phy_start request after completing the current request. So I think it is safe to use that in a loop. > Please share you thought. > > > pm8001_ha = sha->lldd_ha; > > /* SAS_RE_INITIALIZATION not available in SPCv/ve */ > > if (pm8001_ha->chip_id == chip_8001) > > PM8001_CHIP_DISP->sas_re_init_req(pm8001_ha); > > - for (i = 0; i < pm8001_ha->chip->n_phy; ++i) > > - PM8001_CHIP_DISP->phy_start_req(pm8001_ha, i); > > + > > + if (pm8001_ha->pdev->device == 0x8001 || > > + pm8001_ha->pdev->device == 0x8081 || > > + (pm8001_ha->spinup_interval != 0)) { > > + for (i = 0; i < pm8001_ha->chip->n_phy; ++i) > > + PM8001_CHIP_DISP->phy_start_req(pm8001_ha, i); > > + } else { > > + for (i = 0; i < pm8001_ha->chip->n_phy; ++i) { > > + spin_lock_irqsave(&pm8001_ha->lock, lock_flags); > > + pm8001_ha->phy_started = i; > > + pm8001_ha->scan_completion = ∁ > > + pm8001_ha->phystart_timedout = 1; > > + spin_unlock_irqrestore(&pm8001_ha->lock, lock_flags); > > + PM8001_CHIP_DISP->phy_start_req(pm8001_ha, i); > > + wait_for_completion_timeout(&comp, > > + msecs_to_jiffies(500)); > > + if (pm8001_ha->phystart_timedout) > > + PM8001_MSG_DBG(pm8001_ha, pm8001_printk( > > + "Timeout happened for phyid = %d\n", i)); > > + } > > + spin_lock_irqsave(&pm8001_ha->lock, lock_flags); > > + pm8001_ha->phy_started = -1; > > + pm8001_ha->scan_completion = NULL; > > + spin_unlock_irqrestore(&pm8001_ha->lock, lock_flags); > > + } > > } > > > > Thanks, > John
