Re: [PATCH what RFC] ice: implement support for 0x0601 opcode

[Aki <please@ignore.pl>]

On 20/11/2025 11:53, Dawid Osuchowski wrote:
> Signed-off-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>
> ---
> PHY Type support is a little wonky and based on 0x0607, but I wanted to get this patch out for comments.

Of course.  Thank you for sending a patch.

> ---
>   samples/linux_0601.log      |  12 ++
>   what/ice/opcodes/0x0601.lua | 238 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 250 insertions(+)
>   create mode 100644 samples/linux_0601.log
>   create mode 100644 what/ice/opcodes/0x0601.lua
> 
> diff --git a/samples/linux_0601.log b/samples/linux_0601.log
> new file mode 100644
> index 000000000000..23dee128ff59
> --- /dev/null
> +++ b/samples/linux_0601.log
> @@ -0,0 +1,12 @@
> +[    5.000000] ice 0000:01:00.0: AQ Command: opcode 0x0601, flags 0x3400, datalen 0x0018, retval 0x0000
> +	cookie (h,l) 0x00000000 0x00000000
> +	param (0,1)  0x00000000 0x00000000
> +	addr (h,l)   0x00000004 0x3CB75000
> +[    5.000000] ice 0x00000004 0x3CB75000 00000000: 00 40 80 00 00 00 00 00 00 00 00 00 00 00 00 00
> +[    5.000000] ice 0x00000004 0x3CB75000 00000010: e0 00 00 00 00 00 04 00
> +[    5.000000] ice 0000:01:00.0: AQ Response: opcode 0x0601, flags 0x3403, datalen 0x0018, retval 0x0000
> +	cookie (h,l) 0x00000000 0x00000000
> +	param (0,1)  0x00000000 0x00000000
> +	addr (h,l)   0x00000004 0x3CB75000
> +[    5.000000] ice 0x00000004 0x3CB75000 00000000: 00 40 80 00 00 00 00 00 00 00 00 00 00 00 00 00
> +[    5.000000] ice 0x00000004 0x3CB75000 00000010: e0 00 00 00 00 00 04 00

I think we can append to samples/$platform.log for now.  In the long
run, I think having the same directory tree structure under samples/
as in what/.  As in: 0x0601.log would be in samples/ice/opcodes/; but
there is very little stuff available now, plus this was before I started
tinkering with multiple platforms.

Append it to samples/linux.log for now.

The gist is we want

$ make test

to smoke everything that's available.

> diff --git a/what/ice/opcodes/0x0601.lua b/what/ice/opcodes/0x0601.lua
> new file mode 100644
> index 000000000000..e53c51005617
> --- /dev/null
> +++ b/what/ice/opcodes/0x0601.lua
> @@ -0,0 +1,238 @@
> +local blobs = require "what.blobs"
> +local bits = require "what.bits"
> +local flags = require "what.ice.flags"
> +local bit = bits.bit

bit is unused.  Remove it, please.

> +local set_phy_config = {}
> +local dump = [[
> +---
> +opcode: 0x0601 (Set PHY Config)
> +kind: %s %s
> +flags: %s
> +retval: 0x%x
> +cookie: 0x%08x 0x%08x
> +%s...]]
> +
> +
> +function set_phy_config:dump ()
> +	return dump:format(
> +		self.queue, self.kind,
> +		flags.dump(self.flags),
> +		self.retval,
> +		self.cookie_h, self.cookie_l,
> +		self:dump_indirect_buffer())
> +end

Yeah, comment for myself to reduce the code duplication opcodes
boilerplate generates.

> +
> +
> +--- 3.3.3.2.1 Extended PHY Capabilities 128-Bit Word Structure[E810]
> +--- Table 3-90. 128-Bit Word Extended PHY Capabilities[E810]
> +local extended_phy_capabilities = {
> +	-- Word[0]:
> +	"100BASE-TX",
> +	"100M-SGMII",

...

> +	"200G-AUI2-C2C",
> +	nil,
> +	-- Word[3]: every bit is Reserved
> +}

Can you move this to what.ice.extended_phy_capabilities and require it
here and in 0x0607 in a separate patch.  Can be in a series.

> +

...

> +--- Table 3-28. Set PHY Config Command Data Structure[E810]
> +local indirect_buffer = blobs.new {
> +	-- TODO: Consider handling >8 bytes in one range definition.
> +	-- TODO: Consider supporting default value if lookup fails
> +	["0.0-3.7"] = blobs.insert_if_empty("PHY Type", bit_list(extended_phy_capabilities, 32)),
> +	["4.0-7.7"] = blobs.insert_if_empty("PHY Type", bit_list(extended_phy_capabilities, 32, 32)),
> +	["8.0-11.7"] = blobs.insert_if_empty("PHY Type", bit_list(extended_phy_capabilities, 32, 64)),
> +	["12.0-15.7"] = blobs.insert_if_empty("PHY Type", bit_list(extended_phy_capabilities, 32, 96)),

This will not work as expected.  We need something like:

	["0.0-3.7"] = blobs.concat("PHY Type", bit_list(...)),

That would concatenate lists together (or: "append" at the end of
lists/strings).  This does not need a resolution from your side, if you
address comments above but skip this, I'll merge this and work on this
myself since this could be tricky (as in: super Lua-specific).

> +	["16.0-16.1"] = blobs.list "Pause Ability" {

...

> +		[1] = "Strict",
> +	},
> +}

Note to self to cross-check this with datasheet in v2.

Thanks again!