Date:   Fri, 27 May 2022 13:55:51 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Jens Axboe <axboe@...nel.dk>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v3 20/23] dt-bindings: ata: ahci: Add Baikal-T1 AHCI SATA
 controller DT schema

On Tue, May 24, 2022 at 10:33:45AM -0500, Rob Herring wrote:
> On Sun, May 22, 2022 at 11:49:31PM +0300, Serge Semin wrote:
> > On Tue, May 17, 2022 at 03:13:32PM -0500, Rob Herring wrote:
> > > On Thu, May 12, 2022 at 02:18:07AM +0300, Serge Semin wrote:
> > > > Baikal-T1 AHCI controller is based on the DWC AHCI SATA IP-core v4.10a
> > > > with the next specific settings: two SATA ports, cascaded CSR access based
> > > > on two clock domains (APB and AXI), selectable source of the reference
> > > > clock (though stable work is currently available from the external source
> > > > only), two reset lanes for the application and SATA ports domains. Other
> > > > than that the device is fully compatible with the generic DWC AHCI SATA
> > > > bindings.
> > > > 
> > > > Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> > > > 
> > > > ---
> > > > 
> > > > Changelog v2:
> > > > - Rename 'syscon' property to 'baikal,bt1-syscon'.
> > > > - Drop macro usage from the example node.
> > > > ---
> > > >  .../bindings/ata/baikal,bt1-ahci.yaml         | 127 ++++++++++++++++++
> > > >  1 file changed, 127 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml b/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml
> > > > new file mode 100644
> > > > index 000000000000..7c2eae75434f
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml
> > > > @@ -0,0 +1,127 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/ata/baikal,bt1-ahci.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Baikal-T1 SoC AHCI SATA controller
> > > > +
> > > > +maintainers:
> > > > +  - Serge Semin <fancer.lancer@...il.com>
> > > > +
> > > > +description: |
> > > > +  AHCI SATA controller embedded into the Baikal-T1 SoC is based on the
> > > > +  DWC AHCI SATA v4.10a IP-core.
> > > > +
> > > > +allOf:
> > > > +  - $ref: snps,dwc-ahci.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    contains:
> > > > +      const: baikal,bt1-ahci
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: Peripheral APB bus clock source
> > > > +      - description: Application AXI BIU clock
> > > > +      - description: Internal SATA Ports reference clock
> > > > +      - description: External SATA Ports reference clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: pclk
> > > > +      - const: aclk
> > > > +      - const: ref_int
> > > > +      - const: ref_ext
> > > > +
> > > > +  resets:
> > > > +    items:
> > > > +      - description: Application AXI BIU domain reset
> > > > +      - description: SATA Ports clock domain reset
> > > > +
> > > > +  reset-names:
> > > > +    items:
> > > > +      - const: arst
> > > > +      - const: ref
> > > > +
> > > > +  baikal,bt1-syscon:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description:
> > > > +      Phandle reference to the CCU system controller. It is required to
> > > > +      switch between internal and external SATA reference clock sources.
> > > 
> > 
> > > Seems like the CCU system ctrlr should be a clock provider that provides 
> > > 'ref' clock and then assigned-clocks can be used to pick internal vs. 
> > > external ref.
> > 

> > By assigned-clocks do you mean using the "assigned-clock-parents"
> > property? 
> 
> Yes, I meant any of those properties.
> 
> > Does it mean creating additional clocks exported from the
> > CCU controller, which could have got one of the two parental clocks?
> 
> Yes, I believe so.

Ok. I hoped to avoid this since it isn't that easy... but seems like I
have not choice now.(

> 
> 
> > > > +
> > > > +  ports-implemented:
> > > > +    maximum: 0x3
> > > > +
> > > > +patternProperties:
> > > > +  "^sata-port@[0-9a-e]$":
> > > > +    type: object
> > > 
> > >        unevaluatedProperties: false
> > > 
> > 
> > > and then a $ref to a sata-port schema.
> > 

> > Can I set additional sata-port properties constraints afterwards? Like
> > I've done for the reg, snps,tx-ts-max and snps,rx-ts-max properties
> > here?
> 
> Yes. All the constraints are effectively ANDed together.

Ok.

> 
> > > > +
> > > > +    properties:
> > > > +      reg:
> > > > +        minimum: 0
> > > > +        maximum: 1
> > > > +
> > > > +      snps,tx-ts-max:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description:
> > > > +          Due to having AXI3 bus interface utilized the maximum Tx DMA
> > > > +          transaction size can't exceed 16 beats (AxLEN[3:0]).
> > > > +        minimum: 1
> > > > +        maximum: 16
> > > > +
> > > > +      snps,rx-ts-max:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description:
> > > > +          Due to having AXI3 bus interface utilized the maximum Rx DMA
> > > > +          transaction size can't exceed 16 beats (AxLEN[3:0]).
> > > 
> > 
> > > That's not a per port limitation (even though it's a per port register)? 
> > > I think this should be implied by the compatible string.
> > 
> > The snps,{rx,tx}-ts-max property is a per-port property. I'd better
> > explicitly set the property limitation here rather than having the
> > value implicitly clamped by hardware especially seeing the limitation
> > is set by the formulae
> > (CC_MSTR_BURST_LEN * M_HDATA_WIDTH/32)) / (M_HDATA_WIDTH/32),
> > which consists of the IP-core synthesized parameters.
> 
> I did not say use the h/w default.
> 

> What I asking is do you have any need for this to be different per port? 
> Seems unlikely given it's just 1 bus interface for all ports IIRC. I 
> can't see why you would want to tune the performance per port to 
> anything but the max burst length. If you have no need, use the 
> compatible string to determine what to set the register value to.

Well it's not what I need, it's about the way the system and AHCI
interfaces are used for on the particular platform. The Tx/Rx DMA max
burst length affects the system interconnect bus response latency (bus
to which all the system devices are attached to: CPU cores, DDR
controller, Ethernet, PCIe, SATA, etc). The higher the max-burst
length the higher the latency for the other devices to start executing
their IO ops. At the same time maximizing the burst length increases
the corresponding device performance. Should there be a high priority
storage (like system/swap SSD) and a low priority device (data hard drive)
attached to the AHCI ports, I would rise the max burst length of the
hi-prio device and decrease it for the other one. As such the
high-priority traffic would flow with max speed, while the
low-priority device would work slower than the other(s) giving a
chance for the other devices to start their system bus transfers more
frequently. All of that is the platform-specific settings.

> 
> > > Really, firmware should configure this IMO.
> > 
> > We don't have comprehensive firmware setting these and generic HBA parameters.
> > In our case dtb is the main platform firmware.
> 

> No u-boot?

Aside with the default u-boot bootloader there are customers using their
own boot-up software. In addition to that the controller can be
hard-reset before being used in the kernel, which defaults all the
registers state including the state of the PnDMACR one.

-Sergey 

> 
> Rob

Powered by blists - more mailing lists