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

[Akinobu Mita]

Hi Joao,

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