From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joao Pinto Subject: Re: [PATCH v7 3/3] add support for DWC UFS Host Controller Date: Mon, 15 Feb 2016 11:16:26 +0000 Message-ID: <56C1B38A.6070901@synopsys.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Akinobu Mita , Joao Pinto Cc: vinayak holikatti , Julian Calaby , Christoph Hellwig , Arnd Bergmann , Mark Rutland , Gilad Broner , Subhash Jadavani , CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w@public.gmane.org, Ian Campbell , LKML , "linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "open list:OPEN FIRMWARE AND..." List-Id: linux-scsi@vger.kernel.org Hi Akinobu, On 2/13/2016 1:27 PM, Akinobu Mita wrote: > Hi Joao, > > 2016-02-11 21:13 GMT+09:00 Joao Pinto : >> +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. We can do that, it would make the function body lighter and more easy to read I agree with you! > > 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_*)? I agree with you, they should have since they are DWC specific. > >> 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. These are not DWC specific, that's why I added them to the common unipro.h > Thanks. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752765AbcBOLQh (ORCPT ); Mon, 15 Feb 2016 06:16:37 -0500 Received: from us01smtprelay-2.synopsys.com ([198.182.47.9]:48506 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750970AbcBOLQf (ORCPT ); Mon, 15 Feb 2016 06:16:35 -0500 Subject: Re: [PATCH v7 3/3] add support for DWC UFS Host Controller To: Akinobu Mita , Joao Pinto References: CC: vinayak holikatti , Julian Calaby , Christoph Hellwig , "Arnd Bergmann" , Mark Rutland , Gilad Broner , Subhash Jadavani , , Ian Campbell , LKML , "linux-scsi@vger.kernel.org" , "open list:OPEN FIRMWARE AND..." From: Joao Pinto Message-ID: <56C1B38A.6070901@synopsys.com> Date: Mon, 15 Feb 2016 11:16:26 +0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.13.184.19] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Akinobu, On 2/13/2016 1:27 PM, Akinobu Mita wrote: > Hi Joao, > > 2016-02-11 21:13 GMT+09:00 Joao Pinto : >> +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. We can do that, it would make the function body lighter and more easy to read I agree with you! > > 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_*)? I agree with you, they should have since they are DWC specific. > >> 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. These are not DWC specific, that's why I added them to the common unipro.h > Thanks.