[PATCH net-next 0/3][pull request] 100GbE Intel Wired LAN Driver Updates 2022-05-17

[PATCH net-next 0/3][pull request] 100GbE Intel Wired LAN Driver Updates 2022-05-17

[Tony Nguyen]

This series contains updates to ice driver only.

Karol changes calculation of u16 to unsigned int for GNSS related
calculations. He also adds implementation for GNSS write; data is
written to the GNSS module through TTY device using u-blox UBX protocol.

The following are changes since commit 65a9dedc11d615d8f104a48d38b4fa226967b4ed:
  net: phy: marvell: Add errata section 5.1 for Alaska PHY
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 100GbE

Karol Kolacinski (3):
  ice: remove u16 arithmetic in ice_gnss
  ice: add i2c write command
  ice: add write functionality for GNSS TTY

 drivers/net/ethernet/intel/ice/ice.h          |   4 +-
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   7 +-
 drivers/net/ethernet/intel/ice/ice_common.c   |  51 +++-
 drivers/net/ethernet/intel/ice/ice_common.h   |   4 +
 drivers/net/ethernet/intel/ice/ice_gnss.c     | 252 +++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_gnss.h     |  26 +-
 6 files changed, 304 insertions(+), 40 deletions(-)

-- 
2.35.1

[PATCH net-next 1/3] ice: remove u16 arithmetic in ice_gnss

[Tony Nguyen]

From: Karol Kolacinski <karol.kolacinski@intel.com>

Change u16 to unsigned int where arithmetic occurs.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_gnss.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 57586a2e6dec..c6d755f707aa 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -17,13 +17,13 @@ static void ice_gnss_read(struct kthread_work *work)
 	struct gnss_serial *gnss = container_of(work, struct gnss_serial,
 						read_work.work);
 	struct ice_aqc_link_topo_addr link_topo;
-	u8 i2c_params, bytes_read;
+	unsigned int i, bytes_read, data_len;
 	struct tty_port *port;
 	struct ice_pf *pf;
 	struct ice_hw *hw;
 	__be16 data_len_b;
 	char *buf = NULL;
-	u16 i, data_len;
+	u8 i2c_params;
 	int err = 0;
 
 	pf = gnss->back;
@@ -65,7 +65,7 @@ static void ice_gnss_read(struct kthread_work *work)
 		mdelay(10);
 	}
 
-	data_len = min(data_len, (u16)PAGE_SIZE);
+	data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
 	data_len = tty_buffer_request_room(port, data_len);
 	if (!data_len) {
 		err = -ENOMEM;
@@ -74,9 +74,10 @@ static void ice_gnss_read(struct kthread_work *work)
 
 	/* Read received data */
 	for (i = 0; i < data_len; i += bytes_read) {
-		u16 bytes_left = data_len - i;
+		unsigned int bytes_left = data_len - i;
 
-		bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
+		bytes_read = min_t(typeof(bytes_left), bytes_left,
+				   ICE_MAX_I2C_DATA_SIZE);
 
 		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
 				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
-- 
2.35.1

[PATCH net-next 2/3] ice: add i2c write command

[Tony Nguyen]

From: Karol Kolacinski <karol.kolacinski@intel.com>

Add the possibility to write to connected i2c devices using the AQ
command. FW may reject the write if the device is not on allowlist.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  7 +--
 drivers/net/ethernet/intel/ice/ice_common.c   | 51 ++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_common.h   |  4 ++
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index b25e27c4d887..bedc19f12cbd 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1401,7 +1401,7 @@ struct ice_aqc_get_link_topo {
 	u8 rsvd[9];
 };
 
-/* Read I2C (direct, 0x06E2) */
+/* Read/Write I2C (direct, 0x06E2/0x06E3) */
 struct ice_aqc_i2c {
 	struct ice_aqc_link_topo_addr topo_addr;
 	__le16 i2c_addr;
@@ -1411,7 +1411,7 @@ struct ice_aqc_i2c {
 
 	u8 rsvd;
 	__le16 i2c_bus_addr;
-	u8 rsvd2[4];
+	u8 i2c_data[4]; /* Used only by write command, reserved in read. */
 };
 
 /* Read I2C Response (direct, 0x06E2) */
@@ -2130,7 +2130,7 @@ struct ice_aq_desc {
 		struct ice_aqc_get_link_status get_link_status;
 		struct ice_aqc_event_lan_overflow lan_overflow;
 		struct ice_aqc_get_link_topo get_link_topo;
-		struct ice_aqc_i2c read_i2c;
+		struct ice_aqc_i2c read_write_i2c;
 		struct ice_aqc_read_i2c_resp read_i2c_resp;
 	} params;
 };
@@ -2247,6 +2247,7 @@ enum ice_adminq_opc {
 	ice_aqc_opc_set_mac_lb				= 0x0620,
 	ice_aqc_opc_get_link_topo			= 0x06E0,
 	ice_aqc_opc_read_i2c				= 0x06E2,
+	ice_aqc_opc_write_i2c				= 0x06E3,
 	ice_aqc_opc_set_port_id_led			= 0x06E9,
 	ice_aqc_opc_set_gpio				= 0x06EC,
 	ice_aqc_opc_get_gpio				= 0x06ED,
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 9619bdb9e49a..1999c19a786e 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -4823,7 +4823,7 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
 	int status;
 
 	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_read_i2c);
-	cmd = &desc.params.read_i2c;
+	cmd = &desc.params.read_write_i2c;
 
 	if (!data)
 		return -EINVAL;
@@ -4850,6 +4850,55 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
 	return status;
 }
 
+/**
+ * ice_aq_write_i2c
+ * @hw: pointer to the hw struct
+ * @topo_addr: topology address for a device to communicate with
+ * @bus_addr: 7-bit I2C bus address
+ * @addr: I2C memory address (I2C offset) with up to 16 bits
+ * @params: I2C parameters: bit [4] - I2C address type, bits [3:0] - data size to write (0-7 bytes)
+ * @data: pointer to data (0 to 4 bytes) to be written to the I2C device
+ * @cd: pointer to command details structure or NULL
+ *
+ * Write I2C (0x06E3)
+ *
+ * * Return:
+ * * 0             - Successful write to the i2c device
+ * * -EINVAL       - Data size greater than 4 bytes
+ * * -EIO          - FW error
+ */
+int
+ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
+		 u16 bus_addr, __le16 addr, u8 params, u8 *data,
+		 struct ice_sq_cd *cd)
+{
+	struct ice_aq_desc desc = { 0 };
+	struct ice_aqc_i2c *cmd;
+	unsigned int i;
+	u8 data_size;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_write_i2c);
+	cmd = &desc.params.read_write_i2c;
+
+	data_size = FIELD_GET(ICE_AQC_I2C_DATA_SIZE_M, params);
+
+	/* data_size limited to 4 */
+	if (data_size > 4)
+		return -EINVAL;
+
+	cmd->i2c_bus_addr = cpu_to_le16(bus_addr);
+	cmd->topo_addr = topo_addr;
+	cmd->i2c_params = params;
+	cmd->i2c_addr = addr;
+
+	for (i = 0; i < data_size; i++) {
+		cmd->i2c_data[i] = *data;
+		data++;
+	}
+
+	return ice_aq_send_cmd(hw, &desc, NULL, 0, cd);
+}
+
 /**
  * ice_aq_set_driver_param - Set driver parameter to share via firmware
  * @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 872ea7d2332d..61b7c60db689 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -214,5 +214,9 @@ int
 ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
 		u16 bus_addr, __le16 addr, u8 params, u8 *data,
 		struct ice_sq_cd *cd);
+int
+ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
+		 u16 bus_addr, __le16 addr, u8 params, u8 *data,
+		 struct ice_sq_cd *cd);
 bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw);
 #endif /* _ICE_COMMON_H_ */
-- 
2.35.1

[PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Tony Nguyen]

From: Karol Kolacinski <karol.kolacinski@intel.com>

Add the possibility to write raw bytes to the GNSS module through the
first TTY device. This allows user to configure the module using the
publicly available u-blox UBX protocol (version 29) commands.

Create a second read-only TTY device.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h      |   4 +-
 drivers/net/ethernet/intel/ice/ice_gnss.c | 241 +++++++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_gnss.h |  26 ++-
 3 files changed, 240 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 60453b3b8d23..5c472ed99c7a 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -544,8 +544,8 @@ struct ice_pf {
 	u32 msg_enable;
 	struct ice_ptp ptp;
 	struct tty_driver *ice_gnss_tty_driver;
-	struct tty_port gnss_tty_port;
-	struct gnss_serial *gnss_serial;
+	struct tty_port *gnss_tty_port[ICE_GNSS_TTY_MINOR_DEVICES];
+	struct gnss_serial *gnss_serial[ICE_GNSS_TTY_MINOR_DEVICES];
 	u16 num_rdma_msix;		/* Total MSIX vectors for RDMA driver */
 	u16 rdma_base_vector;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index c6d755f707aa..4b7b762cd787 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -1,10 +1,102 @@
 // SPDX-License-Identifier: GPL-2.0
-/* Copyright (C) 2018-2021, Intel Corporation. */
+/* Copyright (C) 2021-2022, Intel Corporation. */
 
 #include "ice.h"
 #include "ice_lib.h"
 #include <linux/tty_driver.h>
 
+/**
+ * ice_gnss_do_write - Write data to internal GNSS
+ * @pf: board private structure
+ * @buf: command buffer
+ * @size: command buffer size
+ *
+ * Write UBX command data to the GNSS receiver
+ */
+static unsigned int
+ice_gnss_do_write(struct ice_pf *pf, unsigned char *buf, unsigned int size)
+{
+	struct ice_aqc_link_topo_addr link_topo;
+	struct ice_hw *hw = &pf->hw;
+	unsigned int offset = 0;
+	int err = 0;
+
+	memset(&link_topo, 0, sizeof(struct ice_aqc_link_topo_addr));
+	link_topo.topo_params.index = ICE_E810T_GNSS_I2C_BUS;
+	link_topo.topo_params.node_type_ctx |=
+		FIELD_PREP(ICE_AQC_LINK_TOPO_NODE_CTX_M,
+			   ICE_AQC_LINK_TOPO_NODE_CTX_OVERRIDE);
+
+	/* It's not possible to write a single byte to u-blox.
+	 * Write all bytes in a loop until there are 6 or less bytes left. If
+	 * there are exactly 6 bytes left, the last write would be only a byte.
+	 * In this case, do 4+2 bytes writes instead of 5+1. Otherwise, do the
+	 * last 2 to 5 bytes write.
+	 */
+	while (size - offset > ICE_GNSS_UBX_WRITE_BYTES + 1) {
+		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
+				       cpu_to_le16(buf[offset]),
+				       ICE_MAX_I2C_WRITE_BYTES,
+				       &buf[offset + 1], NULL);
+		if (err)
+			goto exit;
+
+		offset += ICE_GNSS_UBX_WRITE_BYTES;
+	}
+
+	/* Single byte would be written. Write 4 bytes instead of 5. */
+	if (size - offset == ICE_GNSS_UBX_WRITE_BYTES + 1) {
+		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
+				       cpu_to_le16(buf[offset]),
+				       ICE_MAX_I2C_WRITE_BYTES - 1,
+				       &buf[offset + 1], NULL);
+		if (err)
+			goto exit;
+
+		offset += ICE_GNSS_UBX_WRITE_BYTES - 1;
+	}
+
+	/* Do the last write, 2 to 5 bytes. */
+	err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
+			       cpu_to_le16(buf[offset]), size - offset - 1,
+			       &buf[offset + 1], NULL);
+	if (!err)
+		offset = size;
+
+exit:
+	if (err)
+		dev_err(ice_pf_to_dev(pf), "GNSS failed to write, offset=%u, size=%u, err=%d\n",
+			offset, size, err);
+
+	return offset;
+}
+
+/**
+ * ice_gnss_write_pending - Write all pending data to internal GNSS
+ * @work: GNSS write work structure
+ */
+static void ice_gnss_write_pending(struct kthread_work *work)
+{
+	struct gnss_serial *gnss = container_of(work, struct gnss_serial,
+						write_work);
+	struct ice_pf *pf = gnss->back;
+
+	if (!list_empty(&gnss->queue)) {
+		struct gnss_write_buf *write_buf = NULL;
+		unsigned int bytes;
+
+		write_buf = list_first_entry(&gnss->queue,
+					     struct gnss_write_buf, queue);
+
+		bytes = ice_gnss_do_write(pf, write_buf->buf, write_buf->size);
+		dev_dbg(ice_pf_to_dev(pf), "%u bytes written to GNSS\n", bytes);
+
+		list_del(&write_buf->queue);
+		kfree(write_buf->buf);
+		kfree(write_buf);
+	}
+}
+
 /**
  * ice_gnss_read - Read data from internal GNSS module
  * @work: GNSS read work structure
@@ -104,8 +196,9 @@ static void ice_gnss_read(struct kthread_work *work)
 /**
  * ice_gnss_struct_init - Initialize GNSS structure for the TTY
  * @pf: Board private structure
+ * @index: TTY device index
  */
-static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf)
+static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf, int index)
 {
 	struct device *dev = ice_pf_to_dev(pf);
 	struct kthread_worker *kworker;
@@ -118,9 +211,11 @@ static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf)
 	mutex_init(&gnss->gnss_mutex);
 	gnss->open_count = 0;
 	gnss->back = pf;
-	pf->gnss_serial = gnss;
+	pf->gnss_serial[index] = gnss;
 
 	kthread_init_delayed_work(&gnss->read_work, ice_gnss_read);
+	INIT_LIST_HEAD(&gnss->queue);
+	kthread_init_work(&gnss->write_work, ice_gnss_write_pending);
 	/* Allocate a kworker for handling work required for the GNSS TTY
 	 * writes.
 	 */
@@ -156,10 +251,10 @@ static int ice_gnss_tty_open(struct tty_struct *tty, struct file *filp)
 	tty->driver_data = NULL;
 
 	/* Get the serial object associated with this tty pointer */
-	gnss = pf->gnss_serial;
+	gnss = pf->gnss_serial[tty->index];
 	if (!gnss) {
 		/* Initialize GNSS struct on the first device open */
-		gnss = ice_gnss_struct_init(pf);
+		gnss = ice_gnss_struct_init(pf, tty->index);
 		if (!gnss)
 			return -ENOMEM;
 	}
@@ -212,25 +307,100 @@ static void ice_gnss_tty_close(struct tty_struct *tty, struct file *filp)
 }
 
 /**
- * ice_gnss_tty_write - Dummy TTY write function to avoid kernel panic
+ * ice_gnss_tty_write - Write GNSS data
  * @tty: pointer to the tty_struct
  * @buf: pointer to the user data
- * @cnt: the number of characters that was able to be sent to the hardware (or
- *       queued to be sent at a later time)
+ * @count: the number of characters queued to be sent to the HW
+ *
+ * The write function call is called by the user when there is data to be sent
+ * to the hardware. First the tty core receives the call, and then it passes the
+ * data on to the tty driver's write function. The tty core also tells the tty
+ * driver the size of the data being sent.
+ * If any errors happen during the write call, a negative error value should be
+ * returned instead of the number of characters queued to be written.
  */
 static int
-ice_gnss_tty_write(struct tty_struct *tty, const unsigned char *buf, int cnt)
+ice_gnss_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
-	return 0;
+	struct gnss_write_buf *write_buf;
+	struct gnss_serial *gnss;
+	unsigned char *cmd_buf;
+	struct ice_pf *pf;
+	int err = count;
+
+	/* We cannot write a single byte using our I2C implementation. */
+	if (count <= 1 || count > ICE_GNSS_TTY_WRITE_BUF)
+		return -EINVAL;
+
+	gnss = tty->driver_data;
+	if (!gnss)
+		return -EFAULT;
+
+	pf = (struct ice_pf *)tty->driver->driver_state;
+	if (!pf)
+		return -EFAULT;
+
+	/* Only allow to write on TTY 0 */
+	if (gnss != pf->gnss_serial[0])
+		return -EIO;
+
+	mutex_lock(&gnss->gnss_mutex);
+
+	if (!gnss->open_count) {
+		err = -EINVAL;
+		goto exit;
+	}
+
+	cmd_buf = kcalloc(count, sizeof(*buf), GFP_KERNEL);
+	if (!cmd_buf) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	memcpy(cmd_buf, buf, count);
+
+	/* Send the data out to a hardware port */
+	write_buf = kzalloc(sizeof(*write_buf), GFP_KERNEL);
+	if (!write_buf) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	write_buf->buf = cmd_buf;
+	write_buf->size = count;
+	INIT_LIST_HEAD(&write_buf->queue);
+	list_add_tail(&write_buf->queue, &gnss->queue);
+	kthread_queue_work(gnss->kworker, &gnss->write_work);
+exit:
+	mutex_unlock(&gnss->gnss_mutex);
+	return err;
 }
 
 /**
- * ice_gnss_tty_write_room - Dummy TTY write_room function to avoid kernel panic
+ * ice_gnss_tty_write_room - Returns the numbers of characters to be written.
  * @tty: pointer to the tty_struct
+ *
+ * This routine returns the numbers of characters the tty driver will accept
+ * for queuing to be written or 0 if either the TTY is not open or user
+ * tries to write to the TTY other than the first.
  */
 static unsigned int ice_gnss_tty_write_room(struct tty_struct *tty)
 {
-	return 0;
+	struct gnss_serial *gnss = tty->driver_data;
+
+	/* Only allow to write on TTY 0*/
+	if (!gnss || gnss != gnss->back->gnss_serial[0])
+		return 0;
+
+	mutex_lock(&gnss->gnss_mutex);
+
+	if (!gnss->open_count) {
+		mutex_unlock(&gnss->gnss_mutex);
+		return 0;
+	}
+
+	mutex_unlock(&gnss->gnss_mutex);
+	return ICE_GNSS_TTY_WRITE_BUF;
 }
 
 static const struct tty_operations tty_gps_ops = {
@@ -250,11 +420,13 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
 	const int ICE_TTYDRV_NAME_MAX = 14;
 	struct tty_driver *tty_driver;
 	char *ttydrv_name;
+	unsigned int i;
 	int err;
 
-	tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW);
+	tty_driver = tty_alloc_driver(ICE_GNSS_TTY_MINOR_DEVICES,
+				      TTY_DRIVER_REAL_RAW);
 	if (IS_ERR(tty_driver)) {
-		dev_err(ice_pf_to_dev(pf), "Failed to allocate memory for GNSS TTY\n");
+		dev_err(dev, "Failed to allocate memory for GNSS TTY\n");
 		return NULL;
 	}
 
@@ -284,23 +456,32 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
 	tty_driver->driver_state = pf;
 	tty_set_operations(tty_driver, &tty_gps_ops);
 
-	pf->gnss_serial = NULL;
+	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
+		pf->gnss_tty_port[i] = kzalloc(sizeof(*pf->gnss_tty_port[i]),
+					       GFP_KERNEL);
+		pf->gnss_serial[i] = NULL;
 
-	tty_port_init(&pf->gnss_tty_port);
-	tty_port_link_device(&pf->gnss_tty_port, tty_driver, 0);
+		tty_port_init(pf->gnss_tty_port[i]);
+		tty_port_link_device(pf->gnss_tty_port[i], tty_driver, i);
+	}
 
 	err = tty_register_driver(tty_driver);
 	if (err) {
-		dev_err(ice_pf_to_dev(pf), "Failed to register TTY driver err=%d\n",
-			err);
+		dev_err(dev, "Failed to register TTY driver err=%d\n", err);
 
-		tty_port_destroy(&pf->gnss_tty_port);
+		for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
+			tty_port_destroy(pf->gnss_tty_port[i]);
+			kfree(pf->gnss_tty_port[i]);
+		}
 		kfree(ttydrv_name);
 		tty_driver_kref_put(pf->ice_gnss_tty_driver);
 
 		return NULL;
 	}
 
+	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++)
+		dev_info(dev, "%s%d registered\n", ttydrv_name, i);
+
 	return tty_driver;
 }
 
@@ -328,17 +509,25 @@ void ice_gnss_init(struct ice_pf *pf)
  */
 void ice_gnss_exit(struct ice_pf *pf)
 {
+	unsigned int i;
+
 	if (!test_bit(ICE_FLAG_GNSS, pf->flags) || !pf->ice_gnss_tty_driver)
 		return;
 
-	tty_port_destroy(&pf->gnss_tty_port);
+	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
+		if (pf->gnss_tty_port[i]) {
+			tty_port_destroy(pf->gnss_tty_port[i]);
+			kfree(pf->gnss_tty_port[i]);
+		}
 
-	if (pf->gnss_serial) {
-		struct gnss_serial *gnss = pf->gnss_serial;
+		if (pf->gnss_serial[i]) {
+			struct gnss_serial *gnss = pf->gnss_serial[i];
 
-		kthread_cancel_delayed_work_sync(&gnss->read_work);
-		kfree(gnss);
-		pf->gnss_serial = NULL;
+			kthread_cancel_work_sync(&gnss->write_work);
+			kthread_cancel_delayed_work_sync(&gnss->read_work);
+			kfree(gnss);
+			pf->gnss_serial[i] = NULL;
+		}
 	}
 
 	tty_unregister_driver(pf->ice_gnss_tty_driver);
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
index 9211adb2372c..1f569a6e09a1 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-/* Copyright (C) 2018-2021, Intel Corporation. */
+/* Copyright (C) 2021-2022, Intel Corporation. */
 
 #ifndef _ICE_GNSS_H_
 #define _ICE_GNSS_H_
@@ -8,14 +8,30 @@
 #include <linux/tty_flip.h>
 
 #define ICE_E810T_GNSS_I2C_BUS		0x2
+#define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
+#define ICE_GNSS_TTY_MINOR_DEVICES	2
+#define ICE_GNSS_TTY_WRITE_BUF		250
+#define ICE_MAX_I2C_DATA_SIZE		FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
+#define ICE_MAX_I2C_WRITE_BYTES		4
+
+/* u-blox specific definitions */
 #define ICE_GNSS_UBX_I2C_BUS_ADDR	0x42
 /* Data length register is big endian */
 #define ICE_GNSS_UBX_DATA_LEN_H		0xFD
 #define ICE_GNSS_UBX_DATA_LEN_WIDTH	2
 #define ICE_GNSS_UBX_EMPTY_DATA		0xFF
-#define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
-#define ICE_MAX_I2C_DATA_SIZE		FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
+/* For u-blox writes are performed without address so the first byte to write is
+ * passed as I2C addr parameter.
+ */
+#define ICE_GNSS_UBX_WRITE_BYTES	(ICE_MAX_I2C_WRITE_BYTES + 1)
 #define ICE_MAX_UBX_READ_TRIES		255
+#define ICE_MAX_UBX_ACK_READ_TRIES	4095
+
+struct gnss_write_buf {
+	struct list_head queue;
+	unsigned int size;
+	unsigned char *buf;
+};
 
 /**
  * struct gnss_serial - data used to initialize GNSS TTY port
@@ -25,6 +41,8 @@
  * @gnss_mutex: gnss_mutex used to protect GNSS serial operations
  * @kworker: kwork thread for handling periodic work
  * @read_work: read_work function for handling GNSS reads
+ * @write_work: write_work function for handling GNSS writes
+ * @queue: write buffers queue
  */
 struct gnss_serial {
 	struct ice_pf *back;
@@ -33,6 +51,8 @@ struct gnss_serial {
 	struct mutex gnss_mutex; /* protects GNSS serial structure */
 	struct kthread_worker *kworker;
 	struct kthread_delayed_work read_work;
+	struct kthread_work write_work;
+	struct list_head queue;
 };
 
 #if IS_ENABLED(CONFIG_TTY)
-- 
2.35.1

Re: [PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Jakub Kicinski]

On Tue, 17 May 2022 14:19:35 -0700 Tony Nguyen wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Add the possibility to write raw bytes to the GNSS module through the
> first TTY device. This allows user to configure the module using the
> publicly available u-blox UBX protocol (version 29) commands.
> 
> Create a second read-only TTY device.

Can you say more about how the TTY devices are discovered, and why there
are two of them? Would be great if that info was present under
Documentation/

Re: [PATCH net-next 2/3] ice: add i2c write command

[Paolo Abeni]

On Tue, 2022-05-17 at 14:19 -0700, Tony Nguyen wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Add the possibility to write to connected i2c devices using the AQ
> command. FW may reject the write if the device is not on allowlist.
> 
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  7 +--
>  drivers/net/ethernet/intel/ice/ice_common.c   | 51 ++++++++++++++++++-
>  drivers/net/ethernet/intel/ice/ice_common.h   |  4 ++
>  3 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index b25e27c4d887..bedc19f12cbd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -1401,7 +1401,7 @@ struct ice_aqc_get_link_topo {
>  	u8 rsvd[9];
>  };
>  
> -/* Read I2C (direct, 0x06E2) */
> +/* Read/Write I2C (direct, 0x06E2/0x06E3) */
>  struct ice_aqc_i2c {
>  	struct ice_aqc_link_topo_addr topo_addr;
>  	__le16 i2c_addr;
> @@ -1411,7 +1411,7 @@ struct ice_aqc_i2c {
>  
>  	u8 rsvd;
>  	__le16 i2c_bus_addr;
> -	u8 rsvd2[4];
> +	u8 i2c_data[4]; /* Used only by write command, reserved in read. */
>  };
>  
>  /* Read I2C Response (direct, 0x06E2) */
> @@ -2130,7 +2130,7 @@ struct ice_aq_desc {
>  		struct ice_aqc_get_link_status get_link_status;
>  		struct ice_aqc_event_lan_overflow lan_overflow;
>  		struct ice_aqc_get_link_topo get_link_topo;
> -		struct ice_aqc_i2c read_i2c;
> +		struct ice_aqc_i2c read_write_i2c;
>  		struct ice_aqc_read_i2c_resp read_i2c_resp;
>  	} params;
>  };
> @@ -2247,6 +2247,7 @@ enum ice_adminq_opc {
>  	ice_aqc_opc_set_mac_lb				= 0x0620,
>  	ice_aqc_opc_get_link_topo			= 0x06E0,
>  	ice_aqc_opc_read_i2c				= 0x06E2,
> +	ice_aqc_opc_write_i2c				= 0x06E3,
>  	ice_aqc_opc_set_port_id_led			= 0x06E9,
>  	ice_aqc_opc_set_gpio				= 0x06EC,
>  	ice_aqc_opc_get_gpio				= 0x06ED,
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 9619bdb9e49a..1999c19a786e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -4823,7 +4823,7 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
>  	int status;
>  
>  	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_read_i2c);
> -	cmd = &desc.params.read_i2c;
> +	cmd = &desc.params.read_write_i2c;
>  
>  	if (!data)
>  		return -EINVAL;
> @@ -4850,6 +4850,55 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
>  	return status;
>  }
>  
> +/**
> + * ice_aq_write_i2c
> + * @hw: pointer to the hw struct
> + * @topo_addr: topology address for a device to communicate with
> + * @bus_addr: 7-bit I2C bus address
> + * @addr: I2C memory address (I2C offset) with up to 16 bits
> + * @params: I2C parameters: bit [4] - I2C address type, bits [3:0] - data size to write (0-7 bytes)
> + * @data: pointer to data (0 to 4 bytes) to be written to the I2C device
> + * @cd: pointer to command details structure or NULL
> + *
> + * Write I2C (0x06E3)
> + *
> + * * Return:
> + * * 0             - Successful write to the i2c device
> + * * -EINVAL       - Data size greater than 4 bytes
> + * * -EIO          - FW error
> + */
> +int
> +ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
> +		 u16 bus_addr, __le16 addr, u8 params, u8 *data,
> +		 struct ice_sq_cd *cd)
> +{
> +	struct ice_aq_desc desc = { 0 };
> +	struct ice_aqc_i2c *cmd;
> +	unsigned int i;
> +	u8 data_size;
> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_write_i2c);
> +	cmd = &desc.params.read_write_i2c;
> +
> +	data_size = FIELD_GET(ICE_AQC_I2C_DATA_SIZE_M, params);
> +
> +	/* data_size limited to 4 */
> +	if (data_size > 4)
> +		return -EINVAL;
> +
> +	cmd->i2c_bus_addr = cpu_to_le16(bus_addr);
> +	cmd->topo_addr = topo_addr;
> +	cmd->i2c_params = params;
> +	cmd->i2c_addr = addr;
> +
> +	for (i = 0; i < data_size; i++) {
> +		cmd->i2c_data[i] = *data;
> +		data++;
> +	}

Why not:
	memcpy(cmd->i2c_data, data, data_size);

	?

Thanks!

Paolo

Re: [PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Paolo Abeni]

On Tue, 2022-05-17 at 14:19 -0700, Tony Nguyen wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Add the possibility to write raw bytes to the GNSS module through the
> first TTY device. This allows user to configure the module using the
> publicly available u-blox UBX protocol (version 29) commands.
> 
> Create a second read-only TTY device.
> 
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice.h      |   4 +-
>  drivers/net/ethernet/intel/ice/ice_gnss.c | 241 +++++++++++++++++++---
>  drivers/net/ethernet/intel/ice/ice_gnss.h |  26 ++-
>  3 files changed, 240 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 60453b3b8d23..5c472ed99c7a 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -544,8 +544,8 @@ struct ice_pf {
>  	u32 msg_enable;
>  	struct ice_ptp ptp;
>  	struct tty_driver *ice_gnss_tty_driver;
> -	struct tty_port gnss_tty_port;
> -	struct gnss_serial *gnss_serial;
> +	struct tty_port *gnss_tty_port[ICE_GNSS_TTY_MINOR_DEVICES];
> +	struct gnss_serial *gnss_serial[ICE_GNSS_TTY_MINOR_DEVICES];
>  	u16 num_rdma_msix;		/* Total MSIX vectors for RDMA driver */
>  	u16 rdma_base_vector;
>  
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index c6d755f707aa..4b7b762cd787 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -1,10 +1,102 @@
>  // SPDX-License-Identifier: GPL-2.0
> -/* Copyright (C) 2018-2021, Intel Corporation. */
> +/* Copyright (C) 2021-2022, Intel Corporation. */
>  
>  #include "ice.h"
>  #include "ice_lib.h"
>  #include <linux/tty_driver.h>
>  
> +/**
> + * ice_gnss_do_write - Write data to internal GNSS
> + * @pf: board private structure
> + * @buf: command buffer
> + * @size: command buffer size
> + *
> + * Write UBX command data to the GNSS receiver
> + */
> +static unsigned int
> +ice_gnss_do_write(struct ice_pf *pf, unsigned char *buf, unsigned int size)
> +{
> +	struct ice_aqc_link_topo_addr link_topo;
> +	struct ice_hw *hw = &pf->hw;
> +	unsigned int offset = 0;
> +	int err = 0;
> +
> +	memset(&link_topo, 0, sizeof(struct ice_aqc_link_topo_addr));
> +	link_topo.topo_params.index = ICE_E810T_GNSS_I2C_BUS;
> +	link_topo.topo_params.node_type_ctx |=
> +		FIELD_PREP(ICE_AQC_LINK_TOPO_NODE_CTX_M,
> +			   ICE_AQC_LINK_TOPO_NODE_CTX_OVERRIDE);
> +
> +	/* It's not possible to write a single byte to u-blox.
> +	 * Write all bytes in a loop until there are 6 or less bytes left. If
> +	 * there are exactly 6 bytes left, the last write would be only a byte.
> +	 * In this case, do 4+2 bytes writes instead of 5+1. Otherwise, do the
> +	 * last 2 to 5 bytes write.
> +	 */
> +	while (size - offset > ICE_GNSS_UBX_WRITE_BYTES + 1) {
> +		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> +				       cpu_to_le16(buf[offset]),
> +				       ICE_MAX_I2C_WRITE_BYTES,
> +				       &buf[offset + 1], NULL);
> +		if (err)
> +			goto exit;
> +
> +		offset += ICE_GNSS_UBX_WRITE_BYTES;
> +	}
> +
> +	/* Single byte would be written. Write 4 bytes instead of 5. */
> +	if (size - offset == ICE_GNSS_UBX_WRITE_BYTES + 1) {
> +		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> +				       cpu_to_le16(buf[offset]),
> +				       ICE_MAX_I2C_WRITE_BYTES - 1,
> +				       &buf[offset + 1], NULL);
> +		if (err)
> +			goto exit;
> +
> +		offset += ICE_GNSS_UBX_WRITE_BYTES - 1;
> +	}
> +
> +	/* Do the last write, 2 to 5 bytes. */
> +	err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> +			       cpu_to_le16(buf[offset]), size - offset - 1,
> +			       &buf[offset + 1], NULL);
> +	if (!err)
> +		offset = size;
> +
> +exit:
> +	if (err)
> +		dev_err(ice_pf_to_dev(pf), "GNSS failed to write, offset=%u, size=%u, err=%d\n",
> +			offset, size, err);
> +
> +	return offset;


IMHO more readable with:
	if (err)
		goto err_out;

	return size;

err_out:
	dev_err(ice_pf_to_dev(pf), "GNSS failed to write, offset=%u, size=%u, err=%d\n",
			offset, size, err);
	return offset;

(plus adjusting the goto above)

Thanks!

Paolo

Re: [PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Kolacinski, Karol]

On Thu, 19 May 2022 06:57 +0200 Jakub Kicinski wrote:
> > Create a second read-only TTY device.
> 
> Can you say more about how the TTY devices are discovered, and why there
> are two of them? Would be great if that info was present under
> Documentation/

Both TTY devices are used for the same GNSS module. First one is
read/write and the second one is read only. They are discovered by
checking ICE_E810T_P0_GNSS_PRSNT_N bit in ICE_PCA9575_P0_IN register.
This means that the GNSS module is physically present on the PCB.
The design with one RW and one RO TTY device was requested by
customers.

Thanks!
Karol

Re: [PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Kolacinski, Karol]

On Thu, 19 May 2022 15:45 +0200 Paolo Abeni wrote
> > +
> > +exit:
> > +     if (err)
> > +             dev_err(ice_pf_to_dev(pf), "GNSS failed to write, offset=%u, size=%u, err=%d\n",
> > +                     offset, size, err);
> > +
> > +     return offset;
> 
> 
> IMHO more readable with:
>         if (err)
>                 goto err_out;
> 
>         return size;
> 
> err_out:
>         dev_err(ice_pf_to_dev(pf), "GNSS failed to write, offset=%u, size=%u, err=%d\n",
>                         offset, size, err);
>         return offset;
> 
> (plus adjusting the goto above)
> 
> Thanks!
> 
> Paolo

Done, looks better, thanks!

Karol

Re: [PATCH net-next 2/3] ice: add i2c write command

[Kolacinski, Karol]

> On Thu, 19 May 2022 15:40 +0200 Paolo Abeni wrote:
> > +     for (i = 0; i < data_size; i++) {
> > +             cmd->i2c_data[i] = *data;
> > +             data++;
> > +     }
> Why not:
>         memcpy(cmd->i2c_data, data, data_size);
> 
>         ?
> 
> Thanks!
> 
> Paolo

Nice catch, thanks!

Karol

Re: [PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Jakub Kicinski]

On Mon, 23 May 2022 16:56:44 +0000 Kolacinski, Karol wrote:
> On Thu, 19 May 2022 06:57 +0200 Jakub Kicinski wrote:
> > > Create a second read-only TTY device.  
> > 
> > Can you say more about how the TTY devices are discovered, and why there
> > are two of them? Would be great if that info was present under
> > Documentation/  
> 
> Both TTY devices are used for the same GNSS module. First one is
> read/write and the second one is read only. They are discovered by
> checking ICE_E810T_P0_GNSS_PRSNT_N bit in ICE_PCA9575_P0_IN register.
> This means that the GNSS module is physically present on the PCB.
> The design with one RW and one RO TTY device was requested by
> customers.

I meant discovered at the uAPI level. Imagine I plug in a shiny E810T
with a GNSS receiver into a box, the PRSNT_N bit is set and the driver
registered the ttys. How do I find the GNSS under /dev ?

Re: [PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Kolacinski, Karol]

On Mon, 23 May 2022 19:58 +0000 Jakub Kicinski wrote:
> I meant discovered at the uAPI level. Imagine I plug in a shiny E810T
> with a GNSS receiver into a box, the PRSNT_N bit is set and the driver
> registered the ttys. How do I find the GNSS under /dev ?

If the module is physically present, driver creates 2 TTYs for each supported
device in /dev, ttyGNSS_<device>:<function>_0 and _1. First one (_0) is RW and
the second one is RO.
The protocol of write commands is dependent on the GNSS module as the driver
writes raw bytes from the TTY to the GNSS i2c.

I added this to Documentation/.

Thanks,
Karol

Re: [PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Jakub Kicinski]

On Thu, 23 Jun 2022 13:39:35 +0000 Kolacinski, Karol wrote:
> On Mon, 23 May 2022 19:58 +0000 Jakub Kicinski wrote:
> > I meant discovered at the uAPI level. Imagine I plug in a shiny E810T
> > with a GNSS receiver into a box, the PRSNT_N bit is set and the driver
> > registered the ttys. How do I find the GNSS under /dev ?  
> 
> If the module is physically present, driver creates 2 TTYs for each supported
> device in /dev, ttyGNSS_<device>:<function>_0 and _1. First one (_0) is RW and
> the second one is RO.
> The protocol of write commands is dependent on the GNSS module as the driver
> writes raw bytes from the TTY to the GNSS i2c.
> 
> I added this to Documentation/.

Sounds good, thanks!