Re: [PATCH 3/3] ata: sata_dwc_460ex: get rid of global data

[Andy Shevchenko]

On Tue, 2015-12-15 at 23:25 +0000, Mans Rullgard wrote:
> This moves all global data into the driver private struct, thus
> permitting multiple devices of this type to be used.
> 

Nice!

Btw, last time Linus complained about new warnings. Most of them I have
fixed when moved to external DMA driver. Leftovers IIRC are related to
address space. Are you going to fix them? Otherwise it might be a
headache for him again with strong wording to our address I suppose.

> Signed-off-by: Mans Rullgard <mans@xxxxxxxxx>
> ---
> Âdrivers/ata/sata_dwc_460ex.c | 80 ++++++++++++++++++++------------
> ------------
> Â1 file changed, 36 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/ata/sata_dwc_460ex.c
> b/drivers/ata/sata_dwc_460ex.c
> index d07aae1..919f870 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -146,6 +146,8 @@ struct sata_dwc_device {
> Â	struct ata_host		*host;
> Â	u8 __iomem		*reg_base;
> Â	struct sata_dwc_regs	*sata_dwc_regs;	/* DW
> Synopsys SATA specific */
> +	u32			sactive_issued;
> +	u32			sactive_queued;
> Â	struct phy		*phy;
> Â#ifdef CONFIG_SATA_DWC_OLD_DMA
> Â	struct dw_dma_chip	*dma;
> @@ -190,14 +192,6 @@ enum {
> Â	SATA_DWC_DMA_PENDING_RX		= 2,
> Â};
> Â
> -struct sata_dwc_host_priv {
> -	void	__iomem	Â*scr_addr_sstatus;
> -	u32	sata_dwc_sactive_issued ;
> -	u32	sata_dwc_sactive_queued ;
> -};
> -
> -static struct sata_dwc_host_priv host_pvt;
> -
> Â/*
> Â * Prototypes
> Â */
> @@ -448,21 +442,22 @@ static int sata_dwc_scr_write(struct ata_link
> *link, unsigned int scr, u32 val)
> Â	return 0;
> Â}
> Â
> -static u32 core_scr_read(unsigned int scr)
> +static u32 core_scr_read(struct sata_dwc_device *hsdev, unsigned int
> scr)
> Â{
> -	return in_le32(host_pvt.scr_addr_sstatus + (scr * 4));
> +	return in_le32(hsdev->reg_base + SATA_DWC_SCR_OFFSET + (scr
> * 4));
> Â}
> Â
> -static void core_scr_write(unsigned int scr, u32 val)
> +static void core_scr_write(struct sata_dwc_device *hsdev, unsigned
> int scr,
> +			ÂÂÂu32 val)
> Â{
> -	out_le32(host_pvt.scr_addr_sstatus + (scr * 4), val);
> +	out_le32(hsdev->reg_base + SATA_DWC_SCR_OFFSET + (scr * 4),
> val);
> Â}
> Â
> -static void clear_serror(void)
> +static void clear_serror(struct sata_dwc_device *hsdev)
> Â{
> Â	u32 val;
> -	val = core_scr_read(SCR_ERROR);
> -	core_scr_write(SCR_ERROR, val);
> +	val = core_scr_read(hsdev, SCR_ERROR);
> +	core_scr_write(hsdev, SCR_ERROR, val);
> Â}
> Â
> Âstatic void clear_interrupt_bit(struct sata_dwc_device *hsdev, u32
> bit)
> @@ -489,7 +484,7 @@ static void sata_dwc_error_intr(struct ata_port
> *ap,
> Â
> Â	ata_ehi_clear_desc(ehi);
> Â
> -	serror = core_scr_read(SCR_ERROR);
> +	serror = core_scr_read(hsdev, SCR_ERROR);
> Â	status = ap->ops->sff_check_status(ap);
> Â
> Â	tag = ap->link.active_tag;
> @@ -500,7 +495,7 @@ static void sata_dwc_error_intr(struct ata_port
> *ap,
> Â		hsdevp->dma_pending[tag], hsdevp->cmd_issued[tag]);
> Â
> Â	/* Clear error register and interrupt bit */
> -	clear_serror();
> +	clear_serror(hsdev);
> Â	clear_interrupt_bit(hsdev, SATA_DWC_INTPR_ERR);
> Â
> Â	/* This is the only error happening now.ÂÂTODO check for
> exact error */
> @@ -539,7 +534,7 @@ static irqreturn_t sata_dwc_isr(int irq, void
> *dev_instance)
> Â	int handled, num_processed, port = 0;
> Â	uint intpr, sactive, sactive2, tag_mask;
> Â	struct sata_dwc_device_port *hsdevp;
> -	host_pvt.sata_dwc_sactive_issued = 0;
> +	hsdev->sactive_issued = 0;
> Â
> Â	spin_lock_irqsave(&host->lock, flags);
> Â
> @@ -568,7 +563,7 @@ static irqreturn_t sata_dwc_isr(int irq, void
> *dev_instance)
> Â		if (hsdevp->cmd_issued[tag] !=
> SATA_DWC_CMD_ISSUED_PEND)
> Â			dev_warn(ap->dev, "CMD tag=%d not
> pending?\n", tag);
> Â
> -		host_pvt.sata_dwc_sactive_issued |=
> qcmd_tag_to_mask(tag);
> +		hsdev->sactive_issued |= qcmd_tag_to_mask(tag);
> Â
> Â		qc = ata_qc_from_tag(ap, tag);
> Â		/*
> @@ -582,11 +577,11 @@ static irqreturn_t sata_dwc_isr(int irq, void
> *dev_instance)
> Â		handled = 1;
> Â		goto DONE;
> Â	}
> -	sactive = core_scr_read(SCR_ACTIVE);
> -	tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^
> sactive;
> +	sactive = core_scr_read(hsdev, SCR_ACTIVE);
> +	tag_mask = (hsdev->sactive_issued | sactive) ^ sactive;
> Â
> Â	/* If no sactive issued and tag_mask is zero then this is
> not NCQ */
> -	if (host_pvt.sata_dwc_sactive_issued == 0 && tag_mask == 0)
> {
> +	if (hsdev->sactive_issued == 0 && tag_mask == 0) {
> Â		if (ap->link.active_tag == ATA_TAG_POISON)
> Â			tag = 0;
> Â		else
> @@ -656,22 +651,19 @@ DRVSTILLBUSY:
> Â	Â*/
> Â
> Â	Â/* process completed commands */
> -	sactive = core_scr_read(SCR_ACTIVE);
> -	tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^
> sactive;
> +	sactive = core_scr_read(hsdev, SCR_ACTIVE);
> +	tag_mask = (hsdev->sactive_issued | sactive) ^ sactive;
> Â
> -	if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1
> || \
> -							tag_mask >
> 1) {
> +	if (sactive != 0 || hsdev->sactive_issued > 1 ||ÂÂtag_mask >
> 1) {
> Â		dev_dbg(ap->dev,
> Â			"%s
> NCQ:sactive=0x%08xÂÂsactive_issued=0x%08x tag_mask=0x%08x\n",
> -			__func__, sactive,
> host_pvt.sata_dwc_sactive_issued,
> -			tag_mask);
> +			__func__, sactive, hsdev->sactive_issued,
> tag_mask);
> Â	}
> Â
> -	if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \
> -					(host_pvt.sata_dwc_sactive_i
> ssued)) {
> +	if ((tag_mask | hsdev->sactive_issued) != hsdev-
> >sactive_issued) {
> Â		dev_warn(ap->dev,
> -			Â"Bad tag mask?ÂÂsactive=0x%08x
> (host_pvt.sata_dwc_sactive_issued)=0x%08xÂÂtag_mask=0x%08x\n",
> -			Âsactive, host_pvt.sata_dwc_sactive_issued,
> tag_mask);
> +			Â"Bad tag mask?ÂÂsactive=0x%08x
> sactive_issued=0x%08xÂÂtag_mask=0x%08x\n",
> +			Âsactive, hsdev->sactive_issued, tag_mask);
> Â	}
> Â
> Â	/* read just to clear ... not bad if currently still busy */
> @@ -733,7 +725,7 @@ STILLBUSY:
> Â	Â* we were processing --we read status as part of processing
> a completed
> Â	Â* command).
> Â	Â*/
> -	sactive2 = core_scr_read(SCR_ACTIVE);
> +	sactive2 = core_scr_read(hsdev, SCR_ACTIVE);
> Â	if (sactive2 != sactive) {
> Â		dev_dbg(ap->dev,
> Â			"More completed - sactive=0x%x
> sactive2=0x%x\n",
> @@ -819,8 +811,9 @@ static int sata_dwc_qc_complete(struct ata_port
> *ap, struct ata_queued_cmd *qc,
> Â	u8 status = 0;
> Â	u32 mask = 0x0;
> Â	u8 tag = qc->tag;
> +	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> Â	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> -	host_pvt.sata_dwc_sactive_queued = 0;
> +	hsdev->sactive_queued = 0;
> Â	dev_dbg(ap->dev, "%s checkstatus? %x\n", __func__,
> check_status);
> Â
> Â	if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_TX)
> @@ -833,10 +826,8 @@ static int sata_dwc_qc_complete(struct ata_port
> *ap, struct ata_queued_cmd *qc,
> Â
> Â	/* clear active bit */
> Â	mask = (~(qcmd_tag_to_mask(tag)));
> -	host_pvt.sata_dwc_sactive_queued =
> (host_pvt.sata_dwc_sactive_queued) \
> -						& mask;
> -	host_pvt.sata_dwc_sactive_issued =
> (host_pvt.sata_dwc_sactive_issued) \
> -						& mask;
> +	hsdev->sactive_queued = hsdev->sactive_queued & mask;
> +	hsdev->sactive_issued = hsdev->sactive_issued & mask;
> Â	ata_qc_complete(qc);
> Â	return 0;
> Â}
> @@ -961,7 +952,7 @@ static int sata_dwc_port_start(struct ata_port
> *ap)
> Â	}
> Â
> Â	/* Clear any error bits before libata starts issuing
> commands */
> -	clear_serror();
> +	clear_serror(hsdev);
> Â	ap->private_data = hsdevp;
> Â	dev_dbg(ap->dev, "%s: done\n", __func__);
> Â	return 0;
> @@ -999,6 +990,7 @@ static void sata_dwc_exec_command_by_tag(struct
> ata_port *ap,
> Â{
> Â	unsigned long flags;
> Â	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> +	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> Â
> Â	dev_dbg(ap->dev, "%s cmd(0x%02x): %s tag=%d\n", __func__,
> tf->command,
> Â		ata_get_cmd_descript(tf->command), tag);
> @@ -1012,7 +1004,7 @@ static void sata_dwc_exec_command_by_tag(struct
> ata_port *ap,
> Â	Â* managed SError register for the disk needs to be done
> before the
> Â	Â* task file is loaded.
> Â	Â*/
> -	clear_serror();
> +	clear_serror(hsdev);
> Â	ata_sff_exec_command(ap, tf);
> Â}
> Â
> @@ -1065,7 +1057,7 @@ static void sata_dwc_bmdma_start_by_tag(struct
> ata_queued_cmd *qc, u8 tag)
> Â	sata_dwc_tf_dump(ap, &qc->tf);
> Â
> Â	if (start_dma) {
> -		reg = core_scr_read(SCR_ERROR);
> +		reg = core_scr_read(hsdev, SCR_ERROR);
> Â		if (reg & SATA_DWC_SERROR_ERR_BITS) {
> Â			dev_err(ap->dev, "%s: ****** SError=0x%08x
> ******\n",
> Â				__func__, reg);
> @@ -1128,6 +1120,7 @@ static unsigned int sata_dwc_qc_issue(struct
> ata_queued_cmd *qc)
> Â	u32 sactive;
> Â	u8 tag = qc->tag;
> Â	struct ata_port *ap = qc->ap;
> +	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> Â
> Â#ifdef DEBUG_NCQ
> Â	if (qc->tag > 0 || ap->link.sactive > 1)
> @@ -1144,9 +1137,9 @@ static unsigned int sata_dwc_qc_issue(struct
> ata_queued_cmd *qc)
> Â	sata_dwc_qc_prep_by_tag(qc, tag);
> Â
> Â	if (ata_is_ncq(qc->tf.protocol)) {
> -		sactive = core_scr_read(SCR_ACTIVE);
> +		sactive = core_scr_read(hsdev, SCR_ACTIVE);
> Â		sactive |= (0x00000001 << tag);
> -		core_scr_write(SCR_ACTIVE, sactive);
> +		core_scr_write(hsdev, SCR_ACTIVE, sactive);
> Â
> Â		dev_dbg(qc->ap->dev,
> Â			"%s: tag=%d ap->link.sactive = 0x%08x
> sactive=0x%08x\n",
> @@ -1289,7 +1282,6 @@ static int sata_dwc_probe(struct
> platform_device *ofdev)
> Â	/* Setup port */
> Â	host->ports[0]->ioaddr.cmd_addr = base;
> Â	host->ports[0]->ioaddr.scr_addr = base +
> SATA_DWC_SCR_OFFSET;
> -	host_pvt.scr_addr_sstatus = base + SATA_DWC_SCR_OFFSET;
> Â	sata_dwc_setup_port(&host->ports[0]->ioaddr, (unsigned
> long)base);
> Â
> Â	/* Read the ID and Version Registers */

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/