RE: [PATCH net-next v2 1/2] net: txgbe: Add basic support for new AML devices

["Jiawen Wu" <jiawenwu@trustnetic.com>]

> > +	/* index to unique seq id for each mbox message */
> > +	send_hdr->index = wx->swfw_index;
> > +	send_cmd = send_hdr->cmd;
> > +
> > +	dword_len = length >> 2;
> > +	/* write data to SW-FW mbox array */
> > +	for (i = 0; i < dword_len; i++) {
> > +		wr32a(wx, WX_SW2FW_MBOX, i, (__force u32)cpu_to_le32(buffer[i]));
> > +		/* write flush */
> > +		rd32a(wx, WX_SW2FW_MBOX, i);
> 
> do you really need to flush all registers?

Yes, hardware requires this.

> 
> > +	}
> > +
> > +	/* generate interrupt to notify FW */
> > +	wr32m(wx, WX_SW2FW_MBOX_CMD, WX_SW2FW_MBOX_CMD_VLD, 0);
> > +	wr32m(wx, WX_SW2FW_MBOX_CMD, WX_SW2FW_MBOX_CMD_VLD, WX_SW2FW_MBOX_CMD_VLD);
> > +
> > +	dword_len = hdr_size >> 2;
> > +
> > +	/* polling reply from FW */
> > +	timeout = 50;
> > +	do {
> > +		timeout--;
> > +		usleep_range(1000, 2000);
> > +
> > +		/* read hdr */
> > +		for (bi = 0; bi < dword_len; bi++)
> > +			buffer[bi] = rd32a(wx, WX_FW2SW_MBOX, bi);
> 
> no need for le32_to_cpu()?
> (if so, reexamine whole patch)

Indeed need.

> > +/**
> > + *  wx_host_interface_command - Issue command to manageability block
> > + *  @wx: 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
> 
> In other places you were using cpu_to_le32(), this comment seems to
> contradict that

FW structures are big endian, and we agreed to transfer in the mailbox registers
according to 32-bit little endian. However, buffers contain 8 bits or 16 bits field
which is composed using CPU byte order. So we use cpu_to_le32() and le32_to_cpu()
in this driver.