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

[Dov Levenglick]

> 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@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-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/