> 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). 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-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
