Re: [PATCH 3/3] add support for DWC UFS Host Controller

From: Joao Pinto <Joao.Pinto@synopsys.com>
To: Mark Rutland <mark.rutland@arm.com>, Arnd Bergmann <arnd@arndb.de>
Cc: Joao Pinto <Joao.Pinto@synopsys.com>, <santosh.sy@samsung.com>,
	<h.vinayak@samsung.com>, <julian.calaby@gmail.com>,
	<akinobu.mita@gmail.com>, <hch@infradead.org>,
	<gbroner@codeaurora.org>, <subhashj@codeaurora.org>,
	<CARLOS.PALMINHA@synopsys.com>, <ijc+devicetree@hellion.org.uk>,
	<linux-kernel@vger.kernel.org>, <linux-scsi@vger.kernel.org>,
	<devicetree@vger.kernel.org>
Subject: Re: [PATCH 3/3] add support for DWC UFS Host Controller
Date: Mon, 8 Feb 2016 15:17:11 +0000	[thread overview]
Message-ID: <56B8B177.2050000@synopsys.com> (raw)
In-Reply-To: <20160204162700.GD17587@leverpostej>

Hi Mark and Arnd,

I am planning the v2 of this patch set. I have a doubt in the version
compatibility strings... The core driver must support the UFS 2.0 controller and
this patch set includes a patch that adds 2.0 capabilities to it. The core
driver can get from the controller's version and with that use or not a specific
2.0 feature.

What would be the real added-value of having a compatibility string like
"snps,ufshcd-1.1" and "snps,ufshcd-2.0" if the driver can perform as 2.0 if it
detects a 2.0 controller? Are you saying that a user that puts "snps,ufshcd-1.1"
in the DT compatibility string disables the UFS 2.0 in the core driver despite
the controller is 2.0? Please clarify.

Thanks,
Joao

On 2/4/2016 4:27 PM, Mark Rutland wrote:
> On Wed, Feb 03, 2016 at 03:54:48PM +0000, Joao Pinto wrote:
>> Hi,
>>
>> On 2/3/2016 3:39 PM, Arnd Bergmann wrote:
>>> On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote:
>>>>
>>>> Hi Arnd,
>>>>
>>>> On 2/3/2016 12:54 PM, Arnd Bergmann wrote:
>>>>> On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote:
>>>>>>
>>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>>>>>
>>>>> This needs a changelog comment, like every patch.
>>>>>
>>>>>> @@ -0,0 +1,16 @@
>>>>>> +* Universal Flash Storage (UFS) DesignWare Host Controller
>>>>>> +
>>>>>> +DWC_UFSHC nodes are defined to describe on-chip UFS host controllers.
>>>>>> +Each UFS controller instance should have its own node.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible        : compatible list, contains "snps,ufshcd"
>>>>>
>>>>> Are there multiple versions of this controller? Usually for designware
>>>>> parts the version is known, so we should document which versions exist
>>>>
>>>> This controller recent releases was 2.0, but we released last year 1.1. The
>>>> driver works with both. The driver must work with all DWC UFS versions.
>>>
>>> Ok, then make the driver match on the "snps,ufshcd-1.1" compatible
>>> string, but document both strings in the binding document, and make
>>> it mandatory to specify the 1.1 version as a compatible fallback.
>>>
>>> If we ever need to handle a quirk for the 2.0 version then, it can
>>> easily be done.
>>
>> We need the driver to support UFS 2.0 because it is our latest release and is
>> the done that Synopsys is focused now. We could call it "snps, ufshcd-2.0" then.
>> What do you think?
> 
> Arnd's point was that the driver can handle only "snps,ufshcd-1.1" for
> now, and in your DT you can have:
> 
> 	compatible = "snps,ufshcd-2.0", "snps,ufshcd-1.1";
> 
> That allows driver to handle 2.0 and 1.1 without knowing anything about
> 2.0 for now. If in future the two need to be handled differently we can
> update the driver to explicitly match "snps,ufshcd-2.0".
> 
> Regardless, both compatible string should go in the documentation, and
> it should be explicitly mentioned that "snps,ufshcd-1.1" should be used
> as a fallback entry.
> 
> Mark.
> 