linux-kernel - Re: [PATCH v3 20/23] dt-bindings: ata: ahci: Add Baikal-T1 AHCI SATA controller DT schema

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 24 May 2022 10:33:45 -0500
From:   Rob Herring <robh@...nel.org>
To:     Serge Semin <fancer.lancer@...il.com>
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 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.


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

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

> > 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?

Rob