Date:	Fri, 8 Jan 2016 16:49:10 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	John Garry <john.garry@...wei.com>
Cc:	JBottomley@...n.com, martin.petersen@...cle.com,
	robh+dt@...nel.org, pawel.moll@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	linuxarm@...wei.com, zhangfei.gao@...aro.org, xuwei5@...ilicon.com,
	john.garry2@...l.dcu.ie, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org, arnd@...db.de,
	devicetree@...r.kernel.org
Subject: Re: [PATCH 01/23] devicetree: bindings: hisi_sas: add v2 HW bindings

On Fri, Jan 08, 2016 at 03:34:37PM +0000, John Garry wrote:
> 
> >>>>+Optional main node properties:
> >>>>+ - am-max-trans : limit controller for am max transmissions
> >>>
> >>>Is this a boolean? Number?
> >>>
> >>
> >>This is a boolean. It is for dealing with a quirk in the chipset: an
> >>instance of the controller in the hip06 chipset requires registers
> >>set with a different init value.
> >
> >Ok. I think the property at needs a better description for that.
> >
> >It's not clear to me how "limit controller for am max transmissions"
> >maps to writing a specific value to some registers, but I don't know
> >much about SAS.
> >
> >Is this some well-known thing, or values specific to hip06?
> >
> >Thanks,
> >Mark.
> >
> 
> 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?

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

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
tell:

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

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

Ok.

Thanks,
Mark.

Powered by blists - more mailing lists