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