netdev - Re: [PATCH v2 net-next 2/2] net: phy: air_en8811h: Add the Airoha EN8811H PHY driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <b27e44db-d9c5-49f0-8b81-2f55cfaacb4d@gmail.com>
Date: Tue, 5 Mar 2024 09:13:41 +0100
From: Eric Woudstra <ericwouds@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiner Kallweit <hkallweit1@...il.com>,
 Russell King <linux@...linux.org.uk>,
 Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Frank Wunderlich <frank-w@...lic-files.de>,
 Daniel Golle <daniel@...rotopia.org>, Lucien Jheng
 <lucien.jheng@...oha.com>, Zhi-Jun You <hujy652@...tonmail.com>,
 netdev@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 net-next 2/2] net: phy: air_en8811h: Add the Airoha
 EN8811H PHY driver

Hi Andrew,

First of all, thanks for taking the time to look at the code so
extensively.

On 3/3/24 18:29, Andrew Lunn wrote:
>> +enum {
>> +	AIR_PHY_LED_DUR_BLINK_32M,
>> +	AIR_PHY_LED_DUR_BLINK_64M,
>> +	AIR_PHY_LED_DUR_BLINK_128M,
>> +	AIR_PHY_LED_DUR_BLINK_256M,
>> +	AIR_PHY_LED_DUR_BLINK_512M,
>> +	AIR_PHY_LED_DUR_BLINK_1024M,
> 
> DUR meaning duration? It has a blinks on for a little over a
> kilometre? So a wave length of a little over 2 kilometres, or a
> frequency of around 0.0005Hz :-)

It is the M for milliseconds. I can add a comment to clarify this.
If you set to 1024M, it will blink with a period of a roughly 1 second.

>> +static int __air_buckpbus_reg_write(struct phy_device *phydev,
>> +				    u32 pbus_address, u32 pbus_data,
>> +				    bool set_mode)
>> +{
>> +	int ret;
>> +
>> +	if (set_mode) {
>> +		ret = __phy_write(phydev, AIR_BPBUS_MODE,
>> +				  AIR_BPBUS_MODE_ADDR_FIXED);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
> 
> What does set_mode mean?

I use this boolean to prevent writing the same value twice to the
AIR_BPBUS_MODE register, when doing an atomic modify operation. The
AIR_BPBUS_MODE is already set in the read operation, so it does not
need to be set again to the same value at the write operation.
Sadly, the address registers for read and write are different, so
I could not optimize the modify operation any more.

>> +static int en8811h_load_firmware(struct phy_device *phydev)
>> +{
>> +	struct device *dev = &phydev->mdio.dev;
>> +	const struct firmware *fw1, *fw2;
>> +	int ret;
>> +
>> +	ret = request_firmware_direct(&fw1, EN8811H_MD32_DM, dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = request_firmware_direct(&fw2, EN8811H_MD32_DSP, dev);
>> +	if (ret < 0)
>> +		goto en8811h_load_firmware_rel1;
>> +
> 
> How big are these firmwares? This will map the entire contents into
> memory. There is an alternative interface which allows you to get the
> firmware in chunks. I the firmware is big, just getting 4K at a time
> might be better, especially if this is an OpenWRT class device.

The file sizes are 131072 and 16384 bytes. If you think this is too big,
I could look into using the alternative interface.

>> +static int en8811h_restart_host(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
>> +				     EN8811H_FW_CTRL_1_START);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
>> +				     EN8811H_FW_CTRL_1_FINISH);
>> +}
> 
> What is host in this context?

This is the EN8811H internal host to the PHY.

> Vendors do like making LED control unique. I've not seen any other MAC
> or PHY where you can blink for activity at a given speed. You cannot
> have 10 and 100 at the same time, so why are there different bits for
> them?
> 
> I _think_ this can be simplified
> ...
> Does this work?

I started out with that, but the hardware can do more. It allows
for a setup as described:

 100M link up triggers led0, only led0 blinking on traffic
1000M link up triggers led1, only led1 blinking on traffic
2500M link up triggers led0 and led1, both blinking on traffic

#define AIR_DEFAULT_TRIGGER_LED0 (BIT(TRIGGER_NETDEV_LINK_2500) | \
				 BIT(TRIGGER_NETDEV_LINK_100)  | \
				 BIT(TRIGGER_NETDEV_RX)        | \
				 BIT(TRIGGER_NETDEV_TX))
#define AIR_DEFAULT_TRIGGER_LED1 (BIT(TRIGGER_NETDEV_LINK_2500) | \
				 BIT(TRIGGER_NETDEV_LINK_1000) | \
				 BIT(TRIGGER_NETDEV_RX)        | \
				 BIT(TRIGGER_NETDEV_TX))

With the simpler code and just the slightest traffic, both leds
are blinking and no way to read the speed anymore from the leds.

So I modified it to make the most use of the possibilities of the
EN881H hardware. The EN8811H can then be used with a standard 2-led
rj45 socket.