* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-07 15:32 ` ygardi
0 siblings, 0 replies; 43+ messages in thread
From: ygardi @ 2015-06-07 15:32 UTC (permalink / raw)
To: Akinobu Mita
Cc: Yaniv Gardi, merez, dovl, Jej B, LKML, linux-scsi@vger.kernel.org,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
> 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.
2) you are right. the fix added as you suggested.
let us know your thoughts about the V3 once it's uploaded
thanks
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-07 15:32 ` ygardi
@ 2015-06-08 14:47 ` Akinobu Mita
-1 siblings, 0 replies; 43+ messages in thread
From: Akinobu Mita @ 2015-06-08 14:47 UTC (permalink / raw)
To: Yaniv Gardi
Cc: merez, dovl, Jej B, LKML, linux-scsi@vger.kernel.org,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala
2015-06-08 0:32 GMT+09:00 <ygardi@codeaurora.org>:
> 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.
I'm worrying the case ufshcd_pltfrm_probe() is called when ufshcd-pltfrm
module is installed but ufs-qcom module is _not_ installed yet, where
ufshcd-pltfrm and ufs-qcom are both built as loadable modules.
In this case, of_platform_populate() in ufshcd_pltfrm_probe() doesn't
invoke ufs-qcom probe, does it? So I suggested using deferred probe
infrastructure by returning -EPROBE_DEFER.
> 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.
>
> 2) you are right. the fix added as you suggested.
Thanks for fixing it. But a little more work is needed in v3,
I'll leave a comment to v3.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-08 14:47 ` Akinobu Mita
0 siblings, 0 replies; 43+ messages in thread
From: Akinobu Mita @ 2015-06-08 14:47 UTC (permalink / raw)
To: Yaniv Gardi
Cc: merez, dovl, Jej B, LKML, linux-scsi@vger.kernel.org,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
2015-06-08 0:32 GMT+09:00 <ygardi@codeaurora.org>:
> 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.
I'm worrying the case ufshcd_pltfrm_probe() is called when ufshcd-pltfrm
module is installed but ufs-qcom module is _not_ installed yet, where
ufshcd-pltfrm and ufs-qcom are both built as loadable modules.
In this case, of_platform_populate() in ufshcd_pltfrm_probe() doesn't
invoke ufs-qcom probe, does it? So I suggested using deferred probe
infrastructure by returning -EPROBE_DEFER.
> 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.
>
> 2) you are right. the fix added as you suggested.
Thanks for fixing it. But a little more work is needed in v3,
I'll leave a comment to v3.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-07 15:32 ` ygardi
@ 2015-06-08 15:02 ` Rob Herring
-1 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2015-06-08 15:02 UTC (permalink / raw)
To: ygardi
Cc: Akinobu Mita, merez, dovl, Jej B, LKML,
linux-scsi@vger.kernel.org, linux-arm-msm, Santosh Y,
linux-scsi-owner, Subhash Jadavani, Paul Bolle, Gilad Broner,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili
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.
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-08 15:02 ` Rob Herring
0 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2015-06-08 15:02 UTC (permalink / raw)
To: ygardi
Cc: Akinobu Mita, merez, dovl, Jej B, LKML,
linux-scsi@vger.kernel.org, linux-arm-msm, Santosh Y,
linux-scsi-owner, Subhash Jadavani, Paul Bolle, Gilad Broner,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
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.
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-08 15:02 ` Rob Herring
@ 2015-06-09 5:53 ` Dov Levenglick
-1 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-09 5:53 UTC (permalink / raw)
To: Rob Herring
Cc: ygardi, Akinobu Mita, merez, dovl, Jej B, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma
> 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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-09 5:53 ` Dov Levenglick
0 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-09 5:53 UTC (permalink / raw)
To: Rob Herring
Cc: ygardi, Akinobu Mita, merez, dovl, Jej B, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
> 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
^ permalink raw reply [flat|nested] 43+ messages in thread[parent not found: <bfade46f9d953e10240acb835105b81d.squirrel-mMfbam+mt9083fI46fginR2eb7JE58TQ@public.gmane.org>]
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-09 5:53 ` Dov Levenglick
@ 2015-06-09 12:53 ` Rob Herring
-1 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2015-06-09 12:53 UTC (permalink / raw)
To: Dov Levenglick
Cc: Yaniv Gardi, Akinobu Mita, merez-Rm6X0d1/PG5y9aJCnZT0Uw,
dovl-Rm6X0d1/PG5y9aJCnZT0Uw, Jej B, LKML,
linux-scsi-u79uwXL29TY76Z2rM5mHXA, linux-arm-msm, Santosh Y,
linux-scsi-owner-u79uwXL29TY76Z2rM5mHXA, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma,
Raviv Shvili <rshvili>
On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>>> 2015-06-05 5:53 GMT+09:00 <ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>:
[...]
>>> 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().
That binding has the same problem. Please don't propagate that. There
is no point in a sub-node in this case.
> Do you see a benefit in the SDHCi implementation?
Yes, it does not let the kernel driver design dictate the hardware description.
Rob
--
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-09 12:53 ` Rob Herring
0 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2015-06-09 12:53 UTC (permalink / raw)
To: Dov Levenglick
Cc: Yaniv Gardi, Akinobu Mita, merez, dovl, Jej B, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org> wrote:
>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
[...]
>>> 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().
That binding has the same problem. Please don't propagate that. There
is no point in a sub-node in this case.
> Do you see a benefit in the SDHCi implementation?
Yes, it does not let the kernel driver design dictate the hardware description.
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-09 12:53 ` Rob Herring
@ 2015-06-17 7:42 ` Dov Levenglick
-1 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-17 7:42 UTC (permalink / raw)
To: Rob Herring
Cc: Dov Levenglick, Yaniv Gardi, Akinobu Mita, merez, dovl, Jej B,
LKML, linux-scsi, linux-arm-msm, Santosh Y, linux-scsi-owner,
Subhash Jadavani, Paul Bolle, Gilad Broner, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig
> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
> wrote:
>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>
> [...]
>
>>>> 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().
>
> That binding has the same problem. Please don't propagate that. There
> is no point in a sub-node in this case.
>
>> Do you see a benefit in the SDHCi implementation?
>
> Yes, it does not let the kernel driver design dictate the hardware
> description.
>
> Rob
>
Hi Rob,
We appear to be having a philosophical disagreement on the practicality of
designing the ufshcd variant's implementation - in other words, we
disagree on the proper design pattern to follow here.
If I understand correctly, you are concerned with a design pattern wherein
a generic implementation is wrapped - at the device-tree level - in a
variant implementation. The main reason for your concern is that you don't
want the "kernel driver design dictate the hardware description".
We considered this point when we suggested our implementation (both before
and after you raised it) and reached the conclusion that - while an
important consideration - it should not be the prevailing one. I believe
that you will agree once you read the reasoning. What guided us was the
following:
1. Keep our change minimal.
2. Keep our patch in line with known design patterns in the kernel.
3. Have our patch extend the existing solution rather than reinvent it.
It is the 3rd point that is most important to this discussion, since UFS
has already been deployed by various vendors and is used by OEM. Changing
ufshcd to a set of library functions that would be called by variants
would necessarily introduce a significant change to the code flow in many
places and would pose a backward compatibility issue. By using the tried
and tested pattern of subnodes in the dts we were able to keep the change
simple, succinct, understandable, maintainable and backward compatible. In
fact, the entire logic tying of the generic implementation to the variant
takes ~20 lines of code - both short and elegant.
As to your concern, while I understand it and understand the underlying
logic, I don't think that it should outweigh the other considerations that
I outlined here.
Dov
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-17 7:42 ` Dov Levenglick
0 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-17 7:42 UTC (permalink / raw)
To: Rob Herring
Cc: Dov Levenglick, Yaniv Gardi, Akinobu Mita, merez, dovl, Jej B,
LKML, linux-scsi, linux-arm-msm, Santosh Y, linux-scsi-owner,
Subhash Jadavani, Paul Bolle, Gilad Broner, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
> wrote:
>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>
> [...]
>
>>>> 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().
>
> That binding has the same problem. Please don't propagate that. There
> is no point in a sub-node in this case.
>
>> Do you see a benefit in the SDHCi implementation?
>
> Yes, it does not let the kernel driver design dictate the hardware
> description.
>
> Rob
>
Hi Rob,
We appear to be having a philosophical disagreement on the practicality of
designing the ufshcd variant's implementation - in other words, we
disagree on the proper design pattern to follow here.
If I understand correctly, you are concerned with a design pattern wherein
a generic implementation is wrapped - at the device-tree level - in a
variant implementation. The main reason for your concern is that you don't
want the "kernel driver design dictate the hardware description".
We considered this point when we suggested our implementation (both before
and after you raised it) and reached the conclusion that - while an
important consideration - it should not be the prevailing one. I believe
that you will agree once you read the reasoning. What guided us was the
following:
1. Keep our change minimal.
2. Keep our patch in line with known design patterns in the kernel.
3. Have our patch extend the existing solution rather than reinvent it.
It is the 3rd point that is most important to this discussion, since UFS
has already been deployed by various vendors and is used by OEM. Changing
ufshcd to a set of library functions that would be called by variants
would necessarily introduce a significant change to the code flow in many
places and would pose a backward compatibility issue. By using the tried
and tested pattern of subnodes in the dts we were able to keep the change
simple, succinct, understandable, maintainable and backward compatible. In
fact, the entire logic tying of the generic implementation to the variant
takes ~20 lines of code - both short and elegant.
As to your concern, while I understand it and understand the underlying
logic, I don't think that it should outweigh the other considerations that
I outlined here.
Dov
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-17 7:42 ` Dov Levenglick
@ 2015-06-17 12:46 ` Rob Herring
-1 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2015-06-17 12:46 UTC (permalink / raw)
To: Dov Levenglick
Cc: Yaniv Gardi, Akinobu Mita, merez, dovl, Jej B, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma,
Raviv Shvili <rshvili>
On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org> wrote:
>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
>> wrote:
>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>>
>> [...]
>>
>>>>> 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().
>>
>> That binding has the same problem. Please don't propagate that. There
>> is no point in a sub-node in this case.
>>
>>> Do you see a benefit in the SDHCi implementation?
>>
>> Yes, it does not let the kernel driver design dictate the hardware
>> description.
>>
>> Rob
>>
>
> Hi Rob,
> We appear to be having a philosophical disagreement on the practicality of
> designing the ufshcd variant's implementation - in other words, we
> disagree on the proper design pattern to follow here.
> If I understand correctly, you are concerned with a design pattern wherein
> a generic implementation is wrapped - at the device-tree level - in a
> variant implementation. The main reason for your concern is that you don't
> want the "kernel driver design dictate the hardware description".
>
> We considered this point when we suggested our implementation (both before
> and after you raised it) and reached the conclusion that - while an
> important consideration - it should not be the prevailing one. I believe
> that you will agree once you read the reasoning. What guided us was the
> following:
> 1. Keep our change minimal.
> 2. Keep our patch in line with known design patterns in the kernel.
> 3. Have our patch extend the existing solution rather than reinvent it.
>
> It is the 3rd point that is most important to this discussion, since UFS
> has already been deployed by various vendors and is used by OEM. Changing
> ufshcd to a set of library functions that would be called by variants
> would necessarily introduce a significant change to the code flow in many
> places and would pose a backward compatibility issue. By using the tried
> and tested pattern of subnodes in the dts we were able to keep the change
> simple, succinct, understandable, maintainable and backward compatible. In
> fact, the entire logic tying of the generic implementation to the variant
> takes ~20 lines of code - both short and elegant.
The DWC3 binding does this and nothing else that I'm aware of. This
hardly makes for a common pattern. If you really want to split this to
2 devices, you can create platform devices without having a DT node.
If you want to convince me this is the right approach for the binding
then you need to convince me the h/w is actually split this way and
there is functionality separate from the licensed IP.
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-17 12:46 ` Rob Herring
0 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2015-06-17 12:46 UTC (permalink / raw)
To: Dov Levenglick
Cc: Yaniv Gardi, Akinobu Mita, merez, dovl, Jej B, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, James E.J. Bottomley,
Dolev Raviv, Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org> wrote:
>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
>> wrote:
>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>>
>> [...]
>>
>>>>> 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().
>>
>> That binding has the same problem. Please don't propagate that. There
>> is no point in a sub-node in this case.
>>
>>> Do you see a benefit in the SDHCi implementation?
>>
>> Yes, it does not let the kernel driver design dictate the hardware
>> description.
>>
>> Rob
>>
>
> Hi Rob,
> We appear to be having a philosophical disagreement on the practicality of
> designing the ufshcd variant's implementation - in other words, we
> disagree on the proper design pattern to follow here.
> If I understand correctly, you are concerned with a design pattern wherein
> a generic implementation is wrapped - at the device-tree level - in a
> variant implementation. The main reason for your concern is that you don't
> want the "kernel driver design dictate the hardware description".
>
> We considered this point when we suggested our implementation (both before
> and after you raised it) and reached the conclusion that - while an
> important consideration - it should not be the prevailing one. I believe
> that you will agree once you read the reasoning. What guided us was the
> following:
> 1. Keep our change minimal.
> 2. Keep our patch in line with known design patterns in the kernel.
> 3. Have our patch extend the existing solution rather than reinvent it.
>
> It is the 3rd point that is most important to this discussion, since UFS
> has already been deployed by various vendors and is used by OEM. Changing
> ufshcd to a set of library functions that would be called by variants
> would necessarily introduce a significant change to the code flow in many
> places and would pose a backward compatibility issue. By using the tried
> and tested pattern of subnodes in the dts we were able to keep the change
> simple, succinct, understandable, maintainable and backward compatible. In
> fact, the entire logic tying of the generic implementation to the variant
> takes ~20 lines of code - both short and elegant.
The DWC3 binding does this and nothing else that I'm aware of. This
hardly makes for a common pattern. If you really want to split this to
2 devices, you can create platform devices without having a DT node.
If you want to convince me this is the right approach for the binding
then you need to convince me the h/w is actually split this way and
there is functionality separate from the licensed IP.
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-17 12:46 ` Rob Herring
@ 2015-06-17 13:17 ` Dov Levenglick
-1 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-17 13:17 UTC (permalink / raw)
To: Rob Herring
Cc: Dov Levenglick, Yaniv Gardi, Akinobu Mita, Jej B, LKML,
linux-scsi, linux-arm-msm, Santosh Y, linux-scsi-owner,
Subhash Jadavani, Paul Bolle, Gilad Broner, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma
> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org>
> wrote:
>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
>>> wrote:
>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>>>
>>> [...]
>>>
>>>>>> 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().
>>>
>>> That binding has the same problem. Please don't propagate that. There
>>> is no point in a sub-node in this case.
>>>
>>>> Do you see a benefit in the SDHCi implementation?
>>>
>>> Yes, it does not let the kernel driver design dictate the hardware
>>> description.
>>>
>>> Rob
>>>
>>
>> Hi Rob,
>> We appear to be having a philosophical disagreement on the practicality
>> of
>> designing the ufshcd variant's implementation - in other words, we
>> disagree on the proper design pattern to follow here.
>> If I understand correctly, you are concerned with a design pattern
>> wherein
>> a generic implementation is wrapped - at the device-tree level - in a
>> variant implementation. The main reason for your concern is that you
>> don't
>> want the "kernel driver design dictate the hardware description".
>>
>> We considered this point when we suggested our implementation (both
>> before
>> and after you raised it) and reached the conclusion that - while an
>> important consideration - it should not be the prevailing one. I believe
>> that you will agree once you read the reasoning. What guided us was the
>> following:
>> 1. Keep our change minimal.
>> 2. Keep our patch in line with known design patterns in the kernel.
>> 3. Have our patch extend the existing solution rather than reinvent it.
>>
>> It is the 3rd point that is most important to this discussion, since UFS
>> has already been deployed by various vendors and is used by OEM.
>> Changing
>> ufshcd to a set of library functions that would be called by variants
>> would necessarily introduce a significant change to the code flow in
>> many
>> places and would pose a backward compatibility issue. By using the tried
>> and tested pattern of subnodes in the dts we were able to keep the
>> change
>> simple, succinct, understandable, maintainable and backward compatible.
>> In
>> fact, the entire logic tying of the generic implementation to the
>> variant
>> takes ~20 lines of code - both short and elegant.
>
> The DWC3 binding does this and nothing else that I'm aware of. This
> hardly makes for a common pattern. If you really want to split this to
> 2 devices, you can create platform devices without having a DT node.
>
> If you want to convince me this is the right approach for the binding
> then you need to convince me the h/w is actually split this way and
> there is functionality separate from the licensed IP.
>
> Rob
>
I don't understand the challenge that you just posed. It is clear from our
implementation that there is the standard and variants thereof. I know
this to be a fact on the processors that we are working on.
Furthermore, although I didn't check each and every result in the search,
of_platform_populate is used in more locations than dwc3 and at least a
few of them seem have be using the same paradigm as ours
(http://lxr.free-electrons.com/ident?i=of_platform_populate).
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-17 13:17 ` Dov Levenglick
0 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-17 13:17 UTC (permalink / raw)
To: Rob Herring
Cc: Dov Levenglick, Yaniv Gardi, Akinobu Mita, Jej B, LKML,
linux-scsi, linux-arm-msm, Santosh Y, linux-scsi-owner,
Subhash Jadavani, Paul Bolle, Gilad Broner, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org>
> wrote:
>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
>>> wrote:
>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>>>
>>> [...]
>>>
>>>>>> 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().
>>>
>>> That binding has the same problem. Please don't propagate that. There
>>> is no point in a sub-node in this case.
>>>
>>>> Do you see a benefit in the SDHCi implementation?
>>>
>>> Yes, it does not let the kernel driver design dictate the hardware
>>> description.
>>>
>>> Rob
>>>
>>
>> Hi Rob,
>> We appear to be having a philosophical disagreement on the practicality
>> of
>> designing the ufshcd variant's implementation - in other words, we
>> disagree on the proper design pattern to follow here.
>> If I understand correctly, you are concerned with a design pattern
>> wherein
>> a generic implementation is wrapped - at the device-tree level - in a
>> variant implementation. The main reason for your concern is that you
>> don't
>> want the "kernel driver design dictate the hardware description".
>>
>> We considered this point when we suggested our implementation (both
>> before
>> and after you raised it) and reached the conclusion that - while an
>> important consideration - it should not be the prevailing one. I believe
>> that you will agree once you read the reasoning. What guided us was the
>> following:
>> 1. Keep our change minimal.
>> 2. Keep our patch in line with known design patterns in the kernel.
>> 3. Have our patch extend the existing solution rather than reinvent it.
>>
>> It is the 3rd point that is most important to this discussion, since UFS
>> has already been deployed by various vendors and is used by OEM.
>> Changing
>> ufshcd to a set of library functions that would be called by variants
>> would necessarily introduce a significant change to the code flow in
>> many
>> places and would pose a backward compatibility issue. By using the tried
>> and tested pattern of subnodes in the dts we were able to keep the
>> change
>> simple, succinct, understandable, maintainable and backward compatible.
>> In
>> fact, the entire logic tying of the generic implementation to the
>> variant
>> takes ~20 lines of code - both short and elegant.
>
> The DWC3 binding does this and nothing else that I'm aware of. This
> hardly makes for a common pattern. If you really want to split this to
> 2 devices, you can create platform devices without having a DT node.
>
> If you want to convince me this is the right approach for the binding
> then you need to convince me the h/w is actually split this way and
> there is functionality separate from the licensed IP.
>
> Rob
>
I don't understand the challenge that you just posed. It is clear from our
implementation that there is the standard and variants thereof. I know
this to be a fact on the processors that we are working on.
Furthermore, although I didn't check each and every result in the search,
of_platform_populate is used in more locations than dwc3 and at least a
few of them seem have be using the same paradigm as ours
(http://lxr.free-electrons.com/ident?i=of_platform_populate).
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-17 13:17 ` Dov Levenglick
@ 2015-06-17 13:37 ` Rob Herring
-1 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2015-06-17 13:37 UTC (permalink / raw)
To: Dov Levenglick
Cc: Yaniv Gardi, Akinobu Mita, Jej B, LKML, linux-scsi, linux-arm-msm,
Santosh Y, linux-scsi-owner, Subhash Jadavani, Paul Bolle,
Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala
On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick <dovl@codeaurora.org> wrote:
>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org>
>> wrote:
>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
>>>> wrote:
>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>>>>
>>>> [...]
>>>>
>>>>>>> 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().
>>>>
>>>> That binding has the same problem. Please don't propagate that. There
>>>> is no point in a sub-node in this case.
>>>>
>>>>> Do you see a benefit in the SDHCi implementation?
>>>>
>>>> Yes, it does not let the kernel driver design dictate the hardware
>>>> description.
>>>>
>>>> Rob
>>>>
>>>
>>> Hi Rob,
>>> We appear to be having a philosophical disagreement on the practicality
>>> of
>>> designing the ufshcd variant's implementation - in other words, we
>>> disagree on the proper design pattern to follow here.
>>> If I understand correctly, you are concerned with a design pattern
>>> wherein
>>> a generic implementation is wrapped - at the device-tree level - in a
>>> variant implementation. The main reason for your concern is that you
>>> don't
>>> want the "kernel driver design dictate the hardware description".
>>>
>>> We considered this point when we suggested our implementation (both
>>> before
>>> and after you raised it) and reached the conclusion that - while an
>>> important consideration - it should not be the prevailing one. I believe
>>> that you will agree once you read the reasoning. What guided us was the
>>> following:
>>> 1. Keep our change minimal.
>>> 2. Keep our patch in line with known design patterns in the kernel.
>>> 3. Have our patch extend the existing solution rather than reinvent it.
>>>
>>> It is the 3rd point that is most important to this discussion, since UFS
>>> has already been deployed by various vendors and is used by OEM.
>>> Changing
>>> ufshcd to a set of library functions that would be called by variants
>>> would necessarily introduce a significant change to the code flow in
>>> many
>>> places and would pose a backward compatibility issue. By using the tried
>>> and tested pattern of subnodes in the dts we were able to keep the
>>> change
>>> simple, succinct, understandable, maintainable and backward compatible.
>>> In
>>> fact, the entire logic tying of the generic implementation to the
>>> variant
>>> takes ~20 lines of code - both short and elegant.
>>
>> The DWC3 binding does this and nothing else that I'm aware of. This
>> hardly makes for a common pattern. If you really want to split this to
>> 2 devices, you can create platform devices without having a DT node.
>>
>> If you want to convince me this is the right approach for the binding
>> then you need to convince me the h/w is actually split this way and
>> there is functionality separate from the licensed IP.
>>
>> Rob
>>
>
> I don't understand the challenge that you just posed. It is clear from our
> implementation that there is the standard and variants thereof. I know
> this to be a fact on the processors that we are working on.
>
> Furthermore, although I didn't check each and every result in the search,
> of_platform_populate is used in more locations than dwc3 and at least a
> few of them seem have be using the same paradigm as ours
> (http://lxr.free-electrons.com/ident?i=of_platform_populate).
You can ignore everything under arch/ as those are root level calls.
Most of the rest of the list are devices which have multiple unrelated
functions such as PMICs or system controllers which are perfectly
valid uses of of_platform_populate. That leaves at most 10 examples
that may or may not have good bindings. 10 out of hundreds of drivers
in the kernel hardly makes for a pattern to follow.
Let me be perfectly clear on the binding as is: NAK. I'm done replying
until you propose something different.
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-17 13:37 ` Rob Herring
0 siblings, 0 replies; 43+ messages in thread
From: Rob Herring @ 2015-06-17 13:37 UTC (permalink / raw)
To: Dov Levenglick
Cc: Yaniv Gardi, Akinobu Mita, Jej B, LKML, linux-scsi, linux-arm-msm,
Santosh Y, linux-scsi-owner, Subhash Jadavani, Paul Bolle,
Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick <dovl@codeaurora.org> wrote:
>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org>
>> wrote:
>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
>>>> wrote:
>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>>>>
>>>> [...]
>>>>
>>>>>>> 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().
>>>>
>>>> That binding has the same problem. Please don't propagate that. There
>>>> is no point in a sub-node in this case.
>>>>
>>>>> Do you see a benefit in the SDHCi implementation?
>>>>
>>>> Yes, it does not let the kernel driver design dictate the hardware
>>>> description.
>>>>
>>>> Rob
>>>>
>>>
>>> Hi Rob,
>>> We appear to be having a philosophical disagreement on the practicality
>>> of
>>> designing the ufshcd variant's implementation - in other words, we
>>> disagree on the proper design pattern to follow here.
>>> If I understand correctly, you are concerned with a design pattern
>>> wherein
>>> a generic implementation is wrapped - at the device-tree level - in a
>>> variant implementation. The main reason for your concern is that you
>>> don't
>>> want the "kernel driver design dictate the hardware description".
>>>
>>> We considered this point when we suggested our implementation (both
>>> before
>>> and after you raised it) and reached the conclusion that - while an
>>> important consideration - it should not be the prevailing one. I believe
>>> that you will agree once you read the reasoning. What guided us was the
>>> following:
>>> 1. Keep our change minimal.
>>> 2. Keep our patch in line with known design patterns in the kernel.
>>> 3. Have our patch extend the existing solution rather than reinvent it.
>>>
>>> It is the 3rd point that is most important to this discussion, since UFS
>>> has already been deployed by various vendors and is used by OEM.
>>> Changing
>>> ufshcd to a set of library functions that would be called by variants
>>> would necessarily introduce a significant change to the code flow in
>>> many
>>> places and would pose a backward compatibility issue. By using the tried
>>> and tested pattern of subnodes in the dts we were able to keep the
>>> change
>>> simple, succinct, understandable, maintainable and backward compatible.
>>> In
>>> fact, the entire logic tying of the generic implementation to the
>>> variant
>>> takes ~20 lines of code - both short and elegant.
>>
>> The DWC3 binding does this and nothing else that I'm aware of. This
>> hardly makes for a common pattern. If you really want to split this to
>> 2 devices, you can create platform devices without having a DT node.
>>
>> If you want to convince me this is the right approach for the binding
>> then you need to convince me the h/w is actually split this way and
>> there is functionality separate from the licensed IP.
>>
>> Rob
>>
>
> I don't understand the challenge that you just posed. It is clear from our
> implementation that there is the standard and variants thereof. I know
> this to be a fact on the processors that we are working on.
>
> Furthermore, although I didn't check each and every result in the search,
> of_platform_populate is used in more locations than dwc3 and at least a
> few of them seem have be using the same paradigm as ours
> (http://lxr.free-electrons.com/ident?i=of_platform_populate).
You can ignore everything under arch/ as those are root level calls.
Most of the rest of the list are devices which have multiple unrelated
functions such as PMICs or system controllers which are perfectly
valid uses of of_platform_populate. That leaves at most 10 examples
that may or may not have good bindings. 10 out of hundreds of drivers
in the kernel hardly makes for a pattern to follow.
Let me be perfectly clear on the binding as is: NAK. I'm done replying
until you propose something different.
Rob
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-17 13:37 ` Rob Herring
@ 2015-06-17 14:21 ` Dov Levenglick
-1 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-17 14:21 UTC (permalink / raw)
To: James.Bottomley
Cc: Rob Herring, Dov Levenglick, Yaniv Gardi, Akinobu Mita, Jej B,
LKML, linux-scsi, linux-arm-msm, Santosh Y, linux-scsi-owner,
Subhash Jadavani, Paul Bolle, Gilad Broner, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig
Hi James,
Rob raises a point that we don't agree with. On the other hand, we are not
capable of convincing him in the validity of our approach - we are at an
impasse.
I would like to point out that our approach was reviewed by Paul and Mita
(external reviewers) and neither of them had the objection that Rob has.
I urge you to go over this thread, where both sides raised points and
argued their cases. We are going to need your call on this so that we can
move forward.
Thanks,
Dov
> On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick <dovl@codeaurora.org>
> wrote:
>>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org>
>>> wrote:
>>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
>>>>> wrote:
>>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> 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().
>>>>>
>>>>> That binding has the same problem. Please don't propagate that. There
>>>>> is no point in a sub-node in this case.
>>>>>
>>>>>> Do you see a benefit in the SDHCi implementation?
>>>>>
>>>>> Yes, it does not let the kernel driver design dictate the hardware
>>>>> description.
>>>>>
>>>>> Rob
>>>>>
>>>>
>>>> Hi Rob,
>>>> We appear to be having a philosophical disagreement on the
>>>> practicality
>>>> of
>>>> designing the ufshcd variant's implementation - in other words, we
>>>> disagree on the proper design pattern to follow here.
>>>> If I understand correctly, you are concerned with a design pattern
>>>> wherein
>>>> a generic implementation is wrapped - at the device-tree level - in a
>>>> variant implementation. The main reason for your concern is that you
>>>> don't
>>>> want the "kernel driver design dictate the hardware description".
>>>>
>>>> We considered this point when we suggested our implementation (both
>>>> before
>>>> and after you raised it) and reached the conclusion that - while an
>>>> important consideration - it should not be the prevailing one. I
>>>> believe
>>>> that you will agree once you read the reasoning. What guided us was
>>>> the
>>>> following:
>>>> 1. Keep our change minimal.
>>>> 2. Keep our patch in line with known design patterns in the kernel.
>>>> 3. Have our patch extend the existing solution rather than reinvent
>>>> it.
>>>>
>>>> It is the 3rd point that is most important to this discussion, since
>>>> UFS
>>>> has already been deployed by various vendors and is used by OEM.
>>>> Changing
>>>> ufshcd to a set of library functions that would be called by variants
>>>> would necessarily introduce a significant change to the code flow in
>>>> many
>>>> places and would pose a backward compatibility issue. By using the
>>>> tried
>>>> and tested pattern of subnodes in the dts we were able to keep the
>>>> change
>>>> simple, succinct, understandable, maintainable and backward
>>>> compatible.
>>>> In
>>>> fact, the entire logic tying of the generic implementation to the
>>>> variant
>>>> takes ~20 lines of code - both short and elegant.
>>>
>>> The DWC3 binding does this and nothing else that I'm aware of. This
>>> hardly makes for a common pattern. If you really want to split this to
>>> 2 devices, you can create platform devices without having a DT node.
>>>
>>> If you want to convince me this is the right approach for the binding
>>> then you need to convince me the h/w is actually split this way and
>>> there is functionality separate from the licensed IP.
>>>
>>> Rob
>>>
>>
>> I don't understand the challenge that you just posed. It is clear from
>> our
>> implementation that there is the standard and variants thereof. I know
>> this to be a fact on the processors that we are working on.
>>
>> Furthermore, although I didn't check each and every result in the
>> search,
>> of_platform_populate is used in more locations than dwc3 and at least a
>> few of them seem have be using the same paradigm as ours
>> (http://lxr.free-electrons.com/ident?i=of_platform_populate).
>
> You can ignore everything under arch/ as those are root level calls.
> Most of the rest of the list are devices which have multiple unrelated
> functions such as PMICs or system controllers which are perfectly
> valid uses of of_platform_populate. That leaves at most 10 examples
> that may or may not have good bindings. 10 out of hundreds of drivers
> in the kernel hardly makes for a pattern to follow.
>
> Let me be perfectly clear on the binding as is: NAK. I'm done replying
> until you propose something different.
>
> 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-17 14:21 ` Dov Levenglick
0 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-17 14:21 UTC (permalink / raw)
To: James.Bottomley
Cc: Rob Herring, Dov Levenglick, Yaniv Gardi, Akinobu Mita, Jej B,
LKML, linux-scsi, linux-arm-msm, Santosh Y, linux-scsi-owner,
Subhash Jadavani, Paul Bolle, Gilad Broner, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Vinayak Holikatti, James E.J. Bottomley, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
Hi James,
Rob raises a point that we don't agree with. On the other hand, we are not
capable of convincing him in the validity of our approach - we are at an
impasse.
I would like to point out that our approach was reviewed by Paul and Mita
(external reviewers) and neither of them had the objection that Rob has.
I urge you to go over this thread, where both sides raised points and
argued their cases. We are going to need your call on this so that we can
move forward.
Thanks,
Dov
> On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick <dovl@codeaurora.org>
> wrote:
>>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org>
>>> wrote:
>>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
>>>>> wrote:
>>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>>>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> 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().
>>>>>
>>>>> That binding has the same problem. Please don't propagate that. There
>>>>> is no point in a sub-node in this case.
>>>>>
>>>>>> Do you see a benefit in the SDHCi implementation?
>>>>>
>>>>> Yes, it does not let the kernel driver design dictate the hardware
>>>>> description.
>>>>>
>>>>> Rob
>>>>>
>>>>
>>>> Hi Rob,
>>>> We appear to be having a philosophical disagreement on the
>>>> practicality
>>>> of
>>>> designing the ufshcd variant's implementation - in other words, we
>>>> disagree on the proper design pattern to follow here.
>>>> If I understand correctly, you are concerned with a design pattern
>>>> wherein
>>>> a generic implementation is wrapped - at the device-tree level - in a
>>>> variant implementation. The main reason for your concern is that you
>>>> don't
>>>> want the "kernel driver design dictate the hardware description".
>>>>
>>>> We considered this point when we suggested our implementation (both
>>>> before
>>>> and after you raised it) and reached the conclusion that - while an
>>>> important consideration - it should not be the prevailing one. I
>>>> believe
>>>> that you will agree once you read the reasoning. What guided us was
>>>> the
>>>> following:
>>>> 1. Keep our change minimal.
>>>> 2. Keep our patch in line with known design patterns in the kernel.
>>>> 3. Have our patch extend the existing solution rather than reinvent
>>>> it.
>>>>
>>>> It is the 3rd point that is most important to this discussion, since
>>>> UFS
>>>> has already been deployed by various vendors and is used by OEM.
>>>> Changing
>>>> ufshcd to a set of library functions that would be called by variants
>>>> would necessarily introduce a significant change to the code flow in
>>>> many
>>>> places and would pose a backward compatibility issue. By using the
>>>> tried
>>>> and tested pattern of subnodes in the dts we were able to keep the
>>>> change
>>>> simple, succinct, understandable, maintainable and backward
>>>> compatible.
>>>> In
>>>> fact, the entire logic tying of the generic implementation to the
>>>> variant
>>>> takes ~20 lines of code - both short and elegant.
>>>
>>> The DWC3 binding does this and nothing else that I'm aware of. This
>>> hardly makes for a common pattern. If you really want to split this to
>>> 2 devices, you can create platform devices without having a DT node.
>>>
>>> If you want to convince me this is the right approach for the binding
>>> then you need to convince me the h/w is actually split this way and
>>> there is functionality separate from the licensed IP.
>>>
>>> Rob
>>>
>>
>> I don't understand the challenge that you just posed. It is clear from
>> our
>> implementation that there is the standard and variants thereof. I know
>> this to be a fact on the processors that we are working on.
>>
>> Furthermore, although I didn't check each and every result in the
>> search,
>> of_platform_populate is used in more locations than dwc3 and at least a
>> few of them seem have be using the same paradigm as ours
>> (http://lxr.free-electrons.com/ident?i=of_platform_populate).
>
> You can ignore everything under arch/ as those are root level calls.
> Most of the rest of the list are devices which have multiple unrelated
> functions such as PMICs or system controllers which are perfectly
> valid uses of of_platform_populate. That leaves at most 10 examples
> that may or may not have good bindings. 10 out of hundreds of drivers
> in the kernel hardly makes for a pattern to follow.
>
> Let me be perfectly clear on the binding as is: NAK. I'm done replying
> until you propose something different.
>
> 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-17 14:21 ` Dov Levenglick
@ 2015-06-17 14:31 ` James Bottomley
-1 siblings, 0 replies; 43+ messages in thread
From: James Bottomley @ 2015-06-17 14:31 UTC (permalink / raw)
To: Dov Levenglick
Cc: Rob Herring, Yaniv Gardi, Akinobu Mita, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala
On Wed, 2015-06-17 at 14:21 +0000, Dov Levenglick wrote:
> Hi James,
> Rob raises a point that we don't agree with. On the other hand, we are not
> capable of convincing him in the validity of our approach - we are at an
> impasse.
> I would like to point out that our approach was reviewed by Paul and Mita
> (external reviewers) and neither of them had the objection that Rob has.
> I urge you to go over this thread, where both sides raised points and
> argued their cases. We are going to need your call on this so that we can
> move forward.
The dispute is about device tree bindings. While I could override a NAK
in the subsystem I maintain, I'm not going to when it comes from the
maintainer of the device tree subsystem on a subject that's his province
of expertise, not mine.
Please agree on what the bindings should look like and then resubmit the
driver.
James
> Thanks,
> Dov
>
>
> > On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick <dovl@codeaurora.org>
> > wrote:
> >>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org>
> >>> wrote:
> >>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
> >>>>> wrote:
> >>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
> >>>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>>>> 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().
> >>>>>
> >>>>> That binding has the same problem. Please don't propagate that. There
> >>>>> is no point in a sub-node in this case.
> >>>>>
> >>>>>> Do you see a benefit in the SDHCi implementation?
> >>>>>
> >>>>> Yes, it does not let the kernel driver design dictate the hardware
> >>>>> description.
> >>>>>
> >>>>> Rob
> >>>>>
> >>>>
> >>>> Hi Rob,
> >>>> We appear to be having a philosophical disagreement on the
> >>>> practicality
> >>>> of
> >>>> designing the ufshcd variant's implementation - in other words, we
> >>>> disagree on the proper design pattern to follow here.
> >>>> If I understand correctly, you are concerned with a design pattern
> >>>> wherein
> >>>> a generic implementation is wrapped - at the device-tree level - in a
> >>>> variant implementation. The main reason for your concern is that you
> >>>> don't
> >>>> want the "kernel driver design dictate the hardware description".
> >>>>
> >>>> We considered this point when we suggested our implementation (both
> >>>> before
> >>>> and after you raised it) and reached the conclusion that - while an
> >>>> important consideration - it should not be the prevailing one. I
> >>>> believe
> >>>> that you will agree once you read the reasoning. What guided us was
> >>>> the
> >>>> following:
> >>>> 1. Keep our change minimal.
> >>>> 2. Keep our patch in line with known design patterns in the kernel.
> >>>> 3. Have our patch extend the existing solution rather than reinvent
> >>>> it.
> >>>>
> >>>> It is the 3rd point that is most important to this discussion, since
> >>>> UFS
> >>>> has already been deployed by various vendors and is used by OEM.
> >>>> Changing
> >>>> ufshcd to a set of library functions that would be called by variants
> >>>> would necessarily introduce a significant change to the code flow in
> >>>> many
> >>>> places and would pose a backward compatibility issue. By using the
> >>>> tried
> >>>> and tested pattern of subnodes in the dts we were able to keep the
> >>>> change
> >>>> simple, succinct, understandable, maintainable and backward
> >>>> compatible.
> >>>> In
> >>>> fact, the entire logic tying of the generic implementation to the
> >>>> variant
> >>>> takes ~20 lines of code - both short and elegant.
> >>>
> >>> The DWC3 binding does this and nothing else that I'm aware of. This
> >>> hardly makes for a common pattern. If you really want to split this to
> >>> 2 devices, you can create platform devices without having a DT node.
> >>>
> >>> If you want to convince me this is the right approach for the binding
> >>> then you need to convince me the h/w is actually split this way and
> >>> there is functionality separate from the licensed IP.
> >>>
> >>> Rob
> >>>
> >>
> >> I don't understand the challenge that you just posed. It is clear from
> >> our
> >> implementation that there is the standard and variants thereof. I know
> >> this to be a fact on the processors that we are working on.
> >>
> >> Furthermore, although I didn't check each and every result in the
> >> search,
> >> of_platform_populate is used in more locations than dwc3 and at least a
> >> few of them seem have be using the same paradigm as ours
> >> (http://lxr.free-electrons.com/ident?i=of_platform_populate).
> >
> > You can ignore everything under arch/ as those are root level calls.
> > Most of the rest of the list are devices which have multiple unrelated
> > functions such as PMICs or system controllers which are perfectly
> > valid uses of of_platform_populate. That leaves at most 10 examples
> > that may or may not have good bindings. 10 out of hundreds of drivers
> > in the kernel hardly makes for a pattern to follow.
> >
> > Let me be perfectly clear on the binding as is: NAK. I'm done replying
> > until you propose something different.
> >
> > 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
>
> --
> 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
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-17 14:31 ` James Bottomley
0 siblings, 0 replies; 43+ messages in thread
From: James Bottomley @ 2015-06-17 14:31 UTC (permalink / raw)
To: Dov Levenglick
Cc: Rob Herring, Yaniv Gardi, Akinobu Mita, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
On Wed, 2015-06-17 at 14:21 +0000, Dov Levenglick wrote:
> Hi James,
> Rob raises a point that we don't agree with. On the other hand, we are not
> capable of convincing him in the validity of our approach - we are at an
> impasse.
> I would like to point out that our approach was reviewed by Paul and Mita
> (external reviewers) and neither of them had the objection that Rob has.
> I urge you to go over this thread, where both sides raised points and
> argued their cases. We are going to need your call on this so that we can
> move forward.
The dispute is about device tree bindings. While I could override a NAK
in the subsystem I maintain, I'm not going to when it comes from the
maintainer of the device tree subsystem on a subject that's his province
of expertise, not mine.
Please agree on what the bindings should look like and then resubmit the
driver.
James
> Thanks,
> Dov
>
>
> > On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick <dovl@codeaurora.org>
> > wrote:
> >>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@codeaurora.org>
> >>> wrote:
> >>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
> >>>>> wrote:
> >>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
> >>>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>>>> 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().
> >>>>>
> >>>>> That binding has the same problem. Please don't propagate that. There
> >>>>> is no point in a sub-node in this case.
> >>>>>
> >>>>>> Do you see a benefit in the SDHCi implementation?
> >>>>>
> >>>>> Yes, it does not let the kernel driver design dictate the hardware
> >>>>> description.
> >>>>>
> >>>>> Rob
> >>>>>
> >>>>
> >>>> Hi Rob,
> >>>> We appear to be having a philosophical disagreement on the
> >>>> practicality
> >>>> of
> >>>> designing the ufshcd variant's implementation - in other words, we
> >>>> disagree on the proper design pattern to follow here.
> >>>> If I understand correctly, you are concerned with a design pattern
> >>>> wherein
> >>>> a generic implementation is wrapped - at the device-tree level - in a
> >>>> variant implementation. The main reason for your concern is that you
> >>>> don't
> >>>> want the "kernel driver design dictate the hardware description".
> >>>>
> >>>> We considered this point when we suggested our implementation (both
> >>>> before
> >>>> and after you raised it) and reached the conclusion that - while an
> >>>> important consideration - it should not be the prevailing one. I
> >>>> believe
> >>>> that you will agree once you read the reasoning. What guided us was
> >>>> the
> >>>> following:
> >>>> 1. Keep our change minimal.
> >>>> 2. Keep our patch in line with known design patterns in the kernel.
> >>>> 3. Have our patch extend the existing solution rather than reinvent
> >>>> it.
> >>>>
> >>>> It is the 3rd point that is most important to this discussion, since
> >>>> UFS
> >>>> has already been deployed by various vendors and is used by OEM.
> >>>> Changing
> >>>> ufshcd to a set of library functions that would be called by variants
> >>>> would necessarily introduce a significant change to the code flow in
> >>>> many
> >>>> places and would pose a backward compatibility issue. By using the
> >>>> tried
> >>>> and tested pattern of subnodes in the dts we were able to keep the
> >>>> change
> >>>> simple, succinct, understandable, maintainable and backward
> >>>> compatible.
> >>>> In
> >>>> fact, the entire logic tying of the generic implementation to the
> >>>> variant
> >>>> takes ~20 lines of code - both short and elegant.
> >>>
> >>> The DWC3 binding does this and nothing else that I'm aware of. This
> >>> hardly makes for a common pattern. If you really want to split this to
> >>> 2 devices, you can create platform devices without having a DT node.
> >>>
> >>> If you want to convince me this is the right approach for the binding
> >>> then you need to convince me the h/w is actually split this way and
> >>> there is functionality separate from the licensed IP.
> >>>
> >>> Rob
> >>>
> >>
> >> I don't understand the challenge that you just posed. It is clear from
> >> our
> >> implementation that there is the standard and variants thereof. I know
> >> this to be a fact on the processors that we are working on.
> >>
> >> Furthermore, although I didn't check each and every result in the
> >> search,
> >> of_platform_populate is used in more locations than dwc3 and at least a
> >> few of them seem have be using the same paradigm as ours
> >> (http://lxr.free-electrons.com/ident?i=of_platform_populate).
> >
> > You can ignore everything under arch/ as those are root level calls.
> > Most of the rest of the list are devices which have multiple unrelated
> > functions such as PMICs or system controllers which are perfectly
> > valid uses of of_platform_populate. That leaves at most 10 examples
> > that may or may not have good bindings. 10 out of hundreds of drivers
> > in the kernel hardly makes for a pattern to follow.
> >
> > Let me be perfectly clear on the binding as is: NAK. I'm done replying
> > until you propose something different.
> >
> > 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
>
> --
> 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
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-17 14:31 ` James Bottomley
@ 2015-06-17 14:38 ` Dov Levenglick
-1 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-17 14:38 UTC (permalink / raw)
To: James Bottomley, Rob Herring
Cc: Dov Levenglick, Yaniv Gardi, Akinobu Mita, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala
> On Wed, 2015-06-17 at 14:21 +0000, Dov Levenglick wrote:
>> Hi James,
>> Rob raises a point that we don't agree with. On the other hand, we are
>> not
>> capable of convincing him in the validity of our approach - we are at an
>> impasse.
>> I would like to point out that our approach was reviewed by Paul and
>> Mita
>> (external reviewers) and neither of them had the objection that Rob has.
>> I urge you to go over this thread, where both sides raised points and
>> argued their cases. We are going to need your call on this so that we
>> can
>> move forward.
>
> The dispute is about device tree bindings. While I could override a NAK
> in the subsystem I maintain, I'm not going to when it comes from the
> maintainer of the device tree subsystem on a subject that's his province
> of expertise, not mine.
>
> Please agree on what the bindings should look like and then resubmit the
> driver.
>
> James
>
Hi James & Rob,
Until this point I thought that this was a disagreement within the
confines of the scsi list. I was not aware of Rob's position as a
maintainer of the device tree subsystem.
We will take this offline with Rob and come back with a solution that
works for everyone.
Thanks,
Dov
>
>> Thanks,
>> Dov
>>
>>
>> > On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick <dovl@codeaurora.org>
>> > wrote:
>> >>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick
>> <dovl@codeaurora.org>
>> >>> wrote:
>> >>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick
>> >>>>> <dovl@codeaurora.org>
>> >>>>> wrote:
>> >>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>> >>>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>> >>>>>
>> >>>>> [...]
>> >>>>>
>> >>>>>>>> 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().
>> >>>>>
>> >>>>> That binding has the same problem. Please don't propagate that.
>> >>>>> There
>> >>>>> is no point in a sub-node in this case.
>> >>>>>
>> >>>>>> Do you see a benefit in the SDHCi implementation?
>> >>>>>
>> >>>>> Yes, it does not let the kernel driver design dictate the hardware
>> >>>>> description.
>> >>>>>
>> >>>>> Rob
>> >>>>>
>> >>>>
>> >>>> Hi Rob,
>> >>>> We appear to be having a philosophical disagreement on the
>> >>>> practicality
>> >>>> of
>> >>>> designing the ufshcd variant's implementation - in other words, we
>> >>>> disagree on the proper design pattern to follow here.
>> >>>> If I understand correctly, you are concerned with a design pattern
>> >>>> wherein
>> >>>> a generic implementation is wrapped - at the device-tree level - in
>> a
>> >>>> variant implementation. The main reason for your concern is that
>> you
>> >>>> don't
>> >>>> want the "kernel driver design dictate the hardware description".
>> >>>>
>> >>>> We considered this point when we suggested our implementation (both
>> >>>> before
>> >>>> and after you raised it) and reached the conclusion that - while an
>> >>>> important consideration - it should not be the prevailing one. I
>> >>>> believe
>> >>>> that you will agree once you read the reasoning. What guided us was
>> >>>> the
>> >>>> following:
>> >>>> 1. Keep our change minimal.
>> >>>> 2. Keep our patch in line with known design patterns in the kernel.
>> >>>> 3. Have our patch extend the existing solution rather than reinvent
>> >>>> it.
>> >>>>
>> >>>> It is the 3rd point that is most important to this discussion,
>> since
>> >>>> UFS
>> >>>> has already been deployed by various vendors and is used by OEM.
>> >>>> Changing
>> >>>> ufshcd to a set of library functions that would be called by
>> variants
>> >>>> would necessarily introduce a significant change to the code flow
>> in
>> >>>> many
>> >>>> places and would pose a backward compatibility issue. By using the
>> >>>> tried
>> >>>> and tested pattern of subnodes in the dts we were able to keep the
>> >>>> change
>> >>>> simple, succinct, understandable, maintainable and backward
>> >>>> compatible.
>> >>>> In
>> >>>> fact, the entire logic tying of the generic implementation to the
>> >>>> variant
>> >>>> takes ~20 lines of code - both short and elegant.
>> >>>
>> >>> The DWC3 binding does this and nothing else that I'm aware of. This
>> >>> hardly makes for a common pattern. If you really want to split this
>> to
>> >>> 2 devices, you can create platform devices without having a DT node.
>> >>>
>> >>> If you want to convince me this is the right approach for the
>> binding
>> >>> then you need to convince me the h/w is actually split this way and
>> >>> there is functionality separate from the licensed IP.
>> >>>
>> >>> Rob
>> >>>
>> >>
>> >> I don't understand the challenge that you just posed. It is clear
>> from
>> >> our
>> >> implementation that there is the standard and variants thereof. I
>> know
>> >> this to be a fact on the processors that we are working on.
>> >>
>> >> Furthermore, although I didn't check each and every result in the
>> >> search,
>> >> of_platform_populate is used in more locations than dwc3 and at least
>> a
>> >> few of them seem have be using the same paradigm as ours
>> >> (http://lxr.free-electrons.com/ident?i=of_platform_populate).
>> >
>> > You can ignore everything under arch/ as those are root level calls.
>> > Most of the rest of the list are devices which have multiple unrelated
>> > functions such as PMICs or system controllers which are perfectly
>> > valid uses of of_platform_populate. That leaves at most 10 examples
>> > that may or may not have good bindings. 10 out of hundreds of drivers
>> > in the kernel hardly makes for a pattern to follow.
>> >
>> > Let me be perfectly clear on the binding as is: NAK. I'm done replying
>> > until you propose something different.
>> >
>> > 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
>>
>> --
>> 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
@ 2015-06-17 14:38 ` Dov Levenglick
0 siblings, 0 replies; 43+ messages in thread
From: Dov Levenglick @ 2015-06-17 14:38 UTC (permalink / raw)
To: James Bottomley, Rob Herring
Cc: Dov Levenglick, Yaniv Gardi, Akinobu Mita, LKML, linux-scsi,
linux-arm-msm, Santosh Y, linux-scsi-owner, Subhash Jadavani,
Paul Bolle, Gilad Broner, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Vinayak Holikatti, Dolev Raviv,
Christoph Hellwig, Sujit Reddy Thumma, Raviv Shvili,
Sahitya Tummala, open list:OPEN FIRMWARE AND...
> On Wed, 2015-06-17 at 14:21 +0000, Dov Levenglick wrote:
>> Hi James,
>> Rob raises a point that we don't agree with. On the other hand, we are
>> not
>> capable of convincing him in the validity of our approach - we are at an
>> impasse.
>> I would like to point out that our approach was reviewed by Paul and
>> Mita
>> (external reviewers) and neither of them had the objection that Rob has.
>> I urge you to go over this thread, where both sides raised points and
>> argued their cases. We are going to need your call on this so that we
>> can
>> move forward.
>
> The dispute is about device tree bindings. While I could override a NAK
> in the subsystem I maintain, I'm not going to when it comes from the
> maintainer of the device tree subsystem on a subject that's his province
> of expertise, not mine.
>
> Please agree on what the bindings should look like and then resubmit the
> driver.
>
> James
>
Hi James & Rob,
Until this point I thought that this was a disagreement within the
confines of the scsi list. I was not aware of Rob's position as a
maintainer of the device tree subsystem.
We will take this offline with Rob and come back with a solution that
works for everyone.
Thanks,
Dov
>
>> Thanks,
>> Dov
>>
>>
>> > On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick <dovl@codeaurora.org>
>> > wrote:
>> >>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick
>> <dovl@codeaurora.org>
>> >>> wrote:
>> >>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick
>> >>>>> <dovl@codeaurora.org>
>> >>>>> wrote:
>> >>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, <ygardi@codeaurora.org> wrote:
>> >>>>>>>>> 2015-06-05 5:53 GMT+09:00 <ygardi@codeaurora.org>:
>> >>>>>
>> >>>>> [...]
>> >>>>>
>> >>>>>>>> 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().
>> >>>>>
>> >>>>> That binding has the same problem. Please don't propagate that.
>> >>>>> There
>> >>>>> is no point in a sub-node in this case.
>> >>>>>
>> >>>>>> Do you see a benefit in the SDHCi implementation?
>> >>>>>
>> >>>>> Yes, it does not let the kernel driver design dictate the hardware
>> >>>>> description.
>> >>>>>
>> >>>>> Rob
>> >>>>>
>> >>>>
>> >>>> Hi Rob,
>> >>>> We appear to be having a philosophical disagreement on the
>> >>>> practicality
>> >>>> of
>> >>>> designing the ufshcd variant's implementation - in other words, we
>> >>>> disagree on the proper design pattern to follow here.
>> >>>> If I understand correctly, you are concerned with a design pattern
>> >>>> wherein
>> >>>> a generic implementation is wrapped - at the device-tree level - in
>> a
>> >>>> variant implementation. The main reason for your concern is that
>> you
>> >>>> don't
>> >>>> want the "kernel driver design dictate the hardware description".
>> >>>>
>> >>>> We considered this point when we suggested our implementation (both
>> >>>> before
>> >>>> and after you raised it) and reached the conclusion that - while an
>> >>>> important consideration - it should not be the prevailing one. I
>> >>>> believe
>> >>>> that you will agree once you read the reasoning. What guided us was
>> >>>> the
>> >>>> following:
>> >>>> 1. Keep our change minimal.
>> >>>> 2. Keep our patch in line with known design patterns in the kernel.
>> >>>> 3. Have our patch extend the existing solution rather than reinvent
>> >>>> it.
>> >>>>
>> >>>> It is the 3rd point that is most important to this discussion,
>> since
>> >>>> UFS
>> >>>> has already been deployed by various vendors and is used by OEM.
>> >>>> Changing
>> >>>> ufshcd to a set of library functions that would be called by
>> variants
>> >>>> would necessarily introduce a significant change to the code flow
>> in
>> >>>> many
>> >>>> places and would pose a backward compatibility issue. By using the
>> >>>> tried
>> >>>> and tested pattern of subnodes in the dts we were able to keep the
>> >>>> change
>> >>>> simple, succinct, understandable, maintainable and backward
>> >>>> compatible.
>> >>>> In
>> >>>> fact, the entire logic tying of the generic implementation to the
>> >>>> variant
>> >>>> takes ~20 lines of code - both short and elegant.
>> >>>
>> >>> The DWC3 binding does this and nothing else that I'm aware of. This
>> >>> hardly makes for a common pattern. If you really want to split this
>> to
>> >>> 2 devices, you can create platform devices without having a DT node.
>> >>>
>> >>> If you want to convince me this is the right approach for the
>> binding
>> >>> then you need to convince me the h/w is actually split this way and
>> >>> there is functionality separate from the licensed IP.
>> >>>
>> >>> Rob
>> >>>
>> >>
>> >> I don't understand the challenge that you just posed. It is clear
>> from
>> >> our
>> >> implementation that there is the standard and variants thereof. I
>> know
>> >> this to be a fact on the processors that we are working on.
>> >>
>> >> Furthermore, although I didn't check each and every result in the
>> >> search,
>> >> of_platform_populate is used in more locations than dwc3 and at least
>> a
>> >> few of them seem have be using the same paradigm as ours
>> >> (http://lxr.free-electrons.com/ident?i=of_platform_populate).
>> >
>> > You can ignore everything under arch/ as those are root level calls.
>> > Most of the rest of the list are devices which have multiple unrelated
>> > functions such as PMICs or system controllers which are perfectly
>> > valid uses of of_platform_populate. That leaves at most 10 examples
>> > that may or may not have good bindings. 10 out of hundreds of drivers
>> > in the kernel hardly makes for a pattern to follow.
>> >
>> > Let me be perfectly clear on the binding as is: NAK. I'm done replying
>> > until you propose something different.
>> >
>> > 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
>>
>> --
>> 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
^ permalink raw reply [flat|nested] 43+ messages in thread