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

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

> 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