Hi Sergey,

On Tue, Jun 21, 2022 at 3:12 AM Serge Semin [off-list ref] wrote:

On Tue, Jun 21, 2022 at 09:00:36AM +0200, Krzysztof Kozlowski wrote:

quoted

On 20/06/2022 22:04, Serge Semin wrote:

quoted

On Mon, Jun 20, 2022 at 09:46:25PM +0200, Krzysztof Kozlowski wrote:

quoted

On 20/06/2022 21:30, Serge Semin wrote:

quoted

On Mon, Jun 13, 2022 at 12:56:47PM -0700, Brad Larson wrote:

quoted

From: Brad Larson <blarson@amd.com>

The AMD Pensando Elba SoC has integrated the DW APB SPI Controller

Signed-off-by: Brad Larson <blarson@amd.com>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
index e25d44c218f2..2a55b947cffc 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml

@@ -73,6 +73,8 @@ properties:
               - renesas,r9a06g032-spi # RZ/N1D
               - renesas,r9a06g033-spi # RZ/N1S
           - const: renesas,rzn1-spi   # RZ/N1

quoted

+      - description: AMD Pensando Elba SoC SPI Controller
+        const: amd,pensando-elba-spi

Not enough. The driver requires to have a phandle reference to the
Pensando System Controller. So the property like
"amd,pensando-elba-syscon" is also needed to be added to the DT schema
otherwise should the dt-schema tool correctly handle the
"unevaluatedProperties: false" setting (Rob says it isn't fully
supported at the moment), the dtbs_check procedure will fail on your
dts evaluation.

quoted

The property was here before, now removed, so I assume it was also
removed from the driver and DTS. Isn't that the case?

Ah, the property has been indeed removed. The driver now searches for
the system controller by the next compatible string:
"amd,pensando-elba-syscon" using the
syscon_regmap_lookup_by_compatible() method. My mistake. Sorry for the
noise.

quoted

quoted

* Though personally I'd prefer to have a property with the phandle
reference in order to signify the connection between the system controller
and the SPI-controller. Otherwise the implicit DT bindings like having
the "amd,pensando-elba-syscon"-compatible syscon gets to be
hidden behind the DT scene. But seeing we have already got the Microsemi
platform with such semantic, I can't insist on fixing this.

I agree entirely, this should be explicit syscon-type property. Looking
up for compatibles:
 - creates hidden (not expressed via bindings) dependency between nodes,
 - is not portable and several people struggled with it later and needed
backward-compatible code (many examples, let's just give recent one: [1])


[1]
https://lore.kernel.org/all/20220619151225.209029-10-tmaimon77@gmail.com/ (local)

Seems even more reasonable now. Thanks for providing a bright example
justifying the property-based approach.

@Brad, could you get back the property with a phandle to the syscon
DT-node? (No need in adding the CS CSR address as the phandle argument,
just a phandle.)

Replying to the sequence of review inputs.  The below change to
version 5 brings back the phandle without the earlier argument that
complicated the binding (was the motivation for moving in this
direction in earlier version).  Passes dtbs_check.

--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c

@@ -285,12 +285,17 @@ static void dw_spi_elba_set_cs(struct spi_device

*spi, bool enable)
 static int dw_spi_elba_init(struct platform_device *pdev,
                            struct dw_spi_mmio *dwsmmio)
 {
+       struct device_node *np = pdev->dev.of_node;
+       struct device_node *node;
        struct dw_spi_elba *dwselba;
        struct regmap *regmap;

-       regmap = syscon_regmap_lookup_by_compatible("amd,pensando-elba-syscon");
-       if (IS_ERR(regmap))
-               return PTR_ERR(regmap);
+       node = of_parse_phandle(np, "pensando,elba-syscon-spics", 0);
+       if (node) {
+               regmap = syscon_node_to_regmap(node);
+               if (IS_ERR(regmap))
+                       return PTR_ERR(regmap);
+       }

diff --git a/arch/arm64/boot/dts/amd/elba.dtsi

b/arch/arm64/boot/dts/amd/elba.dtsi
index 9739641261c3..ec48be4cfe48 100644

--- a/arch/arm64/boot/dts/amd/elba.dtsi
+++ b/arch/arm64/boot/dts/amd/elba.dtsi

@@ -98,6 +98,7 @@
                        reg = <0x0 0x2800 0x0 0x100>;
                        #address-cells = <1>;
                        #size-cells = <0>;
+                       pensando,elba-syscon-spics = <&syscon>;
                        clocks = <&ahb_clk>;
                        interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
                        num-cs = <2>;

--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml

@@ -37,6 +37,15 @@ allOf:
     else:
       required:
         - interrupts
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amd,pensando-elba-spi
+    then:
+      required:
+        - pensando,elba-syscon-spics

 properties:
   compatible:

Regards,
Brad

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel