[PATCH net-next v2 0/2] Add support for PHY link active-level configuration in RZN1 MIIC driver

[PATCH net-next v2 0/2] Add support for PHY link active-level configuration in RZN1 MIIC driver

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series introduces support for configuring the active level of
the PHY-link signals in the Renesas RZN1 MIIC driver. The first patch adds
a new device tree binding property `renesas,miic-phylink-active-low` to
specify whether the PHY-link signals are active low. The second patch
implements the logic in the driver to read this property and configure the
MIIC_PHYLINK register accordingly.

v1->v2:
- Updated commit message to elaborate the necessity of PHY link signals.

Cheers,
Prabhakar

Lad Prabhakar (2):
  dt-bindings: net: pcs: renesas,rzn1-miic: Add
    renesas,miic-phylink-active-low property
  net: pcs: rzn1-miic: Add support for PHY link active-level
    configuration

 .../bindings/net/pcs/renesas,rzn1-miic.yaml   |   7 ++
 drivers/net/pcs/pcs-rzn1-miic.c               | 108 +++++++++++++++++-
 2 files changed, 113 insertions(+), 2 deletions(-)

-- 
2.52.0

[PATCH net-next v2 1/2] dt-bindings: net: pcs: renesas,rzn1-miic: Add renesas,miic-phylink-active-low property

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add the renesas,miic-phylink-active-low property to allow configuring the
active level of PHY link status signals provided by the MIIC block.

EtherPHY link-up and link-down status is required as a hardware feature
independent of whether GMAC or ETHSW is used. With GMAC, link status is
obtained via MDC/MDIO and handled in software. In contrast, ETHSW exposes
dedicated PHY link pins that provide this information directly in
hardware.

These PHY link signals are required not only for host-controlled traffic
but also for switch-only forwarding paths where frames are exchanged
between external nodes without CPU involvement. This is particularly
important for redundancy protocols such as DLR (Device Level Ring),
which depend on fast detection of link-down events caused by cable or
port failures. Handling such events purely in software introduces
latency, which is why ETHSW provides dedicated hardware link pins.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2:
- Updated commit message to elaborate the necessity of PHY link signals.
---
 .../devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml     | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
index 3adbcf56d2be..825ae8a91e8b 100644
--- a/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
+++ b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
@@ -86,6 +86,13 @@ patternProperties:
           and include/dt-bindings/net/renesas,r9a09g077-pcs-miic.h for RZ/N2H, RZ/T2H SoCs.
         $ref: /schemas/types.yaml#/definitions/uint32
 
+      renesas,miic-phylink-active-low:
+        type: boolean
+        description: Indicates that the PHY-link signal provided by the Ethernet switch,
+          EtherCAT, or SERCOS3 interface is active low. When present, this property
+          sets the corresponding signal polarity to active low. When omitted, the signal
+          defaults to active high.
+
     required:
       - reg
       - renesas,miic-input
-- 
2.52.0

Re: [PATCH net-next v2 1/2] dt-bindings: net: pcs: renesas,rzn1-miic: Add renesas,miic-phylink-active-low property

[Rob Herring (Arm)]

On Fri, 09 Jan 2026 14:22:49 +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add the renesas,miic-phylink-active-low property to allow configuring the
> active level of PHY link status signals provided by the MIIC block.
> 
> EtherPHY link-up and link-down status is required as a hardware feature
> independent of whether GMAC or ETHSW is used. With GMAC, link status is
> obtained via MDC/MDIO and handled in software. In contrast, ETHSW exposes
> dedicated PHY link pins that provide this information directly in
> hardware.
> 
> These PHY link signals are required not only for host-controlled traffic
> but also for switch-only forwarding paths where frames are exchanged
> between external nodes without CPU involvement. This is particularly
> important for redundancy protocols such as DLR (Device Level Ring),
> which depend on fast detection of link-down events caused by cable or
> port failures. Handling such events purely in software introduces
> latency, which is why ETHSW provides dedicated hardware link pins.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v1->v2:
> - Updated commit message to elaborate the necessity of PHY link signals.
> ---
>  .../devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml     | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH net-next v2 2/2] net: pcs: rzn1-miic: Add support for PHY link active-level configuration

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add support to configure the PHY link signal active level per converter
using the DT property "renesas,miic-phylink-active-low".

Introduce the MIIC_PHYLINK register definition and extend the MIIC driver
with a new `phylink` structure to store the mask and value for PHY link
configuration. Implement `miic_configure_phylink()` to determine the bit
position and polarity for each port based on the SoC type, such as RZ/N1
or RZ/T2H/N2H.

The accumulated configuration is stored during DT parsing and applied
later in `miic_probe()` after hardware initialization, since the MIIC
registers can only be modified safely once the hardware setup is complete.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2:
- No changes.
---
 drivers/net/pcs/pcs-rzn1-miic.c | 108 +++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
index 885f17c32643..cc090f27e559 100644
--- a/drivers/net/pcs/pcs-rzn1-miic.c
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -28,6 +28,8 @@
 
 #define MIIC_MODCTRL			0x8
 
+#define MIIC_PHYLINK			0x14
+
 #define MIIC_CONVCTRL(port)		(0x100 + (port) * 4)
 
 #define MIIC_CONVCTRL_CONV_SPEED	GENMASK(1, 0)
@@ -177,6 +179,16 @@ static const char * const rzt2h_reset_ids[] = {
 	"crst",
 };
 
+/**
+ * struct phylink - Phylink configuration
+ * @mask: Mask of phylink bits
+ * @val: Value of phylink bits
+ */
+struct phylink {
+	u32 mask;
+	u32 val;
+};
+
 /**
  * struct miic - MII converter structure
  * @base: base address of the MII converter
@@ -184,6 +196,7 @@ static const char * const rzt2h_reset_ids[] = {
  * @lock: Lock used for read-modify-write access
  * @rsts: Reset controls for the MII converter
  * @of_data: Pointer to OF data
+ * @phylink: Phylink configuration
  */
 struct miic {
 	void __iomem *base;
@@ -191,6 +204,12 @@ struct miic {
 	spinlock_t lock;
 	struct reset_control_bulk_data rsts[MIIC_MAX_NUM_RSTS];
 	const struct miic_of_data *of_data;
+	struct phylink phylink;
+};
+
+enum miic_type {
+	MIIC_TYPE_RZN1,
+	MIIC_TYPE_RZT2H,
 };
 
 /**
@@ -210,6 +229,7 @@ struct miic {
  * @init_unlock_lock_regs: Flag to indicate if registers need to be unlocked
  *  before access.
  * @miic_write: Function pointer to write a value to a MIIC register
+ * @type: Type of MIIC
  */
 struct miic_of_data {
 	struct modctrl_match *match_table;
@@ -226,6 +246,7 @@ struct miic_of_data {
 	u8 reset_count;
 	bool init_unlock_lock_regs;
 	void (*miic_write)(struct miic *miic, int offset, u32 value);
+	enum miic_type type;
 };
 
 /**
@@ -581,10 +602,82 @@ static int miic_match_dt_conf(struct miic *miic, s8 *dt_val, u32 *mode_cfg)
 	return -EINVAL;
 }
 
+static void miic_configure_phylink(struct miic *miic, u32 conf,
+				   u32 port, bool active_low)
+{
+	bool polarity_active_high;
+	u32 mask, val;
+	int shift;
+
+	/* determine shift and polarity for this conf */
+	if (miic->of_data->type == MIIC_TYPE_RZN1) {
+		switch (conf) {
+		/* switch ports => bits [3:0] (shift 0), active when low */
+		case MIIC_SWITCH_PORTA:
+		case MIIC_SWITCH_PORTB:
+		case MIIC_SWITCH_PORTC:
+		case MIIC_SWITCH_PORTD:
+			shift = 0;
+			polarity_active_high = false;
+			break;
+
+		/* EtherCAT ports => bits [7:4] (shift 4), active when high */
+		case MIIC_ETHERCAT_PORTA:
+		case MIIC_ETHERCAT_PORTB:
+		case MIIC_ETHERCAT_PORTC:
+			shift = 4;
+			polarity_active_high = true;
+			break;
+
+		/* Sercos ports => bits [11:8] (shift 8), active when high */
+		case MIIC_SERCOS_PORTA:
+		case MIIC_SERCOS_PORTB:
+			shift = 8;
+			polarity_active_high = true;
+			break;
+
+		default:
+			return;
+		}
+	} else {
+		switch (conf) {
+		/* ETHSW ports => bits [3:0] (shift 0), active when low */
+		case ETHSS_ETHSW_PORT0:
+		case ETHSS_ETHSW_PORT1:
+		case ETHSS_ETHSW_PORT2:
+			shift = 0;
+			polarity_active_high = false;
+			break;
+
+		/* ESC ports => bits [7:4] (shift 4), active when high */
+		case ETHSS_ESC_PORT0:
+		case ETHSS_ESC_PORT1:
+		case ETHSS_ESC_PORT2:
+			shift = 4;
+			polarity_active_high = true;
+			break;
+
+		default:
+			return;
+		}
+	}
+
+	mask = BIT(port) << shift;
+
+	if (polarity_active_high)
+		val = (active_low ? 0 : BIT(port)) << shift;
+	else
+		val = (active_low ? BIT(port) : 0) << shift;
+
+	miic->phylink.mask |= mask;
+	miic->phylink.val = (miic->phylink.val & ~mask) | (val & mask);
+}
+
 static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
 {
 	struct device_node *np = miic->dev->of_node;
 	struct device_node *conv;
+	bool active_low;
 	int port, ret;
 	s8 *dt_val;
 	u32 conf;
@@ -605,8 +698,15 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
 
 		/* Adjust for 0 based index */
 		port += !miic->of_data->miic_port_start;
-		if (of_property_read_u32(conv, "renesas,miic-input", &conf) == 0)
-			dt_val[port] = conf;
+		if (of_property_read_u32(conv, "renesas,miic-input", &conf))
+			continue;
+
+		dt_val[port] = conf;
+
+		active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");
+
+		miic_configure_phylink(miic, conf, port - !miic->of_data->miic_port_start,
+				       active_low);
 	}
 
 	ret = miic_match_dt_conf(miic, dt_val, mode_cfg);
@@ -696,6 +796,8 @@ static int miic_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_runtime_pm;
 
+	miic_reg_rmw(miic, MIIC_PHYLINK, miic->phylink.mask, miic->phylink.val);
+
 	/* miic_create() relies on that fact that data are attached to the
 	 * platform device to determine if the driver is ready so this needs to
 	 * be the last thing to be done after everything is initialized
@@ -729,6 +831,7 @@ static struct miic_of_data rzn1_miic_of_data = {
 	.sw_mode_mask = GENMASK(4, 0),
 	.init_unlock_lock_regs = true,
 	.miic_write = miic_reg_writel_unlocked,
+	.type = MIIC_TYPE_RZN1,
 };
 
 static struct miic_of_data rzt2h_miic_of_data = {
@@ -745,6 +848,7 @@ static struct miic_of_data rzt2h_miic_of_data = {
 	.reset_ids = rzt2h_reset_ids,
 	.reset_count = ARRAY_SIZE(rzt2h_reset_ids),
 	.miic_write = miic_reg_writel_locked,
+	.type = MIIC_TYPE_RZT2H,
 };
 
 static const struct of_device_id miic_of_mtable[] = {
-- 
2.52.0

Re: [PATCH net-next v2 2/2] net: pcs: rzn1-miic: Add support for PHY link active-level configuration

[Russell King (Oracle)]

On Fri, Jan 09, 2026 at 02:22:50PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add support to configure the PHY link signal active level per converter
> using the DT property "renesas,miic-phylink-active-low".
> 
> Introduce the MIIC_PHYLINK register definition and extend the MIIC driver
> with a new `phylink` structure to store the mask and value for PHY link
> configuration. Implement `miic_configure_phylink()` to determine the bit
> position and polarity for each port based on the SoC type, such as RZ/N1
> or RZ/T2H/N2H.
> 
> The accumulated configuration is stored during DT parsing and applied
> later in `miic_probe()` after hardware initialization, since the MIIC
> registers can only be modified safely once the hardware setup is complete.

Please do not re-use "phylink", we have a subsystem in the kernel named
as such, and, for example, it too defines "struct phylink".

> +/**
> + * struct phylink - Phylink configuration
> + * @mask: Mask of phylink bits
> + * @val: Value of phylink bits
> + */
> +struct phylink {
> +	u32 mask;
> +	u32 val;
> +};
> +

You don't get a warning for this, because, although you have:

#include <linux/phylink.h>

which delares "struct phylink" as:

struct phylink;

The definition of this structure is entirely private to
drivers/net/phy/phylink.c and is intentionally not exposed.

By redefining "struct phylink" here, it means that anyone using gdb
is going to run into problems - which version of this struct is the
right one for any particular pointer.

You describe this feature as "PHY-link" and "PHY link" in your commit
and cover messages. Please use "phy_link" and "PHY_LINK" as identifies
for this so that grep can distinguish between your PHY link feature
and the phylink infrastructure.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 2/2] net: pcs: rzn1-miic: Add support for PHY link active-level configuration

[Lad, Prabhakar]

Hi Russell,

Thank you for the review.

On Fri, Jan 9, 2026 at 2:46 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Fri, Jan 09, 2026 at 02:22:50PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add support to configure the PHY link signal active level per converter
> > using the DT property "renesas,miic-phylink-active-low".
> >
> > Introduce the MIIC_PHYLINK register definition and extend the MIIC driver
> > with a new `phylink` structure to store the mask and value for PHY link
> > configuration. Implement `miic_configure_phylink()` to determine the bit
> > position and polarity for each port based on the SoC type, such as RZ/N1
> > or RZ/T2H/N2H.
> >
> > The accumulated configuration is stored during DT parsing and applied
> > later in `miic_probe()` after hardware initialization, since the MIIC
> > registers can only be modified safely once the hardware setup is complete.
>
> Please do not re-use "phylink", we have a subsystem in the kernel named
> as such, and, for example, it too defines "struct phylink".
>
> > +/**
> > + * struct phylink - Phylink configuration
> > + * @mask: Mask of phylink bits
> > + * @val: Value of phylink bits
> > + */
> > +struct phylink {
> > +     u32 mask;
> > +     u32 val;
> > +};
> > +
>
> You don't get a warning for this, because, although you have:
>
> #include <linux/phylink.h>
>
> which delares "struct phylink" as:
>
> struct phylink;
>
> The definition of this structure is entirely private to
> drivers/net/phy/phylink.c and is intentionally not exposed.
>
> By redefining "struct phylink" here, it means that anyone using gdb
> is going to run into problems - which version of this struct is the
> right one for any particular pointer.
>
Ack, I will rename the struct to `miic_phy_link_cfg`.

> You describe this feature as "PHY-link" and "PHY link" in your commit
> and cover messages. Please use "phy_link" and "PHY_LINK" as identifies
> for this so that grep can distinguish between your PHY link feature
> and the phylink infrastructure.
>
Ok, I will use "phy_link"/ "PHY_LINK".

Cheers,
Prabhakar

Re: [PATCH net-next v2 2/2] net: pcs: rzn1-miic: Add support for PHY link active-level configuration

[Russell King (Oracle)]

On Fri, Jan 09, 2026 at 02:22:50PM +0000, Prabhakar wrote:
> +static void miic_configure_phylink(struct miic *miic, u32 conf,
> +				   u32 port, bool active_low)
> +{
> +	bool polarity_active_high;
> +	u32 mask, val;
> +	int shift;
> +
> +	/* determine shift and polarity for this conf */
> +	if (miic->of_data->type == MIIC_TYPE_RZN1) {
> +		switch (conf) {
> +		/* switch ports => bits [3:0] (shift 0), active when low */
> +		case MIIC_SWITCH_PORTA:
> +		case MIIC_SWITCH_PORTB:
> +		case MIIC_SWITCH_PORTC:
> +		case MIIC_SWITCH_PORTD:
> +			shift = 0;
> +			polarity_active_high = false;
> +			break;
> +
> +		/* EtherCAT ports => bits [7:4] (shift 4), active when high */
> +		case MIIC_ETHERCAT_PORTA:
> +		case MIIC_ETHERCAT_PORTB:
> +		case MIIC_ETHERCAT_PORTC:
> +			shift = 4;
> +			polarity_active_high = true;
> +			break;
> +
> +		/* Sercos ports => bits [11:8] (shift 8), active when high */
> +		case MIIC_SERCOS_PORTA:
> +		case MIIC_SERCOS_PORTB:
> +			shift = 8;
> +			polarity_active_high = true;
> +			break;
> +
> +		default:
> +			return;
> +		}
> +	} else {
> +		switch (conf) {
> +		/* ETHSW ports => bits [3:0] (shift 0), active when low */
> +		case ETHSS_ETHSW_PORT0:
> +		case ETHSS_ETHSW_PORT1:
> +		case ETHSS_ETHSW_PORT2:
> +			shift = 0;
> +			polarity_active_high = false;
> +			break;
> +
> +		/* ESC ports => bits [7:4] (shift 4), active when high */
> +		case ETHSS_ESC_PORT0:
> +		case ETHSS_ESC_PORT1:
> +		case ETHSS_ESC_PORT2:
> +			shift = 4;
> +			polarity_active_high = true;
> +			break;
> +
> +		default:
> +			return;
> +		}
> +	}
> +
> +	mask = BIT(port) << shift;
> +
> +	if (polarity_active_high)
> +		val = (active_low ? 0 : BIT(port)) << shift;
> +	else
> +		val = (active_low ? BIT(port) : 0) << shift;

Looking closer at this, I think this is confusing.

The underlying purpose here is to set mask and val to change the state of
a single bit in the PHY link register for each call to this function,
accumulating the changes in your misnamed "struct phylink".

Given that "mask" can be used to compute the value to describe the bit,
and that is made up of "shift" that describes the start of the bitfield
and "port" that describes the bit within the bitfield, then surely:

	mask = BIT(port + shift);

would be saner?

Next, the creation of "val". This is either zero or the same value of
mask depending on active_low and polarity_active_high. The truth table
here is:

polarity_active_high	active_low	result
0			0		0
0			1		mask
1			0		mask
1			1		0

This is a classical an exclusive-or truth table in the world of logic,
or could be regarded as an inquality relationship (result is mask
when polarity_active_high differs from active_low, otherwise zero).

Thus:

	/* Set the bit when polarity_active_high differs from active_low */
	val = polarity_active_high != active_low ? mask : 0;

Or, even simpler, this could become overall:

	mask = BIT(port + shift);

	miic->phylink.mask |= mask;
	if (polarity_active_high != active_low)
		miic->phylink.val |= mask;
	else
		miic->phylink.val &= ~mask;

> @@ -605,8 +698,15 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
>  
>  		/* Adjust for 0 based index */
>  		port += !miic->of_data->miic_port_start;
> -		if (of_property_read_u32(conv, "renesas,miic-input", &conf) == 0)
> -			dt_val[port] = conf;
> +		if (of_property_read_u32(conv, "renesas,miic-input", &conf))
> +			continue;
> +
> +		dt_val[port] = conf;
> +
> +		active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");
> +
> +		miic_configure_phylink(miic, conf, port - !miic->of_data->miic_port_start,
> +				       active_low);

I think this is also over-complicated. Wouldn't it be better to only
deal with the miic_port_start at the one place that it matters?

                if (of_property_read_u32(conv, "reg", &port))
                        continue;

                if (of_property_read_u32(conv, "renesas,miic-input", &conf))
                        continue;

                dt_val[port + !miic->of_data->miic_port_start] = conf;

                active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");

                miic_configure_phylink(miic, conf, port, active_low);

?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 2/2] net: pcs: rzn1-miic: Add support for PHY link active-level configuration

[Lad, Prabhakar]

Hi Russell,

Thank you for the review.

On Fri, Jan 9, 2026 at 3:51 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Fri, Jan 09, 2026 at 02:22:50PM +0000, Prabhakar wrote:
> > +static void miic_configure_phylink(struct miic *miic, u32 conf,
> > +                                u32 port, bool active_low)
> > +{
> > +     bool polarity_active_high;
> > +     u32 mask, val;
> > +     int shift;
> > +
> > +     /* determine shift and polarity for this conf */
> > +     if (miic->of_data->type == MIIC_TYPE_RZN1) {
> > +             switch (conf) {
> > +             /* switch ports => bits [3:0] (shift 0), active when low */
> > +             case MIIC_SWITCH_PORTA:
> > +             case MIIC_SWITCH_PORTB:
> > +             case MIIC_SWITCH_PORTC:
> > +             case MIIC_SWITCH_PORTD:
> > +                     shift = 0;
> > +                     polarity_active_high = false;
> > +                     break;
> > +
> > +             /* EtherCAT ports => bits [7:4] (shift 4), active when high */
> > +             case MIIC_ETHERCAT_PORTA:
> > +             case MIIC_ETHERCAT_PORTB:
> > +             case MIIC_ETHERCAT_PORTC:
> > +                     shift = 4;
> > +                     polarity_active_high = true;
> > +                     break;
> > +
> > +             /* Sercos ports => bits [11:8] (shift 8), active when high */
> > +             case MIIC_SERCOS_PORTA:
> > +             case MIIC_SERCOS_PORTB:
> > +                     shift = 8;
> > +                     polarity_active_high = true;
> > +                     break;
> > +
> > +             default:
> > +                     return;
> > +             }
> > +     } else {
> > +             switch (conf) {
> > +             /* ETHSW ports => bits [3:0] (shift 0), active when low */
> > +             case ETHSS_ETHSW_PORT0:
> > +             case ETHSS_ETHSW_PORT1:
> > +             case ETHSS_ETHSW_PORT2:
> > +                     shift = 0;
> > +                     polarity_active_high = false;
> > +                     break;
> > +
> > +             /* ESC ports => bits [7:4] (shift 4), active when high */
> > +             case ETHSS_ESC_PORT0:
> > +             case ETHSS_ESC_PORT1:
> > +             case ETHSS_ESC_PORT2:
> > +                     shift = 4;
> > +                     polarity_active_high = true;
> > +                     break;
> > +
> > +             default:
> > +                     return;
> > +             }
> > +     }
> > +
> > +     mask = BIT(port) << shift;
> > +
> > +     if (polarity_active_high)
> > +             val = (active_low ? 0 : BIT(port)) << shift;
> > +     else
> > +             val = (active_low ? BIT(port) : 0) << shift;
>
> Looking closer at this, I think this is confusing.
>
> The underlying purpose here is to set mask and val to change the state of
> a single bit in the PHY link register for each call to this function,
> accumulating the changes in your misnamed "struct phylink".
>
> Given that "mask" can be used to compute the value to describe the bit,
> and that is made up of "shift" that describes the start of the bitfield
> and "port" that describes the bit within the bitfield, then surely:
>
>         mask = BIT(port + shift);
>
> would be saner?
>
Agreed.

> Next, the creation of "val". This is either zero or the same value of
> mask depending on active_low and polarity_active_high. The truth table
> here is:
>
> polarity_active_high    active_low      result
> 0                       0               0
> 0                       1               mask
> 1                       0               mask
> 1                       1               0
>
> This is a classical an exclusive-or truth table in the world of logic,
> or could be regarded as an inquality relationship (result is mask
> when polarity_active_high differs from active_low, otherwise zero).
>
> Thus:
>
>         /* Set the bit when polarity_active_high differs from active_low */
>         val = polarity_active_high != active_low ? mask : 0;
>
> Or, even simpler, this could become overall:
>
>         mask = BIT(port + shift);
>
>         miic->phylink.mask |= mask;
>         if (polarity_active_high != active_low)
>                 miic->phylink.val |= mask;
>         else
>                 miic->phylink.val &= ~mask;
>
Ack, this simplifies the code. I’ll rework the code along the lines
you suggested.

> > @@ -605,8 +698,15 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
> >
> >               /* Adjust for 0 based index */
> >               port += !miic->of_data->miic_port_start;
> > -             if (of_property_read_u32(conv, "renesas,miic-input", &conf) == 0)
> > -                     dt_val[port] = conf;
> > +             if (of_property_read_u32(conv, "renesas,miic-input", &conf))
> > +                     continue;
> > +
> > +             dt_val[port] = conf;
> > +
> > +             active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");
> > +
> > +             miic_configure_phylink(miic, conf, port - !miic->of_data->miic_port_start,
> > +                                    active_low);
>
> I think this is also over-complicated. Wouldn't it be better to only
> deal with the miic_port_start at the one place that it matters?
>
>                 if (of_property_read_u32(conv, "reg", &port))
>                         continue;
>
>                 if (of_property_read_u32(conv, "renesas,miic-input", &conf))
>                         continue;
>
>                 dt_val[port + !miic->of_data->miic_port_start] = conf;
>
>                 active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");
>
>                 miic_configure_phylink(miic, conf, port, active_low);
>
> ?
>
Agreed with your point about miic_port_start. Handling the offset only
at the point where dt_val[] is indexed, and passing the unmodified
port into miic_configure_phylink(), simplifies the flow and avoids the
current back-and-forth adjustments. I’ll restructure that part
accordingly.

I’ll post an updated version with these cleanups in the next revision.

Cheers,
Prabhakar