Add compatible strings for the twelve lan969x SKU's (Stock Keeping Unit)
that we support, and verify that the devicetree target is supported by
the chip target.
Each SKU supports different bandwidths and features (see [1] for
details). We want to be able to run a SKU with a lower bandwidth and/or
feature set, than what is supported by the actual chip. In order to
accomplish this we:
- add new field sparx5->target_dt that reflects the target from the
devicetree (compatible string).
- compare the devicetree target with the actual chip target. If the
bandwidth and features provided by the devicetree target is
supported by the chip, we approve - otherwise reject.
- set the core clock and features based on the devicetree target
[1] https://www.microchip.com/en-us/product/lan9698
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
drivers/net/ethernet/microchip/sparx5/Makefile | 1 +
.../net/ethernet/microchip/sparx5/sparx5_main.c | 194 ++++++++++++++++++++-
.../net/ethernet/microchip/sparx5/sparx5_main.h | 1 +
3 files changed, 193 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile
index 3435ca86dd70..8fe302415563 100644
--- a/drivers/net/ethernet/microchip/sparx5/Makefile
+++ b/drivers/net/ethernet/microchip/sparx5/Makefile
@@ -19,3 +19,4 @@ sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o
# Provide include files
ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/vcap
ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/fdma
+ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/lan969x
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
index 5c986c373b3e..edbe639d98c5 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
@@ -24,6 +24,8 @@
#include <linux/types.h>
#include <linux/reset.h>
+#include "lan969x.h" /* lan969x_desc */
+
#include "sparx5_main_regs.h"
#include "sparx5_main.h"
#include "sparx5_port.h"
@@ -227,6 +229,168 @@ bool is_sparx5(struct sparx5 *sparx5)
}
}
+/* Set the devicetree target based on the compatible string */
+static int sparx5_set_target_dt(struct sparx5 *sparx5)
+{
+ struct device_node *node = sparx5->pdev->dev.of_node;
+
+ if (is_sparx5(sparx5))
+ /* For Sparx5 the devicetree target is always the chip target */
+ sparx5->target_dt = sparx5->target_ct;
+ else if (of_device_is_compatible(node, "microchip,lan9691-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9691VAO;
+ else if (of_device_is_compatible(node, "microchip,lan9692-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9692VAO;
+ else if (of_device_is_compatible(node, "microchip,lan9693-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9693VAO;
+ else if (of_device_is_compatible(node, "microchip,lan9694-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9694;
+ else if (of_device_is_compatible(node, "microchip,lan9695-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9694TSN;
+ else if (of_device_is_compatible(node, "microchip,lan9696-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9696;
+ else if (of_device_is_compatible(node, "microchip,lan9697-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9696TSN;
+ else if (of_device_is_compatible(node, "microchip,lan9698-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9698;
+ else if (of_device_is_compatible(node, "microchip,lan9699-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9698TSN;
+ else if (of_device_is_compatible(node, "microchip,lan969a-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9694RED;
+ else if (of_device_is_compatible(node, "microchip,lan969b-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9696RED;
+ else if (of_device_is_compatible(node, "microchip,lan969c-switch"))
+ sparx5->target_dt = SPX5_TARGET_CT_LAN9698RED;
+ else
+ return -EINVAL;
+
+ return 0;
+}
+
+/* Compare the devicetree target with the chip target.
+ * Make sure the chip target supports the features and bandwidth requested
+ * from the devicetree target.
+ */
+static int sparx5_verify_target(struct sparx5 *sparx5)
+{
+ switch (sparx5->target_dt) {
+ case SPX5_TARGET_CT_7546:
+ case SPX5_TARGET_CT_7549:
+ case SPX5_TARGET_CT_7552:
+ case SPX5_TARGET_CT_7556:
+ case SPX5_TARGET_CT_7558:
+ case SPX5_TARGET_CT_7546TSN:
+ case SPX5_TARGET_CT_7549TSN:
+ case SPX5_TARGET_CT_7552TSN:
+ case SPX5_TARGET_CT_7556TSN:
+ case SPX5_TARGET_CT_7558TSN:
+ return 0;
+ case SPX5_TARGET_CT_LAN9698RED:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9696RED:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9696RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9694RED:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9694RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9698TSN:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9698TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9696TSN:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9696TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9694TSN:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9694TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9694RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9693VAO:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9693VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698TSN)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9692VAO:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9692VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9693VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698TSN)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9691VAO:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9691VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9692VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9693VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9694TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9694RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9698:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9698 ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9693VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698TSN)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9696:
+ if (sparx5->target_ct == SPX5_TARGET_CT_LAN9696 ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698 ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9692VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9693VAO ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9696TSN ||
+ sparx5->target_ct == SPX5_TARGET_CT_LAN9698TSN)
+ return 0;
+ break;
+
+ case SPX5_TARGET_CT_LAN9694:
+ return 0;
+
+ default:
+ pr_err("Unknown target: %x", sparx5->target_dt);
+ return -EINVAL;
+ }
+
+ pr_err("Chip target: %x does not support the target: %x",
+ sparx5->target_ct, sparx5->target_dt);
+
+ return -EINVAL;
+}
+
static int sparx5_create_targets(struct sparx5 *sparx5)
{
const struct sparx5_main_io_resource *iomap = sparx5->data->iomap;
@@ -441,7 +605,7 @@ static int sparx5_init_coreclock(struct sparx5 *sparx5)
* If 'VTSS_CORE_CLOCK_DEFAULT' then the highest supported
* freq. is used
*/
- switch (sparx5->target_ct) {
+ switch (sparx5->target_dt) {
case SPX5_TARGET_CT_7546:
if (sparx5->coreclock == SPX5_CORE_CLOCK_DEFAULT)
freq = SPX5_CORE_CLOCK_250MHZ;
@@ -491,7 +655,7 @@ static int sparx5_init_coreclock(struct sparx5 *sparx5)
break;
default:
dev_err(sparx5->dev, "Target (%#04x) not supported\n",
- sparx5->target_ct);
+ sparx5->target_dt);
return -ENODEV;
}
@@ -512,7 +676,7 @@ static int sparx5_init_coreclock(struct sparx5 *sparx5)
default:
dev_err(sparx5->dev,
"%d coreclock not supported on (%#04x)\n",
- sparx5->coreclock, sparx5->target_ct);
+ sparx5->coreclock, sparx5->target_dt);
return -EINVAL;
}
@@ -914,6 +1078,16 @@ static int mchp_sparx5_probe(struct platform_device *pdev)
sparx5->target_ct = (enum spx5_target_chiptype)
GCB_CHIP_ID_PART_ID_GET(sparx5->chip_id);
+ /* Set the devicetree target based on the compatible string. */
+ err = sparx5_set_target_dt(sparx5);
+ if (err)
+ goto cleanup_config;
+
+ /* Verify that the chip target supports the devicetree target */
+ err = sparx5_verify_target(sparx5);
+ if (err)
+ goto cleanup_config;
+
/* Initialize Switchcore and internal RAMs */
err = sparx5_init_switchcore(sparx5);
if (err) {
@@ -1051,6 +1225,20 @@ static const struct sparx5_match_data sparx5_desc = {
static const struct of_device_id mchp_sparx5_match[] = {
{ .compatible = "microchip,sparx5-switch", .data = &sparx5_desc },
+#ifdef CONFIG_LAN969X_SWITCH
+ { .compatible = "microchip,lan9691-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan9692-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan9693-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan9694-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan9695-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan9696-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan9697-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan9698-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan9699-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan969a-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan969b-switch", .data = &lan969x_desc },
+ { .compatible = "microchip,lan969c-switch", .data = &lan969x_desc },
+#endif
{ }
};
MODULE_DEVICE_TABLE(of, mchp_sparx5_match);
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 1828e2a7d610..8a2b74d0bd35 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -337,6 +337,7 @@ struct sparx5 {
struct device *dev;
u32 chip_id;
enum spx5_target_chiptype target_ct;
+ enum spx5_target_chiptype target_dt; /* target from devicetree */
void __iomem *regs[NUM_TARGETS];
int port_count;
struct mutex lock; /* MAC reg lock */
--
2.34.1On Mon, Oct 21, 2024 at 03:58:51PM +0200, Daniel Machon wrote:
> Add compatible strings for the twelve lan969x SKU's (Stock Keeping Unit)
> that we support, and verify that the devicetree target is supported by
> the chip target.
>
> Each SKU supports different bandwidths and features (see [1] for
> details). We want to be able to run a SKU with a lower bandwidth and/or
> feature set, than what is supported by the actual chip. In order to
> accomplish this we:
>
> - add new field sparx5->target_dt that reflects the target from the
> devicetree (compatible string).
>
> - compare the devicetree target with the actual chip target. If the
> bandwidth and features provided by the devicetree target is
> supported by the chip, we approve - otherwise reject.
>
> - set the core clock and features based on the devicetree target
>
> [1] https://www.microchip.com/en-us/product/lan9698
>
> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
> drivers/net/ethernet/microchip/sparx5/Makefile | 1 +
> .../net/ethernet/microchip/sparx5/sparx5_main.c | 194 ++++++++++++++++++++-
> .../net/ethernet/microchip/sparx5/sparx5_main.h | 1 +
> 3 files changed, 193 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile
> index 3435ca86dd70..8fe302415563 100644
> --- a/drivers/net/ethernet/microchip/sparx5/Makefile
> +++ b/drivers/net/ethernet/microchip/sparx5/Makefile
> @@ -19,3 +19,4 @@ sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o
> # Provide include files
> ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/vcap
> ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/fdma
> +ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/lan969x
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> index 5c986c373b3e..edbe639d98c5 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> @@ -24,6 +24,8 @@
> #include <linux/types.h>
> #include <linux/reset.h>
>
> +#include "lan969x.h" /* lan969x_desc */
> +
> #include "sparx5_main_regs.h"
> #include "sparx5_main.h"
> #include "sparx5_port.h"
> @@ -227,6 +229,168 @@ bool is_sparx5(struct sparx5 *sparx5)
> }
> }
>
> +/* Set the devicetree target based on the compatible string */
> +static int sparx5_set_target_dt(struct sparx5 *sparx5)
> +{
> + struct device_node *node = sparx5->pdev->dev.of_node;
> +
> + if (is_sparx5(sparx5))
> + /* For Sparx5 the devicetree target is always the chip target */
> + sparx5->target_dt = sparx5->target_ct;
> + else if (of_device_is_compatible(node, "microchip,lan9691-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9691VAO;
> + else if (of_device_is_compatible(node, "microchip,lan9692-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9692VAO;
> + else if (of_device_is_compatible(node, "microchip,lan9693-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9693VAO;
> + else if (of_device_is_compatible(node, "microchip,lan9694-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694;
> + else if (of_device_is_compatible(node, "microchip,lan9695-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694TSN;
> + else if (of_device_is_compatible(node, "microchip,lan9696-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696;
> + else if (of_device_is_compatible(node, "microchip,lan9697-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696TSN;
> + else if (of_device_is_compatible(node, "microchip,lan9698-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698;
> + else if (of_device_is_compatible(node, "microchip,lan9699-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698TSN;
> + else if (of_device_is_compatible(node, "microchip,lan969a-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694RED;
> + else if (of_device_is_compatible(node, "microchip,lan969b-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696RED;
> + else if (of_device_is_compatible(node, "microchip,lan969c-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698RED;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +/* Compare the devicetree target with the chip target.
> + * Make sure the chip target supports the features and bandwidth requested
> + * from the devicetree target.
> + */
> +static int sparx5_verify_target(struct sparx5 *sparx5)
> +{
> + switch (sparx5->target_dt) {
> + case SPX5_TARGET_CT_7546:
> + case SPX5_TARGET_CT_7549:
> + case SPX5_TARGET_CT_7552:
> + case SPX5_TARGET_CT_7556:
> + case SPX5_TARGET_CT_7558:
> + case SPX5_TARGET_CT_7546TSN:
> + case SPX5_TARGET_CT_7549TSN:
> + case SPX5_TARGET_CT_7552TSN:
> + case SPX5_TARGET_CT_7556TSN:
> + case SPX5_TARGET_CT_7558TSN:
> + return 0;
All this is weird. Why would you verify? You were matched, it cannot be
mis-matching.
> + case SPX5_TARGET_CT_LAN9698RED:
> + if (sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
What is "ct"? sorry, all this code is a big no.
Best regards,
Krzysztof
Hi Krzysztof,
> > Add compatible strings for the twelve lan969x SKU's (Stock Keeping Unit)
> > that we support, and verify that the devicetree target is supported by
> > the chip target.
> >
> > Each SKU supports different bandwidths and features (see [1] for
> > details). We want to be able to run a SKU with a lower bandwidth and/or
> > feature set, than what is supported by the actual chip. In order to
> > accomplish this we:
> >
> > - add new field sparx5->target_dt that reflects the target from the
> > devicetree (compatible string).
> >
> > - compare the devicetree target with the actual chip target. If the
> > bandwidth and features provided by the devicetree target is
> > supported by the chip, we approve - otherwise reject.
> >
> > - set the core clock and features based on the devicetree target
> >
> > [1] https://www.microchip.com/en-us/product/lan9698
> >
> > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> > ---
> > drivers/net/ethernet/microchip/sparx5/Makefile | 1 +
> > .../net/ethernet/microchip/sparx5/sparx5_main.c | 194 ++++++++++++++++++++-
> > .../net/ethernet/microchip/sparx5/sparx5_main.h | 1 +
> > 3 files changed, 193 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile
> > index 3435ca86dd70..8fe302415563 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/Makefile
> > +++ b/drivers/net/ethernet/microchip/sparx5/Makefile
> > @@ -19,3 +19,4 @@ sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o
> > # Provide include files
> > ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/vcap
> > ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/fdma
> > +ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/lan969x
> > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> > index 5c986c373b3e..edbe639d98c5 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> > @@ -24,6 +24,8 @@
> > #include <linux/types.h>
> > #include <linux/reset.h>
> >
> > +#include "lan969x.h" /* lan969x_desc */
> > +
> > #include "sparx5_main_regs.h"
> > #include "sparx5_main.h"
> > #include "sparx5_port.h"
> > @@ -227,6 +229,168 @@ bool is_sparx5(struct sparx5 *sparx5)
> > }
> > }
> >
> > +/* Set the devicetree target based on the compatible string */
> > +static int sparx5_set_target_dt(struct sparx5 *sparx5)
> > +{
> > + struct device_node *node = sparx5->pdev->dev.of_node;
> > +
> > + if (is_sparx5(sparx5))
> > + /* For Sparx5 the devicetree target is always the chip target */
> > + sparx5->target_dt = sparx5->target_ct;
> > + else if (of_device_is_compatible(node, "microchip,lan9691-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9691VAO;
> > + else if (of_device_is_compatible(node, "microchip,lan9692-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9692VAO;
> > + else if (of_device_is_compatible(node, "microchip,lan9693-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9693VAO;
> > + else if (of_device_is_compatible(node, "microchip,lan9694-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9694;
> > + else if (of_device_is_compatible(node, "microchip,lan9695-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9694TSN;
> > + else if (of_device_is_compatible(node, "microchip,lan9696-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9696;
> > + else if (of_device_is_compatible(node, "microchip,lan9697-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9696TSN;
> > + else if (of_device_is_compatible(node, "microchip,lan9698-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9698;
> > + else if (of_device_is_compatible(node, "microchip,lan9699-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9698TSN;
> > + else if (of_device_is_compatible(node, "microchip,lan969a-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9694RED;
> > + else if (of_device_is_compatible(node, "microchip,lan969b-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9696RED;
> > + else if (of_device_is_compatible(node, "microchip,lan969c-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9698RED;
> > + else
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/* Compare the devicetree target with the chip target.
> > + * Make sure the chip target supports the features and bandwidth requested
> > + * from the devicetree target.
> > + */
> > +static int sparx5_verify_target(struct sparx5 *sparx5)
> > +{
> > + switch (sparx5->target_dt) {
> > + case SPX5_TARGET_CT_7546:
> > + case SPX5_TARGET_CT_7549:
> > + case SPX5_TARGET_CT_7552:
> > + case SPX5_TARGET_CT_7556:
> > + case SPX5_TARGET_CT_7558:
> > + case SPX5_TARGET_CT_7546TSN:
> > + case SPX5_TARGET_CT_7549TSN:
> > + case SPX5_TARGET_CT_7552TSN:
> > + case SPX5_TARGET_CT_7556TSN:
> > + case SPX5_TARGET_CT_7558TSN:
> > + return 0;
>
> All this is weird. Why would you verify? You were matched, it cannot be
> mis-matching.
We are verifying that the match (target/compatible string) from the
device tree is supported by the chip. Maybe I wasn't too clear about the
intend in v1.
Each target supports different bandwidths and features. If you have a
lan9698 chip, it must, obviously, be possible to run it as a lan9698
target. However, some targets can be run on chip targets other than
themselves, given that the chip supports the bandwidth and features of
the provided target. In contrary, trying to run as a target with a
feature not supported by the chip, or a bandwidth higher than what the
chip supports, should be rejected.
Without this logic, the chip id is read and a target is determined. That
means on a lan9698 chip you will always match the lan9698 target.
With the new logic, it is possible to run as a different target than
what is read from the chip id, given that the target you are trying to
run as, is supported by the chip.
>
> > + case SPX5_TARGET_CT_LAN9698RED:
> > + if (sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
>
> What is "ct"? sorry, all this code is a big no.
In this case we were matched as a SPX5_TARGET_CT_LAN9698RED target. We
are verifying that the chip target (target_ct, which is read from the
chip) supports the target we were matched as.
> Krzysztof
>
This is a feature that we would like, as it gives the flexibility of
running different targets on the same chip. Now if this is something
that cannot be accepted, I will have to ditch this part.
Let me know.
/Daniel
On 23/10/2024 13:00, Daniel Machon wrote:
> Hi Krzysztof,
>
>>> Add compatible strings for the twelve lan969x SKU's (Stock Keeping Unit)
>>> that we support, and verify that the devicetree target is supported by
>>> the chip target.
>>>
>>> Each SKU supports different bandwidths and features (see [1] for
>>> details). We want to be able to run a SKU with a lower bandwidth and/or
>>> feature set, than what is supported by the actual chip. In order to
>>> accomplish this we:
>>>
>>> - add new field sparx5->target_dt that reflects the target from the
>>> devicetree (compatible string).
>>>
>>> - compare the devicetree target with the actual chip target. If the
>>> bandwidth and features provided by the devicetree target is
>>> supported by the chip, we approve - otherwise reject.
>>>
>>> - set the core clock and features based on the devicetree target
>>>
>>> [1] https://www.microchip.com/en-us/product/lan9698
>>>
>>> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
>>> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
>>> ---
>>> drivers/net/ethernet/microchip/sparx5/Makefile | 1 +
>>> .../net/ethernet/microchip/sparx5/sparx5_main.c | 194 ++++++++++++++++++++-
>>> .../net/ethernet/microchip/sparx5/sparx5_main.h | 1 +
>>> 3 files changed, 193 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile
>>> index 3435ca86dd70..8fe302415563 100644
>>> --- a/drivers/net/ethernet/microchip/sparx5/Makefile
>>> +++ b/drivers/net/ethernet/microchip/sparx5/Makefile
>>> @@ -19,3 +19,4 @@ sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o
>>> # Provide include files
>>> ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/vcap
>>> ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/fdma
>>> +ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/lan969x
>>> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
>>> index 5c986c373b3e..edbe639d98c5 100644
>>> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
>>> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
>>> @@ -24,6 +24,8 @@
>>> #include <linux/types.h>
>>> #include <linux/reset.h>
>>>
>>> +#include "lan969x.h" /* lan969x_desc */
>>> +
>>> #include "sparx5_main_regs.h"
>>> #include "sparx5_main.h"
>>> #include "sparx5_port.h"
>>> @@ -227,6 +229,168 @@ bool is_sparx5(struct sparx5 *sparx5)
>>> }
>>> }
>>>
>>> +/* Set the devicetree target based on the compatible string */
>>> +static int sparx5_set_target_dt(struct sparx5 *sparx5)
>>> +{
>>> + struct device_node *node = sparx5->pdev->dev.of_node;
>>> +
>>> + if (is_sparx5(sparx5))
>>> + /* For Sparx5 the devicetree target is always the chip target */
>>> + sparx5->target_dt = sparx5->target_ct;
>>> + else if (of_device_is_compatible(node, "microchip,lan9691-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9691VAO;
>>> + else if (of_device_is_compatible(node, "microchip,lan9692-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9692VAO;
>>> + else if (of_device_is_compatible(node, "microchip,lan9693-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9693VAO;
>>> + else if (of_device_is_compatible(node, "microchip,lan9694-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694;
>>> + else if (of_device_is_compatible(node, "microchip,lan9695-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694TSN;
>>> + else if (of_device_is_compatible(node, "microchip,lan9696-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696;
>>> + else if (of_device_is_compatible(node, "microchip,lan9697-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696TSN;
>>> + else if (of_device_is_compatible(node, "microchip,lan9698-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698;
>>> + else if (of_device_is_compatible(node, "microchip,lan9699-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698TSN;
>>> + else if (of_device_is_compatible(node, "microchip,lan969a-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694RED;
>>> + else if (of_device_is_compatible(node, "microchip,lan969b-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696RED;
>>> + else if (of_device_is_compatible(node, "microchip,lan969c-switch"))
>>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698RED;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/* Compare the devicetree target with the chip target.
>>> + * Make sure the chip target supports the features and bandwidth requested
>>> + * from the devicetree target.
>>> + */
>>> +static int sparx5_verify_target(struct sparx5 *sparx5)
>>> +{
>>> + switch (sparx5->target_dt) {
>>> + case SPX5_TARGET_CT_7546:
>>> + case SPX5_TARGET_CT_7549:
>>> + case SPX5_TARGET_CT_7552:
>>> + case SPX5_TARGET_CT_7556:
>>> + case SPX5_TARGET_CT_7558:
>>> + case SPX5_TARGET_CT_7546TSN:
>>> + case SPX5_TARGET_CT_7549TSN:
>>> + case SPX5_TARGET_CT_7552TSN:
>>> + case SPX5_TARGET_CT_7556TSN:
>>> + case SPX5_TARGET_CT_7558TSN:
>>> + return 0;
>>
>> All this is weird. Why would you verify? You were matched, it cannot be
>> mis-matching.
>
> We are verifying that the match (target/compatible string) from the
> device tree is supported by the chip. Maybe I wasn't too clear about the
> intend in v1.
>
> Each target supports different bandwidths and features. If you have a
> lan9698 chip, it must, obviously, be possible to run it as a lan9698
> target. However, some targets can be run on chip targets other than
> themselves, given that the chip supports the bandwidth and features of
> the provided target. In contrary, trying to run as a target with a
> feature not supported by the chip, or a bandwidth higher than what the
> chip supports, should be rejected.
But you are not supposed to compare DT with what you auto-detected.
Detect your hardware, test if it is supported and then bail out.
None of above explains the code.
>
> Without this logic, the chip id is read and a target is determined. That
> means on a lan9698 chip you will always match the lan9698 target.
That's not the job of kernel.
>
> With the new logic, it is possible to run as a different target than
> what is read from the chip id, given that the target you are trying to
> run as, is supported by the chip.
So just run on different target.
>
>>
>>> + case SPX5_TARGET_CT_LAN9698RED:
>>> + if (sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
>>
>> What is "ct"? sorry, all this code is a big no.
>
> In this case we were matched as a SPX5_TARGET_CT_LAN9698RED target. We
> are verifying that the chip target (target_ct, which is read from the
> chip) supports the target we were matched as.
>
>> Krzysztof
>>
>
> This is a feature that we would like, as it gives the flexibility of
> running different targets on the same chip. Now if this is something
> that cannot be accepted, I will have to ditch this part.
I have no clue what the "target" is but so far it looks like you try to
validate DT against detected device. That's not how it should work.
Best regards,
Krzysztof
> > Hi Krzysztof,
> >
> >>> Add compatible strings for the twelve lan969x SKU's (Stock Keeping Unit)
> >>> that we support, and verify that the devicetree target is supported by
> >>> the chip target.
> >>>
> >>> Each SKU supports different bandwidths and features (see [1] for
> >>> details). We want to be able to run a SKU with a lower bandwidth and/or
> >>> feature set, than what is supported by the actual chip. In order to
> >>> accomplish this we:
> >>>
> >>> - add new field sparx5->target_dt that reflects the target from the
> >>> devicetree (compatible string).
> >>>
> >>> - compare the devicetree target with the actual chip target. If the
> >>> bandwidth and features provided by the devicetree target is
> >>> supported by the chip, we approve - otherwise reject.
> >>>
> >>> - set the core clock and features based on the devicetree target
> >>>
> >>> [1] https://www.microchip.com/en-us/product/lan9698
> >>>
> >>> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> >>> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> >>> ---
> >>> drivers/net/ethernet/microchip/sparx5/Makefile | 1 +
> >>> .../net/ethernet/microchip/sparx5/sparx5_main.c | 194 ++++++++++++++++++++-
> >>> .../net/ethernet/microchip/sparx5/sparx5_main.h | 1 +
> >>> 3 files changed, 193 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile
> >>> index 3435ca86dd70..8fe302415563 100644
> >>> --- a/drivers/net/ethernet/microchip/sparx5/Makefile
> >>> +++ b/drivers/net/ethernet/microchip/sparx5/Makefile
> >>> @@ -19,3 +19,4 @@ sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o
> >>> # Provide include files
> >>> ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/vcap
> >>> ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/fdma
> >>> +ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/lan969x
> >>> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> >>> index 5c986c373b3e..edbe639d98c5 100644
> >>> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> >>> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> >>> @@ -24,6 +24,8 @@
> >>> #include <linux/types.h>
> >>> #include <linux/reset.h>
> >>>
> >>> +#include "lan969x.h" /* lan969x_desc */
> >>> +
> >>> #include "sparx5_main_regs.h"
> >>> #include "sparx5_main.h"
> >>> #include "sparx5_port.h"
> >>> @@ -227,6 +229,168 @@ bool is_sparx5(struct sparx5 *sparx5)
> >>> }
> >>> }
> >>>
> >>> +/* Set the devicetree target based on the compatible string */
> >>> +static int sparx5_set_target_dt(struct sparx5 *sparx5)
> >>> +{
> >>> + struct device_node *node = sparx5->pdev->dev.of_node;
> >>> +
> >>> + if (is_sparx5(sparx5))
> >>> + /* For Sparx5 the devicetree target is always the chip target */
> >>> + sparx5->target_dt = sparx5->target_ct;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9691-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9691VAO;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9692-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9692VAO;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9693-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9693VAO;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9694-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9695-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694TSN;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9696-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9697-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696TSN;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9698-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698;
> >>> + else if (of_device_is_compatible(node, "microchip,lan9699-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698TSN;
> >>> + else if (of_device_is_compatible(node, "microchip,lan969a-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694RED;
> >>> + else if (of_device_is_compatible(node, "microchip,lan969b-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696RED;
> >>> + else if (of_device_is_compatible(node, "microchip,lan969c-switch"))
> >>> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698RED;
> >>> + else
> >>> + return -EINVAL;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +/* Compare the devicetree target with the chip target.
> >>> + * Make sure the chip target supports the features and bandwidth requested
> >>> + * from the devicetree target.
> >>> + */
> >>> +static int sparx5_verify_target(struct sparx5 *sparx5)
> >>> +{
> >>> + switch (sparx5->target_dt) {
> >>> + case SPX5_TARGET_CT_7546:
> >>> + case SPX5_TARGET_CT_7549:
> >>> + case SPX5_TARGET_CT_7552:
> >>> + case SPX5_TARGET_CT_7556:
> >>> + case SPX5_TARGET_CT_7558:
> >>> + case SPX5_TARGET_CT_7546TSN:
> >>> + case SPX5_TARGET_CT_7549TSN:
> >>> + case SPX5_TARGET_CT_7552TSN:
> >>> + case SPX5_TARGET_CT_7556TSN:
> >>> + case SPX5_TARGET_CT_7558TSN:
> >>> + return 0;
> >>
> >> All this is weird. Why would you verify? You were matched, it cannot be
> >> mis-matching.
> >
> > We are verifying that the match (target/compatible string) from the
> > device tree is supported by the chip. Maybe I wasn't too clear about the
> > intend in v1.
> >
> > Each target supports different bandwidths and features. If you have a
> > lan9698 chip, it must, obviously, be possible to run it as a lan9698
> > target. However, some targets can be run on chip targets other than
> > themselves, given that the chip supports the bandwidth and features of
> > the provided target. In contrary, trying to run as a target with a
> > feature not supported by the chip, or a bandwidth higher than what the
> > chip supports, should be rejected.
>
> But you are not supposed to compare DT with what you auto-detected.
> Detect your hardware, test if it is supported and then bail out.
>
> None of above explains the code.
>
> >
> > Without this logic, the chip id is read and a target is determined. That
> > means on a lan9698 chip you will always match the lan9698 target.
>
> That's not the job of kernel.
>
> >
> > With the new logic, it is possible to run as a different target than
> > what is read from the chip id, given that the target you are trying to
> > run as, is supported by the chip.
>
> So just run on different target.
>
> >
> >>
> >>> + case SPX5_TARGET_CT_LAN9698RED:
> >>> + if (sparx5->target_ct == SPX5_TARGET_CT_LAN9698RED)
> >>
> >> What is "ct"? sorry, all this code is a big no.
> >
> > In this case we were matched as a SPX5_TARGET_CT_LAN9698RED target. We
> > are verifying that the chip target (target_ct, which is read from the
> > chip) supports the target we were matched as.
> >
> >> Krzysztof
> >>
> >
> > This is a feature that we would like, as it gives the flexibility of
> > running different targets on the same chip. Now if this is something
> > that cannot be accepted, I will have to ditch this part.
>
> I have no clue what the "target" is but so far it looks like you try to
> validate DT against detected device. That's not how it should work.
I will get rid of the verification in v2.
Thanks.
/Daniel
>
> Best regards,
> Krzysztof
>
On Mon, Oct 21, 2024 at 03:58:51PM +0200, Daniel Machon wrote:
> Add compatible strings for the twelve lan969x SKU's (Stock Keeping Unit)
> that we support, and verify that the devicetree target is supported by
> the chip target.
>
> Each SKU supports different bandwidths and features (see [1] for
> details). We want to be able to run a SKU with a lower bandwidth and/or
> feature set, than what is supported by the actual chip. In order to
> accomplish this we:
>
> - add new field sparx5->target_dt that reflects the target from the
> devicetree (compatible string).
>
> - compare the devicetree target with the actual chip target. If the
> bandwidth and features provided by the devicetree target is
> supported by the chip, we approve - otherwise reject.
>
> - set the core clock and features based on the devicetree target
>
> [1] https://www.microchip.com/en-us/product/lan9698
>
> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
> drivers/net/ethernet/microchip/sparx5/Makefile | 1 +
> .../net/ethernet/microchip/sparx5/sparx5_main.c | 194 ++++++++++++++++++++-
> .../net/ethernet/microchip/sparx5/sparx5_main.h | 1 +
> 3 files changed, 193 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile
> index 3435ca86dd70..8fe302415563 100644
> --- a/drivers/net/ethernet/microchip/sparx5/Makefile
> +++ b/drivers/net/ethernet/microchip/sparx5/Makefile
> @@ -19,3 +19,4 @@ sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o
> # Provide include files
> ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/vcap
> ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/fdma
> +ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/lan969x
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> index 5c986c373b3e..edbe639d98c5 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> @@ -24,6 +24,8 @@
> #include <linux/types.h>
> #include <linux/reset.h>
>
> +#include "lan969x.h" /* lan969x_desc */
> +
Hi Daniel,
Perhaps this will change when Krzysztof's comment elsewhere in this thread
is addressed. But as it stands the construction in the above two hunks
appears to cause a build failure.
CC drivers/net/ethernet/microchip/sparx5/sparx5_main.o
In file included from drivers/net/ethernet/microchip/sparx5/sparx5_main.c:27:
./drivers/net/ethernet/microchip/lan969x/lan969x.h:10:10: fatal error: sparx5_main.h: No such file or directory
10 | #include "sparx5_main.h"
| ^~~~~~~~~~~~~~~
My preference would be to move away from adding -I directives and, rather,
use relative includes as is common practice in Networking drivers (at least).
...
--
pw-bot: changes-requested
Hi Simon, > > Add compatible strings for the twelve lan969x SKU's (Stock Keeping Unit) > > that we support, and verify that the devicetree target is supported by > > the chip target. > > > > Each SKU supports different bandwidths and features (see [1] for > > details). We want to be able to run a SKU with a lower bandwidth and/or > > feature set, than what is supported by the actual chip. In order to > > accomplish this we: > > > > - add new field sparx5->target_dt that reflects the target from the > > devicetree (compatible string). > > > > - compare the devicetree target with the actual chip target. If the > > bandwidth and features provided by the devicetree target is > > supported by the chip, we approve - otherwise reject. > > > > - set the core clock and features based on the devicetree target > > > > [1] https://www.microchip.com/en-us/product/lan9698 > > > > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com> > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > --- > > drivers/net/ethernet/microchip/sparx5/Makefile | 1 + > > .../net/ethernet/microchip/sparx5/sparx5_main.c | 194 ++++++++++++++++++++- > > .../net/ethernet/microchip/sparx5/sparx5_main.h | 1 + > > 3 files changed, 193 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile > > index 3435ca86dd70..8fe302415563 100644 > > --- a/drivers/net/ethernet/microchip/sparx5/Makefile > > +++ b/drivers/net/ethernet/microchip/sparx5/Makefile > > @@ -19,3 +19,4 @@ sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o > > # Provide include files > > ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/vcap > > ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/fdma > > +ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/lan969x > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c > > index 5c986c373b3e..edbe639d98c5 100644 > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c > > @@ -24,6 +24,8 @@ > > #include <linux/types.h> > > #include <linux/reset.h> > > > > +#include "lan969x.h" /* lan969x_desc */ > > + > > Hi Daniel, > > Perhaps this will change when Krzysztof's comment elsewhere in this thread > is addressed. But as it stands the construction in the above two hunks > appears to cause a build failure. > > CC drivers/net/ethernet/microchip/sparx5/sparx5_main.o > In file included from drivers/net/ethernet/microchip/sparx5/sparx5_main.c:27: > ./drivers/net/ethernet/microchip/lan969x/lan969x.h:10:10: fatal error: sparx5_main.h: No such file or directory > 10 | #include "sparx5_main.h" > | ^~~~~~~~~~~~~~~ > > My preference would be to move away from adding -I directives and, rather, > use relative includes as is common practice in Networking drivers (at least). > > ... > > -- > pw-bot: changes-requested I didn't see this build failure when I ran my tests, nor did the NIPA tests reveal it. I can only reproduce it if I point to the microchip subdir when building - but maybe that's what you did too? Anyway, I will skip the -I includes and resort to relative includes, as per your request. Thanks. /Daniel
On Tue, Oct 22, 2024 at 12:08:42PM +0000, Daniel Machon wrote: > Hi Simon, > > > > Add compatible strings for the twelve lan969x SKU's (Stock Keeping Unit) > > > that we support, and verify that the devicetree target is supported by > > > the chip target. > > > > > > Each SKU supports different bandwidths and features (see [1] for > > > details). We want to be able to run a SKU with a lower bandwidth and/or > > > feature set, than what is supported by the actual chip. In order to > > > accomplish this we: > > > > > > - add new field sparx5->target_dt that reflects the target from the > > > devicetree (compatible string). > > > > > > - compare the devicetree target with the actual chip target. If the > > > bandwidth and features provided by the devicetree target is > > > supported by the chip, we approve - otherwise reject. > > > > > > - set the core clock and features based on the devicetree target > > > > > > [1] https://www.microchip.com/en-us/product/lan9698 > > > > > > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com> > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > > --- > > > drivers/net/ethernet/microchip/sparx5/Makefile | 1 + > > > .../net/ethernet/microchip/sparx5/sparx5_main.c | 194 ++++++++++++++++++++- > > > .../net/ethernet/microchip/sparx5/sparx5_main.h | 1 + > > > 3 files changed, 193 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile > > > index 3435ca86dd70..8fe302415563 100644 > > > --- a/drivers/net/ethernet/microchip/sparx5/Makefile > > > +++ b/drivers/net/ethernet/microchip/sparx5/Makefile > > > @@ -19,3 +19,4 @@ sparx5-switch-$(CONFIG_DEBUG_FS) += sparx5_vcap_debugfs.o > > > # Provide include files > > > ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/vcap > > > ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/fdma > > > +ccflags-y += -I$(srctree)/drivers/net/ethernet/microchip/lan969x > > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c > > > index 5c986c373b3e..edbe639d98c5 100644 > > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c > > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c > > > @@ -24,6 +24,8 @@ > > > #include <linux/types.h> > > > #include <linux/reset.h> > > > > > > +#include "lan969x.h" /* lan969x_desc */ > > > + > > > > Hi Daniel, > > > > Perhaps this will change when Krzysztof's comment elsewhere in this thread > > is addressed. But as it stands the construction in the above two hunks > > appears to cause a build failure. > > > > CC drivers/net/ethernet/microchip/sparx5/sparx5_main.o > > In file included from drivers/net/ethernet/microchip/sparx5/sparx5_main.c:27: > > ./drivers/net/ethernet/microchip/lan969x/lan969x.h:10:10: fatal error: sparx5_main.h: No such file or directory > > 10 | #include "sparx5_main.h" > > | ^~~~~~~~~~~~~~~ > > > > My preference would be to move away from adding -I directives and, rather, > > use relative includes as is common practice in Networking drivers (at least). > > > > ... > > > > -- > > pw-bot: changes-requested > > I didn't see this build failure when I ran my tests, nor did the NIPA > tests reveal it. I can only reproduce it if I point to the microchip > subdir when building - but maybe that's what you did too? > > Anyway, I will skip the -I includes and resort to relative includes, as > per your request. Thanks. Thanks Daniel, Yes I did see the problem when pointing to the subdir. But I was also able to see it without doing so when using a config based on tinyconfigi on x86_64. I can dig further if you like, or provide that config. But I did see that relative includes (or an extra -I) resolved the problem.
On Mon, Oct 21, 2024 at 03:58:51PM +0200, Daniel Machon wrote:
> @@ -227,6 +229,168 @@ bool is_sparx5(struct sparx5 *sparx5)
> }
> }
>
> +/* Set the devicetree target based on the compatible string */
> +static int sparx5_set_target_dt(struct sparx5 *sparx5)
> +{
> + struct device_node *node = sparx5->pdev->dev.of_node;
> +
> + if (is_sparx5(sparx5))
> + /* For Sparx5 the devicetree target is always the chip target */
> + sparx5->target_dt = sparx5->target_ct;
> + else if (of_device_is_compatible(node, "microchip,lan9691-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9691VAO;
> + else if (of_device_is_compatible(node, "microchip,lan9692-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9692VAO;
> + else if (of_device_is_compatible(node, "microchip,lan9693-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9693VAO;
> + else if (of_device_is_compatible(node, "microchip,lan9694-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694;
> + else if (of_device_is_compatible(node, "microchip,lan9695-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694TSN;
> + else if (of_device_is_compatible(node, "microchip,lan9696-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696;
> + else if (of_device_is_compatible(node, "microchip,lan9697-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696TSN;
> + else if (of_device_is_compatible(node, "microchip,lan9698-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698;
> + else if (of_device_is_compatible(node, "microchip,lan9699-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698TSN;
> + else if (of_device_is_compatible(node, "microchip,lan969a-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9694RED;
> + else if (of_device_is_compatible(node, "microchip,lan969b-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9696RED;
> + else if (of_device_is_compatible(node, "microchip,lan969c-switch"))
> + sparx5->target_dt = SPX5_TARGET_CT_LAN9698RED;
Do not re-implement match data.
>
Hi Krzysztof,
> On Mon, Oct 21, 2024 at 03:58:51PM +0200, Daniel Machon wrote:
> > @@ -227,6 +229,168 @@ bool is_sparx5(struct sparx5 *sparx5)
> > }
> > }
> >
> > +/* Set the devicetree target based on the compatible string */
> > +static int sparx5_set_target_dt(struct sparx5 *sparx5)
> > +{
> > + struct device_node *node = sparx5->pdev->dev.of_node;
> > +
> > + if (is_sparx5(sparx5))
> > + /* For Sparx5 the devicetree target is always the chip target */
> > + sparx5->target_dt = sparx5->target_ct;
> > + else if (of_device_is_compatible(node, "microchip,lan9691-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9691VAO;
> > + else if (of_device_is_compatible(node, "microchip,lan9692-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9692VAO;
> > + else if (of_device_is_compatible(node, "microchip,lan9693-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9693VAO;
> > + else if (of_device_is_compatible(node, "microchip,lan9694-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9694;
> > + else if (of_device_is_compatible(node, "microchip,lan9695-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9694TSN;
> > + else if (of_device_is_compatible(node, "microchip,lan9696-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9696;
> > + else if (of_device_is_compatible(node, "microchip,lan9697-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9696TSN;
> > + else if (of_device_is_compatible(node, "microchip,lan9698-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9698;
> > + else if (of_device_is_compatible(node, "microchip,lan9699-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9698TSN;
> > + else if (of_device_is_compatible(node, "microchip,lan969a-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9694RED;
> > + else if (of_device_is_compatible(node, "microchip,lan969b-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9696RED;
> > + else if (of_device_is_compatible(node, "microchip,lan969c-switch"))
> > + sparx5->target_dt = SPX5_TARGET_CT_LAN9698RED;
>
> Do not re-implement match data.
>
> >
Thank you for your feedback.
I will get rid of this function and instread create match data structs
for each SKU.
/Daniel
© 2016 - 2024 Red Hat, Inc.