On Fri, Feb 3, 2012 at 8:49 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> 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.
Sure, we'll keep in mind.
>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.
>
Ok, we'll do so.
>>+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.
>
I tried,
if( !ucdl_dma_addr || WARN_ON(ucdl_dma_addr & PAGE_SIZE))
pr_err("Memory allocation failed\n");
but I was getting "memory allocation failed error".
Since we need ucdl 128 byte aligned, utrdl and utmrdl 1kb aligned,
Currently I'm testing with the following code,
if( !ucdl_dma_addr || WARN_ON(ucdl_dma_addr & (128 - 1))
pr_err("Memory allocation failed\n");
if( !utrdl_dma_addr || WARN_ON(utrdl_dma_addr & (1024 - 1))
pr_err("Memory allocation failed\n");
and
if( !utmrdl_dma_addr || WARN_ON(utmrdl_dma_addr & (1024 - 1))
pr_err("Memory allocation failed\n");
also I'll make changes to the other things you pointed out.
>>+ 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.
>
ok, we'll do so.
>>+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.
>
ok, we'll update accordingly.
>>+#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.
>
Yes, We'll implement power management in the next patch.
linux/Documentation/SubmittingDrivers suggested to define
.suspend and .resume methods returning -ENOSYS, if not yet implemented.
So it was added.
Thanks for your comments. Please let us know if you have any comments
on the other patches as well.
> Arnd
--
~Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
