Re: [PATCH v3 5/8] dt-bindings: snps,dw-apb-ssi: Add sparx5 support, plus snps,rx-sample-delay-ns property

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
To: Rob Herring <robh@kernel.org>
Cc: Serge Semin <fancer.lancer@gmail.com>,
	Lars Povlsen <lars.povlsen@microchip.com>,
	Mark Brown <broonie@kernel.org>, Peter Rosin <peda@axentia.se>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	<linux-spi@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 5/8] dt-bindings: snps,dw-apb-ssi: Add sparx5 support, plus snps,rx-sample-delay-ns property
Date: Mon, 13 Jul 2020 22:52:19 +0300	[thread overview]
Message-ID: <20200713195219.xfvqknioqw7yyr74@mobilestation> (raw)
In-Reply-To: <20200713192259.GA553903@bogus>

On Mon, Jul 13, 2020 at 01:22:59PM -0600, Rob Herring wrote:
> On Thu, Jul 02, 2020 at 12:13:28PM +0200, Lars Povlsen wrote:
...
> 
> > +
> >  patternProperties:
> >    "^.*@[0-9a-f]+$":
> >      type: object
> > @@ -107,6 +122,14 @@ patternProperties:
> >        spi-tx-bus-width:
> >          const: 1
> > 
> > +      snps,rx-sample-delay-ns:
> 

> We already have 'rx-sample-delay-ns' from Rockchip SPI, so use that. But 
> note that it applies to the SPI node. Does this need to be per SPI 
> child?

It was me, who suggested to Lars to have that parameter moved to the SPI
sub-nodes. As I see it the property is highly dependent on the SPI slave device
the controller is communicating to. Some of the them may need the delay some
may not. It's not always the capacitance thing, but also depends on how good
the MISO signal a particular slave generates. So IMO the Rockchip SPI driver
developer should have moved that property to the sub-nodes too.

On the other hand if the Rx errors are caused by the MISO lane capacitance,
then it will be cumbersome to have the same property duplicated for each
sub-node. Then what about having the property supported by both the SPI
controller and the SPI-child nodes? For instance the SPI-controller
"rx-sample-delay-ns" will provide a default sample delay for each sub-node
instead of zero by default, while the individual sub-node "rx-sample-delay-ns"
property can be used to override the default value.

-Sergey

> 
> BTW, the Rockchip controller appears to be a version of the DW 
> controller.
> 
> > +        description: SPI Rx sample delay offset, unit is nanoseconds.
> > +          The delay from the default sample time before the actual
> > +          sample of the rxd input signal occurs. The "rx_sample_delay"
> > +          is an optional feature of the designware controller, and the
> > +          upper limit is also subject to controller configuration.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> >  unevaluatedProperties: false
> > 
> >  required:
> > @@ -129,5 +152,10 @@ examples:
> >        num-cs = <2>;
> >        cs-gpios = <&gpio0 13 0>,
> >                   <&gpio0 14 0>;
> > +      spi-flash@1 {
> > +        compatible = "spi-nand";
> > +        reg = <1>;
> > +        snps,rx-sample-delay-ns = <7>;
> > +      };
> >      };
> >  ...
> > --
> > 2.27.0