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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aWEj4py2Cv4tPu-5@shell.armlinux.org.uk>
Date: Fri, 9 Jan 2026 15:50:58 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Clément Léger <clement.leger@...tlin.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Magnus Damm <magnus.damm@...il.com>,
	linux-renesas-soc@...r.kernel.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Biju Das <biju.das.jz@...renesas.com>,
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH net-next v2 2/2] net: pcs: rzn1-miic: Add support for PHY
 link active-level configuration

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!