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

[John Garry]

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

Cheers,
John