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

From: John Garry
Date: Tue Jan 12 2016 - 07:17:59 EST

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.


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

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

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




linuxarm mailing list