Re: [PATCH v3 2/2] reset: mchp: sparx5: Extend support for lan966x

The 10/14/2021 14:00, Philipp Zabel wrote:

On Wed, 2021-10-13 at 09:38 +0200, Horatiu Vultur wrote:

quoted

This patch extends sparx5 driver to support also the lan966x. The
process to reset the switch is the same only it has different offsets.
Therefore make the driver more generic and add support for lan966x.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/reset/Kconfig                  |  2 +-
 drivers/reset/reset-microchip-sparx5.c | 81 +++++++++++++++++++++++---
 2 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index be799a5abf8a..36ce6c8bcf1e 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig

@@ -116,7 +116,7 @@ config RESET_LPC18XX

 config RESET_MCHP_SPARX5
      bool "Microchip Sparx5 reset driver"
-     depends on ARCH_SPARX5 || COMPILE_TEST
+     depends on ARCH_SPARX5 || SOC_LAN966 || COMPILE_TEST
      default y if SPARX5_SWITCH
      select MFD_SYSCON
      help

diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index f01e7db8e83b..211ee338e4b6 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c

@@ -6,6 +6,7 @@
  * The Sparx5 Chip Register Model can be browsed at this location:
  * https://github.com/microchip-ung/sparx-5_reginfo
  */
+#include <linux/gpio/consumer.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of_device.h>
 #include <linux/module.h>

@@ -13,15 +14,22 @@
 #include <linux/regmap.h>
 #include <linux/reset-controller.h>

-#define PROTECT_REG    0x84
-#define PROTECT_BIT    BIT(10)
-#define SOFT_RESET_REG 0x00
-#define SOFT_RESET_BIT BIT(1)
+struct reset_props {
+     u32 protect_reg;
+     u32 protect_bit;
+     u32 reset_reg;
+     u32 reset_bit;
+     u32 cuphy_reg;
+     u32 cuphy_bit;
+};

 struct mchp_reset_context {
      struct regmap *cpu_ctrl;
      struct regmap *gcb_ctrl;
+     struct regmap *cuphy_ctrl;
      struct reset_controller_dev rcdev;
+     const struct reset_props *props;
+     struct gpio_desc *phy_reset_gpio;
 };

 static struct regmap_config sparx5_reset_regmap_config = {

@@ -36,17 +44,39 @@ static int sparx5_switch_reset(struct reset_controller_dev *rcdev,
      struct mchp_reset_context *ctx =
              container_of(rcdev, struct mchp_reset_context, rcdev);
      u32 val;
+     int err;

      /* Make sure the core is PROTECTED from reset */
-     regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT, PROTECT_BIT);
+     regmap_update_bits(ctx->cpu_ctrl, ctx->props->protect_reg,
+                        ctx->props->protect_bit, ctx->props->protect_bit);

      /* Start soft reset */
-     regmap_write(ctx->gcb_ctrl, SOFT_RESET_REG, SOFT_RESET_BIT);
+     regmap_write(ctx->gcb_ctrl, ctx->props->reset_reg,
+                  ctx->props->reset_bit);

      /* Wait for soft reset done */
-     return regmap_read_poll_timeout(ctx->gcb_ctrl, SOFT_RESET_REG, val,
-                                     (val & SOFT_RESET_BIT) == 0,
+     err = regmap_read_poll_timeout(ctx->gcb_ctrl, ctx->props->reset_reg, val,
+                                     (val & ctx->props->reset_bit) == 0,
                                      1, 100);
+     if (err)
+             return err;
+
+     if (!ctx->cuphy_ctrl)
+             return 0;
+
+     /* In case there are external PHYs toggle the GPIO to release the reset
+      * of the PHYs
+      */
+     if (ctx->phy_reset_gpio) {
+             gpiod_direction_output(ctx->phy_reset_gpio, 1);
+             gpiod_set_value(ctx->phy_reset_gpio, 0);
+             gpiod_set_value(ctx->phy_reset_gpio, 1);
+             gpiod_set_value(ctx->phy_reset_gpio, 0);
+     }
+
+     /* Release the reset of internal PHY */
+     return regmap_update_bits(ctx->cuphy_ctrl, ctx->props->cuphy_reg,
+                               ctx->props->cuphy_bit, ctx->props->cuphy_bit);
 }

 static const struct reset_control_ops sparx5_reset_ops = {

@@ -111,17 +141,52 @@ static int mchp_sparx5_reset_probe(struct platform_device *pdev)
      if (err)
              return err;

+     /* This resource is required on lan966x, to take the internal PHYs out
+      * of reset

Ah, here we go, required on lan966x. This should be reflected in the
binding yaml.

I will update the binding yaml.

quoted

+      */
+     err = mchp_sparx5_map_syscon(pdev, "cuphy-syscon", &ctx->cuphy_ctrl);
+     if (err && err != -ENODEV)
+             return err;

So -ENODEV should return an error if .cuphy_reg is set?

I am not sure I follow this.
If cuphy-syscon is not set then mchp_sparx5_map_syscon will return
-ENODEV. This can be ignored for sparx5 as this is not required.
If cuphy-syscon is set then if mchp_sparx5_map_syscon returns an error
then report this error.

quoted

+
      ctx->rcdev.owner = THIS_MODULE;
      ctx->rcdev.nr_resets = 1;
      ctx->rcdev.ops = &sparx5_reset_ops;
      ctx->rcdev.of_node = dn;
+     ctx->props = device_get_match_data(&pdev->dev);
+
+     ctx->phy_reset_gpio = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
+                                                   GPIOD_OUT_LOW);
+     if (IS_ERR(ctx->phy_reset_gpio)) {
+             dev_err(&pdev->dev, "Could not get reset GPIO\n");
+             return PTR_ERR(ctx->phy_reset_gpio);

You could use dev_err_probe() here.

Yes, I will use this.

regards
Philipp

-- 
/Horatiu