Date: Tue, 7 Feb 2012 09:16:39 +0200
From: Felipe Balbi <balbi@ti.com>
To: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: James.Bottomley@HansenPartnership.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, patches@linaro.org,
        linux-samsung-soc@vger.kernel.org, saugata.das@linaro.org,
        arnd@arndb.de, venkat@linaro.org, girish.shivananjappa@linaro.org,
        vishak.g@samsung.com, k.rajesh@samsung.com, yejin.moon@samsung.com,
        Santosh Yaraganavi <santoshsy@gmail.com>
Subject: Re: [PATCH 1/4] [SCSI] ufshcd: UFS Host controller driver
Message-ID: <20120207071636.GE3707@legolas.emea.dhcp.ti.com>
Reply-To: balbi@ti.com
References: <1328158649-4137-1-git-send-email-vinholikatti@gmail.com>
 <1328158649-4137-2-git-send-email-vinholikatti@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="udcq9yAoWb9A4FsZ"
Content-Disposition: inline
In-Reply-To: <1328158649-4137-2-git-send-email-vinholikatti@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 8517
Lines: 307


--udcq9yAoWb9A4FsZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

On Thu, Feb 02, 2012 at 10:27:26AM +0530, Vinayak Holikatti wrote:
> +/**
> + * ufshcd_uic_cc_handler - handle UIC command completion
> + * @work: pointer to a work queue structure
> + *
> + * Returns 0 on success, non-zero value on failure
> + */
> +static void ufshcd_uic_cc_handler (struct work_struct *work)
> +{
> +	struct ufs_hba *hba;
> +
> +	hba =3D container_of(work, struct ufs_hba, uic_workq);
> +
> +	if ((UIC_CMD_DME_LINK_STARTUP =3D=3D hba->active_uic_cmd.command) &&

this is so annoying. Please invert all these constructs:

if ((hcd->active_uic_cmd.command =3D=3D UIC_CMD_DME_LINK_STARTUP) ....
and so on.

> +	    !(ufshcd_get_uic_cmd_result(hba))) {
> +
> +		if (ufshcd_make_hba_operational(hba))
> +			dev_err(&hba->pdev->dev,
> +				"cc: hba not operational state\n");
> +		return;
> +	}
> +}
> +
> +/**
> + * ufshcd_sl_intr - Interrupt service routine
> + * @hba: per adapter instance
> + * @intr_status: contains interrupts generated by the controller
> + */
> +static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> +{
> +	if (intr_status & UIC_COMMAND_COMPL)
> +		schedule_work(&hba->uic_workq);
> +}
> +
> +/**
> + * ufshcd_intr - Main interrupt service routine
> + * @irq: irq number
> + * @__hba: pointer to adapter instance
> + *
> + * Returns IRQ_HANDLED - If interrupt is valid
> + *		IRQ_NONE - If invalid interrupt
> + */
> +static irqreturn_t ufshcd_intr(int irq, void *__hba)
> +{
> +	unsigned long flags;
> +	u32 intr_status;
> +	irqreturn_t retval =3D IRQ_NONE;
> +	struct ufs_hba *hba =3D __hba;
> +
> +	spin_lock_irqsave(hba->host->host_lock, flags);
> +	intr_status =3D readl(UFSHCD_MMIO_BASE + REG_INTERRUPT_STATUS);
> +
> +	if (intr_status) {
> +		ufshcd_sl_intr(hba, intr_status);
> +
> +		/* If UFSHCI 1.0 then clear interrupt status register */
> +		if (UFSHCI_VERSION_10 =3D=3D hba->ufs_version)
> +			writel(intr_status,
> +			       (UFSHCD_MMIO_BASE + REG_INTERRUPT_STATUS));
> +		retval =3D IRQ_HANDLED;
> +	}
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	return retval;
> +}
> +
> +static struct scsi_host_template ufshcd_driver_template =3D {
> +	.module			=3D THIS_MODULE,
> +	.name			=3D UFSHCD,
> +	.proc_name		=3D UFSHCD,
> +	.this_id		=3D -1,
> +};
> +
> +/**
> + * ufshcd_shutdown - main funciton to put the controller in reset state
> + * @pdev: pointer to PCI device handle
> + */
> +static void ufshcd_shutdown(struct pci_dev *pdev)
> +{
> +	ufshcd_hba_stop((struct ufs_hba *)pci_get_drvdata(pdev));
> +}
> +
> +#ifdef CONFIG_PM
> +/**
> + * ufshcd_suspend - suspend power management function
> + * @pdev: pointer to PCI device handle
> + * @state: power state
> + *
> + * Returns -ENOSYS
> + */
> +static int ufshcd_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	return -ENOSYS;
> +}
> +
> +/**
> + * ufshcd_resume - resume power management function
> + * @pdev: pointer to PCI device handle
> + *
> + * Returns -ENOSYS
> + */
> +static int ufshcd_resume(struct pci_dev *pdev)
> +{
> +	return -ENOSYS;
> +}
> +#endif /* CONFIG_PM */
> +
> +/**
> + * ufshcd_hba_free - free allocated memory for
> + *			host memory space data structures
> + * @hba: per adapter instance
> + */
> +static void ufshcd_hba_free(struct ufs_hba *hba)
> +{
> +	iounmap(UFSHCD_MMIO_BASE);
> +	ufshcd_free_hba_memory(hba);
> +	pci_release_regions(hba->pdev);
> +}
> +
> +/**
> + * ufshcd_remove - deallocate PCI/SCSI host and host memory space
> + *		data structure memory
> + * @pdev - pointer to PCI handle
> + */
> +static void ufshcd_remove(struct pci_dev *pdev)
> +{
> +	struct ufs_hba *hba =3D pci_get_drvdata(pdev);
> +
> +	/* disable interrupts */
> +	ufshcd_int_config(hba, UFSHCD_INT_DISABLE);
> +	free_irq(pdev->irq, hba);
> +
> +	ufshcd_hba_stop(hba);
> +	ufshcd_hba_free(hba);
> +
> +	scsi_remove_host(hba->host);
> +	scsi_host_put(hba->host);
> +	pci_set_drvdata(pdev, NULL);
> +	pci_clear_master(pdev);
> +	pci_disable_device(pdev);
> +}
> +
> +/**
> + * ufshcd_set_dma_mask - Set dma addressing
> + * @pdev: PCI device struct
> + *
> + * Returns 0 for success, non-zero for failure
> + */
> +static int ufshcd_set_dma_mask(struct pci_dev *pdev)
> +{
> +	int err;
> +
> +	do {
> +		err =3D pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> +		if (!err) {
> +			err =3D pci_set_consistent_dma_mask(pdev,
> +							  DMA_BIT_MASK(64));
> +			break;
> +		}
> +		err =3D pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> +		if (!err)
> +			err =3D pci_set_consistent_dma_mask(pdev,
> +							  DMA_BIT_MASK(32));
> +	} while (0);
> +
> +	return err;
> +}
> +
> +/**
> + * ufshcd_probe - probe routine of the driver
> + * @pdev: pointer to PCI device handle
> + * @id: PCI device id
> + *
> + * Returns 0 on success, non-zero value on failure
> + */
> +static int __devinit
> +ufshcd_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct Scsi_Host *host;
> +	struct ufs_hba *hba;
> +	int ufs_hba_len;
> +	int err;
> +
> +	ufs_hba_len =3D sizeof(struct ufs_hba);
> +	err =3D pci_enable_device(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "pci_enable_device failed\n");
> +		goto out_error;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	host =3D scsi_host_alloc(&ufshcd_driver_template, ufs_hba_len);
> +	if (!host) {
> +		dev_err(&pdev->dev, "scsi_host_alloc failed\n");
> +		err =3D -ENOMEM;
> +		goto out_disable;
> +	}
> +	hba =3D (struct ufs_hba *)host->hostdata;
> +
> +	err =3D pci_request_regions(pdev, UFSHCD);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "request regions failed\n");
> +		goto out_disable;
> +	}
> +
> +	hba->mmio_base =3D pci_ioremap_bar(pdev, 0);
> +	if (!hba->mmio_base) {
> +		dev_err(&pdev->dev, "memory map failed\n");
> +		err =3D -ENOMEM;
> +		goto out_release_regions;
> +	}
> +
> +	err =3D ufshcd_set_dma_mask(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "set dma mask failed\n");
> +		goto out_iounmap;
> +	}
> +
> +	hba->host =3D host;
> +	hba->pdev =3D pdev;
> +
> +	/* Read capabilities registers */
> +	ufshcd_hba_capabilities(hba);
> +
> +	/* Get UFS version supported by the controller */
> +	hba->ufs_version =3D ufshcd_get_ufs_version(hba);
> +
> +	/* Allocate memory for host memory space */
> +	err =3D ufshcd_memory_alloc(hba);
> +	if (err) {
> +		dev_err(&pdev->dev, "Memory allocation failed\n");
> +		goto out_iounmap;
> +	}
> +
> +	/* Configure LRB */
> +	ufshcd_host_memory_configure(hba);
> +
> +	host->can_queue =3D hba->nutrs;
> +	host->max_id =3D UFSHCD_MAX_ID;
> +	host->max_lun =3D UFSHCD_MAX_LUNS;
> +	host->max_channel =3D UFSHCD_MAX_CHANNEL;
> +	host->unique_id =3D host->host_no;
> +
> +	/* Initialize work queues */
> +	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
> +
> +	/* IRQ registration */
> +	err =3D request_irq(pdev->irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);

make this a threaded IRQ and remove your workqueues and tasklets.

> +	if (err) {
> +		dev_err(&pdev->dev, "request irq failed\n");
> +		goto out_lrb_free;
> +	}
> +
> +	pci_set_drvdata(pdev, hba);

would also be cool if you would abstract PCI out of this driver, so it's
easier to be re-used. See how I did it with drivers/usb/dwc3/core.c
drivers/usb/dwc3/dwc3-pci.c and drivers/usb/dwc3/dwc3-omap.c

--=20
balbi

--udcq9yAoWb9A4FsZ
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJPMM/UAAoJEIaOsuA1yqREtvsQAKMVpk7QG2NWngWqe4Hn4SLL
F9ScNIGbvVrAxq3TxXNf2cgGKxz9J4yw3uk86rVGseCY5JEu7qGQLUjYHDNyyaxL
v/NrvflcwVNBzRrSedyDNoJZXxfd2Af/Zu43LG/YD8Noz/SBBIe/CRzecWpSpV8+
U9hTfxnq9VhhsspKBg0qQSyzu9JrOFSAggpbhfNfxWJRc24ef0lAitgPYIzpINP3
5scEv4pBdvm3ukjaNZ4Rp5VqWeZ2Y6ZIkAM9OaGVnQYo2TRIMWc9tgZhOSI/7goK
MWo4MDx84L/MwGFWP0RqU1fjtRk44E2BDzPAKvq8VPwxAP85JnoX+yrdiE7PV5EZ
phiBNTRHFF8+3WI49jNn0NHWHbrxhMl5JYc3btpsu3kNfp0Al6/1OglumU2EJEfn
dbZDC7YHeLPugQSNNX9h0Ex85AHpPxZJc0eG3gYGrrkSXOHWtPk+MsrFENuD1xq0
2wm3DZewscmY2QOl+UkMfCDHp91SQdHg3NnU43U3iIchUDRWp0HbD/irJbszqn23
f8jR+XF/YMCnlDfCcMbs8RQMdVQGD41UyhDX8Z+uDAsqUoasNMQgSr5Ls5pxLgKQ
v87BjbGFgdiypjmg6THsCh0Sz3id6A/RcWREdWZV2F1Yn/XsuYbSesJeJkMKJtsW
hXyTp2+oRen0aQbztY1S
=ylPE
-----END PGP SIGNATURE-----

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