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

[John Garry]

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@xxxxxxxxxx
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm