linux-kernel - Re: [PATCH v3 2/2] scsi: ufs: Add UFS platform driver for Cadence UFS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5BC4E8E4-566E-415A-9B57-36D725D8AB38@global.cadence.com>
Date:   Fri, 14 Sep 2018 07:46:34 +0000
From:   Janek Kotas <jank@...ence.com>
To:     Sekhar Nori <nsekhar@...com>
CC:     Janek Kotas <jank@...ence.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "vinholikatti@...il.com" <vinholikatti@...il.com>,
        "jejb@...ux.vnet.ibm.com" <jejb@...ux.vnet.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] scsi: ufs: Add UFS platform driver for Cadence UFS


> On 29 Aug 2018, at 12:27, Sekhar Nori <nsekhar@...com> wrote:
> 
> On Thursday 09 August 2018 06:55 PM, Janek Kotas wrote:
>> This patch adds a device tree platform driver for
>> Cadence UFS Host Controller.
>> It can be enabled with SCSI_UFS_CDNS_PLATFORM Kconfig option.
>> 
>> Signed-off-by: Jan Kotas <jank@...ence.com>
> 
> [...]
> 
>> diff --git a/drivers/scsi/ufs/cdns-pltfrm.c b/drivers/scsi/ufs/cdns-pltfrm.c
>> new file mode 100644
>> index 00000000..5d9e4875
>> --- /dev/null
>> +++ b/drivers/scsi/ufs/cdns-pltfrm.c
>> @@ -0,0 +1,147 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Platform UFS Host driver for Cadence controller
>> + *
>> + * Copyright (C) 2018 Cadence Design Systems, Inc.
>> + *
>> + * Authors:
>> + *	Jan Kotas <jank@...ence.com>
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/time.h>
>> +
>> +#include "ufshcd-pltfrm.h"
>> +
>> +#define CDNS_UFS_REG_HCLKDIV 0xFC
>> +
>> +/**
>> + * Sets HCLKDIV register value based on the core_clk
>> + * @hba: host controller instance
>> + *
>> + * Return zero for success and non-zero for failure
>> + */
>> +static int cdns_ufs_set_hclkdiv(struct ufs_hba *hba)
>> +{
>> +	struct ufs_clk_info *clki;
>> +	struct list_head *head = &hba->clk_list_head;
>> +	unsigned long core_clk_rate = 0;
>> +	u32 core_clk_div = 0;
>> +
>> +	if (list_empty(head))
>> +		return 0;
>> +
>> +	list_for_each_entry(clki, head, list) {
>> +		if (IS_ERR_OR_NULL(clki->clk))
>> +			continue;
>> +		if (!strcmp(clki->name, "core_clk"))
>> +			core_clk_rate = clk_get_rate(clki->clk);
>> +	}
>> +
>> +	if (!core_clk_rate) {
>> +		dev_err(hba->dev, "%s: unable to find core_clk rate\n",
>> +			__func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	core_clk_div = core_clk_rate / USEC_PER_SEC;
>> +
>> +	ufshcd_writel(hba, core_clk_div, CDNS_UFS_REG_HCLKDIV);
>> +	/* Make sure the register was updated */
>> +	mb();
> 
> Why is it critical to ensure write completed here?

The UniPro clock rate needs to be set before it’s enabled, 
otherwise it will cause a lot of errors at the link layer.

> 
>> +
>> +	return 0;
>> +}
>> +
> 
>> +/**
>> + * cdns_ufs_pltfrm_remove - set driver_data of the device to NULL
> 
> This appear to do more than setting driver_data to NULL.

I will update the description, thank you for pointing that.

> 
>> + * @pdev: pointer to platform device handle
>> + *
>> + * Always returns 0
>> + */
>> +static int cdns_ufs_pltfrm_remove(struct platform_device *pdev)
>> +{
>> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
>> +
>> +	pm_runtime_get_sync(&(pdev)->dev);
> 
> This looks suspect. Why do you need this get_sync() on remove() and
> where is the corresponding put_sync()?

I looked at qcom driver as a reference, I will review this, thank you.

>> +	ufshcd_remove(hba);
>> +	return 0;
>> +}
> 
> Thanks,
> Sekhar

Thank you for your reply.
I will update the patch.

Jan