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@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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
