Re: [PATCH 01/21] dt-bindings: ata: sata: Extend number of SATA ports

[Serge Semin]

On Tue, Mar 29, 2022 at 05:15:12PM +0900, Damien Le Moal wrote:
> On 3/24/22 09:16, Serge Semin wrote:
> > The denoted in the description upper limit only concerns the Port
> > Multipliers, but not the actual SATA ports. It's an external device
> > attached to a SATA port in order to access more than one SATA-drive. So
> > when it's attached to a SATA port it just extends the port capability
> > while the number of actual SATA ports stays the same. For instance on AHCI
> > controllers the number of actual ports is determined by the CAP.NP field
> > and the PI (Ports Implemented) register. AFAICS in general the maximum
> > number of SATA ports depends on the particular controller implementation.
> > Generic AHCI controller can't have more than 32 ports (since CAP.NP is of
> > 5 bits wide and PI register is 32-bits size), while DWC AHCI SATA
> > controller can't be configured with more than 8 ports activated. So let's
> > discard the SATA ports reg-property restrictions and just make sure that
> > it consists of a single reg-item.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >  Documentation/devicetree/bindings/ata/sata-common.yaml | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/ata/sata-common.yaml b/Documentation/devicetree/bindings/ata/sata-common.yaml
> > index 7ac77b1c5850..c619f0ae72fb 100644
> > --- a/Documentation/devicetree/bindings/ata/sata-common.yaml
> > +++ b/Documentation/devicetree/bindings/ata/sata-common.yaml
> > @@ -41,11 +41,10 @@ patternProperties:
> >      properties:
> >        reg:
> >          minimum: 0
> > -        maximum: 14
> 

> Why remove this ? Since AHCI can have up to 32 ports, then change the
> value to 31. As the comment at the top of the file says, this is not
> intended to be a device tree binding spec, but defines properties common
> to most adapters.

Right, but the schema determines the common !SATA! controllers properties,
while you are referring to the AHCI-specific limit. As I said in the patch
log AFAICS in general the SATA controllers may have any number of ports.
The number is determined by the hardware designers needs only. Thus the
actual constraints needs to be specified in the controller-specific
YAML-schema (the one which will $ref to sata-common.yaml).

Though I couldn't find any ATA device driver in the kernel which would
handle a device with even 32 ports, not to mention a greater number.
So replacing it with 31 might be reasonable after all. But me failing
to find any such device doesn't me it can't exist. Thus I've decided to
drop the upper limit completely.

> 
> >          description:
> > -          The ID number of the drive port SATA can potentially use a port
> > -          multiplier making it possible to connect up to 15 disks to a single
> > -          SATA port.
> > +          The ID number of the SATA port. Aside with being directly used
> > +          each port can have a Port Multiplier attached thus allowing to
> > +          access more than one drive by means of a single channel.
> 

> Please add a comma after "Aside with being directly used", otherwise the
> sentence is hard to understand. And replace "channel" with "SATA port" to
> stick with the terms defined here.

Ok.

-Sergey

> 
> >  
> >  additionalProperties: true
> >  
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research