Re: [PATCH v2] ata: add CONFIG_SATA_HOST config option

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

On 11-02-2011 16:33, Bartlomiej Zolnierkiewicz wrote:

> Add CONFIG_SATA_HOST config option (for selecting SATA Host
> support) to make setup easier on PATA-only systems.

> Additionally move SATA-specific code to libata-sata.c which
> allows us to save ~11.5k of the output code size (x86-64) on
> PATA-only systems for CONFIG_SATA_HOST=n:

> CONFIG_SATA_HOST=y:
>     text    data     bss     dec     hex filename
>    44283    6576      57   50916    c6e4 drivers/ata/libata-core.o
>    29054      16       2   29072    7190 drivers/ata/libata-eh.o
>    20085       0      19   20104    4e88 drivers/ata/libata-sff.o
>     8699       0       0    8699    21fb drivers/ata/libata-sata.o

> CONFIG_SATA_HOST=n:
>     text    data     bss     dec     hex filename
>    43754    6576      57   50387    c4d3 drivers/ata/libata-core.o
>    26775      16       2   26793    68a9 drivers/ata/libata-eh.o
>    20144       0      19   20163    4ec3 drivers/ata/libata-sff.o

> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
[...]

> Index: b/drivers/ata/libata-core.c
> ===================================================================
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
[...]
> @@ -573,34 +494,26 @@ void ata_tf_to_fis(const struct ata_task
>  	fis[19] = 0;
>  }

    Hm, what about ata_tf_to_fis()? It shouldn't be needed by non-SATA stuff, 
moreover it'should be only needed by non-SFF controllers...

> +#ifndef CONFIG_SATA_HOST
> +int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
> +		      bool spm_wakeup)
>   {
> -	tf->command	= fis[2];	/* status */
> -	tf->feature	= fis[3];	/* error */
> -
> -	tf->lbal	= fis[4];
> -	tf->lbam	= fis[5];
> -	tf->lbah	= fis[6];
> -	tf->device	= fis[7];
> -
> -	tf->hob_lbal	= fis[8];
> -	tf->hob_lbam	= fis[9];
> -	tf->hob_lbah	= fis[10];
> -
> -	tf->nsect	= fis[12];
> -	tf->hob_nsect	= fis[13];
> +	return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL_GPL(sata_link_scr_lpm);

    Shouldn't there be empty line here?

> +int sata_std_hardreset(struct ata_link *link, unsigned int *class,
> +		       unsigned long deadline)
> +{
> +	return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL_GPL(sata_std_hardreset);

    ... and here?

> Index: b/drivers/ata/libata-sata.c
> ===================================================================
> --- /dev/null
> +++ b/drivers/ata/libata-sata.c
> @@ -0,0 +1,1242 @@
[...]
> +int sata_set_spd(struct ata_link *link)
> +{
> +	u32 scontrol;
> +	int rc;
> +
> +	if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))

    I guess checkpatch.pl protests here...

> +		return rc;
> +
> +	if (!__sata_set_spd_needed(link,&scontrol))
> +		return 0;
> +
> +	if ((rc = sata_scr_write(link, SCR_CONTROL, scontrol)))

    Here too...

> +		return rc;
> +
> +	return 1;
> +}
> +EXPORT_SYMBOL_GPL(sata_set_spd);
[...]
> +int sata_link_debounce(struct ata_link *link, const unsigned long *params,
> +		       unsigned long deadline)
> +{
> +	unsigned long interval = params[0];
> +	unsigned long duration = params[1];
> +	unsigned long last_jiffies, t;
> +	u32 last, cur;
> +	int rc;
> +
> +	t = ata_deadline(jiffies, params[2]);
> +	if (time_before(t, deadline))
> +		deadline = t;
> +
> +	if ((rc = sata_scr_read(link, SCR_STATUS, &cur)))

    And here too...

> +/**
> + *	sata_link_resume - resume SATA link
> + *	@link: ATA link to resume SATA
> + *	@params: timing parameters { interval, duratinon, timeout } in msec
> + *	@deadline: deadline jiffies for the operation
> + *
> + *	Resume SATA phy @link and debounce it.
> + *
> + *	LOCKING:
> + *	Kernel thread context (may sleep)
> + *
> + *	RETURNS:
> + *	0 on success, -errno on failure.
> + */
> +int sata_link_resume(struct ata_link *link, const unsigned long *params,
> +		     unsigned long deadline)
> +{
> +	int tries = ATA_LINK_RESUME_TRIES;
> +	u32 scontrol, serror;
> +	int rc;
> +
> +	if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))

    And here...

> +		return rc;
> +
> +	/*
> +	 * Writes to SControl sometimes get ignored under certain
> +	 * controllers (ata_piix SIDPR).  Make sure DET actually is
> +	 * cleared.
> +	 */
> +	do {
> +		scontrol = (scontrol & 0x0f0) | 0x300;
> +		if ((rc = sata_scr_write(link, SCR_CONTROL, scontrol)))
> +			return rc;
> +		/*
> +		 * Some PHYs react badly if SStatus is pounded
> +		 * immediately after resuming.  Delay 200ms before
> +		 * debouncing.
> +		 */
> +		ata_msleep(link->ap, 200);
> +
> +		/* is SControl restored correctly? */
> +		if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))

    And here...

> +			return rc;
> +	} while ((scontrol&  0xf0f) != 0x300&&  --tries);
> +
> +	if ((scontrol & 0xf0f) != 0x300) {
> +		ata_link_printk(link, KERN_ERR,
> +				"failed to resume link (SControl %X)\n",
> +				scontrol);
> +		return 0;
> +	}
> +
> +	if (tries<  ATA_LINK_RESUME_TRIES)
> +		ata_link_printk(link, KERN_WARNING,
> +				"link resume succeeded after %d retries\n",
> +				ATA_LINK_RESUME_TRIES - tries);
> +
> +	if ((rc = sata_link_debounce(link, params, deadline)))

    And here...

> +		return rc;
> +
> +	/* clear SError, some PHYs require this even for SRST to work */
> +	if (!(rc = sata_scr_read(link, SCR_ERROR, &serror)))

    Here too...

> +		rc = sata_scr_write(link, SCR_ERROR, serror);
> +
> +	return rc != -EINVAL ? rc : 0;
> +}
> +EXPORT_SYMBOL_GPL(sata_link_resume);
[...]
> +int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
> +			unsigned long deadline,
> +			bool *online, int (*check_ready)(struct ata_link *))
> +{
> +	u32 scontrol;
> +	int rc;
> +
> +	DPRINTK("ENTER\n");
> +
> +	if (online)
> +		*online = false;
> +
> +	if (sata_set_spd_needed(link)) {
> +		/* SATA spec says nothing about how to reconfigure
> +		 * spd.  To be on the safe side, turn off phy during
> +		 * reconfiguration.  This works for at least ICH7 AHCI
> +		 * and Sil3124.
> +		 */
> +		if ((rc = sata_scr_read(link, SCR_CONTROL,&scontrol)))
> +			goto out;
> +
> +		scontrol = (scontrol & 0x0f0) | 0x304;
> +
> +		if ((rc = sata_scr_write(link, SCR_CONTROL, scontrol)))

    And here...

> +			goto out;
> +
> +		sata_set_spd(link);
> +	}
> +
> +	/* issue phy wake/reset */
> +	if ((rc = sata_scr_read(link, SCR_CONTROL,&scontrol)))

    Here too...

> +		goto out;
> +
> +	scontrol = (scontrol&  0x0f0) | 0x301;
> +
> +	if ((rc = sata_scr_write_flush(link, SCR_CONTROL, scontrol)))

    And here...
    I understand that you're only moving the code. Perhaps it's worth fixing 
checkpatch.pl's complaints beforehand -- I was going to look at it...

WBR, Sergei