[Intel-wired-lan] [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Intel-wired-lan] [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Jacob Keller]

From: Paul Greenwalt <paul.greenwalt@intel.com>

With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
setting the AQ command read flag, and since the get command is a direct
command there is no need to set the read flag.

Fix this by only setting read flag on set command.

Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644
--- a/drivers/net/ethernet/intel/ice/ice_ddp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
@@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
 			cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM |
 					  ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW;
 
-		if (ice_is_e825c(hw))
-			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
 	} else {
 		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo);
 		cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM;
-	}
 
-	if (!ice_is_e825c(hw))
-		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+		if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 &&
+		    hw->mac_type != ICE_MAC_E830)
+			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+	}
 
 	status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd);
 	if (status)

---
base-commit: f5da7c45188eea71394bf445655cae2df88a7788
change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a

Best regards,
-- 
Jacob Keller <jacob.e.keller@intel.com>

[PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Jacob Keller]

From: Paul Greenwalt <paul.greenwalt@intel.com>

With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
setting the AQ command read flag, and since the get command is a direct
command there is no need to set the read flag.

Fix this by only setting read flag on set command.

Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644
--- a/drivers/net/ethernet/intel/ice/ice_ddp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
@@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
 			cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM |
 					  ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW;
 
-		if (ice_is_e825c(hw))
-			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
 	} else {
 		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo);
 		cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM;
-	}
 
-	if (!ice_is_e825c(hw))
-		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+		if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 &&
+		    hw->mac_type != ICE_MAC_E830)
+			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+	}
 
 	status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd);
 	if (status)

---
base-commit: f5da7c45188eea71394bf445655cae2df88a7788
change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a

Best regards,
-- 
Jacob Keller <jacob.e.keller@intel.com>

Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Michal Swiatkowski]

On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote:
> From: Paul Greenwalt <paul.greenwalt@intel.com>
> 
> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
> setting the AQ command read flag, and since the get command is a direct
> command there is no need to set the read flag.
> 
> Fix this by only setting read flag on set command.

Why it isn't true for other hw? I mean, why not:
if (set)
	RD_FLAG
else 
	NOT_RD_FLAG
Other hw needs RD flag in case of get too?

> 

Don't you need fixes tag?
> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
> index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
> @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
>  			cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM |
>  					  ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW;
>  
> -		if (ice_is_e825c(hw))
> -			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>  	} else {
>  		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo);
>  		cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM;
> -	}
>  
> -	if (!ice_is_e825c(hw))
> -		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +		if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 &&
> +		    hw->mac_type != ICE_MAC_E830)
> +			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +	}
>  
>  	status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd);
>  	if (status)
>

In general looks fine, only one question.

Thanks,
Michal

> ---
> base-commit: f5da7c45188eea71394bf445655cae2df88a7788
> change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a
> 
> Best regards,
> -- 
> Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Michal Swiatkowski]

On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote:
> From: Paul Greenwalt <paul.greenwalt@intel.com>
> 
> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
> setting the AQ command read flag, and since the get command is a direct
> command there is no need to set the read flag.
> 
> Fix this by only setting read flag on set command.

Why it isn't true for other hw? I mean, why not:
if (set)
	RD_FLAG
else 
	NOT_RD_FLAG
Other hw needs RD flag in case of get too?

> 

Don't you need fixes tag?
> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
> index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
> @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
>  			cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM |
>  					  ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW;
>  
> -		if (ice_is_e825c(hw))
> -			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>  	} else {
>  		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo);
>  		cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM;
> -	}
>  
> -	if (!ice_is_e825c(hw))
> -		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +		if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 &&
> +		    hw->mac_type != ICE_MAC_E830)
> +			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +	}
>  
>  	status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd);
>  	if (status)
>

In general looks fine, only one question.

Thanks,
Michal

> ---
> base-commit: f5da7c45188eea71394bf445655cae2df88a7788
> change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a
> 
> Best regards,
> -- 
> Jacob Keller <jacob.e.keller@intel.com>

Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Jacob Keller]

On 2/19/2025 1:37 AM, Michal Swiatkowski wrote:
> On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote:
>> From: Paul Greenwalt <paul.greenwalt@intel.com>
>>
>> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
>> setting the AQ command read flag, and since the get command is a direct
>> command there is no need to set the read flag.
>>
>> Fix this by only setting read flag on set command.
> 
> Why it isn't true for other hw? I mean, why not:
> if (set)
> 	RD_FLAG
> else 
> 	NOT_RD_FLAG
> Other hw needs RD flag in case of get too?
> 

From what I understand, we didn't anticipate this flow changing. E810
and E822 hardware require FLAG_RD for both get and set, while E825-C and
E830 expect FLAG_RD only for set, but not for get.

>>
> 
> Don't you need fixes tag?

You're correct. I'll add it in v2

>> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
>>  			cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM |
>>  					  ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW;
>>  
>> -		if (ice_is_e825c(hw))
>> -			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>> +		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>>  	} else {
>>  		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo);
>>  		cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM;
>> -	}
>>  
>> -	if (!ice_is_e825c(hw))
>> -		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>> +		if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 &&
>> +		    hw->mac_type != ICE_MAC_E830)
>> +			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>> +	}
>>  
>>  	status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd);
>>  	if (status)
>>
> 
> In general looks fine, only one question.
> 
> Thanks,
> Michal

Thanks for the review, I'll send a v2 with this cleaned up and include a
fixes tag.

> 
>> ---
>> base-commit: f5da7c45188eea71394bf445655cae2df88a7788
>> change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a
>>
>> Best regards,
>> -- 
>> Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Jacob Keller]

On 2/19/2025 1:37 AM, Michal Swiatkowski wrote:
> On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote:
>> From: Paul Greenwalt <paul.greenwalt@intel.com>
>>
>> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
>> setting the AQ command read flag, and since the get command is a direct
>> command there is no need to set the read flag.
>>
>> Fix this by only setting read flag on set command.
> 
> Why it isn't true for other hw? I mean, why not:
> if (set)
> 	RD_FLAG
> else 
> 	NOT_RD_FLAG
> Other hw needs RD flag in case of get too?
> 

From what I understand, we didn't anticipate this flow changing. E810
and E822 hardware require FLAG_RD for both get and set, while E825-C and
E830 expect FLAG_RD only for set, but not for get.

>>
> 
> Don't you need fixes tag?

You're correct. I'll add it in v2

>> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
>>  			cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM |
>>  					  ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW;
>>  
>> -		if (ice_is_e825c(hw))
>> -			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>> +		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>>  	} else {
>>  		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo);
>>  		cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM;
>> -	}
>>  
>> -	if (!ice_is_e825c(hw))
>> -		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>> +		if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 &&
>> +		    hw->mac_type != ICE_MAC_E830)
>> +			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>> +	}
>>  
>>  	status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd);
>>  	if (status)
>>
> 
> In general looks fine, only one question.
> 
> Thanks,
> Michal

Thanks for the review, I'll send a v2 with this cleaned up and include a
fixes tag.

> 
>> ---
>> base-commit: f5da7c45188eea71394bf445655cae2df88a7788
>> change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a
>>
>> Best regards,
>> -- 
>> Jacob Keller <jacob.e.keller@intel.com>

Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Michal Swiatkowski]

On Thu, Feb 20, 2025 at 02:45:41PM -0800, Jacob Keller wrote:
> 
> 
> On 2/19/2025 1:37 AM, Michal Swiatkowski wrote:
> > On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote:
> >> From: Paul Greenwalt <paul.greenwalt@intel.com>
> >>
> >> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
> >> setting the AQ command read flag, and since the get command is a direct
> >> command there is no need to set the read flag.
> >>
> >> Fix this by only setting read flag on set command.
> > 
> > Why it isn't true for other hw? I mean, why not:
> > if (set)
> > 	RD_FLAG
> > else 
> > 	NOT_RD_FLAG
> > Other hw needs RD flag in case of get too?
> > 
> 
> From what I understand, we didn't anticipate this flow changing. E810
> and E822 hardware require FLAG_RD for both get and set, while E825-C and
> E830 expect FLAG_RD only for set, but not for get.
> 

Thanks for explanation. Seems resonable from driver perspective and not
so reasonable from firmware, but maybe this difference is somehow
important.

Thanks,
Michal

> >>
> > 
> > Don't you need fixes tag?
> 
> You're correct. I'll add it in v2
> 
> >> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> >> ---
> >>  drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
> >> index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
> >> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
> >> @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
> >>  			cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM |
> >>  					  ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW;
> >>  
> >> -		if (ice_is_e825c(hw))
> >> -			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> >> +		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> >>  	} else {
> >>  		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo);
> >>  		cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM;
> >> -	}
> >>  
> >> -	if (!ice_is_e825c(hw))
> >> -		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> >> +		if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 &&
> >> +		    hw->mac_type != ICE_MAC_E830)
> >> +			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> >> +	}
> >>  
> >>  	status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd);
> >>  	if (status)
> >>
> > 
> > In general looks fine, only one question.
> > 
> > Thanks,
> > Michal
> 
> Thanks for the review, I'll send a v2 with this cleaned up and include a
> fixes tag.
> 
> > 
> >> ---
> >> base-commit: f5da7c45188eea71394bf445655cae2df88a7788
> >> change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a
> >>
> >> Best regards,
> >> -- 
> >> Jacob Keller <jacob.e.keller@intel.com>
> 

Re: [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Michal Swiatkowski]

On Thu, Feb 20, 2025 at 02:45:41PM -0800, Jacob Keller wrote:
> 
> 
> On 2/19/2025 1:37 AM, Michal Swiatkowski wrote:
> > On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote:
> >> From: Paul Greenwalt <paul.greenwalt@intel.com>
> >>
> >> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
> >> setting the AQ command read flag, and since the get command is a direct
> >> command there is no need to set the read flag.
> >>
> >> Fix this by only setting read flag on set command.
> > 
> > Why it isn't true for other hw? I mean, why not:
> > if (set)
> > 	RD_FLAG
> > else 
> > 	NOT_RD_FLAG
> > Other hw needs RD flag in case of get too?
> > 
> 
> From what I understand, we didn't anticipate this flow changing. E810
> and E822 hardware require FLAG_RD for both get and set, while E825-C and
> E830 expect FLAG_RD only for set, but not for get.
> 

Thanks for explanation. Seems resonable from driver perspective and not
so reasonable from firmware, but maybe this difference is somehow
important.

Thanks,
Michal

> >>
> > 
> > Don't you need fixes tag?
> 
> You're correct. I'll add it in v2
> 
> >> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> >> ---
> >>  drivers/net/ethernet/intel/ice/ice_ddp.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
> >> index 03988be03729b76e96188864896527060c8c4d5b..49bd49ab3ccf36c990144894e887341459377a2d 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
> >> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
> >> @@ -2345,15 +2345,15 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
> >>  			cmd->set_flags |= ICE_AQC_TX_TOPO_FLAGS_SRC_RAM |
> >>  					  ICE_AQC_TX_TOPO_FLAGS_LOAD_NEW;
> >>  
> >> -		if (ice_is_e825c(hw))
> >> -			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> >> +		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> >>  	} else {
> >>  		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_tx_topo);
> >>  		cmd->get_flags = ICE_AQC_TX_TOPO_GET_RAM;
> >> -	}
> >>  
> >> -	if (!ice_is_e825c(hw))
> >> -		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> >> +		if (hw->mac_type != ICE_MAC_GENERIC_3K_E825 &&
> >> +		    hw->mac_type != ICE_MAC_E830)
> >> +			desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> >> +	}
> >>  
> >>  	status = ice_aq_send_cmd(hw, &desc, buf, buf_size, cd);
> >>  	if (status)
> >>
> > 
> > In general looks fine, only one question.
> > 
> > Thanks,
> > Michal
> 
> Thanks for the review, I'll send a v2 with this cleaned up and include a
> fixes tag.
> 
> > 
> >> ---
> >> base-commit: f5da7c45188eea71394bf445655cae2df88a7788
> >> change-id: 20250218-jk-e830-ddp-loading-fix-9efdbdfc270a
> >>
> >> Best regards,
> >> -- 
> >> Jacob Keller <jacob.e.keller@intel.com>
> 

Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Jacob Keller]

On 2/20/2025 11:29 PM, Michal Swiatkowski wrote:
> On Thu, Feb 20, 2025 at 02:45:41PM -0800, Jacob Keller wrote:
>>
>>
>> On 2/19/2025 1:37 AM, Michal Swiatkowski wrote:
>>> On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote:
>>>> From: Paul Greenwalt <paul.greenwalt@intel.com>
>>>>
>>>> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
>>>> setting the AQ command read flag, and since the get command is a direct
>>>> command there is no need to set the read flag.
>>>>
>>>> Fix this by only setting read flag on set command.
>>>
>>> Why it isn't true for other hw? I mean, why not:
>>> if (set)
>>> 	RD_FLAG
>>> else 
>>> 	NOT_RD_FLAG
>>> Other hw needs RD flag in case of get too?
>>>
>>
>> From what I understand, we didn't anticipate this flow changing. E810
>> and E822 hardware require FLAG_RD for both get and set, while E825-C and
>> E830 expect FLAG_RD only for set, but not for get.
>>
> 
> Thanks for explanation. Seems resonable from driver perspective and not
> so reasonable from firmware, but maybe this difference is somehow
> important.
> 
> Thanks,
> Michal
> 

Yea, its unfortunate that this changed at all, but we can't really go
back on released firmware versions.

The RD_FLAG is described in the datasheet as:

Flags.RD: Set by driver to indicate that FW needs to read indirect buffer.

It is intended to indicate the presence of the indirect buffer vs a
direct command. I believe what has likely happened is that this is
interpreted in a strict manner so missing the flag causes the commands
to fail. Its quite weird that older firmware required it for a direct
command, so someone probably changed this thinking thats strange..

I haven't been able to dig up more info about this or why it was changed.

Thanks,
Jake

Re: [PATCH iwl-net] ice: fix Get Tx Topology AQ command error on E830

[Jacob Keller]

On 2/20/2025 11:29 PM, Michal Swiatkowski wrote:
> On Thu, Feb 20, 2025 at 02:45:41PM -0800, Jacob Keller wrote:
>>
>>
>> On 2/19/2025 1:37 AM, Michal Swiatkowski wrote:
>>> On Tue, Feb 18, 2025 at 04:46:34PM -0800, Jacob Keller wrote:
>>>> From: Paul Greenwalt <paul.greenwalt@intel.com>
>>>>
>>>> With E830 Get Tx Topology AQ command (opcode 0x0418) returns an error when
>>>> setting the AQ command read flag, and since the get command is a direct
>>>> command there is no need to set the read flag.
>>>>
>>>> Fix this by only setting read flag on set command.
>>>
>>> Why it isn't true for other hw? I mean, why not:
>>> if (set)
>>> 	RD_FLAG
>>> else 
>>> 	NOT_RD_FLAG
>>> Other hw needs RD flag in case of get too?
>>>
>>
>> From what I understand, we didn't anticipate this flow changing. E810
>> and E822 hardware require FLAG_RD for both get and set, while E825-C and
>> E830 expect FLAG_RD only for set, but not for get.
>>
> 
> Thanks for explanation. Seems resonable from driver perspective and not
> so reasonable from firmware, but maybe this difference is somehow
> important.
> 
> Thanks,
> Michal
> 

Yea, its unfortunate that this changed at all, but we can't really go
back on released firmware versions.

The RD_FLAG is described in the datasheet as:

Flags.RD: Set by driver to indicate that FW needs to read indirect buffer.

It is intended to indicate the presence of the indirect buffer vs a
direct command. I believe what has likely happened is that this is
interpreted in a strict manner so missing the flag causes the commands
to fail. Its quite weird that older firmware required it for a direct
command, so someone probably changed this thinking thats strange..

I haven't been able to dig up more info about this or why it was changed.

Thanks,
Jake