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

quoted

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.

quoted

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.

quoted

This would not be a common SAS/SCSI controller property and is
specific to our HW.

Ok.

Thanks,
Mark.

Cheers,
John