* [PATCH 3/3] remoteproc: Add AVM WASP driver
@ 2022-02-21 13:54 Daniel Kestrel
2022-02-21 17:43 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Daniel Kestrel @ 2022-02-21 13:54 UTC (permalink / raw)
Cc: Bjorn Andersson, Mathieu Poirier, Rob Herring, Daniel Kestrel,
linux-remoteproc, devicetree, linux-kernel
Some AVM Fritzbox router boards (3390, 3490, 5490, 5491, 7490),
that are Lantiq XRX200 based, have a memory only ATH79 based
WASP (Wireless Assistant Support Processor) SoC that has wifi
cards connected to it. It does not share anything with the
Lantiq host and has no persistent storage. It has an mdio based
connection for bringing up a small network boot firmware and is
connected to the Lantiq GSWIP switch via gigabit ethernet. This
is used to load an initramfs linux image to it, after the
network boot firmware was started.
In order to initialize this remote processor we need to:
- power on the SoC using startup gpio
- reset the SoC using the reset gpio
- send the network boot firmware using mdio
- send the linux image using raw ethernet frames
This driver allows to start and stop the WASP SoC.
Signed-off-by: Daniel Kestrel <kestrelseventyfour@gmail.com>
---
drivers/remoteproc/Kconfig | 10 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/avm_wasp.c | 1251 +++++++++++++++++++++++++++++++++
drivers/remoteproc/avm_wasp.h | 95 +++
4 files changed, 1357 insertions(+)
create mode 100644 drivers/remoteproc/avm_wasp.c
create mode 100644 drivers/remoteproc/avm_wasp.h
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 166019786653..a761186c5171 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -23,6 +23,16 @@ config REMOTEPROC_CDEV
It's safe to say N if you don't want to use this interface.
+config AVM_WASP_REMOTEPROC
+ tristate "AVM WASP remoteproc support"
+ depends on NET_DSA_LANTIQ_GSWIP
+ help
+ Say y here to support booting the secondary SoC ATH79 target
+ called Wireless Assistant Support Processor (WASP) that some
+ AVM Fritzbox devices (3390, 3490, 5490, 5491, 7490) have built in.
+
+ It's safe to say N here.
+
config IMX_REMOTEPROC
tristate "i.MX remoteproc support"
depends on ARCH_MXC
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 5478c7cb9e07..0ae175c6722f 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -11,6 +11,7 @@ remoteproc-y += remoteproc_sysfs.o
remoteproc-y += remoteproc_virtio.o
remoteproc-y += remoteproc_elf_loader.o
obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
+obj-$(CONFIG_AVM_WASP_REMOTEPROC) += avm_wasp.o
obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
obj-$(CONFIG_IMX_DSP_REMOTEPROC) += imx_dsp_rproc.o
obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
diff --git a/drivers/remoteproc/avm_wasp.c b/drivers/remoteproc/avm_wasp.c
new file mode 100644
index 000000000000..04b7c9005028
--- /dev/null
+++ b/drivers/remoteproc/avm_wasp.c
@@ -0,0 +1,1251 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AVM WASP Remote Processor driver
+ *
+ * Copyright (c) 2019-2020 Andreas Böhler
+ * Copyright (c) 2021-2022 Daniel Kestrel
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_mdio.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+#include <linux/timekeeping.h>
+#include <net/sock.h>
+#include <asm-generic/gpio.h>
+
+#include "remoteproc_internal.h"
+#include "avm_wasp.h"
+
+/**
+ * struct avm_wasp_rproc - avmwasp remote processor priv
+ * @rproc: rproc handle
+ * @pdev: pointer to platform device
+ * @eeprom_blob: pointer to load and save any firmware
+ * @linux_blob: pointer to access initramfs image
+ * @complete: structure for asynchronous firmware load
+ * @mdio_bus: pointer to mii_bus of gswip device for gpio
+ * @startup_gpio: store WASP startup gpio number
+ * @reset_gpio: store WASP reset gpio number
+ * @s_gpio_flg: store WASP startup gpio flags active high/low
+ * @r_gpio_flg: store WASP reset gpio flags active high/low
+ * @netboot_firmware: store name of the network boot firmware
+ * @loader_port: store name of the port wasp is connected to
+ * @sendbuf: send buffer for uploading WASP initramfs firmware
+ * @recvbuf: recv buffer for feedback from WASP
+ * @s_packet: structure for sending packets to WASP
+ * @send_socket: pointer to socket for sending to WASP
+ * @recv_socket: pointer to socket for receiving from WASP
+ * @ifindex: interface index used for WASP communication
+ */
+struct avm_wasp_rproc {
+ struct rproc *rproc;
+ struct platform_device *pdev;
+ const struct firmware *eeprom_blob, *linux_blob;
+ struct completion complete;
+ char *mdio_bus_id;
+ struct mii_bus *mdio_bus;
+ int startup_gpio, reset_gpio;
+ enum of_gpio_flags s_gpio_flg, r_gpio_flg;
+ char *netboot_firmware;
+ char *loader_port;
+ char sendbuf[BUF_SIZE];
+ char recvbuf[BUF_SIZE];
+ struct wasp_packet s_packet;
+ struct socket *send_socket;
+ struct socket *recv_socket;
+ int ifindex;
+};
+
+/**
+ * avm_wasp_firmware_request_cb() - callback handler for firmware load
+ * @eeprom_blob: pointer to struct firmware
+ * @ctx: context passed
+ *
+ * This handler is called after completing the request_firmware_nowait
+ * function by passing the avm_wasp_rproc struct
+ * It saves the firmware in the context and calls complete
+ */
+static void avm_wasp_firmware_request_cb(const struct firmware *eeprom_blob,
+ void *ctx)
+{
+ struct avm_wasp_rproc *avmwasp = ctx;
+
+ if (eeprom_blob)
+ avmwasp->eeprom_blob = eeprom_blob;
+
+ complete(&avmwasp->complete);
+}
+
+/**
+ * avm_wasp_firmware_request() - asynchronous load of passed firmware
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ * @name: char pointer to filename (relative to /lib/firmware)
+ *
+ * Handles setup and execution of the asynchronous firmware request
+ * Used to trigger the load of the ath10k caldata and ath9k eeprom
+ * firmware from the tffs partition of the devices
+ *
+ * Return: 0 on success, -2 if file not found or error from function
+ * request_firmware_nowait
+ */
+static int avm_wasp_firmware_request(struct avm_wasp_rproc *avmwasp,
+ const char *name)
+{
+ int err;
+
+ init_completion(&avmwasp->complete);
+
+ err = request_firmware_nowait(THIS_MODULE, 1, name,
+ &avmwasp->pdev->dev,
+ GFP_KERNEL, avmwasp,
+ avm_wasp_firmware_request_cb);
+ if (err < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Load request for %s failed\n", name);
+ return err;
+ }
+
+ wait_for_completion(&avmwasp->complete);
+
+ if (!avmwasp->eeprom_blob) {
+ dev_err(&avmwasp->pdev->dev,
+ "Unable to load %s\n", name);
+ return -ENOENT;
+ }
+
+ return 0;
+}
+
+/**
+ * avm_wasp_firmware_release() - clean up after firmware load
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ *
+ * Releases the firmware that is in the eeprom_blob firmware
+ * pointer of the private avm_wasp_rproc structure
+ */
+static void avm_wasp_firmware_release(struct avm_wasp_rproc *avmwasp)
+{
+ release_firmware(avmwasp->eeprom_blob);
+ avmwasp->eeprom_blob = NULL;
+}
+
+/**
+ * avm_wasp_netboot_mdio_read() - read with gswip mdio bus
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ * @location: register number of the m_regs_wasp register array
+ *
+ * Reads a value from the specified register for the mdio address
+ * that is used for the connection to the WASP SoC
+ * Mutex on mdio_lock is required to serialize access on bus
+ *
+ * Return: Value that was read from the specified register
+ */
+int avm_wasp_netboot_mdio_read(struct avm_wasp_rproc *avmwasp,
+ int location)
+{
+ int value;
+
+ if (location > M_REGS_WASP_INDEX_MAX || location < 0)
+ return 0;
+ mutex_lock(&avmwasp->mdio_bus->mdio_lock);
+ value = avmwasp->mdio_bus->read(avmwasp->mdio_bus,
+ WASP_ADDR, m_regs_wasp[location]);
+ mutex_unlock(&avmwasp->mdio_bus->mdio_lock);
+ return value;
+}
+
+/**
+ * avm_wasp_netboot_mdio_write() - write with gswip mdio bus
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ * @location: register number of the m_regs_wasp register array
+ * @value: value to be written to the register
+ *
+ * Writes a value to the specified register for the mdio address
+ * that is used for the connection to the WASP SoC
+ * Mutex on mdio_lock is required to serialize access on bus
+ * Makes sure not to write to invalid registers as this can have
+ * unpredictable results
+ */
+void avm_wasp_netboot_mdio_write(struct avm_wasp_rproc *avmwasp,
+ int location, int value)
+{
+ if (location > M_REGS_WASP_INDEX_MAX || location < 0)
+ return;
+ mutex_lock(&avmwasp->mdio_bus->mdio_lock);
+ avmwasp->mdio_bus->write(avmwasp->mdio_bus, WASP_ADDR,
+ m_regs_wasp[location], value);
+ mutex_unlock(&avmwasp->mdio_bus->mdio_lock);
+}
+
+/**
+ * avm_wasp_netboot_mdio_write_u32_split() - write 32bit value
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ * @location: register number of the m_regs_wasp register array
+ * @value: value to be written to the register
+ *
+ * As the mdio registers are 16bit, this function writes a 32bit value
+ * to two subsequent registers starting with the specified register
+ * for the mdio address that is used for the connection to the WASP SoC
+ */
+void avm_wasp_netboot_mdio_write_u32_split(struct avm_wasp_rproc *avmwasp,
+ int location, const u32 value)
+{
+ avm_wasp_netboot_mdio_write(avmwasp, location,
+ ((value & 0xffff0000) >> 16));
+ avm_wasp_netboot_mdio_write(avmwasp, location + 1,
+ (value & 0x0000ffff));
+}
+
+/**
+ * avm_wasp_netboot_write_header() - write header to WASP
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ * @start_address: address where to load the firmware to on WASP
+ * @len: length of the network boot firmware
+ * @exec_address: address where to start execution on WASP
+ *
+ * Writes the header to WASP using mdio to initiate the start of
+ * transferring the network boot firmware to WASP
+ *
+ * Return: 0 on Success or -14 if writing header failed based on return
+ * code from WASP
+ */
+static int avm_wasp_netboot_write_header(struct avm_wasp_rproc *avmwasp,
+ const u32 start_addr, const u32 len,
+ const u32 exec_addr)
+{
+ int regval;
+ int timeout = WASP_TIMEOUT_COUNT;
+
+ avm_wasp_netboot_mdio_write_u32_split(avmwasp, 1, start_addr);
+ avm_wasp_netboot_mdio_write_u32_split(avmwasp, 3, len);
+ avm_wasp_netboot_mdio_write_u32_split(avmwasp, 5, exec_addr);
+ avm_wasp_netboot_mdio_write(avmwasp, 0, WASP_CMD_SET_PARAMS);
+
+ do {
+ udelay(WASP_POLL_SLEEP_US);
+ regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
+ timeout--;
+ } while ((regval != WASP_RESP_OK) && (timeout > 0));
+
+ if (regval != WASP_RESP_OK) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error writing header to WASP! Status = %d\n", regval);
+ return -EFAULT;
+ }
+ return 0;
+}
+
+/**
+ * avm_wasp_netboot_write_checksum() - write checksum to WASP
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ * @checksum: calculated checksum value to be sent to WASP
+ *
+ * Writes the calculated checksum for the given network boot firmware
+ * to WASP using mdio as the second step
+ *
+ * Return: 0 on Success or -14 if writing checksum failed based on return
+ * code from WASP
+ */
+static int avm_wasp_netboot_write_checksum(struct avm_wasp_rproc *avmwasp,
+ const uint32_t checksum)
+{
+ int regval;
+ int timeout = WASP_TIMEOUT_COUNT;
+
+ avm_wasp_netboot_mdio_write_u32_split(avmwasp, 1, checksum);
+ if (m_model == MODEL_3390) {
+ avm_wasp_netboot_mdio_write_u32_split(avmwasp, 3, 0x0000);
+ avm_wasp_netboot_mdio_write(avmwasp, 0,
+ WASP_CMD_SET_CHECKSUM_3390);
+ } else if (m_model == MODEL_X490)
+ avm_wasp_netboot_mdio_write(avmwasp, 0,
+ WASP_CMD_SET_CHECKSUM_X490);
+
+ do {
+ udelay(WASP_POLL_SLEEP_US);
+ regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
+ timeout--;
+ } while ((regval != WASP_RESP_OK) && (timeout > 0));
+
+ if (regval != WASP_RESP_OK) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error writing checksum to WASP! Status = %d\n",
+ regval);
+ return -EFAULT;
+ }
+ return 0;
+}
+
+/**
+ * avm_wasp_netboot_write_chunk() - write chunk of data to WASP
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ * @data: pointer to data
+ * @len: length of data (should not exceed 14 bytes)
+ *
+ * Writes up to 14 bytes of data into the 7 16bit mdio registers
+ * to WASP using mdio
+ *
+ * Return: 0 on Success, -14 if data length is mor than 14 bytes or
+ * -2 if writing the data failed based on return code from WASP
+ */
+static int avm_wasp_netboot_write_chunk(struct avm_wasp_rproc *avmwasp,
+ const char *data, const int len)
+{
+ int regval, i, j;
+ int timeout = WASP_TIMEOUT_COUNT;
+
+ if (len > WASP_CHUNK_SIZE || len < 0 || !data)
+ return -EFAULT;
+ for (i = 0, j = 1; i < len; i += 4, j += 2)
+ avm_wasp_netboot_mdio_write_u32_split(avmwasp, j,
+ *((uint32_t *)
+ (data + i)));
+
+ avm_wasp_netboot_mdio_write(avmwasp, 0, WASP_CMD_SET_DATA);
+
+ do {
+ udelay(WASP_POLL_SLEEP_US);
+ regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
+ timeout--;
+ } while ((regval != WASP_RESP_OK) && (timeout > 0));
+
+ if (regval != WASP_RESP_OK && regval != WASP_RESP_WAIT &&
+ regval != WASP_RESP_COMPLETED) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error writing chunk to WASP: m_reg_status = 0x%x!\n",
+ regval);
+ return -EFAULT;
+ }
+ return 0;
+}
+
+/**
+ * avm_wasp_netboot_calc_checksum() - calculate netboot firmware checksum
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ *
+ * Calculates the checksum by using the eeprom_blob from the private
+ * avm_wasp_rproc structure
+ *
+ * Return: Calculated checksum or -14 on Error
+ */
+static uint32_t avm_wasp_netboot_calc_checksum(struct avm_wasp_rproc *avmwasp)
+{
+ u32 checksum = 0xffffffff;
+ u32 cs;
+ int count = -1;
+ size_t size;
+ const u8 *firmware;
+ const u8 *firmware_end;
+
+ if (!avmwasp->eeprom_blob)
+ return -EFAULT;
+ size = avmwasp->eeprom_blob->size;
+ firmware = avmwasp->eeprom_blob->data;
+ firmware_end = firmware + size;
+
+ if (!firmware || size <= 0)
+ return -EFAULT;
+
+ while (firmware < firmware_end) {
+ cs = (firmware[0] << 24 | firmware[1] << 16 |
+ firmware[2] << 8 | firmware[3]);
+ checksum = checksum - cs;
+ count++;
+ firmware += 4;
+ }
+
+ checksum = checksum - count;
+ return checksum;
+}
+
+/**
+ * avm_wasp_netboot_load_firmware() - load netboot firmware to WASP
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ *
+ * Implements the process to send header, checksum and the firmware
+ * blob in 14 byte chunks to the WASP processor using mdio
+ * Includes checks between the steps and sending commands to start
+ * the network boot firmware
+ *
+ * Return: 0 on Success, -2 if no firmware is present, -19 if no
+ * firmware or -14 if other errors have occurred
+ */
+int avm_wasp_netboot_load_firmware(struct avm_wasp_rproc *avmwasp)
+{
+ const u8 *firmware;
+ const u8 *firmware_end;
+ int ret, regval, regval2, count, cont = 1;
+
+ count = WASP_WAIT_TIMEOUT_COUNT;
+
+ while (count > 0 && (avm_wasp_netboot_mdio_read(avmwasp, 0)
+ != WASP_RESP_OK)) {
+ count -= 1;
+ mdelay(WASP_WAIT_SLEEP);
+ }
+
+ if (avm_wasp_netboot_mdio_read(avmwasp, 0)
+ != WASP_RESP_OK) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error: WASP processor not ready\n");
+
+ return -ENODEV;
+ }
+
+ ret = request_firmware_direct((const struct firmware **)
+ &avmwasp->eeprom_blob,
+ avmwasp->netboot_firmware, &avmwasp->pdev->dev);
+ if (ret) {
+ dev_err(&avmwasp->pdev->dev,
+ "Could not find network boot firmware\n");
+ return -ENOENT;
+ }
+
+ firmware = avmwasp->eeprom_blob->data;
+ firmware_end = firmware + avmwasp->eeprom_blob->size;
+
+ if (!firmware || avmwasp->eeprom_blob->size <= 0)
+ return -EFAULT;
+
+ if (avm_wasp_netboot_write_header(avmwasp, start_addr,
+ avmwasp->eeprom_blob->size,
+ exec_addr) < 0)
+ return -EFAULT;
+
+ if (avm_wasp_netboot_write_checksum(avmwasp,
+ avm_wasp_netboot_calc_checksum
+ (avmwasp)) < 0)
+ return -EFAULT;
+
+ while (firmware < firmware_end) {
+ if ((firmware_end - firmware) >= WASP_CHUNK_SIZE) {
+ if (avm_wasp_netboot_write_chunk(avmwasp, firmware,
+ WASP_CHUNK_SIZE) < 0)
+ return -EFAULT;
+ } else {
+ if (avm_wasp_netboot_write_chunk(avmwasp, firmware,
+ (firmware_end -
+ firmware)) < 0)
+ return -EFAULT;
+ }
+ firmware += WASP_CHUNK_SIZE;
+ }
+
+ mdelay(WASP_WAIT_SLEEP);
+
+ if (m_model == MODEL_3390)
+ avm_wasp_netboot_mdio_write(avmwasp, 0,
+ WASP_CMD_START_FIRMWARE_3390);
+ else if (m_model == MODEL_X490)
+ avm_wasp_netboot_mdio_write(avmwasp, 0,
+ WASP_CMD_START_FIRMWARE_X490);
+
+ avm_wasp_firmware_release(avmwasp);
+
+ mdelay(WASP_WAIT_SLEEP);
+ count = 0;
+
+ while ((avm_wasp_netboot_mdio_read(avmwasp, 0)
+ != WASP_RESP_READY_TO_START) &&
+ (count < WASP_WAIT_TIMEOUT_COUNT)) {
+ mdelay(WASP_WAIT_SLEEP);
+ count++;
+ }
+ if (count >= WASP_WAIT_TIMEOUT_COUNT) {
+ dev_err(&avmwasp->pdev->dev,
+ "Timed out waiting for WASP ready to start.\n");
+ return -EFAULT;
+ }
+
+ if (m_model == MODEL_3390)
+ avm_wasp_netboot_mdio_write(avmwasp, 0,
+ WASP_CMD_START_FIRMWARE_3390);
+ else if (m_model == MODEL_X490)
+ avm_wasp_netboot_mdio_write(avmwasp, 0,
+ WASP_CMD_SET_CHECKSUM_X490);
+
+ mdelay(WASP_WAIT_SLEEP);
+
+ if (m_model == MODEL_3390) {
+ count = 0;
+ while ((avm_wasp_netboot_mdio_read(avmwasp, 0) !=
+ WASP_RESP_OK) &&
+ (count < WASP_WAIT_TIMEOUT_COUNT)) {
+ mdelay(WASP_WAIT_SLEEP);
+ count++;
+ }
+ if (count >= WASP_WAIT_TIMEOUT_COUNT) {
+ dev_err(&avmwasp->pdev->dev,
+ "Timed out waiting for WASP OK.\n");
+ return -EFAULT;
+ }
+ if (avm_wasp_netboot_write_chunk(avmwasp, mac_data,
+ WASP_CHUNK_SIZE) < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error sending MAC address!\n");
+ return -EFAULT;
+ }
+ } else if (m_model == MODEL_X490) {
+ cont = 1;
+ while (cont) {
+ count = 0;
+ while ((avm_wasp_netboot_mdio_read(avmwasp, 0)
+ != WASP_RESP_OK) &&
+ (count < WASP_WAIT_TIMEOUT_COUNT)) {
+ mdelay(WASP_WAIT_SLEEP);
+ count++;
+ }
+ if (count >= WASP_WAIT_TIMEOUT_COUNT) {
+ dev_err(&avmwasp->pdev->dev,
+ "Timed out waiting for WASP OK.\n");
+ return -EFAULT;
+ }
+ regval = avm_wasp_netboot_mdio_read(avmwasp, 1);
+ regval2 = avm_wasp_netboot_mdio_read(avmwasp, 2);
+ avm_wasp_netboot_mdio_write(avmwasp, 0,
+ WASP_CMD_SET_CHECKSUM_X490
+ );
+ if (regval == 0 && regval2 != 0)
+ cont = regval2;
+ else
+ cont--;
+ }
+
+ count = 0;
+ while ((avm_wasp_netboot_mdio_read(avmwasp, 0) !=
+ WASP_RESP_OK) &&
+ (count < WASP_TIMEOUT_COUNT)) {
+ udelay(WASP_BOOT_SLEEP_US);
+ count++;
+ }
+ if (count >= WASP_TIMEOUT_COUNT) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error waiting for checksum OK response.\n");
+ return -EFAULT;
+ }
+
+ avm_wasp_netboot_mdio_write(avmwasp, 1, 0x00);
+ avm_wasp_netboot_mdio_write(avmwasp, 0,
+ WASP_CMD_START_FIRMWARE2_X490);
+
+ regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
+ if (regval != WASP_RESP_OK) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error starting WASP network boot: 0x%x\n",
+ regval);
+ return -EFAULT;
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * avm_wasp_load_initramfs_image() - load initramfs image to WASP
+ * @avmwasp: pointer to drivers private avm_wasp_rproc structure
+ *
+ * Uses the lan port specified from DT to load the initramfs to
+ * WASP after the network boot firmware was successfully started.
+ * Communication is done by using raw sockets.
+ * The port of the lantiq gswip device will be started if not
+ * already up and running.
+ * There are several commands and status values which are checked.
+ * First a discovery packet is received and then each data packet
+ * is acknowledged by the WASP network boot firmware.
+ * First packet needs to prepend the load address and last packet
+ * needs to append the execution address.
+ *
+ * Return: 0 on Success, -14 if errors with the WASP send protocol
+ * have occurred or the error returned from the failed operating
+ * system function or service
+ */
+int avm_wasp_load_initramfs_image(struct avm_wasp_rproc *avmwasp)
+{
+ int done = 0;
+ int reuse = 1;
+ int num_chunks = 0;
+ int chunk_counter = 1;
+ int ret, packet_counter, data_offset;
+ int send_len = 0;
+ short interface_flags;
+ ssize_t numbytes;
+ ssize_t read;
+ const u8 *firmware;
+ const u8 *firmware_end;
+ struct wasp_packet *packet = (struct wasp_packet *)
+ (avmwasp->recvbuf + sizeof(struct ethhdr));
+ struct ethhdr *recv_eh = (struct ethhdr *)avmwasp->recvbuf;
+ struct msghdr recv_socket_hdr;
+ struct kvec recv_vec;
+ struct ethhdr *send_eh = (struct ethhdr *)avmwasp->sendbuf;
+ struct sockaddr_ll send_socket_address;
+ struct msghdr send_socket_hdr;
+ struct kvec send_vec;
+ struct net_device *send_netdev;
+ struct sockaddr send_sock_addr;
+ struct timeval {
+ __kernel_old_time_t tv_sec;
+ __kernel_suseconds_t tv_usec;
+ } timeout;
+ time64_t start_time, current_time;
+
+ if (!avmwasp->linux_blob) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error accessing initramfs image");
+ goto err;
+ }
+
+ ret = sock_create_kern(&init_net, PF_PACKET, SOCK_RAW,
+ htons(ETHER_TYPE_ATH_ECPS_FRAME),
+ &avmwasp->recv_socket);
+ if (ret < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error opening recv socket: %d", ret);
+ goto err;
+ }
+
+ ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET, SO_REUSEADDR,
+ KERNEL_SOCKPTR(&reuse), sizeof(reuse));
+ if (ret < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error SO_REUSEADDR recv socket: %d", ret);
+ goto err_recv;
+ }
+
+ ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET,
+ SO_BINDTODEVICE,
+ KERNEL_SOCKPTR(avmwasp->loader_port),
+ IFNAMSIZ - 1);
+ if (ret < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error SO_BINDTODEVICE recv socket: %d", ret);
+ goto err_recv;
+ }
+
+ timeout.tv_sec = 10;
+ timeout.tv_usec = 0;
+ ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET,
+ SO_RCVTIMEO_OLD,
+ KERNEL_SOCKPTR(&timeout), sizeof(timeout));
+ if (ret < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error SO_RCVTIMEO recv socket: %d", ret);
+ goto err_recv;
+ }
+
+ ret = sock_create_kern(&init_net, AF_PACKET, SOCK_RAW, IPPROTO_RAW,
+ &avmwasp->send_socket);
+ if (ret < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error opening send socket: %d", ret);
+ goto err_recv;
+ }
+
+ timeout.tv_sec = 10;
+ timeout.tv_usec = 0;
+ ret = sock_setsockopt(avmwasp->send_socket, SOL_SOCKET,
+ SO_SNDTIMEO_OLD,
+ KERNEL_SOCKPTR(&timeout), sizeof(timeout));
+ if (ret < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error SO_SNDTIMEO send socket: %d", ret);
+ goto err_send;
+ }
+
+ rcu_read_lock();
+ send_netdev = dev_get_by_name_rcu(sock_net(avmwasp->send_socket->sk),
+ avmwasp->loader_port);
+ if (send_netdev)
+ interface_flags = (short)dev_get_flags(send_netdev);
+ rcu_read_unlock();
+
+ if (IS_ERR_OR_NULL(send_netdev)) {
+ dev_err(&avmwasp->pdev->dev, "Error accessing net device.\n");
+ ret = -ENODEV;
+ goto err_send;
+ }
+
+ interface_flags |= IFF_PROMISC | IFF_UP | IFF_RUNNING;
+ rtnl_lock();
+ ret = dev_change_flags(send_netdev, interface_flags, NULL);
+ rtnl_unlock();
+
+ if (ret) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error changing interface flags: %d\n", ret);
+ goto err_send;
+ }
+
+ avmwasp->ifindex = send_netdev->ifindex;
+ ret = dev_get_mac_address(&send_sock_addr,
+ sock_net(avmwasp->send_socket->sk),
+ avmwasp->loader_port);
+ if (ret < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error getting mac address: %d\n", ret);
+ goto err_send;
+ }
+
+ memset(avmwasp->sendbuf, 0, BUF_SIZE);
+
+ memcpy(send_eh->h_dest, wasp_mac, sizeof(send_eh->h_dest));
+ send_eh->h_proto = ETHER_TYPE_ATH_ECPS_FRAME;
+ memcpy(send_eh->h_source, send_sock_addr.sa_data,
+ sizeof(send_eh->h_source));
+
+ start_time = ktime_get_seconds();
+
+ while (!done) {
+ current_time = ktime_get_seconds();
+ if ((current_time - start_time) > SEND_LOOP_TIMEOUT_SECONDS) {
+ dev_err(&avmwasp->pdev->dev,
+ "Waiting for packet from WASP timed out.\n");
+ ret = -EFAULT;
+ goto err_send;
+ }
+
+ memset(&recv_vec, 0, sizeof(recv_vec));
+ memset(&recv_socket_hdr, 0, sizeof(recv_socket_hdr));
+ recv_vec.iov_base = avmwasp->recvbuf;
+ recv_vec.iov_len = BUF_SIZE;
+ numbytes = kernel_recvmsg(avmwasp->recv_socket,
+ &recv_socket_hdr, &recv_vec, 1,
+ BUF_SIZE, 0);
+
+ if (numbytes < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error receiving any packet or timeout: %d\n",
+ numbytes);
+ ret = -EFAULT;
+ goto err_send;
+ }
+
+ if (numbytes < (sizeof(struct ethhdr) + WASP_HEADER_LEN)) {
+ dev_err(&avmwasp->pdev->dev,
+ "Packet too small, discard and continue.\n");
+ continue;
+ }
+
+ if (recv_eh->h_proto != ETHER_TYPE_ATH_ECPS_FRAME)
+ continue;
+
+ memcpy(wasp_mac, recv_eh->h_source, sizeof(wasp_mac));
+ memset(&avmwasp->s_packet, 0, sizeof(avmwasp->s_packet));
+
+ if (packet->packet_start == PACKET_START) {
+ switch (packet->response) {
+ case RESP_DISCOVER:
+ packet_counter = 0;
+ firmware = avmwasp->linux_blob->data;
+ firmware_end = firmware
+ + avmwasp->linux_blob->size;
+
+ chunk_counter = 1;
+ num_chunks =
+ avmwasp->linux_blob->size / CHUNK_SIZE;
+ if (avmwasp->linux_blob->size % CHUNK_SIZE != 0)
+ num_chunks++;
+ break;
+ case RESP_OK:
+ /* got reply send next packet */
+ break;
+ case RESP_ERROR:
+ dev_err(&avmwasp->pdev->dev,
+ "Received an WASP error packet!\n");
+ ret = -EFAULT;
+ goto err_send;
+ break;
+ case RESP_STARTING:
+ done = 1;
+ ret = 0;
+ continue;
+ break;
+ default:
+ dev_err(&avmwasp->pdev->dev,
+ "Unknown packet! Continue.\n");
+ continue;
+ break;
+ }
+
+ if (packet_counter == 0) {
+ memcpy(avmwasp->s_packet.payload, &m_load_addr,
+ sizeof(m_load_addr));
+ data_offset = sizeof(m_load_addr);
+ } else {
+ data_offset = 0;
+ }
+
+ if (firmware < firmware_end) {
+ if ((firmware_end - firmware) >= CHUNK_SIZE)
+ read = CHUNK_SIZE;
+ else
+ read = firmware_end - firmware;
+ memcpy(&avmwasp->s_packet.payload[data_offset],
+ firmware, read);
+ firmware = firmware + CHUNK_SIZE;
+
+ avmwasp->s_packet.packet_start = PACKET_START;
+ if (chunk_counter == num_chunks) {
+ avmwasp->s_packet.response =
+ CMD_START_FIRMWARE;
+ memcpy(&avmwasp->s_packet.payload
+ [data_offset + read],
+ &m_load_addr, sizeof(m_load_addr));
+ data_offset += sizeof(m_load_addr);
+ } else {
+ avmwasp->s_packet.command =
+ CMD_FIRMWARE_DATA;
+ }
+ avmwasp->s_packet.counter = packet_counter;
+
+ memcpy(avmwasp->sendbuf + sizeof(struct ethhdr),
+ avmwasp->s_packet.data,
+ WASP_HEADER_LEN + read + data_offset);
+ send_len = sizeof(struct ethhdr)
+ + WASP_HEADER_LEN + read + data_offset;
+ send_socket_address.sll_halen = ETH_ALEN;
+ send_socket_address.sll_ifindex =
+ avmwasp->ifindex;
+
+ memset(&send_vec, 0, sizeof(send_vec));
+ send_vec.iov_len = send_len;
+ send_vec.iov_base = avmwasp->sendbuf;
+
+ memset(&send_socket_hdr, 0,
+ sizeof(send_socket_hdr));
+ send_socket_hdr.msg_name = (struct sockaddr *)
+ &send_socket_address;
+ send_socket_hdr.msg_namelen =
+ sizeof(struct sockaddr_ll);
+
+ ret = kernel_sendmsg(avmwasp->send_socket,
+ &send_socket_hdr,
+ &send_vec,
+ 1, send_len);
+ if (ret < 0) {
+ dev_err(&avmwasp->pdev->dev,
+ "Error sending to WASP %d\n",
+ ret);
+ goto err_send;
+ }
+
+ packet_counter += COUNTER_INCR;
+ chunk_counter++;
+ }
+ }
+ }
+
+err_send:
+ avmwasp->send_socket->ops->release(avmwasp->send_socket);
+err_recv:
+ avmwasp->recv_socket->ops->release(avmwasp->recv_socket);
+err:
+ return ret;
+}
+
+/**
+ * avm_wasp_rproc_start() - start the remote processor
+ * @rproc: pointer to the rproc structure
+ *
+ * Starts the remote processor by turning it on using the startup
+ * gpio and initiating the reset process using the reset_gpio.
+ * After that the status is checked if poweron and reset were
+ * successful.
+ * As the first step, the network boot firmware is tried to be loaded
+ * and started.
+ * As a second step, the initramfs image is tried to be loaded
+ * and started.
+ *
+ * Return: 0 on Success, -19 or return code from the called function
+ * if any other error occurred in the process of starting and loading
+ * the firmware files to the WASP processor
+ */
+static int avm_wasp_rproc_start(struct rproc *rproc)
+{
+ struct avm_wasp_rproc *avmwasp = rproc->priv;
+ int ret;
+
+ gpio_set_value(avmwasp->startup_gpio,
+ (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
+ 0 : 1);
+ mdelay(WASP_WAIT_SLEEP);
+ gpio_set_value(avmwasp->reset_gpio,
+ (avmwasp->r_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
+ 1 : 0);
+ mdelay(WASP_WAIT_SLEEP);
+ gpio_set_value(avmwasp->reset_gpio,
+ (avmwasp->r_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
+ 0 : 1);
+ mdelay(WASP_WAIT_SLEEP);
+
+ avmwasp->mdio_bus = mdio_find_bus(avmwasp->mdio_bus_id);
+ if (!avmwasp->mdio_bus) {
+ dev_err(&avmwasp->pdev->dev,
+ "wasp-netboot-mdio bus not found\n");
+ return -ENODEV;
+ }
+
+ ret = avm_wasp_netboot_load_firmware(avmwasp);
+ if (ret) {
+ put_device(&avmwasp->mdio_bus->dev);
+ return ret;
+ }
+
+ put_device(&avmwasp->mdio_bus->dev);
+
+ ret = avm_wasp_load_initramfs_image(avmwasp);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+/**
+ * avm_wasp_rproc_stop() - stop the remote processor
+ * @rproc: pointer to the rproc structure
+ *
+ * To stop the remote processor just the startup gpio is set to 0
+ * and the WASP processor is powered off
+ *
+ * Return: 0 on Success
+ */
+static int avm_wasp_rproc_stop(struct rproc *rproc)
+{
+ struct avm_wasp_rproc *avmwasp = rproc->priv;
+
+ gpio_set_value(avmwasp->startup_gpio,
+ (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
+ 1 : 0);
+
+ return 0;
+}
+
+/**
+ * avm_wasp_rproc_load() - noop to avoid the ELF binary defaults
+ * @rproc: pointer to the rproc structure
+ * @fw: pointer to firmware struct
+ *
+ * If a load function is not defined in the rproc_ops, then all the settings
+ * like checking the firmware binary will default to ELF checks, which fail
+ * in case of the bootable and compressed initramfs image for WASP.
+ * Furthermore during boot its just required to send the firmware to the WASP
+ * processor, its not required to keep it in local memory, as the WASP SoC
+ * has its own memory.
+ *
+ * Return: Always 0
+ */
+static int avm_wasp_rproc_load(struct rproc *rproc, const struct firmware *fw)
+{
+ return 0;
+}
+
+/**
+ * avm_wasp_rproc_boot_addr() - store fw from framework in priv
+ * @rproc: pointer to the rproc structure
+ * @fw: pointer to firmware struct
+ *
+ * Even though firmware files can be loaded without the remote processor
+ * framework, it expects at least one firmware file.
+ * This function stores the initramfs image that is loaded by the remote
+ * processor framework during boot process into the priv for access by
+ * the initramfs load function avm_wasp_load_initramfs_image().
+ *
+ * Return: Address of initramfs image
+ */
+static u64 avm_wasp_rproc_boot_addr(struct rproc *rproc,
+ const struct firmware *fw)
+{
+ struct avm_wasp_rproc *avmwasp = rproc->priv;
+
+ avmwasp->linux_blob = fw;
+
+ return (u64)((u32)fw->data);
+}
+
+static const struct rproc_ops avm_wasp_rproc_ops = {
+ .start = avm_wasp_rproc_start,
+ .stop = avm_wasp_rproc_stop,
+ .load = avm_wasp_rproc_load,
+ .get_boot_addr = avm_wasp_rproc_boot_addr,
+};
+
+static int avm_wasp_rproc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct avm_wasp_rproc *avmwasp;
+ const char *fw_name;
+ struct rproc *rproc;
+ struct device_node *root_node;
+ int ret;
+ u32 phandle;
+ char *model;
+
+ root_node = of_find_node_by_path("/");
+ if (!root_node) {
+ dev_err(dev, "No root node in device tree.\n");
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = of_property_read_string_index(root_node, "compatible",
+ 0, (const char **)&model);
+ of_node_put(root_node);
+ if (ret) {
+ dev_err(dev, "No model in device tree.\n");
+ goto err;
+ }
+
+ /* check model of host device to determine WASP SoC type */
+ if (strstr(model, "3390")) {
+ m_model = MODEL_3390;
+ } else if (strstr(model, "490")) {
+ m_model = MODEL_X490;
+ } else {
+ dev_err(dev, "No WASP on device.\n");
+ ret = -EPERM;
+ goto err;
+ }
+
+ ret = of_property_read_string(dev->of_node, "wasp-initramfs-image",
+ &fw_name);
+ if (ret) {
+ dev_err(dev, "No initramfs image for WASP filename given\n");
+ goto err;
+ }
+
+ rproc = devm_rproc_alloc(dev, "avm,wasp", &avm_wasp_rproc_ops,
+ fw_name, sizeof(*avmwasp));
+ if (!rproc) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ rproc->auto_boot = true;
+
+ avmwasp = rproc->priv;
+ avmwasp->rproc = rproc;
+ avmwasp->pdev = pdev;
+
+ ret = of_property_read_string(dev->of_node, "ath9k-firmware",
+ &fw_name);
+ if (ret) {
+ dev_err(dev, "No ath9k firmware filename given\n");
+ goto err;
+ }
+
+ ret = avm_wasp_firmware_request(avmwasp, fw_name);
+ if (ret) {
+ dev_err(dev, "Could not load ath9k firmware\n");
+ goto err;
+ }
+ avm_wasp_firmware_release(avmwasp);
+
+ if (m_model == MODEL_X490) {
+ ret = of_property_read_string(dev->of_node, "ath10k-caldata",
+ &fw_name);
+ if (ret) {
+ dev_err(dev, "No ath10k caldata filename given\n");
+ goto err;
+ }
+
+ ret = avm_wasp_firmware_request(avmwasp, fw_name);
+ if (ret) {
+ dev_err(dev, "Could not load ath10k caldata\n");
+ goto err;
+ }
+ avm_wasp_firmware_release(avmwasp);
+ }
+
+ ret = of_property_read_u32(dev->of_node, "wasp-initramfs-port",
+ &phandle);
+ if (ret) {
+ dev_err(dev, "No wasp-initramfs-port given\n");
+ goto err;
+ } else {
+ struct device_node *child = of_find_node_by_phandle(phandle);
+
+ if (!child) {
+ dev_err(dev, "Get wasp-initramfs-port child failed\n");
+ ret = -ENODEV;
+ goto err;
+ } else {
+ ret = of_property_read_string(child, "label",
+ (const char **)
+ &avmwasp->loader_port);
+ of_node_put(child);
+ if (ret) {
+ dev_err(dev,
+ "Get wasp-port-label failed\n");
+ goto err;
+ }
+ }
+ }
+
+ ret = of_property_read_u32(dev->of_node, "wasp-netboot-mdio",
+ &phandle);
+ if (ret) {
+ dev_err(dev, "No wasp-netboot-mdio given\n");
+ goto err;
+ } else {
+ struct device_node *mdio_node =
+ of_find_node_by_phandle(phandle);
+
+ if (!mdio_node) {
+ dev_err(dev, "Get wasp-netboot-mdio failed\n");
+ ret = -ENODEV;
+ goto err;
+ } else {
+ avmwasp->mdio_bus = of_mdio_find_bus(mdio_node);
+ of_node_put(mdio_node);
+ if (!avmwasp->mdio_bus) {
+ dev_err(dev, "mdio bus not found\n");
+ ret = -ENODEV;
+ goto err;
+ }
+ avmwasp->mdio_bus_id = avmwasp->mdio_bus->id;
+ put_device(&avmwasp->mdio_bus->dev);
+ }
+ }
+
+ avmwasp->startup_gpio = of_get_named_gpio_flags(dev->of_node,
+ "startup-gpio",
+ 0,
+ &avmwasp->s_gpio_flg);
+ if (!gpio_is_valid(avmwasp->startup_gpio)) {
+ dev_err(dev, "Request wasp-startup gpio failed\n");
+ ret = -ENODEV;
+ goto err;
+ } else {
+ ret = devm_gpio_request_one(dev, avmwasp->startup_gpio,
+ (avmwasp->s_gpio_flg &
+ OF_GPIO_ACTIVE_LOW) ?
+ GPIOF_OUT_INIT_LOW :
+ GPIOF_OUT_INIT_HIGH,
+ "wasp-startup");
+
+ if (ret) {
+ dev_err(dev, "get wasp-startup gpio failed\n");
+ goto err;
+ }
+ }
+
+ avmwasp->reset_gpio = of_get_named_gpio_flags(dev->of_node,
+ "reset-gpio",
+ 0,
+ &avmwasp->r_gpio_flg);
+ if (!gpio_is_valid(avmwasp->reset_gpio)) {
+ dev_err(dev, "Request wasp-reset gpio failed\n");
+ ret = -ENODEV;
+ goto err_free_startup_gpio;
+ } else {
+ ret = devm_gpio_request_one(dev, avmwasp->reset_gpio,
+ (avmwasp->r_gpio_flg &
+ OF_GPIO_ACTIVE_LOW) ?
+ GPIOF_OUT_INIT_LOW :
+ GPIOF_OUT_INIT_HIGH,
+ "wasp-reset");
+
+ if (ret) {
+ dev_err(dev, "get wasp-reset gpio failed\n");
+ goto err_free_startup_gpio;
+ }
+ }
+
+ ret = of_property_read_string(dev->of_node, "wasp-netboot-firmware",
+ (const char **)
+ &avmwasp->netboot_firmware);
+ if (ret) {
+ dev_err(dev, "No WASP network boot firmware filename given\n");
+ goto err_free_reset_gpio;
+ }
+
+ ret = request_firmware_direct((const struct firmware **)
+ &avmwasp->eeprom_blob, avmwasp->netboot_firmware, dev);
+ if (ret) {
+ dev_err(dev, "Could not load WASP network boot firmware\n");
+ goto err_free_reset_gpio;
+ }
+
+ if (avmwasp->eeprom_blob->size > 0xffff) {
+ dev_err(dev, "WASP network boot firmware too big\n");
+ ret = -EINVAL;
+ goto err_free_reset_gpio;
+ }
+
+ avm_wasp_firmware_release(avmwasp);
+
+ dev_set_drvdata(dev, rproc);
+
+ ret = devm_rproc_add(dev, rproc);
+ if (ret) {
+ dev_err(dev, "rproc_add failed\n");
+ goto err_free_reset_gpio;
+ }
+
+ return 0;
+
+err_free_reset_gpio:
+ devm_gpio_free(&avmwasp->pdev->dev, avmwasp->reset_gpio);
+ gpio_set_value(avmwasp->startup_gpio,
+ (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
+ 1 : 0);
+err_free_startup_gpio:
+ devm_gpio_free(&avmwasp->pdev->dev, avmwasp->startup_gpio);
+err:
+ return ret;
+}
+
+static int avm_wasp_rproc_remove(struct platform_device *pdev)
+{
+ struct rproc *rproc = platform_get_drvdata(pdev);
+ struct avm_wasp_rproc *avmwasp = rproc->priv;
+
+ gpio_set_value(avmwasp->startup_gpio,
+ (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
+ 1 : 0);
+ mdelay(WASP_WAIT_SLEEP);
+ devm_gpio_free(&avmwasp->pdev->dev, avmwasp->startup_gpio);
+ devm_gpio_free(&avmwasp->pdev->dev, avmwasp->reset_gpio);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int avm_wasp_rpm_suspend(struct device *dev)
+{
+ return -EBUSY;
+}
+
+static int avm_wasp_rpm_resume(struct device *dev)
+{
+ return 0;
+}
+#endif
+
+static const struct of_device_id avm_wasp_rproc_of_match[] = {
+ { .compatible = "avm,wasp", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, avm_wasp_rproc_of_match);
+
+static struct platform_driver avm_wasp_rproc_driver = {
+ .probe = avm_wasp_rproc_probe,
+ .remove = avm_wasp_rproc_remove,
+ .driver = {
+ .name = "avm_wasp_rproc",
+ .of_match_table = avm_wasp_rproc_of_match,
+ },
+};
+
+module_platform_driver(avm_wasp_rproc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("AVM WASP remote processor boot driver");
+MODULE_AUTHOR("Daniel Kestrel <kestrelseventyfour@gmail.com>");
diff --git a/drivers/remoteproc/avm_wasp.h b/drivers/remoteproc/avm_wasp.h
new file mode 100644
index 000000000000..d0a4542b3420
--- /dev/null
+++ b/drivers/remoteproc/avm_wasp.h
@@ -0,0 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2019-2020 Andreas Böhler
+ * Copyright (c) 2021-2022 Daniel Kestrel
+ */
+
+#ifndef __AVM_WASP_H
+#define __AVM_WASP_H
+
+#define WASP_CHUNK_SIZE 14
+#define M_REGS_WASP_INDEX_MAX 7
+
+#define WASP_ADDR 0x07
+#define WASP_TIMEOUT_COUNT 1000
+#define WASP_WAIT_TIMEOUT_COUNT 20
+
+#define WASP_WRITE_SLEEP_US 20000
+#define WASP_WAIT_SLEEP 100
+#define WASP_POLL_SLEEP_US 200
+#define WASP_BOOT_SLEEP_US 20000
+
+#define WASP_RESP_RETRY 0x0102
+#define WASP_RESP_OK 0x0002
+#define WASP_RESP_WAIT 0x0401
+#define WASP_RESP_COMPLETED 0x0000
+#define WASP_RESP_READY_TO_START 0x0202
+#define WASP_RESP_STARTING 0x00c9
+
+#define WASP_CMD_SET_PARAMS 0x0c01
+#define WASP_CMD_SET_CHECKSUM_3390 0x0801
+#define WASP_CMD_SET_CHECKSUM_X490 0x0401
+#define WASP_CMD_SET_DATA 0x0e01
+#define WASP_CMD_START_FIRMWARE_3390 0x0201
+#define WASP_CMD_START_FIRMWARE_X490 0x0001
+#define WASP_CMD_START_FIRMWARE2_X490 0x0101
+
+static const u32 start_addr = 0xbd003000;
+static const u32 exec_addr = 0xbd003000;
+
+static u16 m_regs_wasp[] = {0x0, 0x2, 0x4, 0x6, 0x8, 0xA, 0xC, 0xE};
+
+static const char mac_data[WASP_CHUNK_SIZE] = {0xaa, 0xaa, 0xaa, 0xaa, 0xaa,
+ 0xaa, 0x04, 0x20, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00};
+
+enum {
+ MODEL_3390,
+ MODEL_X490,
+ MODEL_UNKNOWN
+} m_model = MODEL_UNKNOWN;
+
+#define ETHER_TYPE_ATH_ECPS_FRAME 0x88bd
+#define BUF_SIZE 1056
+#define COUNTER_INCR 4
+#define SEND_LOOP_TIMEOUT_SECONDS 60
+
+#define MAX_PAYLOAD_SIZE 1028
+#define CHUNK_SIZE 1024
+#define WASP_HEADER_LEN 14
+
+#define PACKET_START 0x1200
+#define CMD_FIRMWARE_DATA 0x0104
+#define CMD_START_FIRMWARE 0xd400
+
+#define RESP_DISCOVER 0x0000
+#define RESP_CONFIG 0x1000
+#define RESP_OK 0x0100
+#define RESP_STARTING 0x0200
+#define RESP_ERROR 0x0300
+
+enum {
+ DOWNLOAD_TYPE_UNKNOWN = 0,
+ DOWNLOAD_TYPE_FIRMWARE,
+ DOWNLOAD_TYPE_CONFIG
+} m_download_type = DOWNLOAD_TYPE_UNKNOWN;
+
+static const u32 m_load_addr = 0x81a00000;
+
+static char wasp_mac[] = {0x00, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa};
+
+struct wasp_packet {
+ union {
+ u8 data[MAX_PAYLOAD_SIZE + WASP_HEADER_LEN];
+ struct __packed {
+ u16 packet_start;
+ u8 pad_one[5];
+ u16 command;
+ u16 response;
+ u16 counter;
+ u8 pad_two;
+ u8 payload[MAX_PAYLOAD_SIZE];
+ };
+ };
+} __packed;
+
+#endif /* __AVM_WASP_H */
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] remoteproc: Add AVM WASP driver
2022-02-21 13:54 [PATCH 3/3] remoteproc: Add AVM WASP driver Daniel Kestrel
@ 2022-02-21 17:43 ` kernel test robot
2022-02-21 18:34 ` kernel test robot
2022-02-22 0:30 ` Bjorn Andersson
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2022-02-21 17:43 UTC (permalink / raw)
To: Daniel Kestrel
Cc: kbuild-all, Bjorn Andersson, Mathieu Poirier, Rob Herring,
Daniel Kestrel, linux-remoteproc, devicetree, linux-kernel
Hi Daniel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on remoteproc/rproc-next]
[also build test WARNING on robh/for-next v5.17-rc5 next-20220217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Daniel-Kestrel/Add-support-for-WASP-SoC-on-AVM-router-boards/20220221-215619
base: git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
config: nios2-allyesconfig (https://download.01.org/0day-ci/archive/20220222/202202220129.7LjlrUsi-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/76e19a3c7ae383687205d7be3ac6224253d97704
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Kestrel/Add-support-for-WASP-SoC-on-AVM-router-boards/20220221-215619
git checkout 76e19a3c7ae383687205d7be3ac6224253d97704
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nios2 SHELL=/bin/bash drivers/remoteproc/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/remoteproc/avm_wasp.c:150:5: warning: no previous prototype for 'avm_wasp_netboot_mdio_read' [-Wmissing-prototypes]
150 | int avm_wasp_netboot_mdio_read(struct avm_wasp_rproc *avmwasp,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/remoteproc/avm_wasp.c:176:6: warning: no previous prototype for 'avm_wasp_netboot_mdio_write' [-Wmissing-prototypes]
176 | void avm_wasp_netboot_mdio_write(struct avm_wasp_rproc *avmwasp,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/remoteproc/avm_wasp.c:197:6: warning: no previous prototype for 'avm_wasp_netboot_mdio_write_u32_split' [-Wmissing-prototypes]
197 | void avm_wasp_netboot_mdio_write_u32_split(struct avm_wasp_rproc *avmwasp,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/remoteproc/avm_wasp.c:380:5: warning: no previous prototype for 'avm_wasp_netboot_load_firmware' [-Wmissing-prototypes]
380 | int avm_wasp_netboot_load_firmware(struct avm_wasp_rproc *avmwasp)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/remoteproc/avm_wasp.c:569:5: warning: no previous prototype for 'avm_wasp_load_initramfs_image' [-Wmissing-prototypes]
569 | int avm_wasp_load_initramfs_image(struct avm_wasp_rproc *avmwasp)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/avm_wasp_netboot_mdio_read +150 drivers/remoteproc/avm_wasp.c
138
139 /**
140 * avm_wasp_netboot_mdio_read() - read with gswip mdio bus
141 * @avmwasp: pointer to drivers private avm_wasp_rproc structure
142 * @location: register number of the m_regs_wasp register array
143 *
144 * Reads a value from the specified register for the mdio address
145 * that is used for the connection to the WASP SoC
146 * Mutex on mdio_lock is required to serialize access on bus
147 *
148 * Return: Value that was read from the specified register
149 */
> 150 int avm_wasp_netboot_mdio_read(struct avm_wasp_rproc *avmwasp,
151 int location)
152 {
153 int value;
154
155 if (location > M_REGS_WASP_INDEX_MAX || location < 0)
156 return 0;
157 mutex_lock(&avmwasp->mdio_bus->mdio_lock);
158 value = avmwasp->mdio_bus->read(avmwasp->mdio_bus,
159 WASP_ADDR, m_regs_wasp[location]);
160 mutex_unlock(&avmwasp->mdio_bus->mdio_lock);
161 return value;
162 }
163
164 /**
165 * avm_wasp_netboot_mdio_write() - write with gswip mdio bus
166 * @avmwasp: pointer to drivers private avm_wasp_rproc structure
167 * @location: register number of the m_regs_wasp register array
168 * @value: value to be written to the register
169 *
170 * Writes a value to the specified register for the mdio address
171 * that is used for the connection to the WASP SoC
172 * Mutex on mdio_lock is required to serialize access on bus
173 * Makes sure not to write to invalid registers as this can have
174 * unpredictable results
175 */
> 176 void avm_wasp_netboot_mdio_write(struct avm_wasp_rproc *avmwasp,
177 int location, int value)
178 {
179 if (location > M_REGS_WASP_INDEX_MAX || location < 0)
180 return;
181 mutex_lock(&avmwasp->mdio_bus->mdio_lock);
182 avmwasp->mdio_bus->write(avmwasp->mdio_bus, WASP_ADDR,
183 m_regs_wasp[location], value);
184 mutex_unlock(&avmwasp->mdio_bus->mdio_lock);
185 }
186
187 /**
188 * avm_wasp_netboot_mdio_write_u32_split() - write 32bit value
189 * @avmwasp: pointer to drivers private avm_wasp_rproc structure
190 * @location: register number of the m_regs_wasp register array
191 * @value: value to be written to the register
192 *
193 * As the mdio registers are 16bit, this function writes a 32bit value
194 * to two subsequent registers starting with the specified register
195 * for the mdio address that is used for the connection to the WASP SoC
196 */
> 197 void avm_wasp_netboot_mdio_write_u32_split(struct avm_wasp_rproc *avmwasp,
198 int location, const u32 value)
199 {
200 avm_wasp_netboot_mdio_write(avmwasp, location,
201 ((value & 0xffff0000) >> 16));
202 avm_wasp_netboot_mdio_write(avmwasp, location + 1,
203 (value & 0x0000ffff));
204 }
205
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] remoteproc: Add AVM WASP driver
2022-02-21 13:54 [PATCH 3/3] remoteproc: Add AVM WASP driver Daniel Kestrel
2022-02-21 17:43 ` kernel test robot
@ 2022-02-21 18:34 ` kernel test robot
2022-02-22 0:30 ` Bjorn Andersson
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2022-02-21 18:34 UTC (permalink / raw)
To: Daniel Kestrel
Cc: kbuild-all, Bjorn Andersson, Mathieu Poirier, Rob Herring,
Daniel Kestrel, linux-remoteproc, devicetree, linux-kernel
Hi Daniel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on remoteproc/rproc-next]
[also build test WARNING on robh/for-next v5.17-rc5 next-20220217]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Daniel-Kestrel/Add-support-for-WASP-SoC-on-AVM-router-boards/20220221-215619
base: git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220222/202202220201.CqRPstWg-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/76e19a3c7ae383687205d7be3ac6224253d97704
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Kestrel/Add-support-for-WASP-SoC-on-AVM-router-boards/20220221-215619
git checkout 76e19a3c7ae383687205d7be3ac6224253d97704
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash drivers/remoteproc/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/remoteproc/avm_wasp.c:150:5: warning: no previous prototype for 'avm_wasp_netboot_mdio_read' [-Wmissing-prototypes]
150 | int avm_wasp_netboot_mdio_read(struct avm_wasp_rproc *avmwasp,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/remoteproc/avm_wasp.c:176:6: warning: no previous prototype for 'avm_wasp_netboot_mdio_write' [-Wmissing-prototypes]
176 | void avm_wasp_netboot_mdio_write(struct avm_wasp_rproc *avmwasp,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/remoteproc/avm_wasp.c:197:6: warning: no previous prototype for 'avm_wasp_netboot_mdio_write_u32_split' [-Wmissing-prototypes]
197 | void avm_wasp_netboot_mdio_write_u32_split(struct avm_wasp_rproc *avmwasp,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/remoteproc/avm_wasp.c:380:5: warning: no previous prototype for 'avm_wasp_netboot_load_firmware' [-Wmissing-prototypes]
380 | int avm_wasp_netboot_load_firmware(struct avm_wasp_rproc *avmwasp)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/remoteproc/avm_wasp.c:569:5: warning: no previous prototype for 'avm_wasp_load_initramfs_image' [-Wmissing-prototypes]
569 | int avm_wasp_load_initramfs_image(struct avm_wasp_rproc *avmwasp)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:15,
from include/linux/node.h:18,
from include/linux/cpu.h:17,
from include/linux/of_device.h:5,
from drivers/remoteproc/avm_wasp.c:14:
drivers/remoteproc/avm_wasp.c: In function 'avm_wasp_load_initramfs_image':
>> drivers/remoteproc/avm_wasp.c:724:33: warning: format '%d' expects argument of type 'int', but argument 3 has type 'ssize_t' {aka 'long int'} [-Wformat=]
724 | "Error receiving any packet or timeout: %d\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/remoteproc/avm_wasp.c:723:25: note: in expansion of macro 'dev_err'
723 | dev_err(&avmwasp->pdev->dev,
| ^~~~~~~
drivers/remoteproc/avm_wasp.c:724:74: note: format string is defined here
724 | "Error receiving any packet or timeout: %d\n",
| ~^
| |
| int
| %ld
drivers/remoteproc/avm_wasp.c: In function 'avm_wasp_rproc_boot_addr':
>> drivers/remoteproc/avm_wasp.c:969:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
969 | return (u64)((u32)fw->data);
| ^
vim +724 drivers/remoteproc/avm_wasp.c
549
550 /**
551 * avm_wasp_load_initramfs_image() - load initramfs image to WASP
552 * @avmwasp: pointer to drivers private avm_wasp_rproc structure
553 *
554 * Uses the lan port specified from DT to load the initramfs to
555 * WASP after the network boot firmware was successfully started.
556 * Communication is done by using raw sockets.
557 * The port of the lantiq gswip device will be started if not
558 * already up and running.
559 * There are several commands and status values which are checked.
560 * First a discovery packet is received and then each data packet
561 * is acknowledged by the WASP network boot firmware.
562 * First packet needs to prepend the load address and last packet
563 * needs to append the execution address.
564 *
565 * Return: 0 on Success, -14 if errors with the WASP send protocol
566 * have occurred or the error returned from the failed operating
567 * system function or service
568 */
569 int avm_wasp_load_initramfs_image(struct avm_wasp_rproc *avmwasp)
570 {
571 int done = 0;
572 int reuse = 1;
573 int num_chunks = 0;
574 int chunk_counter = 1;
575 int ret, packet_counter, data_offset;
576 int send_len = 0;
577 short interface_flags;
578 ssize_t numbytes;
579 ssize_t read;
580 const u8 *firmware;
581 const u8 *firmware_end;
582 struct wasp_packet *packet = (struct wasp_packet *)
583 (avmwasp->recvbuf + sizeof(struct ethhdr));
584 struct ethhdr *recv_eh = (struct ethhdr *)avmwasp->recvbuf;
585 struct msghdr recv_socket_hdr;
586 struct kvec recv_vec;
587 struct ethhdr *send_eh = (struct ethhdr *)avmwasp->sendbuf;
588 struct sockaddr_ll send_socket_address;
589 struct msghdr send_socket_hdr;
590 struct kvec send_vec;
591 struct net_device *send_netdev;
592 struct sockaddr send_sock_addr;
593 struct timeval {
594 __kernel_old_time_t tv_sec;
595 __kernel_suseconds_t tv_usec;
596 } timeout;
597 time64_t start_time, current_time;
598
599 if (!avmwasp->linux_blob) {
600 dev_err(&avmwasp->pdev->dev,
601 "Error accessing initramfs image");
602 goto err;
603 }
604
605 ret = sock_create_kern(&init_net, PF_PACKET, SOCK_RAW,
606 htons(ETHER_TYPE_ATH_ECPS_FRAME),
607 &avmwasp->recv_socket);
608 if (ret < 0) {
609 dev_err(&avmwasp->pdev->dev,
610 "Error opening recv socket: %d", ret);
611 goto err;
612 }
613
614 ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET, SO_REUSEADDR,
615 KERNEL_SOCKPTR(&reuse), sizeof(reuse));
616 if (ret < 0) {
617 dev_err(&avmwasp->pdev->dev,
618 "Error SO_REUSEADDR recv socket: %d", ret);
619 goto err_recv;
620 }
621
622 ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET,
623 SO_BINDTODEVICE,
624 KERNEL_SOCKPTR(avmwasp->loader_port),
625 IFNAMSIZ - 1);
626 if (ret < 0) {
627 dev_err(&avmwasp->pdev->dev,
628 "Error SO_BINDTODEVICE recv socket: %d", ret);
629 goto err_recv;
630 }
631
632 timeout.tv_sec = 10;
633 timeout.tv_usec = 0;
634 ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET,
635 SO_RCVTIMEO_OLD,
636 KERNEL_SOCKPTR(&timeout), sizeof(timeout));
637 if (ret < 0) {
638 dev_err(&avmwasp->pdev->dev,
639 "Error SO_RCVTIMEO recv socket: %d", ret);
640 goto err_recv;
641 }
642
643 ret = sock_create_kern(&init_net, AF_PACKET, SOCK_RAW, IPPROTO_RAW,
644 &avmwasp->send_socket);
645 if (ret < 0) {
646 dev_err(&avmwasp->pdev->dev,
647 "Error opening send socket: %d", ret);
648 goto err_recv;
649 }
650
651 timeout.tv_sec = 10;
652 timeout.tv_usec = 0;
653 ret = sock_setsockopt(avmwasp->send_socket, SOL_SOCKET,
654 SO_SNDTIMEO_OLD,
655 KERNEL_SOCKPTR(&timeout), sizeof(timeout));
656 if (ret < 0) {
657 dev_err(&avmwasp->pdev->dev,
658 "Error SO_SNDTIMEO send socket: %d", ret);
659 goto err_send;
660 }
661
662 rcu_read_lock();
663 send_netdev = dev_get_by_name_rcu(sock_net(avmwasp->send_socket->sk),
664 avmwasp->loader_port);
665 if (send_netdev)
666 interface_flags = (short)dev_get_flags(send_netdev);
667 rcu_read_unlock();
668
669 if (IS_ERR_OR_NULL(send_netdev)) {
670 dev_err(&avmwasp->pdev->dev, "Error accessing net device.\n");
671 ret = -ENODEV;
672 goto err_send;
673 }
674
675 interface_flags |= IFF_PROMISC | IFF_UP | IFF_RUNNING;
676 rtnl_lock();
677 ret = dev_change_flags(send_netdev, interface_flags, NULL);
678 rtnl_unlock();
679
680 if (ret) {
681 dev_err(&avmwasp->pdev->dev,
682 "Error changing interface flags: %d\n", ret);
683 goto err_send;
684 }
685
686 avmwasp->ifindex = send_netdev->ifindex;
687 ret = dev_get_mac_address(&send_sock_addr,
688 sock_net(avmwasp->send_socket->sk),
689 avmwasp->loader_port);
690 if (ret < 0) {
691 dev_err(&avmwasp->pdev->dev,
692 "Error getting mac address: %d\n", ret);
693 goto err_send;
694 }
695
696 memset(avmwasp->sendbuf, 0, BUF_SIZE);
697
698 memcpy(send_eh->h_dest, wasp_mac, sizeof(send_eh->h_dest));
699 send_eh->h_proto = ETHER_TYPE_ATH_ECPS_FRAME;
700 memcpy(send_eh->h_source, send_sock_addr.sa_data,
701 sizeof(send_eh->h_source));
702
703 start_time = ktime_get_seconds();
704
705 while (!done) {
706 current_time = ktime_get_seconds();
707 if ((current_time - start_time) > SEND_LOOP_TIMEOUT_SECONDS) {
708 dev_err(&avmwasp->pdev->dev,
709 "Waiting for packet from WASP timed out.\n");
710 ret = -EFAULT;
711 goto err_send;
712 }
713
714 memset(&recv_vec, 0, sizeof(recv_vec));
715 memset(&recv_socket_hdr, 0, sizeof(recv_socket_hdr));
716 recv_vec.iov_base = avmwasp->recvbuf;
717 recv_vec.iov_len = BUF_SIZE;
718 numbytes = kernel_recvmsg(avmwasp->recv_socket,
719 &recv_socket_hdr, &recv_vec, 1,
720 BUF_SIZE, 0);
721
722 if (numbytes < 0) {
723 dev_err(&avmwasp->pdev->dev,
> 724 "Error receiving any packet or timeout: %d\n",
725 numbytes);
726 ret = -EFAULT;
727 goto err_send;
728 }
729
730 if (numbytes < (sizeof(struct ethhdr) + WASP_HEADER_LEN)) {
731 dev_err(&avmwasp->pdev->dev,
732 "Packet too small, discard and continue.\n");
733 continue;
734 }
735
736 if (recv_eh->h_proto != ETHER_TYPE_ATH_ECPS_FRAME)
737 continue;
738
739 memcpy(wasp_mac, recv_eh->h_source, sizeof(wasp_mac));
740 memset(&avmwasp->s_packet, 0, sizeof(avmwasp->s_packet));
741
742 if (packet->packet_start == PACKET_START) {
743 switch (packet->response) {
744 case RESP_DISCOVER:
745 packet_counter = 0;
746 firmware = avmwasp->linux_blob->data;
747 firmware_end = firmware
748 + avmwasp->linux_blob->size;
749
750 chunk_counter = 1;
751 num_chunks =
752 avmwasp->linux_blob->size / CHUNK_SIZE;
753 if (avmwasp->linux_blob->size % CHUNK_SIZE != 0)
754 num_chunks++;
755 break;
756 case RESP_OK:
757 /* got reply send next packet */
758 break;
759 case RESP_ERROR:
760 dev_err(&avmwasp->pdev->dev,
761 "Received an WASP error packet!\n");
762 ret = -EFAULT;
763 goto err_send;
764 break;
765 case RESP_STARTING:
766 done = 1;
767 ret = 0;
768 continue;
769 break;
770 default:
771 dev_err(&avmwasp->pdev->dev,
772 "Unknown packet! Continue.\n");
773 continue;
774 break;
775 }
776
777 if (packet_counter == 0) {
778 memcpy(avmwasp->s_packet.payload, &m_load_addr,
779 sizeof(m_load_addr));
780 data_offset = sizeof(m_load_addr);
781 } else {
782 data_offset = 0;
783 }
784
785 if (firmware < firmware_end) {
786 if ((firmware_end - firmware) >= CHUNK_SIZE)
787 read = CHUNK_SIZE;
788 else
789 read = firmware_end - firmware;
790 memcpy(&avmwasp->s_packet.payload[data_offset],
791 firmware, read);
792 firmware = firmware + CHUNK_SIZE;
793
794 avmwasp->s_packet.packet_start = PACKET_START;
795 if (chunk_counter == num_chunks) {
796 avmwasp->s_packet.response =
797 CMD_START_FIRMWARE;
798 memcpy(&avmwasp->s_packet.payload
799 [data_offset + read],
800 &m_load_addr, sizeof(m_load_addr));
801 data_offset += sizeof(m_load_addr);
802 } else {
803 avmwasp->s_packet.command =
804 CMD_FIRMWARE_DATA;
805 }
806 avmwasp->s_packet.counter = packet_counter;
807
808 memcpy(avmwasp->sendbuf + sizeof(struct ethhdr),
809 avmwasp->s_packet.data,
810 WASP_HEADER_LEN + read + data_offset);
811 send_len = sizeof(struct ethhdr)
812 + WASP_HEADER_LEN + read + data_offset;
813 send_socket_address.sll_halen = ETH_ALEN;
814 send_socket_address.sll_ifindex =
815 avmwasp->ifindex;
816
817 memset(&send_vec, 0, sizeof(send_vec));
818 send_vec.iov_len = send_len;
819 send_vec.iov_base = avmwasp->sendbuf;
820
821 memset(&send_socket_hdr, 0,
822 sizeof(send_socket_hdr));
823 send_socket_hdr.msg_name = (struct sockaddr *)
824 &send_socket_address;
825 send_socket_hdr.msg_namelen =
826 sizeof(struct sockaddr_ll);
827
828 ret = kernel_sendmsg(avmwasp->send_socket,
829 &send_socket_hdr,
830 &send_vec,
831 1, send_len);
832 if (ret < 0) {
833 dev_err(&avmwasp->pdev->dev,
834 "Error sending to WASP %d\n",
835 ret);
836 goto err_send;
837 }
838
839 packet_counter += COUNTER_INCR;
840 chunk_counter++;
841 }
842 }
843 }
844
845 err_send:
846 avmwasp->send_socket->ops->release(avmwasp->send_socket);
847 err_recv:
848 avmwasp->recv_socket->ops->release(avmwasp->recv_socket);
849 err:
850 return ret;
851 }
852
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] remoteproc: Add AVM WASP driver
2022-02-21 13:54 [PATCH 3/3] remoteproc: Add AVM WASP driver Daniel Kestrel
2022-02-21 17:43 ` kernel test robot
2022-02-21 18:34 ` kernel test robot
@ 2022-02-22 0:30 ` Bjorn Andersson
2022-02-23 14:58 ` Kestrel seventyfour
2 siblings, 1 reply; 5+ messages in thread
From: Bjorn Andersson @ 2022-02-22 0:30 UTC (permalink / raw)
To: Daniel Kestrel
Cc: Mathieu Poirier, Rob Herring, linux-remoteproc, devicetree, linux-kernel
On Mon 21 Feb 05:54 PST 2022, Daniel Kestrel wrote:
> Some AVM Fritzbox router boards (3390, 3490, 5490, 5491, 7490),
> that are Lantiq XRX200 based, have a memory only ATH79 based
> WASP (Wireless Assistant Support Processor) SoC that has wifi
> cards connected to it. It does not share anything with the
> Lantiq host and has no persistent storage. It has an mdio based
> connection for bringing up a small network boot firmware and is
> connected to the Lantiq GSWIP switch via gigabit ethernet. This
> is used to load an initramfs linux image to it, after the
> network boot firmware was started.
>
> In order to initialize this remote processor we need to:
> - power on the SoC using startup gpio
> - reset the SoC using the reset gpio
> - send the network boot firmware using mdio
> - send the linux image using raw ethernet frames
>
> This driver allows to start and stop the WASP SoC.
>
That's different, but seems to be a reasonable fit with the remoteproc
framework.
> Signed-off-by: Daniel Kestrel <kestrelseventyfour@gmail.com>
> ---
> drivers/remoteproc/Kconfig | 10 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/avm_wasp.c | 1251 +++++++++++++++++++++++++++++++++
> drivers/remoteproc/avm_wasp.h | 95 +++
> 4 files changed, 1357 insertions(+)
> create mode 100644 drivers/remoteproc/avm_wasp.c
> create mode 100644 drivers/remoteproc/avm_wasp.h
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 166019786653..a761186c5171 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -23,6 +23,16 @@ config REMOTEPROC_CDEV
>
> It's safe to say N if you don't want to use this interface.
>
> +config AVM_WASP_REMOTEPROC
> + tristate "AVM WASP remoteproc support"
> + depends on NET_DSA_LANTIQ_GSWIP
> + help
> + Say y here to support booting the secondary SoC ATH79 target
> + called Wireless Assistant Support Processor (WASP) that some
> + AVM Fritzbox devices (3390, 3490, 5490, 5491, 7490) have built in.
> +
> + It's safe to say N here.
> +
> config IMX_REMOTEPROC
> tristate "i.MX remoteproc support"
> depends on ARCH_MXC
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 5478c7cb9e07..0ae175c6722f 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -11,6 +11,7 @@ remoteproc-y += remoteproc_sysfs.o
> remoteproc-y += remoteproc_virtio.o
> remoteproc-y += remoteproc_elf_loader.o
> obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
> +obj-$(CONFIG_AVM_WASP_REMOTEPROC) += avm_wasp.o
> obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> obj-$(CONFIG_IMX_DSP_REMOTEPROC) += imx_dsp_rproc.o
> obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
> diff --git a/drivers/remoteproc/avm_wasp.c b/drivers/remoteproc/avm_wasp.c
> new file mode 100644
> index 000000000000..04b7c9005028
> --- /dev/null
> +++ b/drivers/remoteproc/avm_wasp.c
> @@ -0,0 +1,1251 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AVM WASP Remote Processor driver
> + *
> + * Copyright (c) 2019-2020 Andreas Böhler
> + * Copyright (c) 2021-2022 Daniel Kestrel
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/timekeeping.h>
> +#include <net/sock.h>
> +#include <asm-generic/gpio.h>
> +
> +#include "remoteproc_internal.h"
> +#include "avm_wasp.h"
> +
> +/**
> + * struct avm_wasp_rproc - avmwasp remote processor priv
> + * @rproc: rproc handle
> + * @pdev: pointer to platform device
> + * @eeprom_blob: pointer to load and save any firmware
> + * @linux_blob: pointer to access initramfs image
> + * @complete: structure for asynchronous firmware load
> + * @mdio_bus: pointer to mii_bus of gswip device for gpio
> + * @startup_gpio: store WASP startup gpio number
> + * @reset_gpio: store WASP reset gpio number
> + * @s_gpio_flg: store WASP startup gpio flags active high/low
> + * @r_gpio_flg: store WASP reset gpio flags active high/low
> + * @netboot_firmware: store name of the network boot firmware
> + * @loader_port: store name of the port wasp is connected to
> + * @sendbuf: send buffer for uploading WASP initramfs firmware
> + * @recvbuf: recv buffer for feedback from WASP
> + * @s_packet: structure for sending packets to WASP
> + * @send_socket: pointer to socket for sending to WASP
> + * @recv_socket: pointer to socket for receiving from WASP
> + * @ifindex: interface index used for WASP communication
> + */
> +struct avm_wasp_rproc {
> + struct rproc *rproc;
> + struct platform_device *pdev;
> + const struct firmware *eeprom_blob, *linux_blob;
> + struct completion complete;
> + char *mdio_bus_id;
> + struct mii_bus *mdio_bus;
> + int startup_gpio, reset_gpio;
> + enum of_gpio_flags s_gpio_flg, r_gpio_flg;
> + char *netboot_firmware;
> + char *loader_port;
> + char sendbuf[BUF_SIZE];
> + char recvbuf[BUF_SIZE];
Why aren't these just struct wasp_packet? Instead of having a char
buffer that happens to be sized, slightly bigger than a wasp_packet?
> + struct wasp_packet s_packet;
> + struct socket *send_socket;
> + struct socket *recv_socket;
> + int ifindex;
> +};
> +
> +/**
> + * avm_wasp_firmware_request_cb() - callback handler for firmware load
> + * @eeprom_blob: pointer to struct firmware
> + * @ctx: context passed
> + *
> + * This handler is called after completing the request_firmware_nowait
> + * function by passing the avm_wasp_rproc struct
> + * It saves the firmware in the context and calls complete
> + */
> +static void avm_wasp_firmware_request_cb(const struct firmware *eeprom_blob,
> + void *ctx)
> +{
> + struct avm_wasp_rproc *avmwasp = ctx;
> +
> + if (eeprom_blob)
> + avmwasp->eeprom_blob = eeprom_blob;
> +
> + complete(&avmwasp->complete);
> +}
> +
> +/**
> + * avm_wasp_firmware_request() - asynchronous load of passed firmware
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + * @name: char pointer to filename (relative to /lib/firmware)
> + *
> + * Handles setup and execution of the asynchronous firmware request
> + * Used to trigger the load of the ath10k caldata and ath9k eeprom
> + * firmware from the tffs partition of the devices
> + *
> + * Return: 0 on success, -2 if file not found or error from function
Write out ENOENT instead of -2.
> + * request_firmware_nowait
> + */
> +static int avm_wasp_firmware_request(struct avm_wasp_rproc *avmwasp,
> + const char *name)
> +{
> + int err;
> +
> + init_completion(&avmwasp->complete);
> +
> + err = request_firmware_nowait(THIS_MODULE, 1, name,
> + &avmwasp->pdev->dev,
> + GFP_KERNEL, avmwasp,
> + avm_wasp_firmware_request_cb);
> + if (err < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Load request for %s failed\n", name);
> + return err;
> + }
> +
> + wait_for_completion(&avmwasp->complete);
Why do you use nowait and then wait? This seems very similar to
request_firmware()...
> +
> + if (!avmwasp->eeprom_blob) {
> + dev_err(&avmwasp->pdev->dev,
> + "Unable to load %s\n", name);
> + return -ENOENT;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * avm_wasp_firmware_release() - clean up after firmware load
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + *
> + * Releases the firmware that is in the eeprom_blob firmware
> + * pointer of the private avm_wasp_rproc structure
> + */
> +static void avm_wasp_firmware_release(struct avm_wasp_rproc *avmwasp)
> +{
> + release_firmware(avmwasp->eeprom_blob);
> + avmwasp->eeprom_blob = NULL;
> +}
> +
> +/**
> + * avm_wasp_netboot_mdio_read() - read with gswip mdio bus
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + * @location: register number of the m_regs_wasp register array
> + *
> + * Reads a value from the specified register for the mdio address
> + * that is used for the connection to the WASP SoC
> + * Mutex on mdio_lock is required to serialize access on bus
> + *
> + * Return: Value that was read from the specified register
> + */
> +int avm_wasp_netboot_mdio_read(struct avm_wasp_rproc *avmwasp,
> + int location)
> +{
> + int value;
> +
> + if (location > M_REGS_WASP_INDEX_MAX || location < 0)
> + return 0;
> + mutex_lock(&avmwasp->mdio_bus->mdio_lock);
If your mdio_bus requires a lock to handle concurrent IO operations,
then that lock should be pushed into the read/write.
If for some reason you need to serialize a sequence of read/writes, then
it makes sense to have it here.
But it's quite common for this to actually be an "atomic" operation and
that there's no lock needed in the first place.
> + value = avmwasp->mdio_bus->read(avmwasp->mdio_bus,
> + WASP_ADDR, m_regs_wasp[location]);
> + mutex_unlock(&avmwasp->mdio_bus->mdio_lock);
> + return value;
> +}
> +
> +/**
> + * avm_wasp_netboot_mdio_write() - write with gswip mdio bus
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + * @location: register number of the m_regs_wasp register array
> + * @value: value to be written to the register
> + *
> + * Writes a value to the specified register for the mdio address
> + * that is used for the connection to the WASP SoC
> + * Mutex on mdio_lock is required to serialize access on bus
> + * Makes sure not to write to invalid registers as this can have
> + * unpredictable results
> + */
> +void avm_wasp_netboot_mdio_write(struct avm_wasp_rproc *avmwasp,
> + int location, int value)
> +{
> + if (location > M_REGS_WASP_INDEX_MAX || location < 0)
> + return;
> + mutex_lock(&avmwasp->mdio_bus->mdio_lock);
> + avmwasp->mdio_bus->write(avmwasp->mdio_bus, WASP_ADDR,
> + m_regs_wasp[location], value);
> + mutex_unlock(&avmwasp->mdio_bus->mdio_lock);
> +}
> +
> +/**
> + * avm_wasp_netboot_mdio_write_u32_split() - write 32bit value
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + * @location: register number of the m_regs_wasp register array
> + * @value: value to be written to the register
> + *
> + * As the mdio registers are 16bit, this function writes a 32bit value
> + * to two subsequent registers starting with the specified register
> + * for the mdio address that is used for the connection to the WASP SoC
> + */
> +void avm_wasp_netboot_mdio_write_u32_split(struct avm_wasp_rproc *avmwasp,
> + int location, const u32 value)
> +{
> + avm_wasp_netboot_mdio_write(avmwasp, location,
> + ((value & 0xffff0000) >> 16));
> + avm_wasp_netboot_mdio_write(avmwasp, location + 1,
> + (value & 0x0000ffff));
Here you perform two separate writes, this might be wort locking
around if there's a possibility that two threads races and a mix of
their two "value" end up in the registers.
> +}
> +
> +/**
> + * avm_wasp_netboot_write_header() - write header to WASP
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + * @start_address: address where to load the firmware to on WASP
> + * @len: length of the network boot firmware
> + * @exec_address: address where to start execution on WASP
> + *
> + * Writes the header to WASP using mdio to initiate the start of
> + * transferring the network boot firmware to WASP
> + *
> + * Return: 0 on Success or -14 if writing header failed based on return
-EFAULT instead of -14
> + * code from WASP
> + */
> +static int avm_wasp_netboot_write_header(struct avm_wasp_rproc *avmwasp,
> + const u32 start_addr, const u32 len,
> + const u32 exec_addr)
> +{
> + int regval;
> + int timeout = WASP_TIMEOUT_COUNT;
> +
> + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 1, start_addr);
> + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 3, len);
> + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 5, exec_addr);
> + avm_wasp_netboot_mdio_write(avmwasp, 0, WASP_CMD_SET_PARAMS);
> +
> + do {
> + udelay(WASP_POLL_SLEEP_US);
> + regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
> + timeout--;
> + } while ((regval != WASP_RESP_OK) && (timeout > 0));
I wonder if this could be implemented using read_poll_timeout() instead.
> +
> + if (regval != WASP_RESP_OK) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error writing header to WASP! Status = %d\n", regval);
> + return -EFAULT;
> + }
> + return 0;
> +}
> +
> +/**
> + * avm_wasp_netboot_write_checksum() - write checksum to WASP
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + * @checksum: calculated checksum value to be sent to WASP
> + *
> + * Writes the calculated checksum for the given network boot firmware
> + * to WASP using mdio as the second step
> + *
> + * Return: 0 on Success or -14 if writing checksum failed based on return
-EFAULT instead of -14
> + * code from WASP
> + */
> +static int avm_wasp_netboot_write_checksum(struct avm_wasp_rproc *avmwasp,
> + const uint32_t checksum)
> +{
> + int regval;
> + int timeout = WASP_TIMEOUT_COUNT;
> +
> + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 1, checksum);
> + if (m_model == MODEL_3390) {
> + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 3, 0x0000);
> + avm_wasp_netboot_mdio_write(avmwasp, 0,
> + WASP_CMD_SET_CHECKSUM_3390);
> + } else if (m_model == MODEL_X490)
> + avm_wasp_netboot_mdio_write(avmwasp, 0,
> + WASP_CMD_SET_CHECKSUM_X490);
If one of your blocks is wrapped in {}, then please wrap them all in {}.
Btw, no need to wrap this line, it's fairly close to 80 chars anyways.
> +
> + do {
> + udelay(WASP_POLL_SLEEP_US);
> + regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
> + timeout--;
> + } while ((regval != WASP_RESP_OK) && (timeout > 0));
As above. And if read_poll_timeout() doesn't work out, this is repeated
multiple times, seems reasonable to break out to a helper function.
> +
> + if (regval != WASP_RESP_OK) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error writing checksum to WASP! Status = %d\n",
> + regval);
> + return -EFAULT;
> + }
> + return 0;
> +}
> +
> +/**
> + * avm_wasp_netboot_write_chunk() - write chunk of data to WASP
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + * @data: pointer to data
> + * @len: length of data (should not exceed 14 bytes)
> + *
> + * Writes up to 14 bytes of data into the 7 16bit mdio registers
> + * to WASP using mdio
> + *
> + * Return: 0 on Success, -14 if data length is mor than 14 bytes or
> + * -2 if writing the data failed based on return code from WASP
EFAULT, ENOENT
> + */
> +static int avm_wasp_netboot_write_chunk(struct avm_wasp_rproc *avmwasp,
> + const char *data, const int len)
@len sounds like a size_t to me.
> +{
> + int regval, i, j;
> + int timeout = WASP_TIMEOUT_COUNT;
> +
> + if (len > WASP_CHUNK_SIZE || len < 0 || !data)
> + return -EFAULT;
Blank line here would be nice.
> + for (i = 0, j = 1; i < len; i += 4, j += 2)
> + avm_wasp_netboot_mdio_write_u32_split(avmwasp, j,
> + *((uint32_t *)
> + (data + i)));
14 isn't evenly divided in 4 byte chunks, so this doesn't seem to work
as described.
> +
> + avm_wasp_netboot_mdio_write(avmwasp, 0, WASP_CMD_SET_DATA);
> +
> + do {
> + udelay(WASP_POLL_SLEEP_US);
> + regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
> + timeout--;
> + } while ((regval != WASP_RESP_OK) && (timeout > 0));
As above.
> +
> + if (regval != WASP_RESP_OK && regval != WASP_RESP_WAIT &&
> + regval != WASP_RESP_COMPLETED) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error writing chunk to WASP: m_reg_status = 0x%x!\n",
> + regval);
> + return -EFAULT;
> + }
> + return 0;
> +}
> +
> +/**
> + * avm_wasp_netboot_calc_checksum() - calculate netboot firmware checksum
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + *
> + * Calculates the checksum by using the eeprom_blob from the private
> + * avm_wasp_rproc structure
> + *
> + * Return: Calculated checksum or -14 on Error
EFAULT
> + */
> +static uint32_t avm_wasp_netboot_calc_checksum(struct avm_wasp_rproc *avmwasp)
Use u32 instead of uint32_t in the kernel.
> +{
> + u32 checksum = 0xffffffff;
> + u32 cs;
> + int count = -1;
Should this be signed, or is it just as signed as "checksum"?
> + size_t size;
> + const u8 *firmware;
> + const u8 *firmware_end;
> +
> + if (!avmwasp->eeprom_blob)
> + return -EFAULT;
-EFAULT isn't an awesome uint32_t and you're blindly writing it to the
socket below.
That said, it would be much better if you ensured that you didn't end up
in avm_wasp_netboot_load_firmware() in the first place with eeprom_blob
of NULL.
But if nothing else, right before calling this function you check
this...
> + size = avmwasp->eeprom_blob->size;
> + firmware = avmwasp->eeprom_blob->data;
> + firmware_end = firmware + size;
> +
> + if (!firmware || size <= 0)
Can firmware be NULL?
size was checked right before calling the function.
> + return -EFAULT;
> +
> + while (firmware < firmware_end) {
> + cs = (firmware[0] << 24 | firmware[1] << 16 |
> + firmware[2] << 8 | firmware[3]);
So cs is the big endian representation of the data?
I don't see any checks to ensure size is a multiple of 4, so this might
read off the end of the array?
> + checksum = checksum - cs;
> + count++;
> + firmware += 4;
> + }
> +
> + checksum = checksum - count;
> + return checksum;
> +}
> +
> +/**
> + * avm_wasp_netboot_load_firmware() - load netboot firmware to WASP
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + *
> + * Implements the process to send header, checksum and the firmware
> + * blob in 14 byte chunks to the WASP processor using mdio
> + * Includes checks between the steps and sending commands to start
> + * the network boot firmware
> + *
> + * Return: 0 on Success, -2 if no firmware is present, -19 if no
> + * firmware or -14 if other errors have occurred
ENOENT, ENODEV and EFAULT
> + */
> +int avm_wasp_netboot_load_firmware(struct avm_wasp_rproc *avmwasp)
> +{
> + const u8 *firmware;
> + const u8 *firmware_end;
> + int ret, regval, regval2, count, cont = 1;
> +
> + count = WASP_WAIT_TIMEOUT_COUNT;
> +
> + while (count > 0 && (avm_wasp_netboot_mdio_read(avmwasp, 0)
> + != WASP_RESP_OK)) {
> + count -= 1;
> + mdelay(WASP_WAIT_SLEEP);
> + }
> +
> + if (avm_wasp_netboot_mdio_read(avmwasp, 0)
> + != WASP_RESP_OK) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error: WASP processor not ready\n");
> +
> + return -ENODEV;
> + }
> +
> + ret = request_firmware_direct((const struct firmware **)
> + &avmwasp->eeprom_blob,
> + avmwasp->netboot_firmware, &avmwasp->pdev->dev);
> + if (ret) {
> + dev_err(&avmwasp->pdev->dev,
> + "Could not find network boot firmware\n");
> + return -ENOENT;
> + }
> +
> + firmware = avmwasp->eeprom_blob->data;
> + firmware_end = firmware + avmwasp->eeprom_blob->size;
> +
> + if (!firmware || avmwasp->eeprom_blob->size <= 0)
> + return -EFAULT;
EINVAL?
> +
> + if (avm_wasp_netboot_write_header(avmwasp, start_addr,
> + avmwasp->eeprom_blob->size,
> + exec_addr) < 0)
> + return -EFAULT;
> +
> + if (avm_wasp_netboot_write_checksum(avmwasp,
> + avm_wasp_netboot_calc_checksum
Funny, this looks like a variable with the same name as the function,
then I realized that you have the parameters on the next line.
That's not helping anyone read this...
> + (avmwasp)) < 0)
> + return -EFAULT;
> +
> + while (firmware < firmware_end) {
> + if ((firmware_end - firmware) >= WASP_CHUNK_SIZE) {
> + if (avm_wasp_netboot_write_chunk(avmwasp, firmware,
> + WASP_CHUNK_SIZE) < 0)
> + return -EFAULT;
> + } else {
> + if (avm_wasp_netboot_write_chunk(avmwasp, firmware,
> + (firmware_end -
> + firmware)) < 0)
> + return -EFAULT;
> + }
> + firmware += WASP_CHUNK_SIZE;
> + }
while (firmware < firmware_end) {
left = firmware_end - firmware;
if (left > WASP_CHUNK_SIZE)
left = WASP_CHUNK_SIZE;
ret = avm_wasp_netboot_write_chunk(avmwasp, firmware, left);
if (ret < 0)
return -EFAULT;
firmware += left;
}
But not EFAULT...
> +
> + mdelay(WASP_WAIT_SLEEP);
> +
> + if (m_model == MODEL_3390)
> + avm_wasp_netboot_mdio_write(avmwasp, 0,
> + WASP_CMD_START_FIRMWARE_3390);
> + else if (m_model == MODEL_X490)
> + avm_wasp_netboot_mdio_write(avmwasp, 0,
> + WASP_CMD_START_FIRMWARE_X490);
> +
> + avm_wasp_firmware_release(avmwasp);
> +
> + mdelay(WASP_WAIT_SLEEP);
> + count = 0;
> +
> + while ((avm_wasp_netboot_mdio_read(avmwasp, 0)
> + != WASP_RESP_READY_TO_START) &&
> + (count < WASP_WAIT_TIMEOUT_COUNT)) {
> + mdelay(WASP_WAIT_SLEEP);
> + count++;
> + }
> + if (count >= WASP_WAIT_TIMEOUT_COUNT) {
> + dev_err(&avmwasp->pdev->dev,
> + "Timed out waiting for WASP ready to start.\n");
> + return -EFAULT;
> + }
> +
> + if (m_model == MODEL_3390)
> + avm_wasp_netboot_mdio_write(avmwasp, 0,
> + WASP_CMD_START_FIRMWARE_3390);
> + else if (m_model == MODEL_X490)
> + avm_wasp_netboot_mdio_write(avmwasp, 0,
> + WASP_CMD_SET_CHECKSUM_X490);
> +
> + mdelay(WASP_WAIT_SLEEP);
> +
> + if (m_model == MODEL_3390) {
> + count = 0;
> + while ((avm_wasp_netboot_mdio_read(avmwasp, 0) !=
> + WASP_RESP_OK) &&
> + (count < WASP_WAIT_TIMEOUT_COUNT)) {
> + mdelay(WASP_WAIT_SLEEP);
> + count++;
> + }
> + if (count >= WASP_WAIT_TIMEOUT_COUNT) {
> + dev_err(&avmwasp->pdev->dev,
> + "Timed out waiting for WASP OK.\n");
> + return -EFAULT;
> + }
> + if (avm_wasp_netboot_write_chunk(avmwasp, mac_data,
> + WASP_CHUNK_SIZE) < 0) {
ARRAY_SIZE(mac_data) seems more appropriate to denote help the reader
understand that the right amount is actually referred to.
> + dev_err(&avmwasp->pdev->dev,
> + "Error sending MAC address!\n");
> + return -EFAULT;
> + }
> + } else if (m_model == MODEL_X490) {
> + cont = 1;
> + while (cont) {
> + count = 0;
> + while ((avm_wasp_netboot_mdio_read(avmwasp, 0)
> + != WASP_RESP_OK) &&
> + (count < WASP_WAIT_TIMEOUT_COUNT)) {
> + mdelay(WASP_WAIT_SLEEP);
> + count++;
> + }
> + if (count >= WASP_WAIT_TIMEOUT_COUNT) {
> + dev_err(&avmwasp->pdev->dev,
> + "Timed out waiting for WASP OK.\n");
> + return -EFAULT;
Timed out sounds like a ETIMEDOUT, not EFAULT.
> + }
> + regval = avm_wasp_netboot_mdio_read(avmwasp, 1);
> + regval2 = avm_wasp_netboot_mdio_read(avmwasp, 2);
> + avm_wasp_netboot_mdio_write(avmwasp, 0,
> + WASP_CMD_SET_CHECKSUM_X490
> + );
> + if (regval == 0 && regval2 != 0)
> + cont = regval2;
> + else
> + cont--;
> + }
> +
> + count = 0;
> + while ((avm_wasp_netboot_mdio_read(avmwasp, 0) !=
> + WASP_RESP_OK) &&
> + (count < WASP_TIMEOUT_COUNT)) {
> + udelay(WASP_BOOT_SLEEP_US);
> + count++;
> + }
Another read_poll_timeout() like construct. Or perhaps the same?
> + if (count >= WASP_TIMEOUT_COUNT) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error waiting for checksum OK response.\n");
> + return -EFAULT;
> + }
> +
> + avm_wasp_netboot_mdio_write(avmwasp, 1, 0x00);
> + avm_wasp_netboot_mdio_write(avmwasp, 0,
> + WASP_CMD_START_FIRMWARE2_X490);
> +
> + regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
> + if (regval != WASP_RESP_OK) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error starting WASP network boot: 0x%x\n",
> + regval);
> + return -EFAULT;
EFAULT doesn't seem appropriate here either...
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * avm_wasp_load_initramfs_image() - load initramfs image to WASP
> + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> + *
> + * Uses the lan port specified from DT to load the initramfs to
> + * WASP after the network boot firmware was successfully started.
> + * Communication is done by using raw sockets.
> + * The port of the lantiq gswip device will be started if not
> + * already up and running.
> + * There are several commands and status values which are checked.
> + * First a discovery packet is received and then each data packet
> + * is acknowledged by the WASP network boot firmware.
> + * First packet needs to prepend the load address and last packet
> + * needs to append the execution address.
> + *
> + * Return: 0 on Success, -14 if errors with the WASP send protocol
> + * have occurred or the error returned from the failed operating
> + * system function or service
> + */
> +int avm_wasp_load_initramfs_image(struct avm_wasp_rproc *avmwasp)
> +{
> + int done = 0;
bool is a better type for booleans.
> + int reuse = 1;
> + int num_chunks = 0;
> + int chunk_counter = 1;
These seems unsigned
> + int ret, packet_counter, data_offset;
packet_counter sounds unsigned and data_offset sounds size_t.
> + int send_len = 0;
size_t and first access is an assignment, so no need to initialize it
here.
> + short interface_flags;
> + ssize_t numbytes;
> + ssize_t read;
"read" isn't the best name for a variable to denote the chunk size to be
sent.
> + const u8 *firmware;
> + const u8 *firmware_end;
> + struct wasp_packet *packet = (struct wasp_packet *)
> + (avmwasp->recvbuf + sizeof(struct ethhdr));
> + struct ethhdr *recv_eh = (struct ethhdr *)avmwasp->recvbuf;
> + struct msghdr recv_socket_hdr;
> + struct kvec recv_vec;
> + struct ethhdr *send_eh = (struct ethhdr *)avmwasp->sendbuf;
> + struct sockaddr_ll send_socket_address;
> + struct msghdr send_socket_hdr;
> + struct kvec send_vec;
> + struct net_device *send_netdev;
> + struct sockaddr send_sock_addr;
> + struct timeval {
> + __kernel_old_time_t tv_sec;
> + __kernel_suseconds_t tv_usec;
> + } timeout;
Don't we have one of these in the kernel headers somewhere?
> + time64_t start_time, current_time;
> +
> + if (!avmwasp->linux_blob) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error accessing initramfs image");
> + goto err;
> + }
> +
> + ret = sock_create_kern(&init_net, PF_PACKET, SOCK_RAW,
> + htons(ETHER_TYPE_ATH_ECPS_FRAME),
> + &avmwasp->recv_socket);
> + if (ret < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error opening recv socket: %d", ret);
> + goto err;
> + }
> +
> + ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET, SO_REUSEADDR,
> + KERNEL_SOCKPTR(&reuse), sizeof(reuse));
> + if (ret < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error SO_REUSEADDR recv socket: %d", ret);
> + goto err_recv;
> + }
> +
> + ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET,
> + SO_BINDTODEVICE,
> + KERNEL_SOCKPTR(avmwasp->loader_port),
> + IFNAMSIZ - 1);
> + if (ret < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error SO_BINDTODEVICE recv socket: %d", ret);
> + goto err_recv;
> + }
> +
> + timeout.tv_sec = 10;
> + timeout.tv_usec = 0;
> + ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET,
> + SO_RCVTIMEO_OLD,
> + KERNEL_SOCKPTR(&timeout), sizeof(timeout));
> + if (ret < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error SO_RCVTIMEO recv socket: %d", ret);
> + goto err_recv;
> + }
> +
> + ret = sock_create_kern(&init_net, AF_PACKET, SOCK_RAW, IPPROTO_RAW,
> + &avmwasp->send_socket);
Why do you need two sockets to do rx and tx?
> + if (ret < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error opening send socket: %d", ret);
> + goto err_recv;
> + }
> +
> + timeout.tv_sec = 10;
> + timeout.tv_usec = 0;
> + ret = sock_setsockopt(avmwasp->send_socket, SOL_SOCKET,
> + SO_SNDTIMEO_OLD,
> + KERNEL_SOCKPTR(&timeout), sizeof(timeout));
> + if (ret < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error SO_SNDTIMEO send socket: %d", ret);
> + goto err_send;
> + }
> +
> + rcu_read_lock();
> + send_netdev = dev_get_by_name_rcu(sock_net(avmwasp->send_socket->sk),
> + avmwasp->loader_port);
> + if (send_netdev)
> + interface_flags = (short)dev_get_flags(send_netdev);
> + rcu_read_unlock();
> +
> + if (IS_ERR_OR_NULL(send_netdev)) {
> + dev_err(&avmwasp->pdev->dev, "Error accessing net device.\n");
> + ret = -ENODEV;
> + goto err_send;
> + }
> +
> + interface_flags |= IFF_PROMISC | IFF_UP | IFF_RUNNING;
If !send_netdev interface_flags is uninitialized.
> + rtnl_lock();
> + ret = dev_change_flags(send_netdev, interface_flags, NULL);
I'm not entirely familiar with the netdev API, but doesn't this up the
interface? From your remoteproc start function?!
> + rtnl_unlock();
> +
> + if (ret) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error changing interface flags: %d\n", ret);
> + goto err_send;
> + }
> +
> + avmwasp->ifindex = send_netdev->ifindex;
> + ret = dev_get_mac_address(&send_sock_addr,
> + sock_net(avmwasp->send_socket->sk),
> + avmwasp->loader_port);
> + if (ret < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error getting mac address: %d\n", ret);
> + goto err_send;
> + }
> +
> + memset(avmwasp->sendbuf, 0, BUF_SIZE);
> +
> + memcpy(send_eh->h_dest, wasp_mac, sizeof(send_eh->h_dest));
> + send_eh->h_proto = ETHER_TYPE_ATH_ECPS_FRAME;
> + memcpy(send_eh->h_source, send_sock_addr.sa_data,
> + sizeof(send_eh->h_source));
> +
> + start_time = ktime_get_seconds();
> +
> + while (!done) {
> + current_time = ktime_get_seconds();
> + if ((current_time - start_time) > SEND_LOOP_TIMEOUT_SECONDS) {
Isn't the socket io operations in this loop blocking? What prevents you
from being stuck in one of those forever?
> + dev_err(&avmwasp->pdev->dev,
A local copy of dev would be nice to shorten these expressions.
> + "Waiting for packet from WASP timed out.\n");
> + ret = -EFAULT;
> + goto err_send;
> + }
> +
> + memset(&recv_vec, 0, sizeof(recv_vec));
> + memset(&recv_socket_hdr, 0, sizeof(recv_socket_hdr));
> + recv_vec.iov_base = avmwasp->recvbuf;
> + recv_vec.iov_len = BUF_SIZE;
> + numbytes = kernel_recvmsg(avmwasp->recv_socket,
> + &recv_socket_hdr, &recv_vec, 1,
> + BUF_SIZE, 0);
Can you please help me understand how you know that the read message is
from the correct sender and that it's a firmware load message?
> +
> + if (numbytes < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error receiving any packet or timeout: %d\n",
> + numbytes);
> + ret = -EFAULT;
> + goto err_send;
> + }
> +
> + if (numbytes < (sizeof(struct ethhdr) + WASP_HEADER_LEN)) {
> + dev_err(&avmwasp->pdev->dev,
> + "Packet too small, discard and continue.\n");
> + continue;
> + }
> +
> + if (recv_eh->h_proto != ETHER_TYPE_ATH_ECPS_FRAME)
> + continue;
> +
> + memcpy(wasp_mac, recv_eh->h_source, sizeof(wasp_mac));
> + memset(&avmwasp->s_packet, 0, sizeof(avmwasp->s_packet));
> +
> + if (packet->packet_start == PACKET_START) {
And if the received message doesn't start with PACKET_START you simply
discard it?
> + switch (packet->response) {
> + case RESP_DISCOVER:
> + packet_counter = 0;
> + firmware = avmwasp->linux_blob->data;
> + firmware_end = firmware
> + + avmwasp->linux_blob->size;
> +
> + chunk_counter = 1;
> + num_chunks =
> + avmwasp->linux_blob->size / CHUNK_SIZE;
> + if (avmwasp->linux_blob->size % CHUNK_SIZE != 0)
> + num_chunks++;
DIV_ROUND_UP()
> + break;
Please indent the break to match the code block.
> + case RESP_OK:
> + /* got reply send next packet */
So when you receive RESP_DISCOVER or RESP_OK you will do the second half
of this function and send out a chunk of data?
Seems like this would be better represented by falling through from the
RESP_DISCOVER and put the send logic in RESP_OK.
> + break;
> + case RESP_ERROR:
> + dev_err(&avmwasp->pdev->dev,
> + "Received an WASP error packet!\n");
> + ret = -EFAULT;
> + goto err_send;
> + break;
> + case RESP_STARTING:
> + done = 1;
> + ret = 0;
> + continue;
> + break;
> + default:
> + dev_err(&avmwasp->pdev->dev,
> + "Unknown packet! Continue.\n");
> + continue;
> + break;
> + }
> +
> + if (packet_counter == 0) {
> + memcpy(avmwasp->s_packet.payload, &m_load_addr,
> + sizeof(m_load_addr));
> + data_offset = sizeof(m_load_addr);
> + } else {
> + data_offset = 0;
> + }
> +
> + if (firmware < firmware_end) {
firmware and firmware_end are uninitialized if you get here without
first reveiving a RESP_DISCOVER.
> + if ((firmware_end - firmware) >= CHUNK_SIZE)
> + read = CHUNK_SIZE;
> + else
> + read = firmware_end - firmware;
> + memcpy(&avmwasp->s_packet.payload[data_offset],
s_packet isn't used outside this loop, so why is i statically part of
the avmwasp struct? Why aren't these various properties just local
variables?
> + firmware, read);
> + firmware = firmware + CHUNK_SIZE;
> +
> + avmwasp->s_packet.packet_start = PACKET_START;
> + if (chunk_counter == num_chunks) {
> + avmwasp->s_packet.response =
> + CMD_START_FIRMWARE;
> + memcpy(&avmwasp->s_packet.payload
> + [data_offset + read],
> + &m_load_addr, sizeof(m_load_addr));
So m_load_addr goes in the first 4 bytes and the last 4 bytes of the
message?
> + data_offset += sizeof(m_load_addr);
> + } else {
> + avmwasp->s_packet.command =
> + CMD_FIRMWARE_DATA;
> + }
> + avmwasp->s_packet.counter = packet_counter;
> +
> + memcpy(avmwasp->sendbuf + sizeof(struct ethhdr),
> + avmwasp->s_packet.data,
> + WASP_HEADER_LEN + read + data_offset);
> + send_len = sizeof(struct ethhdr)
> + + WASP_HEADER_LEN + read + data_offset;
> + send_socket_address.sll_halen = ETH_ALEN;
> + send_socket_address.sll_ifindex =
> + avmwasp->ifindex;
This doesn't seem to change within the loop, can't this be prepared
outside the loop?
> +
> + memset(&send_vec, 0, sizeof(send_vec));
> + send_vec.iov_len = send_len;
> + send_vec.iov_base = avmwasp->sendbuf;
> +
> + memset(&send_socket_hdr, 0,
> + sizeof(send_socket_hdr));
> + send_socket_hdr.msg_name = (struct sockaddr *)
> + &send_socket_address;
> + send_socket_hdr.msg_namelen =
> + sizeof(struct sockaddr_ll);
Same as send_socket_address.
> +
> + ret = kernel_sendmsg(avmwasp->send_socket,
> + &send_socket_hdr,
> + &send_vec,
> + 1, send_len);
> + if (ret < 0) {
> + dev_err(&avmwasp->pdev->dev,
> + "Error sending to WASP %d\n",
> + ret);
> + goto err_send;
> + }
> +
> + packet_counter += COUNTER_INCR;
Isn't packet_counter always 4 * (chunk_counter - 1)? Why the factor 4?
Can the two counters be consolidated?
> + chunk_counter++;
> + }
> + }
> + }
> +
> +err_send:
> + avmwasp->send_socket->ops->release(avmwasp->send_socket);
> +err_recv:
> + avmwasp->recv_socket->ops->release(avmwasp->recv_socket);
> +err:
> + return ret;
> +}
> +
> +/**
> + * avm_wasp_rproc_start() - start the remote processor
> + * @rproc: pointer to the rproc structure
> + *
> + * Starts the remote processor by turning it on using the startup
> + * gpio and initiating the reset process using the reset_gpio.
> + * After that the status is checked if poweron and reset were
> + * successful.
> + * As the first step, the network boot firmware is tried to be loaded
> + * and started.
> + * As a second step, the initramfs image is tried to be loaded
> + * and started.
> + *
> + * Return: 0 on Success, -19 or return code from the called function
> + * if any other error occurred in the process of starting and loading
> + * the firmware files to the WASP processor
> + */
> +static int avm_wasp_rproc_start(struct rproc *rproc)
> +{
> + struct avm_wasp_rproc *avmwasp = rproc->priv;
> + int ret;
> +
> + gpio_set_value(avmwasp->startup_gpio,
> + (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> + 0 : 1);
As I say below, gpiod_get() will grab you a reference to the gpio and
based on the DT flags it will handle "active high" vs "active low" for
you, all you need to pass here is "is it high or low" (1 or 0).
> + mdelay(WASP_WAIT_SLEEP);
> + gpio_set_value(avmwasp->reset_gpio,
> + (avmwasp->r_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> + 1 : 0);
> + mdelay(WASP_WAIT_SLEEP);
> + gpio_set_value(avmwasp->reset_gpio,
> + (avmwasp->r_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> + 0 : 1);
> + mdelay(WASP_WAIT_SLEEP);
> +
> + avmwasp->mdio_bus = mdio_find_bus(avmwasp->mdio_bus_id);
> + if (!avmwasp->mdio_bus) {
> + dev_err(&avmwasp->pdev->dev,
> + "wasp-netboot-mdio bus not found\n");
> + return -ENODEV;
> + }
> +
> + ret = avm_wasp_netboot_load_firmware(avmwasp);
> + if (ret) {
> + put_device(&avmwasp->mdio_bus->dev);
> + return ret;
How about putting the chip back in reset here?
> + }
> +
> + put_device(&avmwasp->mdio_bus->dev);
> +
> + ret = avm_wasp_load_initramfs_image(avmwasp);
> + if (ret)
And here?
> + return ret;
> +
> + return 0;
if (ret)
return ret;
return 0;
Can succinctly be written "return ret;"
> +}
> +
> +/**
> + * avm_wasp_rproc_stop() - stop the remote processor
> + * @rproc: pointer to the rproc structure
> + *
> + * To stop the remote processor just the startup gpio is set to 0
> + * and the WASP processor is powered off
> + *
> + * Return: 0 on Success
> + */
> +static int avm_wasp_rproc_stop(struct rproc *rproc)
> +{
> + struct avm_wasp_rproc *avmwasp = rproc->priv;
> +
> + gpio_set_value(avmwasp->startup_gpio,
> + (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> + 1 : 0);
> +
> + return 0;
> +}
> +
> +/**
> + * avm_wasp_rproc_load() - noop to avoid the ELF binary defaults
> + * @rproc: pointer to the rproc structure
> + * @fw: pointer to firmware struct
> + *
> + * If a load function is not defined in the rproc_ops, then all the settings
> + * like checking the firmware binary will default to ELF checks, which fail
> + * in case of the bootable and compressed initramfs image for WASP.
> + * Furthermore during boot its just required to send the firmware to the WASP
> + * processor, its not required to keep it in local memory, as the WASP SoC
> + * has its own memory.
> + *
> + * Return: Always 0
> + */
> +static int avm_wasp_rproc_load(struct rproc *rproc, const struct firmware *fw)
If you have to load all your firmware in start() we should come up with
a different way to signal that the default ELF loader should be used, so
that you can skip specifying load().
> +{
> + return 0;
> +}
> +
> +/**
> + * avm_wasp_rproc_boot_addr() - store fw from framework in priv
> + * @rproc: pointer to the rproc structure
> + * @fw: pointer to firmware struct
> + *
> + * Even though firmware files can be loaded without the remote processor
> + * framework, it expects at least one firmware file.
> + * This function stores the initramfs image that is loaded by the remote
> + * processor framework during boot process into the priv for access by
> + * the initramfs load function avm_wasp_load_initramfs_image().
> + *
> + * Return: Address of initramfs image
> + */
> +static u64 avm_wasp_rproc_boot_addr(struct rproc *rproc,
> + const struct firmware *fw)
> +{
> + struct avm_wasp_rproc *avmwasp = rproc->priv;
> +
> + avmwasp->linux_blob = fw;
> +
> + return (u64)((u32)fw->data);
No, the boot_addr should denote that address where the remote processor
is supposed to start executing code from. This is the lower 32 bits of a
virtual address on the Linux side, and shortly after returning from this
function fw->data will be released - making this a dangling "pointer".
> +}
> +
> +static const struct rproc_ops avm_wasp_rproc_ops = {
> + .start = avm_wasp_rproc_start,
> + .stop = avm_wasp_rproc_stop,
> + .load = avm_wasp_rproc_load,
> + .get_boot_addr = avm_wasp_rproc_boot_addr,
> +};
> +
> +static int avm_wasp_rproc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct avm_wasp_rproc *avmwasp;
> + const char *fw_name;
> + struct rproc *rproc;
> + struct device_node *root_node;
> + int ret;
> + u32 phandle;
> + char *model;
> +
> + root_node = of_find_node_by_path("/");
> + if (!root_node) {
> + dev_err(dev, "No root node in device tree.\n");
> + ret = -EFAULT;
> + goto err;
> + }
> +
> + ret = of_property_read_string_index(root_node, "compatible",
> + 0, (const char **)&model);
> + of_node_put(root_node);
> + if (ret) {
> + dev_err(dev, "No model in device tree.\n");
> + goto err;
> + }
> +
> + /* check model of host device to determine WASP SoC type */
> + if (strstr(model, "3390")) {
> + m_model = MODEL_3390;
Don't look at the top-level compatible, give your remoteproc node a
specific compatible and use .data in the of_device_id and
device_get_match_data() to get the right MODEL_*.
> + } else if (strstr(model, "490")) {
> + m_model = MODEL_X490;
> + } else {
> + dev_err(dev, "No WASP on device.\n");
> + ret = -EPERM;
> + goto err;
> + }
> +
> + ret = of_property_read_string(dev->of_node, "wasp-initramfs-image",
> + &fw_name);
> + if (ret) {
> + dev_err(dev, "No initramfs image for WASP filename given\n");
> + goto err;
> + }
> +
> + rproc = devm_rproc_alloc(dev, "avm,wasp", &avm_wasp_rproc_ops,
> + fw_name, sizeof(*avmwasp));
> + if (!rproc) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + rproc->auto_boot = true;
> +
> + avmwasp = rproc->priv;
> + avmwasp->rproc = rproc;
> + avmwasp->pdev = pdev;
> +
> + ret = of_property_read_string(dev->of_node, "ath9k-firmware",
> + &fw_name);
> + if (ret) {
> + dev_err(dev, "No ath9k firmware filename given\n");
> + goto err;
> + }
> +
> + ret = avm_wasp_firmware_request(avmwasp, fw_name);
> + if (ret) {
> + dev_err(dev, "Could not load ath9k firmware\n");
> + goto err;
> + }
> + avm_wasp_firmware_release(avmwasp);
You shouldn't attempt to load the firmware from your probe, the idea is
that remoteproc will load "the firmware" and call load() for you to copy
it in place and then call start() to make your core execute the loaded
firmware.
It seems like you have a bunch of firmware to load at start() time, but
I don't think it's correct to try-load the firmware here and fail
probe() if it's not in place.
> + if (m_model == MODEL_X490) {
> + ret = of_property_read_string(dev->of_node, "ath10k-caldata",
> + &fw_name);
> + if (ret) {
> + dev_err(dev, "No ath10k caldata filename given\n");
> + goto err;
> + }
> +
> + ret = avm_wasp_firmware_request(avmwasp, fw_name);
> + if (ret) {
> + dev_err(dev, "Could not load ath10k caldata\n");
> + goto err;
> + }
> + avm_wasp_firmware_release(avmwasp);
> + }
> +
> + ret = of_property_read_u32(dev->of_node, "wasp-initramfs-port",
> + &phandle);
> + if (ret) {
> + dev_err(dev, "No wasp-initramfs-port given\n");
> + goto err;
> + } else {
There's no need for else here, as your if-case returns a failure.
> + struct device_node *child = of_find_node_by_phandle(phandle);
> +
> + if (!child) {
> + dev_err(dev, "Get wasp-initramfs-port child failed\n");
> + ret = -ENODEV;
> + goto err;
> + } else {
> + ret = of_property_read_string(child, "label",
> + (const char **)
> + &avmwasp->loader_port);
> + of_node_put(child);
> + if (ret) {
> + dev_err(dev,
> + "Get wasp-port-label failed\n");
> + goto err;
> + }
> + }
> + }
> +
> + ret = of_property_read_u32(dev->of_node, "wasp-netboot-mdio",
> + &phandle);
> + if (ret) {
> + dev_err(dev, "No wasp-netboot-mdio given\n");
> + goto err;
> + } else {
> + struct device_node *mdio_node =
> + of_find_node_by_phandle(phandle);
> +
> + if (!mdio_node) {
> + dev_err(dev, "Get wasp-netboot-mdio failed\n");
> + ret = -ENODEV;
> + goto err;
> + } else {
> + avmwasp->mdio_bus = of_mdio_find_bus(mdio_node);
> + of_node_put(mdio_node);
> + if (!avmwasp->mdio_bus) {
> + dev_err(dev, "mdio bus not found\n");
> + ret = -ENODEV;
> + goto err;
> + }
> + avmwasp->mdio_bus_id = avmwasp->mdio_bus->id;
> + put_device(&avmwasp->mdio_bus->dev);
Why are you releasing the refcount of the device that you just found?
Shouldn't this be held until you're no longer referencing it?
> + }
> + }
> +
> + avmwasp->startup_gpio = of_get_named_gpio_flags(dev->of_node,
> + "startup-gpio",
> + 0,
> + &avmwasp->s_gpio_flg);
> + if (!gpio_is_valid(avmwasp->startup_gpio)) {
> + dev_err(dev, "Request wasp-startup gpio failed\n");
> + ret = -ENODEV;
> + goto err;
> + } else {
> + ret = devm_gpio_request_one(dev, avmwasp->startup_gpio,
> + (avmwasp->s_gpio_flg &
> + OF_GPIO_ACTIVE_LOW) ?
> + GPIOF_OUT_INIT_LOW :
> + GPIOF_OUT_INIT_HIGH,
> + "wasp-startup");
> +
> + if (ret) {
> + dev_err(dev, "get wasp-startup gpio failed\n");
> + goto err;
> + }
> + }
avmwasp->startup_gpio = devm_gpio_get(dev, "startup", GPIOD_OUT_LOW);
if (IS_ERR(avmwasp->startup_gpio)) {
ret = dev_err_probe(dev, PTR_ERR(avmwasp->startup_gpio), "failed to get startup gpio\n");
goto err;
}
Would do all this, then depending on the gpio being specified
GPIO_ACTIVE_HIGH or LOW gpio_set_value() will "flip" the value to make
sure that 1 is active and 0 is inactive.
I.e. you don't need s_gpio_flg or r_gpio_flg or the conditional in all
of your gpio_set_value().
> +
> + avmwasp->reset_gpio = of_get_named_gpio_flags(dev->of_node,
> + "reset-gpio",
> + 0,
> + &avmwasp->r_gpio_flg);
> + if (!gpio_is_valid(avmwasp->reset_gpio)) {
> + dev_err(dev, "Request wasp-reset gpio failed\n");
> + ret = -ENODEV;
> + goto err_free_startup_gpio;
> + } else {
> + ret = devm_gpio_request_one(dev, avmwasp->reset_gpio,
> + (avmwasp->r_gpio_flg &
> + OF_GPIO_ACTIVE_LOW) ?
> + GPIOF_OUT_INIT_LOW :
> + GPIOF_OUT_INIT_HIGH,
> + "wasp-reset");
> +
> + if (ret) {
> + dev_err(dev, "get wasp-reset gpio failed\n");
> + goto err_free_startup_gpio;
> + }
> + }
> +
> + ret = of_property_read_string(dev->of_node, "wasp-netboot-firmware",
> + (const char **)
> + &avmwasp->netboot_firmware);
> + if (ret) {
> + dev_err(dev, "No WASP network boot firmware filename given\n");
> + goto err_free_reset_gpio;
> + }
> +
> + ret = request_firmware_direct((const struct firmware **)
> + &avmwasp->eeprom_blob, avmwasp->netboot_firmware, dev);
As above, I don't think you should request firmware here.
> + if (ret) {
> + dev_err(dev, "Could not load WASP network boot firmware\n");
> + goto err_free_reset_gpio;
> + }
> +
> + if (avmwasp->eeprom_blob->size > 0xffff) {
> + dev_err(dev, "WASP network boot firmware too big\n");
> + ret = -EINVAL;
> + goto err_free_reset_gpio;
> + }
> +
> + avm_wasp_firmware_release(avmwasp);
> +
> + dev_set_drvdata(dev, rproc);
platform_set_drvdata()
> +
> + ret = devm_rproc_add(dev, rproc);
> + if (ret) {
> + dev_err(dev, "rproc_add failed\n");
> + goto err_free_reset_gpio;
> + }
> +
> + return 0;
> +
> +err_free_reset_gpio:
> + devm_gpio_free(&avmwasp->pdev->dev, avmwasp->reset_gpio);
> + gpio_set_value(avmwasp->startup_gpio,
> + (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> + 1 : 0);
> +err_free_startup_gpio:
> + devm_gpio_free(&avmwasp->pdev->dev, avmwasp->startup_gpio);
The purpose of using devres allocations is that they will be
automatically freed.
> +err:
> + return ret;
> +}
> +
> +static int avm_wasp_rproc_remove(struct platform_device *pdev)
> +{
> + struct rproc *rproc = platform_get_drvdata(pdev);
> + struct avm_wasp_rproc *avmwasp = rproc->priv;
> +
> + gpio_set_value(avmwasp->startup_gpio,
> + (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> + 1 : 0);
> + mdelay(WASP_WAIT_SLEEP);
> + devm_gpio_free(&avmwasp->pdev->dev, avmwasp->startup_gpio);
> + devm_gpio_free(&avmwasp->pdev->dev, avmwasp->reset_gpio);
As soon as you return from this function any devm allocated resources
will be released. So there's no need for doing that here.
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int avm_wasp_rpm_suspend(struct device *dev)
Unused.
> +{
> + return -EBUSY;
> +}
> +
> +static int avm_wasp_rpm_resume(struct device *dev)
Unused.
> +{
> + return 0;
> +}
> +#endif
> +
> +static const struct of_device_id avm_wasp_rproc_of_match[] = {
> + { .compatible = "avm,wasp", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, avm_wasp_rproc_of_match);
> +
> +static struct platform_driver avm_wasp_rproc_driver = {
> + .probe = avm_wasp_rproc_probe,
> + .remove = avm_wasp_rproc_remove,
> + .driver = {
> + .name = "avm_wasp_rproc",
> + .of_match_table = avm_wasp_rproc_of_match,
> + },
> +};
> +
> +module_platform_driver(avm_wasp_rproc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("AVM WASP remote processor boot driver");
> +MODULE_AUTHOR("Daniel Kestrel <kestrelseventyfour@gmail.com>");
> diff --git a/drivers/remoteproc/avm_wasp.h b/drivers/remoteproc/avm_wasp.h
Are any of these definitions going to be used outside avm_wasp.c?
If not they would be better to have at top of the c-file.
Regards,
Bjorn
> new file mode 100644
> index 000000000000..d0a4542b3420
> --- /dev/null
> +++ b/drivers/remoteproc/avm_wasp.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2019-2020 Andreas Böhler
> + * Copyright (c) 2021-2022 Daniel Kestrel
> + */
> +
> +#ifndef __AVM_WASP_H
> +#define __AVM_WASP_H
> +
> +#define WASP_CHUNK_SIZE 14
> +#define M_REGS_WASP_INDEX_MAX 7
> +
> +#define WASP_ADDR 0x07
> +#define WASP_TIMEOUT_COUNT 1000
> +#define WASP_WAIT_TIMEOUT_COUNT 20
> +
> +#define WASP_WRITE_SLEEP_US 20000
> +#define WASP_WAIT_SLEEP 100
> +#define WASP_POLL_SLEEP_US 200
> +#define WASP_BOOT_SLEEP_US 20000
> +
> +#define WASP_RESP_RETRY 0x0102
> +#define WASP_RESP_OK 0x0002
> +#define WASP_RESP_WAIT 0x0401
> +#define WASP_RESP_COMPLETED 0x0000
> +#define WASP_RESP_READY_TO_START 0x0202
> +#define WASP_RESP_STARTING 0x00c9
> +
> +#define WASP_CMD_SET_PARAMS 0x0c01
> +#define WASP_CMD_SET_CHECKSUM_3390 0x0801
> +#define WASP_CMD_SET_CHECKSUM_X490 0x0401
> +#define WASP_CMD_SET_DATA 0x0e01
> +#define WASP_CMD_START_FIRMWARE_3390 0x0201
> +#define WASP_CMD_START_FIRMWARE_X490 0x0001
> +#define WASP_CMD_START_FIRMWARE2_X490 0x0101
> +
> +static const u32 start_addr = 0xbd003000;
> +static const u32 exec_addr = 0xbd003000;
> +
> +static u16 m_regs_wasp[] = {0x0, 0x2, 0x4, 0x6, 0x8, 0xA, 0xC, 0xE};
> +
> +static const char mac_data[WASP_CHUNK_SIZE] = {0xaa, 0xaa, 0xaa, 0xaa, 0xaa,
> + 0xaa, 0x04, 0x20, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00};
> +
> +enum {
> + MODEL_3390,
> + MODEL_X490,
> + MODEL_UNKNOWN
> +} m_model = MODEL_UNKNOWN;
> +
> +#define ETHER_TYPE_ATH_ECPS_FRAME 0x88bd
> +#define BUF_SIZE 1056
> +#define COUNTER_INCR 4
> +#define SEND_LOOP_TIMEOUT_SECONDS 60
> +
> +#define MAX_PAYLOAD_SIZE 1028
> +#define CHUNK_SIZE 1024
> +#define WASP_HEADER_LEN 14
> +
> +#define PACKET_START 0x1200
> +#define CMD_FIRMWARE_DATA 0x0104
> +#define CMD_START_FIRMWARE 0xd400
> +
> +#define RESP_DISCOVER 0x0000
> +#define RESP_CONFIG 0x1000
> +#define RESP_OK 0x0100
> +#define RESP_STARTING 0x0200
> +#define RESP_ERROR 0x0300
> +
> +enum {
> + DOWNLOAD_TYPE_UNKNOWN = 0,
> + DOWNLOAD_TYPE_FIRMWARE,
> + DOWNLOAD_TYPE_CONFIG
> +} m_download_type = DOWNLOAD_TYPE_UNKNOWN;
> +
> +static const u32 m_load_addr = 0x81a00000;
> +
> +static char wasp_mac[] = {0x00, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa};
> +
> +struct wasp_packet {
> + union {
> + u8 data[MAX_PAYLOAD_SIZE + WASP_HEADER_LEN];
> + struct __packed {
> + u16 packet_start;
> + u8 pad_one[5];
> + u16 command;
> + u16 response;
> + u16 counter;
> + u8 pad_two;
> + u8 payload[MAX_PAYLOAD_SIZE];
> + };
> + };
> +} __packed;
> +
> +#endif /* __AVM_WASP_H */
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] remoteproc: Add AVM WASP driver
2022-02-22 0:30 ` Bjorn Andersson
@ 2022-02-23 14:58 ` Kestrel seventyfour
0 siblings, 0 replies; 5+ messages in thread
From: Kestrel seventyfour @ 2022-02-23 14:58 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Mathieu Poirier, Rob Herring, linux-remoteproc, devicetree, linux-kernel
Hi Bjorn,
I have worked on most of your comments, for some comments I have
questions and answers, please see below.
Am Di., 22. Feb. 2022 um 01:28 Uhr schrieb Bjorn Andersson
<bjorn.andersson@linaro.org>:
>
> On Mon 21 Feb 05:54 PST 2022, Daniel Kestrel wrote:
>
> > Some AVM Fritzbox router boards (3390, 3490, 5490, 5491, 7490),
> > that are Lantiq XRX200 based, have a memory only ATH79 based
> > WASP (Wireless Assistant Support Processor) SoC that has wifi
> > cards connected to it. It does not share anything with the
> > Lantiq host and has no persistent storage. It has an mdio based
> > connection for bringing up a small network boot firmware and is
> > connected to the Lantiq GSWIP switch via gigabit ethernet. This
> > is used to load an initramfs linux image to it, after the
> > network boot firmware was started.
> >
> > In order to initialize this remote processor we need to:
> > - power on the SoC using startup gpio
> > - reset the SoC using the reset gpio
> > - send the network boot firmware using mdio
> > - send the linux image using raw ethernet frames
> >
> > This driver allows to start and stop the WASP SoC.
> >
>
> That's different, but seems to be a reasonable fit with the remoteproc
> framework.
>
> > Signed-off-by: Daniel Kestrel <kestrelseventyfour@gmail.com>
> > ---
> > drivers/remoteproc/Kconfig | 10 +
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/avm_wasp.c | 1251 +++++++++++++++++++++++++++++++++
> > drivers/remoteproc/avm_wasp.h | 95 +++
> > 4 files changed, 1357 insertions(+)
> > create mode 100644 drivers/remoteproc/avm_wasp.c
> > create mode 100644 drivers/remoteproc/avm_wasp.h
> >
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index 166019786653..a761186c5171 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -23,6 +23,16 @@ config REMOTEPROC_CDEV
> >
> > It's safe to say N if you don't want to use this interface.
> >
> > +config AVM_WASP_REMOTEPROC
> > + tristate "AVM WASP remoteproc support"
> > + depends on NET_DSA_LANTIQ_GSWIP
> > + help
> > + Say y here to support booting the secondary SoC ATH79 target
> > + called Wireless Assistant Support Processor (WASP) that some
> > + AVM Fritzbox devices (3390, 3490, 5490, 5491, 7490) have built in.
> > +
> > + It's safe to say N here.
> > +
> > config IMX_REMOTEPROC
> > tristate "i.MX remoteproc support"
> > depends on ARCH_MXC
> > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> > index 5478c7cb9e07..0ae175c6722f 100644
> > --- a/drivers/remoteproc/Makefile
> > +++ b/drivers/remoteproc/Makefile
> > @@ -11,6 +11,7 @@ remoteproc-y += remoteproc_sysfs.o
> > remoteproc-y += remoteproc_virtio.o
> > remoteproc-y += remoteproc_elf_loader.o
> > obj-$(CONFIG_REMOTEPROC_CDEV) += remoteproc_cdev.o
> > +obj-$(CONFIG_AVM_WASP_REMOTEPROC) += avm_wasp.o
> > obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> > obj-$(CONFIG_IMX_DSP_REMOTEPROC) += imx_dsp_rproc.o
> > obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
> > diff --git a/drivers/remoteproc/avm_wasp.c b/drivers/remoteproc/avm_wasp.c
> > new file mode 100644
> > index 000000000000..04b7c9005028
> > --- /dev/null
> > +++ b/drivers/remoteproc/avm_wasp.c
> > @@ -0,0 +1,1251 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * AVM WASP Remote Processor driver
> > + *
> > + * Copyright (c) 2019-2020 Andreas Böhler
> > + * Copyright (c) 2021-2022 Daniel Kestrel
> > + *
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/timekeeping.h>
> > +#include <net/sock.h>
> > +#include <asm-generic/gpio.h>
> > +
> > +#include "remoteproc_internal.h"
> > +#include "avm_wasp.h"
> > +
> > +/**
> > + * struct avm_wasp_rproc - avmwasp remote processor priv
> > + * @rproc: rproc handle
> > + * @pdev: pointer to platform device
> > + * @eeprom_blob: pointer to load and save any firmware
> > + * @linux_blob: pointer to access initramfs image
> > + * @complete: structure for asynchronous firmware load
> > + * @mdio_bus: pointer to mii_bus of gswip device for gpio
> > + * @startup_gpio: store WASP startup gpio number
> > + * @reset_gpio: store WASP reset gpio number
> > + * @s_gpio_flg: store WASP startup gpio flags active high/low
> > + * @r_gpio_flg: store WASP reset gpio flags active high/low
> > + * @netboot_firmware: store name of the network boot firmware
> > + * @loader_port: store name of the port wasp is connected to
> > + * @sendbuf: send buffer for uploading WASP initramfs firmware
> > + * @recvbuf: recv buffer for feedback from WASP
> > + * @s_packet: structure for sending packets to WASP
> > + * @send_socket: pointer to socket for sending to WASP
> > + * @recv_socket: pointer to socket for receiving from WASP
> > + * @ifindex: interface index used for WASP communication
> > + */
> > +struct avm_wasp_rproc {
> > + struct rproc *rproc;
> > + struct platform_device *pdev;
> > + const struct firmware *eeprom_blob, *linux_blob;
> > + struct completion complete;
> > + char *mdio_bus_id;
> > + struct mii_bus *mdio_bus;
> > + int startup_gpio, reset_gpio;
> > + enum of_gpio_flags s_gpio_flg, r_gpio_flg;
> > + char *netboot_firmware;
> > + char *loader_port;
> > + char sendbuf[BUF_SIZE];
> > + char recvbuf[BUF_SIZE];
>
> Why aren't these just struct wasp_packet? Instead of having a char
> buffer that happens to be sized, slightly bigger than a wasp_packet?
>
> > + struct wasp_packet s_packet;
> > + struct socket *send_socket;
> > + struct socket *recv_socket;
> > + int ifindex;
> > +};
> > +
> > +/**
> > + * avm_wasp_firmware_request_cb() - callback handler for firmware load
> > + * @eeprom_blob: pointer to struct firmware
> > + * @ctx: context passed
> > + *
> > + * This handler is called after completing the request_firmware_nowait
> > + * function by passing the avm_wasp_rproc struct
> > + * It saves the firmware in the context and calls complete
> > + */
> > +static void avm_wasp_firmware_request_cb(const struct firmware *eeprom_blob,
> > + void *ctx)
> > +{
> > + struct avm_wasp_rproc *avmwasp = ctx;
> > +
> > + if (eeprom_blob)
> > + avmwasp->eeprom_blob = eeprom_blob;
> > +
> > + complete(&avmwasp->complete);
> > +}
> > +
> > +/**
> > + * avm_wasp_firmware_request() - asynchronous load of passed firmware
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + * @name: char pointer to filename (relative to /lib/firmware)
> > + *
> > + * Handles setup and execution of the asynchronous firmware request
> > + * Used to trigger the load of the ath10k caldata and ath9k eeprom
> > + * firmware from the tffs partition of the devices
> > + *
> > + * Return: 0 on success, -2 if file not found or error from function
>
> Write out ENOENT instead of -2.
>
> > + * request_firmware_nowait
> > + */
> > +static int avm_wasp_firmware_request(struct avm_wasp_rproc *avmwasp,
> > + const char *name)
> > +{
> > + int err;
> > +
> > + init_completion(&avmwasp->complete);
> > +
> > + err = request_firmware_nowait(THIS_MODULE, 1, name,
> > + &avmwasp->pdev->dev,
> > + GFP_KERNEL, avmwasp,
> > + avm_wasp_firmware_request_cb);
> > + if (err < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Load request for %s failed\n", name);
> > + return err;
> > + }
> > +
> > + wait_for_completion(&avmwasp->complete);
>
> Why do you use nowait and then wait? This seems very similar to
> request_firmware()...
>
> > +
> > + if (!avmwasp->eeprom_blob) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Unable to load %s\n", name);
> > + return -ENOENT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * avm_wasp_firmware_release() - clean up after firmware load
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + *
> > + * Releases the firmware that is in the eeprom_blob firmware
> > + * pointer of the private avm_wasp_rproc structure
> > + */
> > +static void avm_wasp_firmware_release(struct avm_wasp_rproc *avmwasp)
> > +{
> > + release_firmware(avmwasp->eeprom_blob);
> > + avmwasp->eeprom_blob = NULL;
> > +}
> > +
> > +/**
> > + * avm_wasp_netboot_mdio_read() - read with gswip mdio bus
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + * @location: register number of the m_regs_wasp register array
> > + *
> > + * Reads a value from the specified register for the mdio address
> > + * that is used for the connection to the WASP SoC
> > + * Mutex on mdio_lock is required to serialize access on bus
> > + *
> > + * Return: Value that was read from the specified register
> > + */
> > +int avm_wasp_netboot_mdio_read(struct avm_wasp_rproc *avmwasp,
> > + int location)
> > +{
> > + int value;
> > +
> > + if (location > M_REGS_WASP_INDEX_MAX || location < 0)
> > + return 0;
> > + mutex_lock(&avmwasp->mdio_bus->mdio_lock);
>
> If your mdio_bus requires a lock to handle concurrent IO operations,
> then that lock should be pushed into the read/write.
>
> If for some reason you need to serialize a sequence of read/writes, then
> it makes sense to have it here.
>
> But it's quite common for this to actually be an "atomic" operation and
> that there's no lock needed in the first place.
>
I will use the mdiobus_read and mdiobus_write functions and they
implement the mutex, if thats ok? Thats how the phys are doing it,
e.g. at803x.c, that share the mdio_bus with the lantiq_gswip.c driver.
I will just be another user of the mdio_bus in addition to the phys and
the lantiq gswip driver.
> > + value = avmwasp->mdio_bus->read(avmwasp->mdio_bus,
> > + WASP_ADDR, m_regs_wasp[location]);
> > + mutex_unlock(&avmwasp->mdio_bus->mdio_lock);
> > + return value;
> > +}
> > +
> > +/**
> > + * avm_wasp_netboot_mdio_write() - write with gswip mdio bus
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + * @location: register number of the m_regs_wasp register array
> > + * @value: value to be written to the register
> > + *
> > + * Writes a value to the specified register for the mdio address
> > + * that is used for the connection to the WASP SoC
> > + * Mutex on mdio_lock is required to serialize access on bus
> > + * Makes sure not to write to invalid registers as this can have
> > + * unpredictable results
> > + */
> > +void avm_wasp_netboot_mdio_write(struct avm_wasp_rproc *avmwasp,
> > + int location, int value)
> > +{
> > + if (location > M_REGS_WASP_INDEX_MAX || location < 0)
> > + return;
> > + mutex_lock(&avmwasp->mdio_bus->mdio_lock);
> > + avmwasp->mdio_bus->write(avmwasp->mdio_bus, WASP_ADDR,
> > + m_regs_wasp[location], value);
> > + mutex_unlock(&avmwasp->mdio_bus->mdio_lock);
> > +}
> > +
> > +/**
> > + * avm_wasp_netboot_mdio_write_u32_split() - write 32bit value
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + * @location: register number of the m_regs_wasp register array
> > + * @value: value to be written to the register
> > + *
> > + * As the mdio registers are 16bit, this function writes a 32bit value
> > + * to two subsequent registers starting with the specified register
> > + * for the mdio address that is used for the connection to the WASP SoC
> > + */
> > +void avm_wasp_netboot_mdio_write_u32_split(struct avm_wasp_rproc *avmwasp,
> > + int location, const u32 value)
> > +{
> > + avm_wasp_netboot_mdio_write(avmwasp, location,
> > + ((value & 0xffff0000) >> 16));
> > + avm_wasp_netboot_mdio_write(avmwasp, location + 1,
> > + (value & 0x0000ffff));
>
> Here you perform two separate writes, this might be wort locking
> around if there's a possibility that two threads races and a mix of
> their two "value" end up in the registers.
>
> > +}
> > +
> > +/**
> > + * avm_wasp_netboot_write_header() - write header to WASP
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + * @start_address: address where to load the firmware to on WASP
> > + * @len: length of the network boot firmware
> > + * @exec_address: address where to start execution on WASP
> > + *
> > + * Writes the header to WASP using mdio to initiate the start of
> > + * transferring the network boot firmware to WASP
> > + *
> > + * Return: 0 on Success or -14 if writing header failed based on return
>
> -EFAULT instead of -14
>
> > + * code from WASP
> > + */
> > +static int avm_wasp_netboot_write_header(struct avm_wasp_rproc *avmwasp,
> > + const u32 start_addr, const u32 len,
> > + const u32 exec_addr)
> > +{
> > + int regval;
> > + int timeout = WASP_TIMEOUT_COUNT;
> > +
> > + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 1, start_addr);
> > + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 3, len);
> > + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 5, exec_addr);
> > + avm_wasp_netboot_mdio_write(avmwasp, 0, WASP_CMD_SET_PARAMS);
> > +
> > + do {
> > + udelay(WASP_POLL_SLEEP_US);
> > + regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
> > + timeout--;
> > + } while ((regval != WASP_RESP_OK) && (timeout > 0));
>
> I wonder if this could be implemented using read_poll_timeout() instead.
>
It worked. However it added about half a kB size to the kernel module per
invokation so I created a separate function to get rid of 6k load module size.
> > +
> > + if (regval != WASP_RESP_OK) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error writing header to WASP! Status = %d\n", regval);
> > + return -EFAULT;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * avm_wasp_netboot_write_checksum() - write checksum to WASP
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + * @checksum: calculated checksum value to be sent to WASP
> > + *
> > + * Writes the calculated checksum for the given network boot firmware
> > + * to WASP using mdio as the second step
> > + *
> > + * Return: 0 on Success or -14 if writing checksum failed based on return
>
> -EFAULT instead of -14
>
> > + * code from WASP
> > + */
> > +static int avm_wasp_netboot_write_checksum(struct avm_wasp_rproc *avmwasp,
> > + const uint32_t checksum)
> > +{
> > + int regval;
> > + int timeout = WASP_TIMEOUT_COUNT;
> > +
> > + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 1, checksum);
> > + if (m_model == MODEL_3390) {
> > + avm_wasp_netboot_mdio_write_u32_split(avmwasp, 3, 0x0000);
> > + avm_wasp_netboot_mdio_write(avmwasp, 0,
> > + WASP_CMD_SET_CHECKSUM_3390);
> > + } else if (m_model == MODEL_X490)
> > + avm_wasp_netboot_mdio_write(avmwasp, 0,
> > + WASP_CMD_SET_CHECKSUM_X490);
>
> If one of your blocks is wrapped in {}, then please wrap them all in {}.
>
> Btw, no need to wrap this line, it's fairly close to 80 chars anyways.
>
> > +
> > + do {
> > + udelay(WASP_POLL_SLEEP_US);
> > + regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
> > + timeout--;
> > + } while ((regval != WASP_RESP_OK) && (timeout > 0));
>
> As above. And if read_poll_timeout() doesn't work out, this is repeated
> multiple times, seems reasonable to break out to a helper function.
>
> > +
> > + if (regval != WASP_RESP_OK) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error writing checksum to WASP! Status = %d\n",
> > + regval);
> > + return -EFAULT;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * avm_wasp_netboot_write_chunk() - write chunk of data to WASP
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + * @data: pointer to data
> > + * @len: length of data (should not exceed 14 bytes)
> > + *
> > + * Writes up to 14 bytes of data into the 7 16bit mdio registers
> > + * to WASP using mdio
> > + *
> > + * Return: 0 on Success, -14 if data length is mor than 14 bytes or
> > + * -2 if writing the data failed based on return code from WASP
>
> EFAULT, ENOENT
>
> > + */
> > +static int avm_wasp_netboot_write_chunk(struct avm_wasp_rproc *avmwasp,
> > + const char *data, const int len)
>
> @len sounds like a size_t to me.
>
> > +{
> > + int regval, i, j;
> > + int timeout = WASP_TIMEOUT_COUNT;
> > +
> > + if (len > WASP_CHUNK_SIZE || len < 0 || !data)
> > + return -EFAULT;
>
> Blank line here would be nice.
>
> > + for (i = 0, j = 1; i < len; i += 4, j += 2)
> > + avm_wasp_netboot_mdio_write_u32_split(avmwasp, j,
> > + *((uint32_t *)
> > + (data + i)));
>
> 14 isn't evenly divided in 4 byte chunks, so this doesn't seem to work
> as described.
>
> > +
> > + avm_wasp_netboot_mdio_write(avmwasp, 0, WASP_CMD_SET_DATA);
> > +
> > + do {
> > + udelay(WASP_POLL_SLEEP_US);
> > + regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
> > + timeout--;
> > + } while ((regval != WASP_RESP_OK) && (timeout > 0));
>
> As above.
>
> > +
> > + if (regval != WASP_RESP_OK && regval != WASP_RESP_WAIT &&
> > + regval != WASP_RESP_COMPLETED) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error writing chunk to WASP: m_reg_status = 0x%x!\n",
> > + regval);
> > + return -EFAULT;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * avm_wasp_netboot_calc_checksum() - calculate netboot firmware checksum
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + *
> > + * Calculates the checksum by using the eeprom_blob from the private
> > + * avm_wasp_rproc structure
> > + *
> > + * Return: Calculated checksum or -14 on Error
>
> EFAULT
>
> > + */
> > +static uint32_t avm_wasp_netboot_calc_checksum(struct avm_wasp_rproc *avmwasp)
>
> Use u32 instead of uint32_t in the kernel.
>
> > +{
> > + u32 checksum = 0xffffffff;
> > + u32 cs;
> > + int count = -1;
>
> Should this be signed, or is it just as signed as "checksum"?
>
> > + size_t size;
> > + const u8 *firmware;
> > + const u8 *firmware_end;
> > +
> > + if (!avmwasp->eeprom_blob)
> > + return -EFAULT;
>
> -EFAULT isn't an awesome uint32_t and you're blindly writing it to the
> socket below.
>
> That said, it would be much better if you ensured that you didn't end up
> in avm_wasp_netboot_load_firmware() in the first place with eeprom_blob
> of NULL.
>
> But if nothing else, right before calling this function you check
> this...
>
> > + size = avmwasp->eeprom_blob->size;
> > + firmware = avmwasp->eeprom_blob->data;
> > + firmware_end = firmware + size;
> > +
> > + if (!firmware || size <= 0)
>
> Can firmware be NULL?
>
> size was checked right before calling the function.
>
> > + return -EFAULT;
> > +
> > + while (firmware < firmware_end) {
> > + cs = (firmware[0] << 24 | firmware[1] << 16 |
> > + firmware[2] << 8 | firmware[3]);
>
> So cs is the big endian representation of the data?
>
> I don't see any checks to ensure size is a multiple of 4, so this might
> read off the end of the array?
>
> > + checksum = checksum - cs;
> > + count++;
> > + firmware += 4;
> > + }
> > +
> > + checksum = checksum - count;
> > + return checksum;
> > +}
> > +
> > +/**
> > + * avm_wasp_netboot_load_firmware() - load netboot firmware to WASP
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + *
> > + * Implements the process to send header, checksum and the firmware
> > + * blob in 14 byte chunks to the WASP processor using mdio
> > + * Includes checks between the steps and sending commands to start
> > + * the network boot firmware
> > + *
> > + * Return: 0 on Success, -2 if no firmware is present, -19 if no
> > + * firmware or -14 if other errors have occurred
>
> ENOENT, ENODEV and EFAULT
>
> > + */
> > +int avm_wasp_netboot_load_firmware(struct avm_wasp_rproc *avmwasp)
> > +{
> > + const u8 *firmware;
> > + const u8 *firmware_end;
> > + int ret, regval, regval2, count, cont = 1;
> > +
> > + count = WASP_WAIT_TIMEOUT_COUNT;
> > +
> > + while (count > 0 && (avm_wasp_netboot_mdio_read(avmwasp, 0)
> > + != WASP_RESP_OK)) {
> > + count -= 1;
> > + mdelay(WASP_WAIT_SLEEP);
> > + }
> > +
> > + if (avm_wasp_netboot_mdio_read(avmwasp, 0)
> > + != WASP_RESP_OK) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error: WASP processor not ready\n");
> > +
> > + return -ENODEV;
> > + }
> > +
> > + ret = request_firmware_direct((const struct firmware **)
> > + &avmwasp->eeprom_blob,
> > + avmwasp->netboot_firmware, &avmwasp->pdev->dev);
> > + if (ret) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Could not find network boot firmware\n");
> > + return -ENOENT;
> > + }
> > +
> > + firmware = avmwasp->eeprom_blob->data;
> > + firmware_end = firmware + avmwasp->eeprom_blob->size;
> > +
> > + if (!firmware || avmwasp->eeprom_blob->size <= 0)
> > + return -EFAULT;
>
> EINVAL?
>
> > +
> > + if (avm_wasp_netboot_write_header(avmwasp, start_addr,
> > + avmwasp->eeprom_blob->size,
> > + exec_addr) < 0)
> > + return -EFAULT;
> > +
> > + if (avm_wasp_netboot_write_checksum(avmwasp,
> > + avm_wasp_netboot_calc_checksum
>
> Funny, this looks like a variable with the same name as the function,
> then I realized that you have the parameters on the next line.
>
> That's not helping anyone read this...
>
> > + (avmwasp)) < 0)
> > + return -EFAULT;
> > +
> > + while (firmware < firmware_end) {
> > + if ((firmware_end - firmware) >= WASP_CHUNK_SIZE) {
> > + if (avm_wasp_netboot_write_chunk(avmwasp, firmware,
> > + WASP_CHUNK_SIZE) < 0)
> > + return -EFAULT;
> > + } else {
> > + if (avm_wasp_netboot_write_chunk(avmwasp, firmware,
> > + (firmware_end -
> > + firmware)) < 0)
> > + return -EFAULT;
> > + }
> > + firmware += WASP_CHUNK_SIZE;
> > + }
>
> while (firmware < firmware_end) {
> left = firmware_end - firmware;
> if (left > WASP_CHUNK_SIZE)
> left = WASP_CHUNK_SIZE;
>
> ret = avm_wasp_netboot_write_chunk(avmwasp, firmware, left);
> if (ret < 0)
> return -EFAULT;
>
> firmware += left;
> }
>
>
> But not EFAULT...
>
> > +
> > + mdelay(WASP_WAIT_SLEEP);
> > +
> > + if (m_model == MODEL_3390)
> > + avm_wasp_netboot_mdio_write(avmwasp, 0,
> > + WASP_CMD_START_FIRMWARE_3390);
> > + else if (m_model == MODEL_X490)
> > + avm_wasp_netboot_mdio_write(avmwasp, 0,
> > + WASP_CMD_START_FIRMWARE_X490);
> > +
> > + avm_wasp_firmware_release(avmwasp);
> > +
> > + mdelay(WASP_WAIT_SLEEP);
> > + count = 0;
> > +
> > + while ((avm_wasp_netboot_mdio_read(avmwasp, 0)
> > + != WASP_RESP_READY_TO_START) &&
> > + (count < WASP_WAIT_TIMEOUT_COUNT)) {
> > + mdelay(WASP_WAIT_SLEEP);
> > + count++;
> > + }
> > + if (count >= WASP_WAIT_TIMEOUT_COUNT) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Timed out waiting for WASP ready to start.\n");
> > + return -EFAULT;
> > + }
> > +
> > + if (m_model == MODEL_3390)
> > + avm_wasp_netboot_mdio_write(avmwasp, 0,
> > + WASP_CMD_START_FIRMWARE_3390);
> > + else if (m_model == MODEL_X490)
> > + avm_wasp_netboot_mdio_write(avmwasp, 0,
> > + WASP_CMD_SET_CHECKSUM_X490);
> > +
> > + mdelay(WASP_WAIT_SLEEP);
> > +
> > + if (m_model == MODEL_3390) {
> > + count = 0;
> > + while ((avm_wasp_netboot_mdio_read(avmwasp, 0) !=
> > + WASP_RESP_OK) &&
> > + (count < WASP_WAIT_TIMEOUT_COUNT)) {
> > + mdelay(WASP_WAIT_SLEEP);
> > + count++;
> > + }
> > + if (count >= WASP_WAIT_TIMEOUT_COUNT) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Timed out waiting for WASP OK.\n");
> > + return -EFAULT;
> > + }
> > + if (avm_wasp_netboot_write_chunk(avmwasp, mac_data,
> > + WASP_CHUNK_SIZE) < 0) {
>
> ARRAY_SIZE(mac_data) seems more appropriate to denote help the reader
> understand that the right amount is actually referred to.
>
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error sending MAC address!\n");
> > + return -EFAULT;
> > + }
> > + } else if (m_model == MODEL_X490) {
> > + cont = 1;
> > + while (cont) {
> > + count = 0;
> > + while ((avm_wasp_netboot_mdio_read(avmwasp, 0)
> > + != WASP_RESP_OK) &&
> > + (count < WASP_WAIT_TIMEOUT_COUNT)) {
> > + mdelay(WASP_WAIT_SLEEP);
> > + count++;
> > + }
> > + if (count >= WASP_WAIT_TIMEOUT_COUNT) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Timed out waiting for WASP OK.\n");
> > + return -EFAULT;
>
> Timed out sounds like a ETIMEDOUT, not EFAULT.
>
> > + }
> > + regval = avm_wasp_netboot_mdio_read(avmwasp, 1);
> > + regval2 = avm_wasp_netboot_mdio_read(avmwasp, 2);
> > + avm_wasp_netboot_mdio_write(avmwasp, 0,
> > + WASP_CMD_SET_CHECKSUM_X490
> > + );
> > + if (regval == 0 && regval2 != 0)
> > + cont = regval2;
> > + else
> > + cont--;
> > + }
> > +
> > + count = 0;
> > + while ((avm_wasp_netboot_mdio_read(avmwasp, 0) !=
> > + WASP_RESP_OK) &&
> > + (count < WASP_TIMEOUT_COUNT)) {
> > + udelay(WASP_BOOT_SLEEP_US);
> > + count++;
> > + }
>
> Another read_poll_timeout() like construct. Or perhaps the same?
>
> > + if (count >= WASP_TIMEOUT_COUNT) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error waiting for checksum OK response.\n");
> > + return -EFAULT;
> > + }
> > +
> > + avm_wasp_netboot_mdio_write(avmwasp, 1, 0x00);
> > + avm_wasp_netboot_mdio_write(avmwasp, 0,
> > + WASP_CMD_START_FIRMWARE2_X490);
> > +
> > + regval = avm_wasp_netboot_mdio_read(avmwasp, 0);
> > + if (regval != WASP_RESP_OK) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error starting WASP network boot: 0x%x\n",
> > + regval);
> > + return -EFAULT;
>
> EFAULT doesn't seem appropriate here either...
>
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * avm_wasp_load_initramfs_image() - load initramfs image to WASP
> > + * @avmwasp: pointer to drivers private avm_wasp_rproc structure
> > + *
> > + * Uses the lan port specified from DT to load the initramfs to
> > + * WASP after the network boot firmware was successfully started.
> > + * Communication is done by using raw sockets.
> > + * The port of the lantiq gswip device will be started if not
> > + * already up and running.
> > + * There are several commands and status values which are checked.
> > + * First a discovery packet is received and then each data packet
> > + * is acknowledged by the WASP network boot firmware.
> > + * First packet needs to prepend the load address and last packet
> > + * needs to append the execution address.
> > + *
> > + * Return: 0 on Success, -14 if errors with the WASP send protocol
> > + * have occurred or the error returned from the failed operating
> > + * system function or service
> > + */
> > +int avm_wasp_load_initramfs_image(struct avm_wasp_rproc *avmwasp)
> > +{
> > + int done = 0;
>
> bool is a better type for booleans.
>
> > + int reuse = 1;
> > + int num_chunks = 0;
> > + int chunk_counter = 1;
>
> These seems unsigned
>
> > + int ret, packet_counter, data_offset;
>
> packet_counter sounds unsigned and data_offset sounds size_t.
>
> > + int send_len = 0;
>
> size_t and first access is an assignment, so no need to initialize it
> here.
>
> > + short interface_flags;
> > + ssize_t numbytes;
> > + ssize_t read;
>
> "read" isn't the best name for a variable to denote the chunk size to be
> sent.
>
> > + const u8 *firmware;
> > + const u8 *firmware_end;
> > + struct wasp_packet *packet = (struct wasp_packet *)
> > + (avmwasp->recvbuf + sizeof(struct ethhdr));
> > + struct ethhdr *recv_eh = (struct ethhdr *)avmwasp->recvbuf;
> > + struct msghdr recv_socket_hdr;
> > + struct kvec recv_vec;
> > + struct ethhdr *send_eh = (struct ethhdr *)avmwasp->sendbuf;
> > + struct sockaddr_ll send_socket_address;
> > + struct msghdr send_socket_hdr;
> > + struct kvec send_vec;
> > + struct net_device *send_netdev;
> > + struct sockaddr send_sock_addr;
> > + struct timeval {
> > + __kernel_old_time_t tv_sec;
> > + __kernel_suseconds_t tv_usec;
> > + } timeout;
>
> Don't we have one of these in the kernel headers somewhere?
>
> > + time64_t start_time, current_time;
> > +
> > + if (!avmwasp->linux_blob) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error accessing initramfs image");
> > + goto err;
> > + }
> > +
> > + ret = sock_create_kern(&init_net, PF_PACKET, SOCK_RAW,
> > + htons(ETHER_TYPE_ATH_ECPS_FRAME),
> > + &avmwasp->recv_socket);
> > + if (ret < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error opening recv socket: %d", ret);
> > + goto err;
> > + }
> > +
> > + ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET, SO_REUSEADDR,
> > + KERNEL_SOCKPTR(&reuse), sizeof(reuse));
> > + if (ret < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error SO_REUSEADDR recv socket: %d", ret);
> > + goto err_recv;
> > + }
> > +
> > + ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET,
> > + SO_BINDTODEVICE,
> > + KERNEL_SOCKPTR(avmwasp->loader_port),
> > + IFNAMSIZ - 1);
> > + if (ret < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error SO_BINDTODEVICE recv socket: %d", ret);
> > + goto err_recv;
> > + }
> > +
> > + timeout.tv_sec = 10;
> > + timeout.tv_usec = 0;
> > + ret = sock_setsockopt(avmwasp->recv_socket, SOL_SOCKET,
> > + SO_RCVTIMEO_OLD,
> > + KERNEL_SOCKPTR(&timeout), sizeof(timeout));
> > + if (ret < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error SO_RCVTIMEO recv socket: %d", ret);
> > + goto err_recv;
> > + }
> > +
> > + ret = sock_create_kern(&init_net, AF_PACKET, SOCK_RAW, IPPROTO_RAW,
> > + &avmwasp->send_socket);
>
> Why do you need two sockets to do rx and tx?
>
I inherited the code and never questioned it, indeed, one socket is enough.
> > + if (ret < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error opening send socket: %d", ret);
> > + goto err_recv;
> > + }
> > +
> > + timeout.tv_sec = 10;
> > + timeout.tv_usec = 0;
> > + ret = sock_setsockopt(avmwasp->send_socket, SOL_SOCKET,
> > + SO_SNDTIMEO_OLD,
> > + KERNEL_SOCKPTR(&timeout), sizeof(timeout));
> > + if (ret < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error SO_SNDTIMEO send socket: %d", ret);
> > + goto err_send;
> > + }
> > +
> > + rcu_read_lock();
> > + send_netdev = dev_get_by_name_rcu(sock_net(avmwasp->send_socket->sk),
> > + avmwasp->loader_port);
> > + if (send_netdev)
> > + interface_flags = (short)dev_get_flags(send_netdev);
> > + rcu_read_unlock();
> > +
> > + if (IS_ERR_OR_NULL(send_netdev)) {
> > + dev_err(&avmwasp->pdev->dev, "Error accessing net device.\n");
> > + ret = -ENODEV;
> > + goto err_send;
> > + }
> > +
> > + interface_flags |= IFF_PROMISC | IFF_UP | IFF_RUNNING;
>
> If !send_netdev interface_flags is uninitialized.
>
If !send_netdev I thought I will break out 3 lines above?
> > + rtnl_lock();
> > + ret = dev_change_flags(send_netdev, interface_flags, NULL);
>
> I'm not entirely familiar with the netdev API, but doesn't this up the
> interface? From your remoteproc start function?!
>
Yes it does, the receive fails with -11 after about 13 seconds when
only IFF_PROMISC and/or IFF_RUNNING is set.
Depending on if the switch/port is active at the time wasp is booted,
its only required if network is still down.
I thought if I reset the flags, I might bring it down while the boot
process brought it up in the mean time?!
Its the port/interface hard wired to the WASP. I hope that is ok,
because otherwise I don't know how to boot it.
> > + rtnl_unlock();
> > +
> > + if (ret) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error changing interface flags: %d\n", ret);
> > + goto err_send;
> > + }
> > +
> > + avmwasp->ifindex = send_netdev->ifindex;
> > + ret = dev_get_mac_address(&send_sock_addr,
> > + sock_net(avmwasp->send_socket->sk),
> > + avmwasp->loader_port);
> > + if (ret < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error getting mac address: %d\n", ret);
> > + goto err_send;
> > + }
> > +
> > + memset(avmwasp->sendbuf, 0, BUF_SIZE);
> > +
> > + memcpy(send_eh->h_dest, wasp_mac, sizeof(send_eh->h_dest));
> > + send_eh->h_proto = ETHER_TYPE_ATH_ECPS_FRAME;
> > + memcpy(send_eh->h_source, send_sock_addr.sa_data,
> > + sizeof(send_eh->h_source));
> > +
> > + start_time = ktime_get_seconds();
> > +
> > + while (!done) {
> > + current_time = ktime_get_seconds();
> > + if ((current_time - start_time) > SEND_LOOP_TIMEOUT_SECONDS) {
>
> Isn't the socket io operations in this loop blocking? What prevents you
> from being stuck in one of those forever?
>
I have set send and receive timeout to 10 seconds. This works. I
tested it by not
copying the local interface mac, so the packets from WASP never arrive. In
combination with the check above, the loop breakout will always work after the
60 seconds I set for SEND_LOOP_TIMEOUT_SECONDS.
> > + dev_err(&avmwasp->pdev->dev,
>
> A local copy of dev would be nice to shorten these expressions.
>
> > + "Waiting for packet from WASP timed out.\n");
> > + ret = -EFAULT;
> > + goto err_send;
> > + }
> > +
> > + memset(&recv_vec, 0, sizeof(recv_vec));
> > + memset(&recv_socket_hdr, 0, sizeof(recv_socket_hdr));
> > + recv_vec.iov_base = avmwasp->recvbuf;
> > + recv_vec.iov_len = BUF_SIZE;
> > + numbytes = kernel_recvmsg(avmwasp->recv_socket,
> > + &recv_socket_hdr, &recv_vec, 1,
> > + BUF_SIZE, 0);
>
> Can you please help me understand how you know that the read message is
> from the correct sender and that it's a firmware load message?
>
There is one interface of the switch which is not externalized (fixed port)
and where the wasp is attached to. The socket is bound to that interface
using setsockopt.
I have used the receive socket as the only socket now. When it is created,
it uses htons(ETHER_TYPE_ATH_ECPS_FRAME), this is the ethernet
frame type that WASP netboot firmware sends and expects. So this is the
filter and the reason, that I never received any other packet type in the loop
during my tests, even when attaching the device to my local network when
using the rmmod and modprobe commands for testing.
> > +
> > + if (numbytes < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error receiving any packet or timeout: %d\n",
> > + numbytes);
> > + ret = -EFAULT;
> > + goto err_send;
> > + }
> > +
> > + if (numbytes < (sizeof(struct ethhdr) + WASP_HEADER_LEN)) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Packet too small, discard and continue.\n");
> > + continue;
> > + }
> > +
> > + if (recv_eh->h_proto != ETHER_TYPE_ATH_ECPS_FRAME)
> > + continue;
> > +
> > + memcpy(wasp_mac, recv_eh->h_source, sizeof(wasp_mac));
> > + memset(&avmwasp->s_packet, 0, sizeof(avmwasp->s_packet));
> > +
> > + if (packet->packet_start == PACKET_START) {
>
> And if the received message doesn't start with PACKET_START you simply
> discard it?
>
Yes, the default for the switch is continue, so it goes back to the
top of the loop,
but it will exit the loop after 60 seconds as mentioned above. This
actually never
occurred so far.
> > + switch (packet->response) {
> > + case RESP_DISCOVER:
> > + packet_counter = 0;
> > + firmware = avmwasp->linux_blob->data;
> > + firmware_end = firmware
> > + + avmwasp->linux_blob->size;
> > +
> > + chunk_counter = 1;
> > + num_chunks =
> > + avmwasp->linux_blob->size / CHUNK_SIZE;
> > + if (avmwasp->linux_blob->size % CHUNK_SIZE != 0)
> > + num_chunks++;
>
> DIV_ROUND_UP()
>
> > + break;
>
> Please indent the break to match the code block.
>
> > + case RESP_OK:
> > + /* got reply send next packet */
>
> So when you receive RESP_DISCOVER or RESP_OK you will do the second half
> of this function and send out a chunk of data?
>
Yes. Earlier I have put the second half into RESP_OK, but that did not work.
> Seems like this would be better represented by falling through from the
> RESP_DISCOVER and put the send logic in RESP_OK.
>
I see, so doing RESP_DISCOVER without break; and then start with RESP_OK
and the logic there?
> > + break;
> > + case RESP_ERROR:
> > + dev_err(&avmwasp->pdev->dev,
> > + "Received an WASP error packet!\n");
> > + ret = -EFAULT;
> > + goto err_send;
> > + break;
> > + case RESP_STARTING:
> > + done = 1;
> > + ret = 0;
> > + continue;
> > + break;
> > + default:
> > + dev_err(&avmwasp->pdev->dev,
> > + "Unknown packet! Continue.\n");
> > + continue;
> > + break;
> > + }
> > +
> > + if (packet_counter == 0) {
> > + memcpy(avmwasp->s_packet.payload, &m_load_addr,
> > + sizeof(m_load_addr));
> > + data_offset = sizeof(m_load_addr);
> > + } else {
> > + data_offset = 0;
> > + }
> > +
> > + if (firmware < firmware_end) {
>
> firmware and firmware_end are uninitialized if you get here without
> first reveiving a RESP_DISCOVER.
>
> > + if ((firmware_end - firmware) >= CHUNK_SIZE)
> > + read = CHUNK_SIZE;
> > + else
> > + read = firmware_end - firmware;
> > + memcpy(&avmwasp->s_packet.payload[data_offset],
>
> s_packet isn't used outside this loop, so why is i statically part of
> the avmwasp struct? Why aren't these various properties just local
> variables?
>
> > + firmware, read);
> > + firmware = firmware + CHUNK_SIZE;
> > +
> > + avmwasp->s_packet.packet_start = PACKET_START;
> > + if (chunk_counter == num_chunks) {
> > + avmwasp->s_packet.response =
> > + CMD_START_FIRMWARE;
> > + memcpy(&avmwasp->s_packet.payload
> > + [data_offset + read],
> > + &m_load_addr, sizeof(m_load_addr));
>
> So m_load_addr goes in the first 4 bytes and the last 4 bytes of the
> message?
>
Yes, its the kernel start address, whereas the addresses used for mdio
are unmapped addresses.
> > + data_offset += sizeof(m_load_addr);
> > + } else {
> > + avmwasp->s_packet.command =
> > + CMD_FIRMWARE_DATA;
> > + }
> > + avmwasp->s_packet.counter = packet_counter;
> > +
> > + memcpy(avmwasp->sendbuf + sizeof(struct ethhdr),
> > + avmwasp->s_packet.data,
> > + WASP_HEADER_LEN + read + data_offset);
> > + send_len = sizeof(struct ethhdr)
> > + + WASP_HEADER_LEN + read + data_offset;
> > + send_socket_address.sll_halen = ETH_ALEN;
> > + send_socket_address.sll_ifindex =
> > + avmwasp->ifindex;
>
> This doesn't seem to change within the loop, can't this be prepared
> outside the loop?
>
I have tried that, then the send did not work, but I have not checked why.
I will do a hexdump before and after and see what has changed, maybe
just a field needs to be reset.
> > +
> > + memset(&send_vec, 0, sizeof(send_vec));
> > + send_vec.iov_len = send_len;
> > + send_vec.iov_base = avmwasp->sendbuf;
> > +
> > + memset(&send_socket_hdr, 0,
> > + sizeof(send_socket_hdr));
> > + send_socket_hdr.msg_name = (struct sockaddr *)
> > + &send_socket_address;
> > + send_socket_hdr.msg_namelen =
> > + sizeof(struct sockaddr_ll);
>
> Same as send_socket_address.
>
> > +
> > + ret = kernel_sendmsg(avmwasp->send_socket,
> > + &send_socket_hdr,
> > + &send_vec,
> > + 1, send_len);
> > + if (ret < 0) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "Error sending to WASP %d\n",
> > + ret);
> > + goto err_send;
> > + }
> > +
> > + packet_counter += COUNTER_INCR;
>
> Isn't packet_counter always 4 * (chunk_counter - 1)? Why the factor 4?
> Can the two counters be consolidated?
>
> > + chunk_counter++;
> > + }
> > + }
> > + }
> > +
> > +err_send:
> > + avmwasp->send_socket->ops->release(avmwasp->send_socket);
> > +err_recv:
> > + avmwasp->recv_socket->ops->release(avmwasp->recv_socket);
> > +err:
> > + return ret;
> > +}
> > +
> > +/**
> > + * avm_wasp_rproc_start() - start the remote processor
> > + * @rproc: pointer to the rproc structure
> > + *
> > + * Starts the remote processor by turning it on using the startup
> > + * gpio and initiating the reset process using the reset_gpio.
> > + * After that the status is checked if poweron and reset were
> > + * successful.
> > + * As the first step, the network boot firmware is tried to be loaded
> > + * and started.
> > + * As a second step, the initramfs image is tried to be loaded
> > + * and started.
> > + *
> > + * Return: 0 on Success, -19 or return code from the called function
> > + * if any other error occurred in the process of starting and loading
> > + * the firmware files to the WASP processor
> > + */
> > +static int avm_wasp_rproc_start(struct rproc *rproc)
> > +{
> > + struct avm_wasp_rproc *avmwasp = rproc->priv;
> > + int ret;
> > +
> > + gpio_set_value(avmwasp->startup_gpio,
> > + (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> > + 0 : 1);
>
> As I say below, gpiod_get() will grab you a reference to the gpio and
> based on the DT flags it will handle "active high" vs "active low" for
> you, all you need to pass here is "is it high or low" (1 or 0).
>
> > + mdelay(WASP_WAIT_SLEEP);
> > + gpio_set_value(avmwasp->reset_gpio,
> > + (avmwasp->r_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> > + 1 : 0);
> > + mdelay(WASP_WAIT_SLEEP);
> > + gpio_set_value(avmwasp->reset_gpio,
> > + (avmwasp->r_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> > + 0 : 1);
> > + mdelay(WASP_WAIT_SLEEP);
> > +
> > + avmwasp->mdio_bus = mdio_find_bus(avmwasp->mdio_bus_id);
> > + if (!avmwasp->mdio_bus) {
> > + dev_err(&avmwasp->pdev->dev,
> > + "wasp-netboot-mdio bus not found\n");
> > + return -ENODEV;
> > + }
> > +
> > + ret = avm_wasp_netboot_load_firmware(avmwasp);
> > + if (ret) {
> > + put_device(&avmwasp->mdio_bus->dev);
> > + return ret;
>
> How about putting the chip back in reset here?
>
> > + }
> > +
> > + put_device(&avmwasp->mdio_bus->dev);
> > +
> > + ret = avm_wasp_load_initramfs_image(avmwasp);
> > + if (ret)
>
> And here?
>
> > + return ret;
> > +
> > + return 0;
>
> if (ret)
> return ret;
> return 0;
>
> Can succinctly be written "return ret;"
>
> > +}
> > +
> > +/**
> > + * avm_wasp_rproc_stop() - stop the remote processor
> > + * @rproc: pointer to the rproc structure
> > + *
> > + * To stop the remote processor just the startup gpio is set to 0
> > + * and the WASP processor is powered off
> > + *
> > + * Return: 0 on Success
> > + */
> > +static int avm_wasp_rproc_stop(struct rproc *rproc)
> > +{
> > + struct avm_wasp_rproc *avmwasp = rproc->priv;
> > +
> > + gpio_set_value(avmwasp->startup_gpio,
> > + (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> > + 1 : 0);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * avm_wasp_rproc_load() - noop to avoid the ELF binary defaults
> > + * @rproc: pointer to the rproc structure
> > + * @fw: pointer to firmware struct
> > + *
> > + * If a load function is not defined in the rproc_ops, then all the settings
> > + * like checking the firmware binary will default to ELF checks, which fail
> > + * in case of the bootable and compressed initramfs image for WASP.
> > + * Furthermore during boot its just required to send the firmware to the WASP
> > + * processor, its not required to keep it in local memory, as the WASP SoC
> > + * has its own memory.
> > + *
> > + * Return: Always 0
> > + */
> > +static int avm_wasp_rproc_load(struct rproc *rproc, const struct firmware *fw)
>
> If you have to load all your firmware in start() we should come up with
> a different way to signal that the default ELF loader should be used, so
> that you can skip specifying load().
>
I have moved the set of the firmware to the priv to this method and removed
the avm_wasp_rproc_boot_addr and this works.
I also removed the netboot firmware load from probe and put it into the
netboot method which also eliminated some of the checks.
> > +{
> > + return 0;
> > +}
> > +
> > +/**
> > + * avm_wasp_rproc_boot_addr() - store fw from framework in priv
> > + * @rproc: pointer to the rproc structure
> > + * @fw: pointer to firmware struct
> > + *
> > + * Even though firmware files can be loaded without the remote processor
> > + * framework, it expects at least one firmware file.
> > + * This function stores the initramfs image that is loaded by the remote
> > + * processor framework during boot process into the priv for access by
> > + * the initramfs load function avm_wasp_load_initramfs_image().
> > + *
> > + * Return: Address of initramfs image
> > + */
> > +static u64 avm_wasp_rproc_boot_addr(struct rproc *rproc,
> > + const struct firmware *fw)
> > +{
> > + struct avm_wasp_rproc *avmwasp = rproc->priv;
> > +
> > + avmwasp->linux_blob = fw;
> > +
> > + return (u64)((u32)fw->data);
>
> No, the boot_addr should denote that address where the remote processor
> is supposed to start executing code from. This is the lower 32 bits of a
> virtual address on the Linux side, and shortly after returning from this
> function fw->data will be released - making this a dangling "pointer".
>
> > +}
> > +
> > +static const struct rproc_ops avm_wasp_rproc_ops = {
> > + .start = avm_wasp_rproc_start,
> > + .stop = avm_wasp_rproc_stop,
> > + .load = avm_wasp_rproc_load,
> > + .get_boot_addr = avm_wasp_rproc_boot_addr,
> > +};
> > +
> > +static int avm_wasp_rproc_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct avm_wasp_rproc *avmwasp;
> > + const char *fw_name;
> > + struct rproc *rproc;
> > + struct device_node *root_node;
> > + int ret;
> > + u32 phandle;
> > + char *model;
> > +
> > + root_node = of_find_node_by_path("/");
> > + if (!root_node) {
> > + dev_err(dev, "No root node in device tree.\n");
> > + ret = -EFAULT;
> > + goto err;
> > + }
> > +
> > + ret = of_property_read_string_index(root_node, "compatible",
> > + 0, (const char **)&model);
> > + of_node_put(root_node);
> > + if (ret) {
> > + dev_err(dev, "No model in device tree.\n");
> > + goto err;
> > + }
> > +
> > + /* check model of host device to determine WASP SoC type */
> > + if (strstr(model, "3390")) {
> > + m_model = MODEL_3390;
>
> Don't look at the top-level compatible, give your remoteproc node a
> specific compatible and use .data in the of_device_id and
> device_get_match_data() to get the right MODEL_*.
>
> > + } else if (strstr(model, "490")) {
> > + m_model = MODEL_X490;
> > + } else {
> > + dev_err(dev, "No WASP on device.\n");
> > + ret = -EPERM;
> > + goto err;
> > + }
> > +
> > + ret = of_property_read_string(dev->of_node, "wasp-initramfs-image",
> > + &fw_name);
> > + if (ret) {
> > + dev_err(dev, "No initramfs image for WASP filename given\n");
> > + goto err;
> > + }
> > +
> > + rproc = devm_rproc_alloc(dev, "avm,wasp", &avm_wasp_rproc_ops,
> > + fw_name, sizeof(*avmwasp));
> > + if (!rproc) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + rproc->auto_boot = true;
> > +
> > + avmwasp = rproc->priv;
> > + avmwasp->rproc = rproc;
> > + avmwasp->pdev = pdev;
> > +
> > + ret = of_property_read_string(dev->of_node, "ath9k-firmware",
> > + &fw_name);
> > + if (ret) {
> > + dev_err(dev, "No ath9k firmware filename given\n");
> > + goto err;
> > + }
> > +
> > + ret = avm_wasp_firmware_request(avmwasp, fw_name);
> > + if (ret) {
> > + dev_err(dev, "Could not load ath9k firmware\n");
> > + goto err;
> > + }
> > + avm_wasp_firmware_release(avmwasp);
>
> You shouldn't attempt to load the firmware from your probe, the idea is
> that remoteproc will load "the firmware" and call load() for you to copy
> it in place and then call start() to make your core execute the loaded
> firmware.
>
> It seems like you have a bunch of firmware to load at start() time, but
> I don't think it's correct to try-load the firmware here and fail
> probe() if it's not in place.
>
After the load of caldata and ath9k eeprom, they are in the file system.
If they are not there, similar to other wifi drivers like the ath10k, a load
script is triggered, which will extract those files from the hardware's
bootloader partition.
But I wonder if I need to wait for initramfs here, but remoteproc seems
to be pretty late loaded.
Its only needed once, that was the reason for putting it into probe.
But I have removed failing when they are not there. Should the failing
then just be a dev_warn?
Should the loads still be put somewhere else than probe?
> > + if (m_model == MODEL_X490) {
> > + ret = of_property_read_string(dev->of_node, "ath10k-caldata",
> > + &fw_name);
> > + if (ret) {
> > + dev_err(dev, "No ath10k caldata filename given\n");
> > + goto err;
> > + }
> > +
> > + ret = avm_wasp_firmware_request(avmwasp, fw_name);
> > + if (ret) {
> > + dev_err(dev, "Could not load ath10k caldata\n");
> > + goto err;
> > + }
> > + avm_wasp_firmware_release(avmwasp);
> > + }
> > +
> > + ret = of_property_read_u32(dev->of_node, "wasp-initramfs-port",
> > + &phandle);
> > + if (ret) {
> > + dev_err(dev, "No wasp-initramfs-port given\n");
> > + goto err;
> > + } else {
>
> There's no need for else here, as your if-case returns a failure.
>
> > + struct device_node *child = of_find_node_by_phandle(phandle);
> > +
> > + if (!child) {
> > + dev_err(dev, "Get wasp-initramfs-port child failed\n");
> > + ret = -ENODEV;
> > + goto err;
> > + } else {
> > + ret = of_property_read_string(child, "label",
> > + (const char **)
> > + &avmwasp->loader_port);
> > + of_node_put(child);
> > + if (ret) {
> > + dev_err(dev,
> > + "Get wasp-port-label failed\n");
> > + goto err;
> > + }
> > + }
> > + }
> > +
> > + ret = of_property_read_u32(dev->of_node, "wasp-netboot-mdio",
> > + &phandle);
> > + if (ret) {
> > + dev_err(dev, "No wasp-netboot-mdio given\n");
> > + goto err;
> > + } else {
> > + struct device_node *mdio_node =
> > + of_find_node_by_phandle(phandle);
> > +
> > + if (!mdio_node) {
> > + dev_err(dev, "Get wasp-netboot-mdio failed\n");
> > + ret = -ENODEV;
> > + goto err;
> > + } else {
> > + avmwasp->mdio_bus = of_mdio_find_bus(mdio_node);
> > + of_node_put(mdio_node);
> > + if (!avmwasp->mdio_bus) {
> > + dev_err(dev, "mdio bus not found\n");
> > + ret = -ENODEV;
> > + goto err;
> > + }
> > + avmwasp->mdio_bus_id = avmwasp->mdio_bus->id;
> > + put_device(&avmwasp->mdio_bus->dev);
>
> Why are you releasing the refcount of the device that you just found?
> Shouldn't this be held until you're no longer referencing it?
>
i thought I just save the mdio bus id to not having to do the of_... lookup
somewhere else and free it. Only reference it, when its needed and not
through the lifetime of the driver. After the WASP is started, the mdio
bus is not required anymore. Not even to stop it.
Should I move the lookup into the rproc_start method or do it otherwise?
> > + }
> > + }
> > +
> > + avmwasp->startup_gpio = of_get_named_gpio_flags(dev->of_node,
> > + "startup-gpio",
> > + 0,
> > + &avmwasp->s_gpio_flg);
> > + if (!gpio_is_valid(avmwasp->startup_gpio)) {
> > + dev_err(dev, "Request wasp-startup gpio failed\n");
> > + ret = -ENODEV;
> > + goto err;
> > + } else {
> > + ret = devm_gpio_request_one(dev, avmwasp->startup_gpio,
> > + (avmwasp->s_gpio_flg &
> > + OF_GPIO_ACTIVE_LOW) ?
> > + GPIOF_OUT_INIT_LOW :
> > + GPIOF_OUT_INIT_HIGH,
> > + "wasp-startup");
> > +
> > + if (ret) {
> > + dev_err(dev, "get wasp-startup gpio failed\n");
> > + goto err;
> > + }
> > + }
>
> avmwasp->startup_gpio = devm_gpio_get(dev, "startup", GPIOD_OUT_LOW);
> if (IS_ERR(avmwasp->startup_gpio)) {
> ret = dev_err_probe(dev, PTR_ERR(avmwasp->startup_gpio), "failed to get startup gpio\n");
> goto err;
> }
>
> Would do all this, then depending on the gpio being specified
> GPIO_ACTIVE_HIGH or LOW gpio_set_value() will "flip" the value to make
> sure that 1 is active and 0 is inactive.
>
> I.e. you don't need s_gpio_flg or r_gpio_flg or the conditional in all
> of your gpio_set_value().
>
> > +
> > + avmwasp->reset_gpio = of_get_named_gpio_flags(dev->of_node,
> > + "reset-gpio",
> > + 0,
> > + &avmwasp->r_gpio_flg);
> > + if (!gpio_is_valid(avmwasp->reset_gpio)) {
> > + dev_err(dev, "Request wasp-reset gpio failed\n");
> > + ret = -ENODEV;
> > + goto err_free_startup_gpio;
> > + } else {
> > + ret = devm_gpio_request_one(dev, avmwasp->reset_gpio,
> > + (avmwasp->r_gpio_flg &
> > + OF_GPIO_ACTIVE_LOW) ?
> > + GPIOF_OUT_INIT_LOW :
> > + GPIOF_OUT_INIT_HIGH,
> > + "wasp-reset");
> > +
> > + if (ret) {
> > + dev_err(dev, "get wasp-reset gpio failed\n");
> > + goto err_free_startup_gpio;
> > + }
> > + }
> > +
> > + ret = of_property_read_string(dev->of_node, "wasp-netboot-firmware",
> > + (const char **)
> > + &avmwasp->netboot_firmware);
> > + if (ret) {
> > + dev_err(dev, "No WASP network boot firmware filename given\n");
> > + goto err_free_reset_gpio;
> > + }
> > +
> > + ret = request_firmware_direct((const struct firmware **)
> > + &avmwasp->eeprom_blob, avmwasp->netboot_firmware, dev);
>
> As above, I don't think you should request firmware here.
>
I have moved it to the rproc start method.
> > + if (ret) {
> > + dev_err(dev, "Could not load WASP network boot firmware\n");
> > + goto err_free_reset_gpio;
> > + }
> > +
> > + if (avmwasp->eeprom_blob->size > 0xffff) {
> > + dev_err(dev, "WASP network boot firmware too big\n");
> > + ret = -EINVAL;
> > + goto err_free_reset_gpio;
> > + }
> > +
> > + avm_wasp_firmware_release(avmwasp);
> > +
> > + dev_set_drvdata(dev, rproc);
>
> platform_set_drvdata()
>
> > +
> > + ret = devm_rproc_add(dev, rproc);
> > + if (ret) {
> > + dev_err(dev, "rproc_add failed\n");
> > + goto err_free_reset_gpio;
> > + }
> > +
> > + return 0;
> > +
> > +err_free_reset_gpio:
> > + devm_gpio_free(&avmwasp->pdev->dev, avmwasp->reset_gpio);
> > + gpio_set_value(avmwasp->startup_gpio,
> > + (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> > + 1 : 0);
> > +err_free_startup_gpio:
> > + devm_gpio_free(&avmwasp->pdev->dev, avmwasp->startup_gpio);
>
> The purpose of using devres allocations is that they will be
> automatically freed.
>
> > +err:
> > + return ret;
> > +}
> > +
> > +static int avm_wasp_rproc_remove(struct platform_device *pdev)
> > +{
> > + struct rproc *rproc = platform_get_drvdata(pdev);
> > + struct avm_wasp_rproc *avmwasp = rproc->priv;
> > +
> > + gpio_set_value(avmwasp->startup_gpio,
> > + (avmwasp->s_gpio_flg & OF_GPIO_ACTIVE_LOW) ?
> > + 1 : 0);
> > + mdelay(WASP_WAIT_SLEEP);
> > + devm_gpio_free(&avmwasp->pdev->dev, avmwasp->startup_gpio);
> > + devm_gpio_free(&avmwasp->pdev->dev, avmwasp->reset_gpio);
>
> As soon as you return from this function any devm allocated resources
> will be released. So there's no need for doing that here.
>
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int avm_wasp_rpm_suspend(struct device *dev)
>
> Unused.
>
> > +{
> > + return -EBUSY;
> > +}
> > +
> > +static int avm_wasp_rpm_resume(struct device *dev)
>
> Unused.
>
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > +static const struct of_device_id avm_wasp_rproc_of_match[] = {
> > + { .compatible = "avm,wasp", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, avm_wasp_rproc_of_match);
> > +
> > +static struct platform_driver avm_wasp_rproc_driver = {
> > + .probe = avm_wasp_rproc_probe,
> > + .remove = avm_wasp_rproc_remove,
> > + .driver = {
> > + .name = "avm_wasp_rproc",
> > + .of_match_table = avm_wasp_rproc_of_match,
> > + },
> > +};
> > +
> > +module_platform_driver(avm_wasp_rproc_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("AVM WASP remote processor boot driver");
> > +MODULE_AUTHOR("Daniel Kestrel <kestrelseventyfour@gmail.com>");
> > diff --git a/drivers/remoteproc/avm_wasp.h b/drivers/remoteproc/avm_wasp.h
>
> Are any of these definitions going to be used outside avm_wasp.c?
> If not they would be better to have at top of the c-file.
>
> Regards,
> Bjorn
>
> > new file mode 100644
> > index 000000000000..d0a4542b3420
> > --- /dev/null
> > +++ b/drivers/remoteproc/avm_wasp.h
> > @@ -0,0 +1,95 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2019-2020 Andreas Böhler
> > + * Copyright (c) 2021-2022 Daniel Kestrel
> > + */
> > +
> > +#ifndef __AVM_WASP_H
> > +#define __AVM_WASP_H
> > +
> > +#define WASP_CHUNK_SIZE 14
> > +#define M_REGS_WASP_INDEX_MAX 7
> > +
> > +#define WASP_ADDR 0x07
> > +#define WASP_TIMEOUT_COUNT 1000
> > +#define WASP_WAIT_TIMEOUT_COUNT 20
> > +
> > +#define WASP_WRITE_SLEEP_US 20000
> > +#define WASP_WAIT_SLEEP 100
> > +#define WASP_POLL_SLEEP_US 200
> > +#define WASP_BOOT_SLEEP_US 20000
> > +
> > +#define WASP_RESP_RETRY 0x0102
> > +#define WASP_RESP_OK 0x0002
> > +#define WASP_RESP_WAIT 0x0401
> > +#define WASP_RESP_COMPLETED 0x0000
> > +#define WASP_RESP_READY_TO_START 0x0202
> > +#define WASP_RESP_STARTING 0x00c9
> > +
> > +#define WASP_CMD_SET_PARAMS 0x0c01
> > +#define WASP_CMD_SET_CHECKSUM_3390 0x0801
> > +#define WASP_CMD_SET_CHECKSUM_X490 0x0401
> > +#define WASP_CMD_SET_DATA 0x0e01
> > +#define WASP_CMD_START_FIRMWARE_3390 0x0201
> > +#define WASP_CMD_START_FIRMWARE_X490 0x0001
> > +#define WASP_CMD_START_FIRMWARE2_X490 0x0101
> > +
> > +static const u32 start_addr = 0xbd003000;
> > +static const u32 exec_addr = 0xbd003000;
> > +
> > +static u16 m_regs_wasp[] = {0x0, 0x2, 0x4, 0x6, 0x8, 0xA, 0xC, 0xE};
> > +
> > +static const char mac_data[WASP_CHUNK_SIZE] = {0xaa, 0xaa, 0xaa, 0xaa, 0xaa,
> > + 0xaa, 0x04, 0x20, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00};
> > +
> > +enum {
> > + MODEL_3390,
> > + MODEL_X490,
> > + MODEL_UNKNOWN
> > +} m_model = MODEL_UNKNOWN;
> > +
> > +#define ETHER_TYPE_ATH_ECPS_FRAME 0x88bd
> > +#define BUF_SIZE 1056
> > +#define COUNTER_INCR 4
> > +#define SEND_LOOP_TIMEOUT_SECONDS 60
> > +
> > +#define MAX_PAYLOAD_SIZE 1028
> > +#define CHUNK_SIZE 1024
> > +#define WASP_HEADER_LEN 14
> > +
> > +#define PACKET_START 0x1200
> > +#define CMD_FIRMWARE_DATA 0x0104
> > +#define CMD_START_FIRMWARE 0xd400
> > +
> > +#define RESP_DISCOVER 0x0000
> > +#define RESP_CONFIG 0x1000
> > +#define RESP_OK 0x0100
> > +#define RESP_STARTING 0x0200
> > +#define RESP_ERROR 0x0300
> > +
> > +enum {
> > + DOWNLOAD_TYPE_UNKNOWN = 0,
> > + DOWNLOAD_TYPE_FIRMWARE,
> > + DOWNLOAD_TYPE_CONFIG
> > +} m_download_type = DOWNLOAD_TYPE_UNKNOWN;
> > +
> > +static const u32 m_load_addr = 0x81a00000;
> > +
> > +static char wasp_mac[] = {0x00, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa};
> > +
> > +struct wasp_packet {
> > + union {
> > + u8 data[MAX_PAYLOAD_SIZE + WASP_HEADER_LEN];
> > + struct __packed {
> > + u16 packet_start;
> > + u8 pad_one[5];
> > + u16 command;
> > + u16 response;
> > + u16 counter;
> > + u8 pad_two;
> > + u8 payload[MAX_PAYLOAD_SIZE];
> > + };
> > + };
> > +} __packed;
> > +
> > +#endif /* __AVM_WASP_H */
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-23 14:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-21 13:54 [PATCH 3/3] remoteproc: Add AVM WASP driver Daniel Kestrel
2022-02-21 17:43 ` kernel test robot
2022-02-21 18:34 ` kernel test robot
2022-02-22 0:30 ` Bjorn Andersson
2022-02-23 14:58 ` Kestrel seventyfour
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.