linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Dov Levenglick" <dovl@codeaurora.org>
To: "Rob Herring" <robherring2@gmail.com>
Cc: "Dov Levenglick" <dovl@codeaurora.org>,
	"Yaniv Gardi" <ygardi@codeaurora.org>,
	"Akinobu Mita" <akinobu.mita@gmail.com>,
	merez@qti.qualcomm.com, dovl@qti.qualcomm.com,
	"Jej B" <james.bottomley@hansenpartnership.com>,
	"LKML" <linux-kernel@vger.kernel.org>,
	linux-scsi@vger.kernel.org,
	"linux-arm-msm" <linux-arm-msm@vger.kernel.org>,
	"Santosh Y" <santoshsy@gmail.com>,
	linux-scsi-owner@vger.kernel.org,
	"Subhash Jadavani" <subhashj@codeaurora.org>,
	"Paul Bolle" <pebolle@tiscali.nl>,
	"Gilad Broner" <gbroner@codeaurora.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Pawel Moll" <pawel.moll@arm.com>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Ian Campbell" <ijc+devicetree@hellion.org.uk>,
	"Kumar Gala" <galak@codeaurora.org>,
	"Vinayak Holikatti" <vinholikatti@gmail.com>,
	"James E.J. Bottomley" <jbottomley@odin.com>,
	"Dolev Raviv" <draviv@codeaurora.org>,
	"Christoph Hellwig" <hch@lst.de>,
	"Sujit Reddy Thumma" <sthumma@codeaurora.org>,
	"Raviv Shvili" <rshvili@codeaurora.org>,
	"Sahitya Tummala" <stummala@codeaurora.org>,
	"open list:OPEN FIRMWARE AND..." <devicetree@vger.kernel.org>
Subject: Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
Date: Wed, 17 Jun 2015 07:42:12 -0000	[thread overview]
Message-ID: <6934f39953e011404dd5d39073e6ebba.squirrel@www.codeaurora.org> (raw)
In-Reply-To: <CAL_Jsq+SWq9=xTjVvQH2tMoq=g_rjk44A7T4TMFKvDiUp_ahCg@mail.gmail.com>

> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick <dovl@codeaurora.org>
> wrote:
>>> On Sun, Jun 7, 2015 at 10:32 AM,  <ygardi@codeaurora.org> wrote:
>>>>> 2015-06-05 5:53 GMT+09:00  <ygardi@codeaurora.org>:
>
> [...]
>
>>>> 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.

As to your concern, while I understand it and understand the underlying
logic, I don't think that it should outweigh the other considerations that
I outlined here.

Dov

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


  reply	other threads:[~2015-06-17  7:42 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-03  9:37 [PATCH v2 0/4] fixing building errors and warnings when components Yaniv Gardi
2015-06-03  9:37 ` [PATCH v2 1/4] phy: qcom-ufs: fix build error when the component is built as a module Yaniv Gardi
2015-06-03  9:37 ` [PATCH v2 2/4] scsi: ufs-qcom: fix compilation warning if compiled " Yaniv Gardi
2015-06-03  9:37 ` [PATCH v2 3/4] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component Yaniv Gardi
2015-06-03  9:37 ` [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device Yaniv Gardi
2015-06-04 14:07   ` Paul Bolle
2015-06-04 14:42     ` Paul Bolle
2015-06-04 20:42     ` ygardi
2015-06-07 15:22     ` ygardi
2015-06-04 14:32   ` Akinobu Mita
2015-06-04 20:53     ` ygardi
2015-06-05 16:47       ` Akinobu Mita
2015-06-07 15:32         ` ygardi
2015-06-08 14:47           ` Akinobu Mita
2015-06-08 15:02           ` Rob Herring
2015-06-09  5:53             ` Dov Levenglick
2015-06-09 12:53               ` Rob Herring
2015-06-17  7:42                 ` Dov Levenglick [this message]
2015-06-17 12:46                   ` Rob Herring
2015-06-17 13:17                     ` Dov Levenglick
2015-06-17 13:37                       ` Rob Herring
2015-06-17 14:21                         ` Dov Levenglick
2015-06-17 14:31                           ` James Bottomley
2015-06-17 14:38                             ` Dov Levenglick
2015-06-08 14:51     ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6934f39953e011404dd5d39073e6ebba.squirrel@www.codeaurora.org \
    --to=dovl@codeaurora.org \
    --cc=akinobu.mita@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dovl@qti.qualcomm.com \
    --cc=draviv@codeaurora.org \
    --cc=galak@codeaurora.org \
    --cc=gbroner@codeaurora.org \
    --cc=hch@lst.de \
    --cc=ijc+devicetree@hellion.org.uk \
    --cc=james.bottomley@hansenpartnership.com \
    --cc=jbottomley@odin.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi-owner@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=merez@qti.qualcomm.com \
    --cc=pawel.moll@arm.com \
    --cc=pebolle@tiscali.nl \
    --cc=robh+dt@kernel.org \
    --cc=robherring2@gmail.com \
    --cc=rshvili@codeaurora.org \
    --cc=santoshsy@gmail.com \
    --cc=sthumma@codeaurora.org \
    --cc=stummala@codeaurora.org \
    --cc=subhashj@codeaurora.org \
    --cc=vinholikatti@gmail.com \
    --cc=ygardi@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).