On Thursday 02 February 2012, Vinayak Holikatti wrote:
> From: Santosh Yaraganavi <santoshsy@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Vinayak Holikatti <vinholikatti@xxxxxxxxx>
> Reviewed-by: Arnd Bergmann <arnd@xxxxxxxx>
> Reviewed-by: Saugata Das <saugata.das@xxxxxxxxxx>
> Reviewed-by: Vishak G <vishak.g@xxxxxxxxxxx>
> Reviewed-by: Girish K S <girish.shivananjappa@xxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
