Date:   Thu, 08 Sep 2022 09:38:19 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Mengyuan Lou <mengyuanlou@...-swift.com>, netdev@...r.kernel.org
Cc:     jiawenwu@...-swift.com
Subject: Re: [PATCH net-next 02/02] net: ngbe: Check some hardware functions

On Mon, 2022-09-05 at 20:52 +0800, Mengyuan Lou wrote:
> Check eeprom ready.
> Check whether WOL is enabled.
> Check whether the MAC is available.
> 
> Signed-off-by: Mengyuan Lou <mengyuanlou@...-swift.com>
> ---
>  drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c   | 311 ++++++++++++++++++
>  drivers/net/ethernet/wangxun/ngbe/ngbe_hw.h   |   8 +
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  54 +++
>  3 files changed, 373 insertions(+)
> 
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c
> index 20c21f99e308..f38dc47b8f32 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c
> @@ -381,3 +381,314 @@ int ngbe_reset_hw(struct ngbe_hw *hw)
>  
>  	return 0;
>  }
> +
> +/**
> + *  ngbe_release_eeprom_semaphore - Release hardware semaphore
> + *  @hw: pointer to hardware structure
> + *
> + *  This function clears hardware semaphore bits.
> + **/
> +static void ngbe_release_eeprom_semaphore(struct ngbe_hw *hw)
> +{
> +	wr32m(hw, NGBE_MIS_SWSM, NGBE_MIS_SWSM_SMBI, 0);
> +	ngbe_flush(hw);
> +}
> +
> +/**
> + *  ngbe_get_eeprom_semaphore - Get hardware semaphore
> + *  @hw: pointer to hardware structure
> + *  Sets the hardware semaphores so EEPROM access can occur for bit-bang method
> + **/
> +static int ngbe_get_eeprom_semaphore(struct ngbe_hw *hw)
> +{
> +	int status = 0;
> +	u32 times = 10;
> +	u32 i;
> +	u32 swsm;
> +
> +	/* Get SMBI software semaphore between device drivers first */
> +	for (i = 0; i < times; i++) {
> +		/* If the SMBI bit is 0 when we read it, then the bit will be
> +		 * set and we have the semaphore
> +		 */
> +		status = read_poll_timeout(rd32, swsm, !(swsm & NGBE_MIS_SWSM_SMBI), 50,
> +					   20000, false, hw, NGBE_MIS_SWSM);
> +		if (!status)
> +			return 0;
> +	}
> +
> +	if (i == times) {
> +		dev_err(ngbe_hw_to_dev(hw),
> +			"Driver can't access the Eeprom - SMBI Semaphore not granted.\n");
> +		/* this release is particularly important because our attempts
> +		 * above to get the semaphore may have succeeded, and if there
> +		 * was a timeout, we should unconditionally clear the semaphore
> +		 * bits to free the driver to make progress
> +		 */
> +		ngbe_release_eeprom_semaphore(hw);

If I read correctly, the firmware is setting/clearing the semaphore bit
to avoid conflicts with the kernel/driver code. I'm wondering if the
above clear would inconditionally mark the semaphore as free even if
the firmware is still holding it? (causing later bugs, hard to track).

It's not clear to me why such clear is needed.

If the later test will succeed, the driver will emit an error and will
acquire the semaphore successfully, and that will be most confusing.

> +		status = -EBUSY;
> +	}
> +	/* one last try
> +	 * If the SMBI bit is 0 when we read it, then the bit will be
> +	 * set and we have the semaphore
> +	 */
> +	swsm = rd32(hw, NGBE_MIS_SWSM);
> +	if (!(swsm & NGBE_MIS_SWSM_SMBI))
> +		status = 0;
> +	return status;
> +}
> +
> +/**
> + *  ngbe_acquire_swfw_sync - Acquire SWFW semaphore
> + *  @hw: pointer to hardware structure
> + *  @mask: Mask to specify which semaphore to acquire
> + *
> + *  Acquires the SWFW semaphore through the GSSR register for the specified
> + *  function (CSR, PHY0, PHY1, EEPROM, Flash)
> + **/
> +static int ngbe_acquire_swfw_sync(struct ngbe_hw *hw, u32 mask)
> +{
> +	u32 gssr = 0;
> +	u32 swmask = mask;
> +	u32 fwmask = mask << 16;
> +	u32 times = 2000;
> +	u32 i;

Please respect the reverse x-mas tree declaration order. Other
instances later and in the previous patch.

> +
> +	for (i = 0; i < times; i++) {
> +		/* SW NVM semaphore bit is used for access to all
> +		 * SW_FW_SYNC bits (not just NVM)
> +		 */
> +		if (ngbe_get_eeprom_semaphore(hw))
> +			return -EBUSY;
> +
> +		gssr = rd32(hw, NGBE_MNG_SWFW_SYNC);
> +		if (!(gssr & (fwmask | swmask))) {
> +			gssr |= swmask;
> +			wr32(hw, NGBE_MNG_SWFW_SYNC, gssr);
> +			ngbe_release_eeprom_semaphore(hw);
> +			return 0;
> +		}
> +		/* Resource is currently in use by FW or SW */
> +		ngbe_release_eeprom_semaphore(hw);
> +	}
> +
> +	/* If time expired clear the bits holding the lock and retry */
> +	if (gssr & (fwmask | swmask))
> +		ngbe_release_swfw_sync(hw, gssr & (fwmask | swmask));
> +
> +	return -EBUSY;
> +}
> +
> +/**
> + *  ngbe_release_swfw_sync - Release SWFW semaphore
> + *  @hw: pointer to hardware structure
> + *  @mask: Mask to specify which semaphore to release
> + *
> + *  Releases the SWFW semaphore through the GSSR register for the specified
> + *  function (CSR, PHY0, PHY1, EEPROM, Flash)
> + **/
> +void ngbe_release_swfw_sync(struct ngbe_hw *hw, u32 mask)
> +{
> +	ngbe_get_eeprom_semaphore(hw);

Why can't ngbe_get_eeprom_semaphore() fail here? 

> +	wr32m(hw, NGBE_MNG_SWFW_SYNC, mask, 0);
> +	ngbe_release_eeprom_semaphore(hw);
> +}
> +
> +/**
> + *  ngbe_host_interface_command - Issue command to manageability block
> + *  @hw: pointer to the HW structure
> + *  @buffer: contains the command to write and where the return status will
> + *   be placed
> + *  @length: length of buffer, must be multiple of 4 bytes
> + *  @timeout: time in ms to wait for command completion
> + *  @return_data: read and return data from the buffer (true) or not (false)
> + *   Needed because FW structures are big endian and decoding of
> + *   these fields can be 8 bit or 16 bit based on command. Decoding
> + *   is not easily understood without making a table of commands.
> + *   So we will leave this up to the caller to read back the data
> + *   in these cases.
> + **/
> +int ngbe_host_interface_command(struct ngbe_hw *hw, u32 *buffer,
> +				u32 length, u32 timeout, bool return_data)
> +{
> +	u32 hicr, i, bi;
> +	u32 hdr_size = sizeof(struct ngbe_hic_hdr);
> +	u16 buf_len;
> +	u32 dword_len;
> +	int err = 0;
> +	u32 buf[64] = {};
> +
> +	if (length == 0 || length > NGBE_HI_MAX_BLOCK_BYTE_LENGTH) {
> +		dev_err(ngbe_hw_to_dev(hw),
> +			"Buffer length failure buffersize=%d.\n", length);
> +		return -EINVAL;
> +	}
> +
> +	if (ngbe_acquire_swfw_sync(hw, NGBE_MNG_SWFW_SYNC_SW_MB) != 0)
> +		return -EBUSY;
> +
> +	/* Calculate length in DWORDs. We must be DWORD aligned */
> +	if ((length % (sizeof(u32))) != 0) {
> +		dev_err(ngbe_hw_to_dev(hw),
> +			"Buffer length failure, not aligned to dword");
> +		err = -EINVAL;
> +		goto rel_out;
> +	}
> +
> +	/*read to clean all status*/
> +	hicr = rd32(hw, NGBE_MNG_MBOX_CTL);
> +	if ((hicr & NGBE_MNG_MBOX_CTL_FWRDY))
> +		dev_err(ngbe_hw_to_dev(hw),
> +			"fwrdy is set before command.\n");
> +	dword_len = length >> 2;
> +	/* The device driver writes the relevant command block
> +	 * into the ram area.
> +	 */
> +	for (i = 0; i < dword_len; i++)
> +		wr32a(hw, NGBE_MNG_MBOX, i, buffer[i]);
> +
> +	/* Setting this bit tells the ARC that a new command is pending. */
> +	wr32m(hw, NGBE_MNG_MBOX_CTL,
> +	      NGBE_MNG_MBOX_CTL_SWRDY, NGBE_MNG_MBOX_CTL_SWRDY);
> +
> +	for (i = 0; i < timeout; i++) {
> +		err = read_poll_timeout(rd32, hicr, hicr & NGBE_MNG_MBOX_CTL_FWRDY, 1000,
> +					20000, false, hw, NGBE_MNG_MBOX_CTL);
> +		if (!err)
> +			break;
> +	}
> +
> +	buf[0] = rd32(hw, NGBE_MNG_MBOX);
> +	/* Check command completion */
> +	if (timeout != 0 && i == timeout) {
> +		dev_err(ngbe_hw_to_dev(hw),
> +			"Command has failed with no status valid.\n");
> +		if ((buffer[0] & 0xff) != (~buf[0] >> 24)) {
> +			err = -EINVAL;
> +			goto rel_out;
> +		}
> +	}
> +
> +	if (!return_data)
> +		goto rel_out;
> +
> +	/* Calculate length in DWORDs */
> +	dword_len = hdr_size >> 2;
> +
> +	/* first pull in the header so we know the buffer length */
> +	for (bi = 0; bi < dword_len; bi++)
> +		buffer[bi] = rd32a(hw, NGBE_MNG_MBOX, bi);
> +
> +	/* If there is any thing in data position pull it in */
> +	buf_len = ((struct ngbe_hic_hdr *)buffer)->buf_len;
> +	if (buf_len == 0)
> +		goto rel_out;
> +
> +	if (length < buf_len + hdr_size) {
> +		dev_err(ngbe_hw_to_dev(hw),
> +			"Buffer not large enough for reply message.\n");
> +		err = -ENOMEM;
> +		goto rel_out;
> +	}
> +
> +	/* Calculate length in DWORDs, add 3 for odd lengths */
> +	dword_len = (buf_len + 3) >> 2;
> +
> +	/* Pull in the rest of the buffer (bi is where we left off) */
> +	for (; bi <= dword_len; bi++)
> +		buffer[bi] = rd32a(hw, NGBE_MNG_MBOX, bi);
> +
> +rel_out:
> +	ngbe_release_swfw_sync(hw, NGBE_MNG_SWFW_SYNC_SW_MB);
> +	return err;
> +}
> +
> +/**
> + *  ngbe_init_eeprom_params - Initialize EEPROM params
> + *  @hw: pointer to hardware structure
> + *
> + *  Initializes the EEPROM parameters ngbe_eeprom_info within the
> + *  ngbe_hw struct in order to set up EEPROM access.
> + **/
> +void ngbe_init_eeprom_params(struct ngbe_hw *hw)
> +{
> +	struct ngbe_eeprom_info *eeprom = &hw->eeprom;
> +
> +	eeprom->semaphore_delay = 10;
> +	eeprom->word_size = 1024 >> 1;
> +	eeprom->sw_region_offset = 0x80;
> +}
> +
> +int ngbe_eeprom_chksum_hostif(struct ngbe_hw *hw)
> +{
> +	int tmp;
> +	int status;
> +	struct ngbe_hic_read_shadow_ram buffer;
> +
> +	buffer.hdr.req.cmd = NGBE_FW_EEPROM_CHECKSUM_CMD;
> +	buffer.hdr.req.buf_lenh = 0;
> +	buffer.hdr.req.buf_lenl = 0;
> +	buffer.hdr.req.checksum = NGBE_FW_CMD_DEFAULT_CHECKSUM;
> +	/* convert offset from words to bytes */
> +	buffer.address = 0;
> +	/* one word */
> +	buffer.length = 0;
> +
> +	status = ngbe_host_interface_command(hw, (u32 *)&buffer,
> +					     sizeof(buffer),
> +					     NGBE_HI_COMMAND_TIMEOUT, false);
> +
> +	if (status < 0)
> +		return status;
> +	tmp = (u32)rd32a(hw, NGBE_MNG_MBOX, 1);

If you declare tmp as u32 the above cast will be needed, also a more
meaningful variable name would help

> +	if (tmp == 0x80658383)

Why this magic mumber? perhaps you can define/use some driver constant.

> +		status = 0;

or just:
		return 0;
	return -EIO;

unless you plan to touch this code path with later patches.

> +	else
> +		return -EIO;
> +
> +	return status;
> +}
> +
> +static int ngbe_read_ee_hostif_data32(struct ngbe_hw *hw, u16 offset, u32 *data)
> +{
> +	int status;
> +	struct ngbe_hic_read_shadow_ram buffer;
> +
> +	buffer.hdr.req.cmd = NGBE_FW_READ_SHADOW_RAM_CMD;
> +	buffer.hdr.req.buf_lenh = 0;
> +	buffer.hdr.req.buf_lenl = NGBE_FW_READ_SHADOW_RAM_LEN;
> +	buffer.hdr.req.checksum = NGBE_FW_CMD_DEFAULT_CHECKSUM;
> +	/* convert offset from words to bytes */
> +	buffer.address = (__force u32)cpu_to_be32(offset * 2);
> +	/* one word */
> +	buffer.length = (__force u16)cpu_to_be16(sizeof(u32));
> +
> +	status = ngbe_host_interface_command(hw, (u32 *)&buffer,
> +					     sizeof(buffer),
> +					     NGBE_HI_COMMAND_TIMEOUT, false);
> +	if (status)
> +		return status;
> +	*data = (u32)rd32a(hw, NGBE_MNG_MBOX, NGBE_FW_NVM_DATA_OFFSET);

Cast likely not needed.


Cheers,

Paolo

Powered by blists - more mailing lists