Re: [PATCH 01/23] devicetree: bindings: hisi_sas: add v2 HW bindings

From: John Garry <john.garry@huawei.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: <devicetree@vger.kernel.org>, <martin.petersen@oracle.com>,
	<pawel.moll@arm.com>, <ijc+devicetree@hellion.org.uk>,
	<JBottomley@odin.com>, <john.garry2@mail.dcu.ie>,
	<linuxarm@huawei.com>, <linux-kernel@vger.kernel.org>,
	<linux-scsi@vger.kernel.org>, <robh+dt@kernel.org>,
	<arnd@arndb.de>, <galak@codeaurora.org>,
	<zhangfei.gao@linaro.org>
Subject: Re: [PATCH 01/23] devicetree: bindings: hisi_sas: add v2 HW bindings
Date: Tue, 12 Jan 2016 12:16:34 +0000	[thread overview]
Message-ID: <5694EEA2.8060000@huawei.com> (raw)
In-Reply-To: <5693B59A.6000705@huawei.com>

On 11/01/2016 14:00, John Garry wrote:
>>> This is a specific issue for hip06 chipset.
>>
>> Ok. So is this:
>>
>> * a bug within the SAS controller in hip06, or:
>>
>> * a requirement/bug of an endpoint attached to the controller, or:
>>
>> * a requirement/bug of some interconnect between the controller and
>>    endpoint, or:
>>
>> * some other integration bug?
>>
>
> This is related to how the SAS controller IP was integrated into the
> chip. It is related to how many bursts are permitted for this controller
> on the AXI bus.
>
>> Please describe what the issue is that you're trying to work around, not
>> only your solution to it.
>>
>>> There is a bug in the HW on hip06 where controller #1 has to set to 2
>>> registers to non-default values to limit "am-max-transmissions".
>>
>> I got that. However, I have no idea what "am-max-transmissions" is, no
>> idea why you need to limit it (hopefully you can describe that a little
>> better above), nor what the semantics are for "limit".
>>
>
> So am-max-transmissions is a SW configurable feature of the controller.
>  From a high-level, it means how many commands we can send in parallel
> to the end device(s) without waiting for a response. It is dependent on
> the chip bus design.
>
>> The description of the property is an imperative, which reads like a
>> description of a specific driver behaviour rather than a property of the
>> hardware that leads to that behaviour being necessary. That's a warning
>> sign that the property is ill-defined, and we may encounter problems in
>> future due to changes in Linux.
>>
>
> Agreed.
>
>> Without knowing _why_ it's necessary to limit this, it's not possible to
>> know if the property is both necessary and sufficient to solve the
>> problem such that it doesn't rear its ugly head in future.
>>
>> For example, if this is simply one way to work around a hip06-specific
>> integration bug that we cannot imagine occurring elsewhere, it may be
>> better to instead key off of a platform-specific compatible string for
>> the v2 SAS controller in hip06. That gives us more freedom to apply
>> different workarounds if we have to.
>>
>> I see that the presence of this property will cause the driver to writes
>> hard-coded values two two registers. Not knowing the format of those
>> registers, their default values, nor how they respond to writes, I can't
>> tell:
>>
>
> As for writing hardcoded values, by default the related registers will
> hold 0x40, which means we can have upto 64 outstanding requests on this
> controller. Due to controller #1 integration restrictions, we can only
> issue 32 requests.
>
>> * If the writes have other effects.
>>
>> * If the limit is a single bit being flipped (i.e. this is a boolean in
>>    hardware too).
>>
>> * If the limit is some arbitrary chosen value which is not described in
>>    the property or the binding, nor what that value is. If we encounter a
>>    similar bug requiring a different bound in future, it may be
>>    problematic to have chosen an arbitrary fixed value, and it may make
>>    more sense to describe the value in the DT.
>>
>> So, please:
>>
>> * Update the DT property description to describe the specific HW issue
>>    that needs to be worked around, with a full description in the commit
>>    message.
>>
>> * Add a comment to the driver to explain what the effect of the register
>>    writes is intended to be, i.e. what value am max transmissions is
>>    being set to, and why that value isn't arbitrary.
>>
>
> As I understand, there are no more restictions/special requirements for
> controller #1. This v2 controller IP will be used in other chips, so we
> may have this issue again - I am seeking information from HW people. As
> such, it may be useful to know this info before decided on how this is
> decribed in the DT.

This issue is specific to only SAS controller #1 in hip06. The 
controller is designed to permit upto 64, but, due to chip bus design, 
this specific controller is limited to support 32.
So, after considering this, would a platform-specific compatible string 
be a better way of decribing this specific controller?

>
>>> This would not be a common SAS/SCSI controller property and is
>>> specific to our HW.
>>
>> Ok.
>>
>> Thanks,
>> Mark.
>>
>
thanks,
John

> _______________________________________________
> linuxarm mailing list
> linuxarm@huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm