From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751415AbcBMN13 (ORCPT <rfc822;w@1wt.eu>);
	Sat, 13 Feb 2016 08:27:29 -0500
Received: from mail-vk0-f67.google.com ([209.85.213.67]:34417 "EHLO
	mail-vk0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751182AbcBMN10 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sat, 13 Feb 2016 08:27:26 -0500
MIME-Version: 1.0
In-Reply-To: <ee97e9b0ece0d819debe403c752d233c6267e02e.1455192008.git.jpinto@synopsys.com>
References: <cover.1455192008.git.jpinto@synopsys.com>
	<ee97e9b0ece0d819debe403c752d233c6267e02e.1455192008.git.jpinto@synopsys.com>
Date: Sat, 13 Feb 2016 22:27:24 +0900
Message-ID: <CAC5umyh6VGfsD+CzuimiMCum=E4UrA0rGj7pgPaRLO_aVen-gQ@mail.gmail.com>
Subject: Re: [PATCH v7 3/3] add support for DWC UFS Host Controller
From: Akinobu Mita <akinobu.mita@gmail.com>
To: Joao Pinto <Joao.Pinto@synopsys.com>
Cc: vinayak holikatti <vinholikatti@gmail.com>,
        Julian Calaby <julian.calaby@gmail.com>,
        Christoph Hellwig <hch@infradead.org>, Arnd Bergmann <arnd@arndb.de>,
        Mark Rutland <mark.rutland@arm.com>,
        Gilad Broner <gbroner@codeaurora.org>,
        Subhash Jadavani <subhashj@codeaurora.org>,
        CARLOS.PALMINHA@synopsys.com,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND..." <devicetree@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Joao,

2016-02-11 21:13 GMT+09:00 Joao Pinto <Joao.Pinto@synopsys.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.