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

From: John Garry
Date: Mon Jan 11 2016 - 09:02:18 EST

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 would not be a common SAS/SCSI controller property and is
specific to our HW.