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

[Eric Woudstra <ericwouds@gmail.com>]

On 3/5/24 14:54, Andrew Lunn wrote:
> 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.

I'll add it.

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

I'll write a dedicated modify function.


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

So far, I only know of the BananaPi-R3mini, which I am using. It has 2GB
of ram. It should be ok.

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

I'll rename host to mcu.

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

Then I will keep this part of the code as in
mt798x_phy_led_hw_control_set(), only adding 2500Mbps.

>>> +	/* Select mode 1, the only mode supported */
> 
>> Maybe a comment about what mode 1 actually is?

After consulting Airoha, I can change it to:

+	/* Select mode 1, the only mode supported.
+	 * The en8811h configures the SerDes as fixed hsgmii.
+	 */

Best Regards,

Eric Woudstra