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
