| Message ID | 1452262542-64589-2-git-send-email-john.garry@huawei.com |
|---|---|
| State | New |
| Headers | show |
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.
>> 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
diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt index 0a7a325..2695023 100644 --- a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt @@ -5,6 +5,7 @@ The HiSilicon SAS controller supports SAS/SATA. Main node required properties: - compatible : value should be as follows: (a) "hisilicon,hip05-sas-v1" for v1 hw in hip05 chipset + (b) "hisilicon,hip06-sas-v2" for v2 hw in hip06 chipset - sas-addr : array of 8 bytes for host SAS address - reg : Address and length of the SAS register - hisilicon,sas-syscon: phandle of syscon used for sas control @@ -13,11 +14,11 @@ Main node required properties: - ctrl-clock-ena-reg : offset to controller clock enable register in ctrl reg - queue-count : number of delivery and completion queues in the controller - phy-count : number of phys accessible by the controller - - interrupts : Interrupts for phys, completion queues, and fatal + - interrupts : For v1 hw: Interrupts for phys, completion queues, and fatal sources; the interrupts are ordered in 3 groups, as follows: - - Phy interrupts - - Completion queue interrupts - - Fatal interrupts + - Phy interrupts + - Completion queue interrupts + - Fatal interrupts Phy interrupts : Each phy has 3 interrupt sources: - broadcast - phyup @@ -25,11 +26,28 @@ Main node required properties: The phy interrupts are ordered into groups of 3 per phy (broadcast, phyup, and abnormal) in increasing order. Completion queue interrupts : each completion queue has 1 - interrupt source. - The interrupts are ordered in increasing order. + interrupt source. The interrupts are ordered in + increasing order. Fatal interrupts : the fatal interrupts are ordered as follows: - ECC - AXI bus + For v2 hw: Interrupts for phys, Sata, and completion queues; + the interrupts are ordered in 3 groups, as follows: + - Phy interrupts + - Sata interrupts + - Completion queue interrupts + Phy interrupts : Each controller has 2 phy interrupts: + - phy up/down + - channel interrupt + Sata interrupts : Each phy on the controller has 1 Sata + interrupt. The interrupts are ordered in increasing + order. + Completion queue interrupts : each completion queue has 1 + interrupt source. The interrupts are ordered in + increasing order. + +Optional main node properties: + - am-max-trans : limit controller for am max transmissions Example: sas0: sas@c1000000 {
Add the dt bindings for HiSi SAS controller v2 HW. The main difference in the controllers from dt perspective is interrupts. Signed-off-by: John Garry <john.garry@huawei.com> --- .../devicetree/bindings/scsi/hisilicon-sas.txt | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) -- 1.9.1