Message-ID: <99541533-625e-4ffb-b980-b2bcd016cfeb@lunn.ch>
Date: Tue, 5 Mar 2024 14:54:19 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Eric Woudstra <ericwouds@...il.com>
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

On Tue, Mar 05, 2024 at 09:13:41AM +0100, Eric Woudstra wrote:
> 
> 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.

Or just add an S. checkpatch does not like camElcAse. So ms will call
a warning. But from context we know it is not mega seconds.

> >> +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.

So there is the potential to have set_mode true when not actually
performing a read/modify/write. Maybe have a dedicated modify
function, and don't expose set_mode?

> 
> >> +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.

What class of device is this? 128K for a PC is nothing. For an OpenWRT
router with 128M of RAM, it might be worth using the other API.

> 
> >> +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.

That is a very PHY centric view of the world. I would say the host is
what is running Linux. I assume this is the datahsheets naming? Maybe
cpu, or mcu is a better name?

> 
> > 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.

The idea is that we first have Linux blink the LEDs in software. This
is controlled via the files in /sys/class/leds/FOO/{link|rx|tx}
etc. If the hardware can do the same blink pattern, it can then be
offloaded to the hardware.

If you disable hardware offload, just have set brightness, can you do
the same pattern?

As i said, vendors do all sorts of odd things with LEDs. I would
prefer we have a common subset most PHY support, and not try to
support every strange mode.

    Andrew

Powered by blists - more mailing lists