Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device

["Dov Levenglick" <dovl@codeaurora.org>]

> On Sun, Jun 7, 2015 at 10:32 AM,  <ygardi@codeaurora.org> wrote:
>>> 2015-06-05 5:53 GMT+09:00  <ygardi@codeaurora.org>:
>>>>> Hi Yaniv,
>>>>>
>>>>> 2015-06-03 18:37 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>>>>>> @@ -321,7 +313,22 @@ static int ufshcd_pltfrm_probe(struct
>>>>>> platform_device *pdev)
>>>>>>                 goto out;
>>>>>>         }
>>>>>>
>>>>>> -       hba->vops = get_variant_ops(&pdev->dev);
>>>>>> +       err = of_platform_populate(node, NULL, NULL, &pdev->dev);
>>>>>> +       if (err)
>>>>>> +               dev_err(&pdev->dev,
>>>>>> +                       "%s: of_platform_populate() failed\n",
>>>>>> __func__);
>>>>>> +
>>>>>> +       ufs_variant_node = of_get_next_available_child(node, NULL);
>>>>>> +
>>>>>> +       if (!ufs_variant_node) {
>>>>>> +               dev_dbg(&pdev->dev, "failed to find ufs_variant_node
>>>>>> child\n");
>>>>>> +       } else {
>>>>>> +               ufs_variant_pdev =
>>>>>> of_find_device_by_node(ufs_variant_node);
>>>>>> +
>>>>>> +               if (ufs_variant_pdev)
>>>>>> +                       hba->vops = (struct ufs_hba_variant_ops *)
>>>>>> +
>>>>>> dev_get_drvdata(&ufs_variant_pdev->dev);
>>>>>> +       }
>>>>>
>>>>> I have no strong objection to 'ufs_variant' sub-node.  But why can't
>>>>> we
>>>>> simply add an of_device_id to ufs_of_match, like below:
>>>>>
>>>>> static const struct of_device_id ufs_of_match[] = {
>>>>>         { .compatible = "jedec,ufs-1.1"},
>>>>> #if IS_ENABLED(SCSI_UFS_QCOM)
>>>>>         { .compatible = "qcom,ufs", .data = &ufs_hba_qcom_vops },
>>>>> #neidf
>>>>>         {},
>>>>> };
>>>>>
>>>>> and get hba->vops by get_variant_ops()?
>>>>>
>>>>
>>>> Hi Mita,
>>>> thanks for your comments.
>>>>
>>>> The whole idea, of having a sub-node which includes all variant
>>>> specific
>>>> attributes is to separate the UFS Platform device component, from the
>>>> need
>>>> to know "qcom" or any other future variant.
>>>> I believe it keeps the code more modular, and clean - meaning - no
>>>> #ifdef's and no need to include all variant attributes inside the
>>>> driver
>>>> DT node.
>>>> in that case, we simply have a DT node that is compatible to the Jdec
>>>> standard, and sub-node to include variant info.
>>>>
>>>> I hope you agree with this new design, since it provides a good answer
>>>> to every future variant that will be added, without the need to change
>>>> the
>>>> platform file.
>>>
>>> Thanks for your explanation, I agree with it.
>>>
>>> I found two problems in the current code, but both can be fixed
>>> relatively easily as described below:
>>>
>>> 1) If ufshcd-pltfrm driver is loaded before ufs-qcom driver,
>>> ufshcd_pltfrm_probe() can't find a ufs_variant device.
>>>
>>> In order to trigger re-probing ufs device when ufs-qcom driver has
>>> been loaded, ufshcd_pltfrm_probe() should return -EPROBE_DEFER in
>>> case 'ufs_variant' sub-node exists and no hba->vops found.
>>>
>>> 2) Nothing prevents ufs-qcom module from being unloaded while the
>>> variant_ops is referenced by ufshcd-pltfrm.
>>>
>>> It can be fixed by incrementing module refcount of ufs_variant module
>>> by __module_get(ufs_variant_pdev->dev.driver->owener) in
>>> ufshcd_pltfrm_probe(), and module_put() in ufshcd_pltfrm_remove()
>>> to descrement the refcount.
>>>
>>
>> again, Mita, your comments are very appreciated.
>>
>> 1)
>> If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually
>> happens
>> always), then the calling to of_platform_populate() which is added,
>> guarantees that ufs-qcom probe will be called and finish, before
>> ufshcd_pltfrm probe continues.
>> so ufs_variant device is always there, and ready.
>> I think it means we are safe - since either way, we make sure ufs-qcom
>> probe will be called and finish before dealing with ufs_variant device
>> in
>> ufshcd_pltfrm probe.
>
> This is due to the fact that you have 2 platform drivers. You should
> only have 1 (and 1 node). If you really think you need 2, then you
> should do like many other common *HCIs do and make the base UFS driver
> a set of library functions that drivers can use or call. Look at EHCI,
> AHCI, SDHCI, etc. for inspiration.

Hi Rob,
We did look at SDHCI and decided to go with this design due to its
simplicity and lack of library functions. Yaniv described the proper flow
of probing and, as we understand things, it is guaranteed to work as
designed.

Furthermore, the design of having a subcore in the dts is used in the
Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example
- both dwc3-msm and dwc3-exynox invoke the probing function in core.c
(i.e. the shared underlying Synopsys USB dwc3 core) by calling
of_platform_populate().

Do you see a benefit in the SDHCi implementation?

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


QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project