On 10/26/18, Liming Sun <lsun@xxxxxxxxxxxx> wrote: >> -----Original Message----- >> From: arndbergmann@xxxxxxxxx [mailto:arndbergmann@xxxxxxxxx] On >> Behalf Of Arnd Bergmann >> Sent: Thursday, October 25, 2018 11:58 AM >> To: Liming Sun <lsun@xxxxxxxxxxxx> >> Cc: Olof Johansson <olof@xxxxxxxxx>; David Woods >> <dwoods@xxxxxxxxxxxx>; Robin Murphy <robin.murphy@xxxxxxx>; arm- >> soc <arm@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm- >> kernel@xxxxxxxxxxxxxxxxxxx >> Subject: Re: [PATCH v4 1/4] soc: Add TmFifo driver for Mellanox BlueField >> Soc >> >> On 10/24/18, Liming Sun <lsun@xxxxxxxxxxxx> wrote: >> > +struct tmfifo_vdev { >> > + struct virtio_device vdev; /* virtual device */ >> > + u8 status; >> > + u64 features; >> > + union { /* virtio config space */ >> > + struct virtio_console_config cons; >> > + struct virtio_net_config net; >> > + } config; >> > + struct tmfifo_vring vrings[TMFIFO_VRING_NUM]; >> > + u8 *tx_buf; /* tx buffer */ >> > + u32 tx_head; /* tx buffer head */ >> > + u32 tx_tail; /* tx buffer tail */ >> > +}; >> >> I suppose you did this to keep the driver simple, but it seems a >> little inflexible >> to only support two specific device types. Wouldn't we also want e.g. >> 9pfs >> or virtio_blk in some configurations? > > We could definitely add more when needed, which should be straightforward > due to the virtio framework. For now only network and console are supported > and ben been verified. Wouldn't that require a new PCI ID to have the driver on the host side match what this side does? I guess I'll see when you post the other driver. >> > +/* TMFIFO device structure */ >> > +struct tmfifo { >> > + struct tmfifo_vdev *vdev[TMFIFO_VDEV_MAX]; /* virtual devices */ >> > + struct platform_device *pdev; /* platform device */ >> > + struct mutex lock; >> > + void __iomem *rx_base; /* mapped register base */ >> > + void __iomem *tx_base; /* mapped register base */ >> > + int tx_fifo_size; /* number of entries of the Tx FIFO */ >> > + int rx_fifo_size; /* number of entries of the Rx FIFO */ >> > + unsigned long pend_events; /* pending bits for deferred process >> */ >> > + int irq[TM_IRQ_CNT]; /* irq numbers */ >> > + struct work_struct work; /* work struct for deferred process >> */ >> > + struct timer_list timer; /* keepalive timer */ >> > + struct tmfifo_vring *vring[2]; /* current Tx/Rx ring */ >> > +}; >> > + >> > +union tmfifo_msg_hdr { >> > + struct { >> > + u8 type; /* message type */ >> > + __be16 len; /* payload length */ >> > + u8 unused[5]; /* reserved, set to 0 */ >> > + } __packed; >> > + u64 data; >> > +}; >> > + >> > +/* >> > + * Default MAC. >> > + * This MAC address will be read from EFI persistent variable if >> > configured. >> > + * It can also be reconfigured with standard Linux tools. >> > + */ >> > +static u8 tmfifo_net_default_mac[6] = {0x00, 0x1A, 0xCA, 0xFF, 0xFF, >> > 0x01}; >> > + >> >> Is a predefined MAC address better than a random one here? >> >> For DT based systems, we tend to also call of_get_mac_address() >> in order to allow setting a unique address from firmware. > > A predefined default MAC address is simpler in this case, which makes > DHCP or PXE boot easier in development environment. > > For production, the MAC address is stored in persistent UEFI variable > on the eeprom, which is read in function tmfifo_get_cfg_mac() which > calls efi.get_variable() to get the MAC address. Ok, fair enough. Generally speaking the recommended way of doing this is to update the DT properties from eeprom when a network driver has no way to store the mac address itself, but I suppose you always have UEFI anyway, and this also makes it work in the same way across both DT and ACPI. >> > +/* Interrupt handler. */ >> > +static irqreturn_t tmfifo_irq_handler(int irq, void *dev_id) >> > +{ >> > + int i = (uintptr_t)dev_id % sizeof(void *); >> > + struct tmfifo *fifo = dev_id - i; >> > + >> > + if (i < TM_IRQ_CNT && !test_and_set_bit(i, &fifo->pend_events)) >> > + schedule_work(&fifo->work); >> > + >> > + return IRQ_HANDLED; >> > +} >> >> Maybe using a request_threaded_irq() would be a better way to defer >> the handler into IRQ context. > > Not sure if I understand this comment correctly... In this case, the > implemented handler > has some mutex_lock() used, which tries to make the logic simple since > multiple services > (network & console) are sharing the same fifo. Thus schedule_work() is > used. schedule_work() and threaded IRQs are just two different ways of deferring into process context where you can do the mutex_lock(). The effect is almost the same, but work queues can be delayed for a substantial amount of time depending on what other work functions have been queued at the same time, and request_threaded_irq() is the more normal way of doing this specifically for an IRQ handler, probably saving a couple of lines of source code. If you have any kind of real-time requirement, you can also assign a specific realtime priority to that interrupt thread. Arnd
