Re: [PATCH v4 11/11] scsi: ufs-exynos: add UFS host support for Exynos SoCs

From: Alim Akhtar
Date: Sun Oct 25 2015 - 08:05:33 EST

Hi Kishon
Thanks again for you review.

On Fri, Oct 23, 2015 at 8:48 PM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
> Hi,
>
> On Thursday 15 October 2015 08:38 AM, Alim Akhtar wrote:
>> +CCing kishon Vijay,
>>
>> On 10/14/2015 06:25 PM, Alim Akhtar wrote:
>>> From: Seungwon Jeon <essuuj@xxxxxxxxx>
>>>
>>> This patch introduces Exynos UFS host controller driver,
>>> which mainly handles vendor-specific operations including
>>> link startup, power mode change and hibernation/unhibernation.
>>>
>>> Signed-off-by: Seungwon Jeon <essuuj@xxxxxxxxx>
>>> Signed-off-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
>>> ---
>>> drivers/scsi/ufs/Kconfig | 12 +
>>> drivers/scsi/ufs/Makefile | 1 +
>>> drivers/scsi/ufs/ufs-exynos-hw.c | 131 ++++
>>> drivers/scsi/ufs/ufs-exynos-hw.h | 43 ++
>>> drivers/scsi/ufs/ufs-exynos.c | 1317
>>> ++++++++++++++++++++++++++++++++++++++
>>> drivers/scsi/ufs/ufs-exynos.h | 247 +++++++
>>> drivers/scsi/ufs/ufshci.h | 26 +-
>>> drivers/scsi/ufs/unipro.h | 47 ++
>>> 8 files changed, 1823 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/scsi/ufs/ufs-exynos-hw.c
>>> create mode 100644 drivers/scsi/ufs/ufs-exynos-hw.h
>>> create mode 100644 drivers/scsi/ufs/ufs-exynos.c
>>> create mode 100644 drivers/scsi/ufs/ufs-exynos.h
>>>
> .
> .
> <snip>
> .
> .
>>> diff --git a/drivers/scsi/ufs/ufs-exynos-hw.c
>>> b/drivers/scsi/ufs/ufs-exynos-hw.c
>>> new file mode 100644
>>> index 000000000000..be6c61541a8f
>>> --- /dev/null
>>> +++ b/drivers/scsi/ufs/ufs-exynos-hw.c
>>> @@ -0,0 +1,131 @@
> .
> .
> <snip>
> .
> .
>>> +
>>> +#define PWR_MODE_STR_LEN 64
>>> +static int exynos_ufs_post_pwr_mode(struct ufs_hba *hba,
>>> + struct ufs_pa_layer_attr *pwr_max,
>>> + struct ufs_pa_layer_attr *pwr_req)
>>> +{
>>> + struct exynos_ufs *ufs = to_exynos_ufs(hba);
>>> + struct exynos_ufs_phy_info *phy_info = phy_get_drvdata(ufs->phy);
>
> This is abusing the interface. phy_get_drvdata is meant to be used only
> by the PHY driver.
>>> + struct exynos_ufs_phy_specific_ops *phy_ops =
>>> + phy_info->phy_specific_ops;
>
> I'm really not happy about having platform specific ops for PHY. We have
> to see if existing PHY ops can be used for this or in worst case add new
> PHY ops.
Well you said you like the controller driver to use only PHY ops[1], I
am sorry If I misunderstood that point, can you please help me to
understand that?
[1]-> https://lkml.org/lkml/2015/9/18/29

>>> + struct uic_pwr_mode *pwr = &ufs->pwr_act;
>>> + char pwr_str[PWR_MODE_STR_LEN] = "";
>>> + int ret = 0;
>>> +
>>> + if (ufs->drv_data->post_pwr_change)
>>> + ufs->drv_data->post_pwr_change(ufs, pwr);
>>> +
>>> + if (IS_UFS_PWR_MODE_HS(pwr->mode)) {
>>> + switch (pwr->hs_series) {
>>> + case PA_HS_MODE_A:
>>> + case PA_HS_MODE_B:
>>> + phy_ops->calibrate_phy(ufs->phy, CFG_POST_PWR_HS,
>>> + PWR_MODE_HS(pwr->gear, pwr->hs_series));
>>> + break;
>>> + }
>>> +
>>> + ret = phy_ops->wait_for_lock_acq(ufs->phy);
>>> + snprintf(pwr_str, sizeof(pwr_str), "Fast%s series_%s G_%d L_%d",
>>> + pwr->mode == FASTAUTO_MODE ? "_Auto" : "",
>>> + pwr->hs_series == PA_HS_MODE_A ? "A" : "B",
>>> + pwr->gear, pwr->lane);
>>> + } else if (IS_UFS_PWR_MODE_PWM(pwr->mode)) {
>>> + snprintf(pwr_str, sizeof(pwr_str), "Slow%s G_%d L_%d",
>>> + pwr->mode == SLOWAUTO_MODE ? "_Auto" : "",
>>> + pwr->gear, pwr->lane);
>>> + }
>>> +
>>> + dev_info(hba->dev, "Power mode change %d : %s\n", ret, pwr_str);
>>> + return ret;
>>> +}
>>> +
>>> +static void exynos_ufs_specify_nexus_t_xfer_req(struct ufs_hba *hba,
>>> + int tag, struct scsi_cmnd *cmd)
>>> +{
>>> + struct exynos_ufs *ufs = to_exynos_ufs(hba);
>>> + u32 type;
>>> +
>>> + type = hci_readl(ufs, HCI_UTRL_NEXUS_TYPE);
>>> +
>>> + if (cmd)
>>> + hci_writel(ufs, type | (1 << tag), HCI_UTRL_NEXUS_TYPE);
>>> + else
>>> + hci_writel(ufs, type & ~(1 << tag), HCI_UTRL_NEXUS_TYPE);
>>> +}
>>> +
>>> +static void exynos_ufs_specify_nexus_t_tm_req(struct ufs_hba *hba,
>>> + int tag, u8 func)
>>> +{
>>> + struct exynos_ufs *ufs = to_exynos_ufs(hba);
>>> + u32 type;
>>> +
>>> + type = hci_readl(ufs, HCI_UTMRL_NEXUS_TYPE);
>>> +
>>> + switch (func) {
>>> + case UFS_ABORT_TASK:
>>> + case UFS_QUERY_TASK:
>>> + hci_writel(ufs, type | (1 << tag), HCI_UTMRL_NEXUS_TYPE);
>>> + break;
>>> + case UFS_ABORT_TASK_SET:
>>> + case UFS_CLEAR_TASK_SET:
>>> + case UFS_LOGICAL_RESET:
>>> + case UFS_QUERY_TASK_SET:
>>> + hci_writel(ufs, type & ~(1 << tag), HCI_UTMRL_NEXUS_TYPE);
>>> + break;
>>> + }
>>> +}
>>> +
>>> +static void exynos_ufs_phy_init(struct exynos_ufs *ufs)
>>> +{
>>> + struct ufs_hba *hba = ufs->hba;
>>> + struct exynos_ufs_phy_info *phy_info = phy_get_drvdata(ufs->phy);
>>> + struct exynos_ufs_phy_specific_ops *phy_ops =
>>> + phy_info->phy_specific_ops;
>>> +
>>> + if (ufs->avail_ln_rx == 0 || ufs->avail_ln_tx == 0) {
>>> + ufshcd_dme_get(hba, UIC_ARG_MIB(PA_AVAILRXDATALANES),
>>> + &ufs->avail_ln_rx);
>>> + ufshcd_dme_get(hba, UIC_ARG_MIB(PA_AVAILTXDATALANES),
>>> + &ufs->avail_ln_tx);
>>> + WARN(ufs->avail_ln_rx != ufs->avail_ln_tx,
>>> + "available data lane is not equal(rx:%d, tx:%d)\n",
>>> + ufs->avail_ln_rx, ufs->avail_ln_tx);
>>> + }
>>> +
>>> + phy_ops->set_lane_cnt(ufs->phy, ufs->avail_ln_rx);
>
> can't bus_width attribute in phy core be reused for this?
>
I will take a look on it.

>>> + phy_ops->calibrate_phy(ufs->phy, CFG_PRE_INIT, PWR_MODE_ANY);
>
> Why can't calibrate PHY be directly done in phy_init?
>
This is just one instance, need to calibrate PHY when the ufs pwr mode changes.

> Thanks
> Kishon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html



--
Regards,
Alim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/