Re: [PATCH 1/4] [SCSI] ufshcd: UFS Host controller driver

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 02 February 2012, Vinayak Holikatti wrote:
> From: Santosh Yaraganavi <santoshsy@gmail.com>
> 
> This patch adds support for Universal Flash Storage(UFS)
> host controllers. The UFS host controller driver
> includes host controller initialization method.
> 
> The Initialization process involves following steps:
>  - Initiate UFS Host Controller initialization process by writing
>    to Host controller enable register
>  - Configure UFS Host controller registers with host memory space
>    datastructure offsets.
>  - Unipro link startup procedure
>  - Check for connected device
>  - Configure UFS host controller to process requests
>  - Enable required interrupts
>  - Configure interrupt aggregation
> 
> Signed-off-by: Santosh Yaraganavi <santoshsy@gmail.com>
> Signed-off-by: Vinayak Holikatti <vinholikatti@gmail.com>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Saugata Das <saugata.das@linaro.org>
> Reviewed-by: Vishak G <vishak.g@samsung.com>
> Reviewed-by: Girish K S <girish.shivananjappa@linaro.org>

Thanks for posting this here. Note that while I did review the patches
in private email, I did not reply with a "Reviewed-by" tag, so you should
not add it yourself. In particular, I had made some additional comments
about the ufshcd_memory_alloc() function that have not been addressed.

Getting the code changed will certainly not be a problem, but please
be careful with assigning those tags in the future.

The only major thing that I see missing is a review from James or
someone else who is familiar with other scsi device drivers. Saugata
and I have (in private) commented on a a number of more general issues
and the comments were addressed before this patch set got sent out.

Unless there are important concerns from the SCSI side, I believe this
is going to be ready to get merged very soon, after the usual nitpicking
is done ;-)

Speaking of nitpicking:

>--- /dev/null
>+++ b/drivers/scsi/ufs/Kconfig
>+
>+#ifndef NULL
>+#define NULL 0
>+#endif  /* NULL */

Please remove this #define, NULL is defined in <linux/stddef.h>

>+#define BYTES_TO_DWORDS(p)     (p >> 2)
>+#define UFSHCD_MMIO_BASE       (hba->mmio_base)

Better remove these macros, too: The are clearly longer than the
expanded text, and less clear.

>+struct ufs_hba {
>+       /* Virtual memory reference */
>+       void *ucdl_virt_addr;
>+       void *utrdl_virt_addr;
>+       void *utmrdl_virt_addr;
>+       void *utrdl_virt_addr_aligned;
>+       void *utmrdl_virt_addr_aligned;
>+       void *ucdl_virt_addr_aligned;
>+
>+       size_t ucdl_size;
>+       size_t utrdl_size;
>+       size_t utmrdl_size;
>+
>+       /* DMA memory reference */
>+       dma_addr_t ucdl_dma_addr;
>+       dma_addr_t utrdl_dma_addr;
>+       dma_addr_t utmrdl_dma_addr;
>+       dma_addr_t utrdl_dma_addr_aligned;
>+       dma_addr_t utmrdl_dma_addr_aligned;
>+       dma_addr_t ucdl_dma_addr_aligned;

You can remove most of these members by simplifying the allocation:

- remove the _aligned variables and use WARN_ON to test that
  the allocated buffers are aligned (they always are)
- remove the sizes because they are computed from the number of
  descriptors
- if possible, remove the _dma_addr members by referencing them only
  in the ufshcd_host_memory_configure() function that can get merged
  into ufshcd_memory_alloc()
- while you're here, change the type of the *_virt_addr to
  struct utp_task_req_desc/utp_transfer_req_desc/utp_transfer_req_cmd_desc
  and remove the _virt_addr postfix.

>+       if (NULL == hba->ucdl_virt_addr) {

Here and in many other places, it's better to use the common kernel style

	if (!hba->ucdl_virt_addr) {

to check the validity of an object.

>+static int ufshcd_make_hba_operational(struct ufs_hba *hba)
>+{
>+       u32 reg;
>+
>+       /* check if device present */
>+       reg = readl((UFSHCD_MMIO_BASE + REG_CONTROLLER_STATUS));
>+       if (ufshcd_is_device_present(reg)) {
>+               dev_err(&hba->pdev->dev, "cc: Device not present\n");
>+               return -EINVAL;
>+       }
>+
>+       /*
>+        * UCRDY, UTMRLDY and UTRLRDY bits must be 1
>+        * DEI, HEI bits must be 0
>+        */
>+       if (!(ufshcd_get_lists_status(reg))) {
>+               writel(UTP_TASK_REQ_LIST_RUN_STOP_BIT,
>+                      (UFSHCD_MMIO_BASE +
>+                       REG_UTP_TASK_REQ_LIST_RUN_STOP));
>+               writel(UTP_TRANSFER_REQ_LIST_RUN_STOP_BIT,
>+                      (UFSHCD_MMIO_BASE +
>+                       REG_UTP_TRANSFER_REQ_LIST_RUN_STOP));
>+       } else {
>+               dev_err(&hba->pdev->dev,
>+                       "Host controller not ready to process requests");
>+               return -EINVAL;
>+       }

I guess ENXIO or EIO would be more fitting here than EINVAL, because you
are not referring to incorrect user input.

>+#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 */

These look wrong. Are you planning to fill them in a later patch? If not,
it's probably better to just remove these functions.

	Arnd