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

[Dov Levenglick]

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@xxxxxxxxxxxxxx>
> wrote:
>>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick <dovl@xxxxxxxxxxxxxx>
>>> wrote:
>>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@xxxxxxxxxxxxxx>
>>>>> wrote:
>>>>>>> On Sun, Jun 7, 2015 at 10:32 AM,  <ygardi@xxxxxxxxxxxxxx> wrote:
>>>>>>>>> 2015-06-05 5:53 GMT+09:00  <ygardi@xxxxxxxxxxxxxx>:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> 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@xxxxxxxxxxxxxxx
> 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/