linux-kernel - Re: [PATCH v7 3/3] add support for DWC UFS Host Controller

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] [thread-next>] [day] [month] [year] [list]
Date:	Sat, 13 Feb 2016 22:27:24 +0900
From:	Akinobu Mita <akinobu.mita@...il.com>
To:	Joao Pinto <Joao.Pinto@...opsys.com>
Cc:	vinayak holikatti <vinholikatti@...il.com>,
	Julian Calaby <julian.calaby@...il.com>,
	Christoph Hellwig <hch@...radead.org>,
	Arnd Bergmann <arnd@...db.de>,
	Mark Rutland <mark.rutland@....com>,
	Gilad Broner <gbroner@...eaurora.org>,
	Subhash Jadavani <subhashj@...eaurora.org>,
	CARLOS.PALMINHA@...opsys.com,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH v7 3/3] add support for DWC UFS Host Controller

Hi Joao,

2016-02-11 21:13 GMT+09:00 Joao Pinto <Joao.Pinto@...opsys.com>:
> +static int ufshcd_dwc_connection_setup(struct ufs_hba *hba)
> +{
> +       int ret = 0;
> +
> +       /* Local side Configuration */
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 0);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(N_DEVICEID), 0);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(N_DEVICEID_VALID), 0);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_PEERDEVICEID), 1);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_PEERCPORTID), 0);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_TRAFFICCLASS), 0);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CPORTFLAGS), 0x6);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CPORTMODE), 1);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 1);
> +       if (ret)
> +               goto out;
> +
> +
> +       /* Peer side Configuration */
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 0);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(N_DEVICEID), 1);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(N_DEVICEID_VALID), 1);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_PEERDEVICEID), 1);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_PEERCPORTID), 0);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_TRAFFICCLASS), 0);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CPORTFLAGS), 0x6);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CPORTMODE), 1);
> +       if (ret)
> +               goto out;
> +
> +       ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 1);
> +       if (ret)
> +               goto out;
> +
> +out:
> +       return ret;
> +}

This looks a bit redundant.  The most part of the functions in this file is
doing ufshcd_dme_set() or ufshcd_dme_peer_set(), so should we
introduce ufshcd_dme_set_attrs() like below?  It will also increase
readability.

struct ufshcd_dme_attr_val {
        u32 attr_sel;
        u32 mib_val;
        u8 peer;
};

int ufshcd_dme_set_attrs(struct ufs_hba *hba,
                        const struct ufshcd_dme_attr_val *v, int n)
{
        for (i = 0; i < n; i++) {
                int ret = ufshcd_dme_set_attr(hba, v[i].attr_sel, ...);

                if (ret)
                        return ret;
        }
        return 0;
}

static int ufshcd_dwc_connection_setup(struct ufs_hba *hba)
{
        const struct ufshcd_dme_attr setup_attrs[] = {
                { UIC_ARG_MIB(T_CONNECTIONSTATE), 0, DME_LOCAL },
                { UIC_ARG_MIB(T_DEVICEID), 0, DME_LOCAL },
                ...
        };

        return ufshcd_dme_set_attrs(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
}

> +static int ufshcd_dwc_setup_tc(struct ufs_hba *hba)
> +{
> +       int ret = 0;
> +
> +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI
> +       dev_info(hba->dev, "Configuring Test Chip 40-bit RMMI");
> +       ret = ufshcd_dwc_setup_40bit_rmmi(hba);
> +       if (ret) {
> +               dev_err(hba->dev, "Configuration failed");

Please add \n in the end of message.
(and there are same issues in this file)

> +
> +/* Clock Divider Values: Hex equivalent of frequency in MHz */
> +enum clk_div_values {
> +       UFSHCD_CLK_DIV_62_5     = 0x3e,
> +       UFSHCD_CLK_DIV_125      = 0x7d,
> +       UFSHCD_CLK_DIV_200      = 0xc8,
> +};

This is used as register value for DWC_UFS_REG_HCLKDIV.  So should they
have similar prefix (DWC_UFS_REG_HCLKDIV_*)?

> diff --git a/drivers/scsi/ufs/unipro.h b/drivers/scsi/ufs/unipro.h

...

> +/*Other attributes*/
> +#define VS_MPHYCFGUPDT         0xD085
> +#define VS_DEBUGOMC            0xD09E
> +#define VS_POWERSTATE          0xD083

Are these vendor specific attributes?  If so, please move them to
ufshci-dwc.h.