Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757260AbbFQOiZ (ORCPT ); Wed, 17 Jun 2015 10:38:25 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:58748 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757138AbbFQOiT (ORCPT ); Wed, 17 Jun 2015 10:38:19 -0400 Message-ID: <3c23cd4feb85680b17203975e48822de.squirrel@www.codeaurora.org> In-Reply-To: <1434551474.2222.23.camel@HansenPartnership.com> References: <1433324255-27510-1-git-send-email-ygardi@codeaurora.org> <1433324255-27510-5-git-send-email-ygardi@codeaurora.org> <763dbc7b708b5d5b18ce0b5adcc41016.squirrel@www.codeaurora.org> <68c91f08f2be6c84055a303ca8aa3fe0.squirrel@www.codeaurora.org> <6934f39953e011404dd5d39073e6ebba.squirrel@www.codeaurora.org> <1434551474.2222.23.camel@HansenPartnership.com> Date: Wed, 17 Jun 2015 14:38:16 -0000 Subject: Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device From: "Dov Levenglick" To: "James Bottomley" , "Rob Herring" Cc: "Dov Levenglick" , "Yaniv Gardi" , "Akinobu Mita" , "LKML" , linux-scsi@vger.kernel.org, "linux-arm-msm" , "Santosh Y" , linux-scsi-owner@vger.kernel.org, "Subhash Jadavani" , "Paul Bolle" , "Gilad Broner" , "Rob Herring" , "Pawel Moll" , "Mark Rutland" , "Ian Campbell" , "Kumar Gala" , "Vinayak Holikatti" , "Dolev Raviv" , "Christoph Hellwig" , "Sujit Reddy Thumma" , "Raviv Shvili" , "Sahitya Tummala" , "open list:OPEN FIRMWARE AND..." User-Agent: SquirrelMail/1.4.22-4.el6 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Priority: 3 (Normal) Importance: Normal Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8425 Lines: 231 > 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 >> > wrote: >> >>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick >> >> >>> wrote: >> >>>>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick >> >>>>> >> >>>>> wrote: >> >>>>>>> On Sun, Jun 7, 2015 at 10:32 AM, wrote: >> >>>>>>>>> 2015-06-05 5:53 GMT+09:00 : >> >>>>> >> >>>>> [...] >> >>>>> >> >>>>>>>> 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@vger.kernel.org >> > 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@vger.kernel.org >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/