Message-ID: <AM6PR04MB6407EAA64AC011F2F1F431998FD89@AM6PR04MB6407.eurprd04.prod.outlook.com>
Date:   Fri, 27 May 2022 02:20:50 +0000
From:   Manjunatha Venkatesh <manjunatha.venkatesh@....com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "will@...nel.org" <will@...nel.org>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        "mb@...htnvm.io" <mb@...htnvm.io>,
        "ckeepax@...nsource.cirrus.com" <ckeepax@...nsource.cirrus.com>,
        "mst@...hat.com" <mst@...hat.com>,
        "javier@...igon.com" <javier@...igon.com>,
        "mikelley@...rosoft.com" <mikelley@...rosoft.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "sunilmut@...rosoft.com" <sunilmut@...rosoft.com>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        "rvmanjumce@...il.com" <rvmanjumce@...il.com>,
        Ashish Deshpande <ashish.deshpande@....com>
Subject: RE: [EXT] Re: [PATCH v3] [PATCH v3] uwb: nxp: sr1xx: UWB driver
 support for sr1xx series chip



> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.org>
> Sent: Friday, May 6, 2022 12:51 AM
> To: Manjunatha Venkatesh <manjunatha.venkatesh@....com>
> Cc: linux-kernel@...r.kernel.org; will@...nel.org; axboe@...nel.dk;
> mb@...htnvm.io; ckeepax@...nsource.cirrus.com; mst@...hat.com;
> javier@...igon.com; mikelley@...rosoft.com; jasowang@...hat.com;
> sunilmut@...rosoft.com; bjorn.andersson@...aro.org;
> rvmanjumce@...il.com; Ashish Deshpande <ashish.deshpande@....com>
> Subject: [EXT] Re: [PATCH v3] [PATCH v3] uwb: nxp: sr1xx: UWB driver
> support for sr1xx series chip
> 
> Caution: EXT Email
> 
> On Wed, May 04, 2022 at 10:43:37PM +0530, Manjunatha Venkatesh wrote:
> > Ultra-wideband (UWB) is a short-range wireless communication protocol
> >
> > sr1xx is a new driver that supports the integrated UWB for Nxp SoCs,
> > especially the sr1xx series and depends on the SPI module.
> >
> > sr1xx driver works with Nxp UWB Subsystem(UWBS) which is FiRa
> Complaint.
> 
> What is the Nxp UWBS and where can it be found?  You have custom ioctls
> here with no public user so we really can't take them, right?
> 
 The sr1xx driver work the SR1XX Family of UWBS, and uses UWB Controller
 Interface (UCI).  The corresponding details are available in the FiRa
 Consortium Website (https://www.firaconsortium.org/).
                    The IOCTLs are used to,
                    1. Switch between and control communication to UWBS in
HBCI and UCI Mode
                    2. Reset control from user space need custom IOCTL

> > Corresponding UCI details available in Fira Consortuim website.
> 
> Have a link for this?
> 
Yes, Its available in the FiRa Consortium Website
(https://www.firaconsortium.org/)

> > sr1xx is flash less device  and it requires firmware download on every
> > device boot.
> 
> Too many spaces in that sentence?
> 
> Lots of devices need firmware, that's not a big deal if you are using the
> firmware api, right?  Wait, you are not using that api, so how is the
firwmare
> being downloaded?
> 
The HBCI Mode (sr1xx BootROM Code Interface) is used both to access low
level information from ROM which would be interesting to User Space,
as well as upload new Firmware to the RAM of the UWBS and same
sequence need to follow on every reboot(Since its flash less device).
Having 3 types of communication (HBCI, UCI, linux firmware download)
potentially  
       adds extra complexity in maintenance, and hence limiting the
interfaces.
 

> > Internally driver will handle two modes of operation.
> > 1.HBCI mode (sr1xx BootROM Code Interface)
> >   Firmware download uses HBCI ptotocol packet structure which is
> >   Nxp proprietary,Firmware File(.bin) stored in user space context
> >   and during device init sequence pick the firmware packet in chunk
> >   and send it to the driver with write() api call.
> 
> That's not ok, use the standard in-kernel firmware download api please.
> 
Clarified in previous comment

> >   Firmware acknowledge for every chunk packet sent and same thing
> >   is monitored,in user space code(HAL layer).
> 
> What does a HAL layer have to do with anything here?
>
Re-clarified / briefly mentioned that payload is processed in User Space.  
(details on what is happening in User Space is not coupled with this
discussion. 
Sorry for confusing by introducing HAL layer in this thread).
 
> >   If any error Firmware download sequence will fail and reset the
device.
> >   If firmware download packet sent successfully at the end device will
> >   send device status notification and its indication of device entered
> >   UCI mode.Here after any command/response/notification will follow
> >   UCI packet structure.
> 
> Again, just use the normal fiwmare download logic and you will not need to
> worry about any of the above.  For obvious reasons you don't want us to
> take a custom firmware api for every individual device that Linux supports
as
> that would be crazy :)
> 
Agreed but users of SR1XX UWBS would like to use low level
HBCI Mode of communication as well and hence driver would get more
Complicated if we use 3 different modes of communication 
(UCI, HCI, firmware download).

> > 2.UCI mode (UWB Command interface)
> >   Once Firmware download finishes sr1xx will switch to UCI mode packet
> >   structure.Here this driver exchange command/response between user
> space
> >   and sr1xx device.
> 
> Please have someone proof read this changelog before you resend.
>
Yes, will get it reviewed internally before next patch submission.
 
> >   Any response or notification received from sr1xx through SPI line
> >   will convey to user space.User space(UCI lib) will take care of
> >   UCI parsing logic.
> 
> As I said when we met to talk about this driver, why do you have a custom
> api/interface at all?  Why can you not just use the normal user/kernel api
for
> SPI devices?
> 
> You should not need any read/write/ioctl api for this driver at all, it's
just a
> simple spi driver from what I can tell.  This should not be complex to
> implement at all.
>
The IO Handshake needed with SR1XX Family of SOCs cannot use the RAW SPI
Module's APIs and hence custom APIs are added for communication with the
UWBS,
With this will get required throughput for UWBS use cases to  avoid multiple
round trip between user  and kernel mode.
 
> >   Its IRQ based driver and sr1xx specific irq handshake mechanism logic
> >   implemented to avoid any race condition between write and read
> >   during ranging sequence.
> 
> I do not understand this sentence at all, sorry.
> 
Reworded the description of the patch message.

> >   UCI mode Write is same as HBCI mode sequence whatever command
> received
> >   from user space will send to the sr1xx via SPI line.
> >   In UCI mode read api called first and waiting on the IRQ line status
> >   in order to avoid missing of interrupts after write sequence.
> >
> >   This driver needs dts config update as per the sr1xx data sheet.
> >   Corresponding document will be added in
> >   Documentation/devicetree/bindings/uwb
> 
> We can not take drivers with dts requirements without the dts updates as
> well, as you know.  Please make that the first patch in the series and
properly
> cc: the needed DT maintainers.  For that reason alone I couldn't take this
> patch if I wanted to.
>
Yes, Will commit the device tree document before next driver patch
submission.
 
> > Link:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fr%2F20220315105205.2381997-1-
> manjunatha.venkatesh%40nxp.
> >
> com&amp;data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C8e62ec
> ff820c45
> >
> dd323508da2ee73a4f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7
> C637873
> >
> 867912779711%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV2luM
> >
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&amp;sdata=nlJ
> 91nGGyk
> > CapiWWaze8doyEOIGD37CH36YKCVktkfI%3D&amp;reserved=0
> 
> What is this a link to?
> 
As part of commit message given correct link as shown below but somehow its
showing differently after submission.
Link:
https://lore.kernel.org/r/20220315105205.2381997-1-manjunatha.venkatesh@nxp.
com

> > Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@....com>
> > ---
> 
> You forgot to list what changed from previous versions below the --- line
like
> the documentation asks you to do.  Please fix that for the next
submission.
> My patch bot normally would just reject the change for that reason alone,
> documentation matters :)
> 
> 
Yes, Will correct this in the next patch submission

> >  MAINTAINERS               |   6 +
> >  drivers/misc/Kconfig      |  15 +
> >  drivers/misc/Makefile     |   1 +
> >  drivers/misc/sr1xx.c      | 784
> ++++++++++++++++++++++++++++++++++++++
> >  include/uapi/misc/sr1xx.h |  79 ++++
> >  5 files changed, 885 insertions(+)
> >  create mode 100644 drivers/misc/sr1xx.c  create mode 100644
> > include/uapi/misc/sr1xx.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > edc96cdb85e8..2896d401dbc4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21888,3 +21888,9 @@ S:    Buried alive in reporters
> >  T:   git
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >  F:   *
> >  F:   */
> > +
> > +UWB
> 
> Note, this is NOT generic UWB.  This is a single spi device driver, right?
> 
Yes, Will correct this in the next patch submission

> > +M:   manjunatha.venkatesh@....com
> 
> No real name?
> 
Yes, Will update this in the next patch submission
> > +S:   Maintained
> > +F:   drivers/misc/sr1xx.c
> > +F:   include/uapi/misc/sr1xx.h
> 
> Please read the top of this file for how to correctly find where to place
your
> new entry.
> 
Yes, Will update this in the next patch submission

> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
> > 41d2bb0ae23a..1ca97d168f26 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -483,6 +483,21 @@ config OPEN_DICE
> >
> >         If unsure, say N.
> >
> > +config NXP_UWB
> > +        tristate "Nxp UCI(Uwb Command Interface) protocol driver
support"
> > +        depends on SPI
> > +        help
> > +        This option enables the UWB driver for Nxp sr1xx
> > +        device. Such device supports UCI packet structure,
> > +        FiRa complaint.
> > +
> > +
> > +
> > +        Say Y here to compile support for sr1xx into the kernel or
> > +        say M to compile it as a module. The module will be called
> > +        sr1xx.ko
> > +
> > +
> 
> Why all the extra blank lines?
> 
> And are you sure you indented the help properly?  Why no tabs like the
rest
> of this file?  Did you run checkpatch.pl on your patch first?
> 
>
Yes, run check patch tool but didn't caught any warnings, anyway will cross
check this and will correct it in next patch submission.
 
> >  source "drivers/misc/c2port/Kconfig"
> >  source "drivers/misc/eeprom/Kconfig"
> >  source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
> > 70e800e9127f..bbd4dd17cabc 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -60,3 +60,4 @@ obj-$(CONFIG_XILINX_SDFEC)  += xilinx_sdfec.o
> >  obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
> >  obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.o
> >  obj-$(CONFIG_OPEN_DICE)              += open-dice.o
> > +obj-$(CONFIG_NXP_UWB)                += sr1xx.o
> > diff --git a/drivers/misc/sr1xx.c b/drivers/misc/sr1xx.c new file mode
> > 100644 index 000000000000..100c36031fd2
> > --- /dev/null
> > +++ b/drivers/misc/sr1xx.c
> > @@ -0,0 +1,784 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * SPI driver for UWB SR1xx
> > + * Copyright (C) 2018-2022 NXP.
> > + *
> > + * Author: Manjunatha Venkatesh <manjunatha.venkatesh@....com>  */
> > +#include <linux/kernel.h> #include <linux/miscdevice.h> #include
> > +<linux/module.h> #include <linux/mutex.h> #include <linux/spinlock.h>
> > +#include <linux/delay.h> #include <linux/fs.h> #include
> > +<linux/gpio.h> #include <linux/init.h> #include <linux/interrupt.h>
> > +#include <linux/io.h> #include <linux/irq.h> #include
> > +<linux/jiffies.h> #include <linux/list.h> #include <linux/of_gpio.h>
> > +#include <linux/platform_device.h> #include <linux/device.h> #include
> > +<linux/poll.h> #include <linux/regulator/consumer.h> #include
> > +<linux/sched.h> #include <linux/slab.h> #include <linux/spi/spi.h>
> > +#include <linux/uaccess.h> #include <uapi/misc/sr1xx.h>
> 
> Do you really need all of these?  If so, great, but I think that's way too
many
> for a simple driver like this.
> 
> 
Cross verified this list and only few required from this list and will
update
the same in the next driver patch submission

> > +
> > +static int sr1xx_dev_open(struct inode *inode, struct file *filp) {
> > +     struct sr1xx_dev *sr1xx_dev = container_of(
> > +             filp->private_data, struct sr1xx_dev, sr1xx_device);
> 
> Odd line break, please run checkpatch.pl.
> 
> I'm not going to review the driver contents based on my above comments as
> this needs lots of reworking and most of the code here can go away.
>
Yes, Will cross check these things before next driver patch submission
 
> One meta-comment though, your uapi .h file:
> 
> > --- /dev/null
> > +++ b/include/uapi/misc/sr1xx.h
> > @@ -0,0 +1,79 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only
> > + *
> > + * Header file for UWB sr1xx device
> > + * Copyright (C) 2018-2022 NXP.
> > + *
> > + * Author: Manjunatha Venkatesh <manjunatha.venkatesh@....com>  */
> > +
> > +#include <linux/types.h>
> > +
> > +#define SR1XX_MAGIC 0xEA
> > +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long) #define
> > +SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)
> > +
> > +#define UCI_HEADER_LEN 4
> > +#define HBCI_HEADER_LEN 4
> > +#define UCI_PAYLOAD_LEN_OFFSET 3
> > +
> > +#define UCI_EXT_PAYLOAD_LEN_IND_OFFSET 1 #define
> > +UCI_EXT_PAYLOAD_LEN_IND_OFFSET_MASK 0x80 #define
> > +UCI_EXT_PAYLOAD_LEN_OFFSET 2
> > +
> > +#define SR1XX_TXBUF_SIZE 4200
> > +#define SR1XX_RXBUF_SIZE 4200
> > +#define SR1XX_MAX_TX_BUF_SIZE 4200
> > +
> > +#define MAX_RETRY_COUNT_FOR_IRQ_CHECK 100 #define
> > +MAX_RETRY_COUNT_FOR_HANDSHAKE 1000
> > +
> > +/* Macro to define SPI clock frequency */ #define SR1XX_SPI_CLOCK
> > +16000000L #define WAKEUP_SRC_TIMEOUT (2000)
> > +
> > +/* Maximum UCI packet size supported from the driver */ #define
> > +MAX_UCI_PKT_SIZE 4200
> > +
> > +struct sr1xx_spi_platform_data {
> > +     unsigned int irq_gpio; /* SR1XX will interrupt host for any ntf */
> > +     unsigned int ce_gpio; /* SW reset gpio */
> > +     unsigned int spi_handshake_gpio; /* Host ready to read data */
> > +};
> > +
> > +/* Device specific macro and structure */ struct sr1xx_dev {
> > +     wait_queue_head_t read_wq; /* Wait queue for read interrupt */
> > +     struct spi_device *spi; /* Spi device structure */
> > +     struct miscdevice sr1xx_device; /* Char device as misc driver */
> > +     unsigned int ce_gpio; /* SW reset gpio */
> > +     unsigned int irq_gpio; /* SR1XX will interrupt host for any ntf */
> > +     unsigned int spi_handshake_gpio; /* Host ready to read data */
> > +     bool irq_enabled; /* Flag to indicate disable/enable irq sequence
*/
> > +     bool irq_received; /* Flag to indicate that irq is received */
> > +     spinlock_t irq_enabled_lock; /* Spin lock for read irq */
> > +     unsigned char *tx_buffer; /* Transmit buffer */
> > +     unsigned char *rx_buffer; /* Receive buffer */
> > +     unsigned int write_count; /* Holds nubers of byte written */
> > +     unsigned int read_count; /* Hold nubers of byte read */
> > +     struct mutex
> > +             sr1xx_access_lock; /* Lock used to synchronize read and
write */
> > +     size_t total_bytes_to_read; /* Total bytes read from the device */
> > +     bool is_extended_len_bit_set; /* Variable to check ext payload Len
*/
> > +     bool read_abort_requested; /* Used to indicate read abort request
*/
> > +     bool is_fw_dwnld_enabled; /* Used to indicate fw download mode */
> > +     int mode; /* Indicate write or read mode */
> > +     long timeout_in_ms; /* Wait event interrupt timeout in ms */ };
> > +
> > +/* Power enable/disable and read abort ioctl arguments */ enum {
> > +PWR_DISABLE = 0, PWR_ENABLE, ABORT_READ_PENDING };
> > +
> > +enum spi_status_codes {
> > +     TRANSCEIVE_SUCCESS,
> > +     TRANSCEIVE_FAIL,
> > +     IRQ_WAIT_REQUEST,
> > +     IRQ_WAIT_TIMEOUT
> > +};
> > +
> > +/* Spi write/read operation mode */
> > +enum spi_operation_modes { SR1XX_WRITE_MODE,
> SR1XX_READ_MODE };
> 
> 
> You have loads of things in here that should NOT be exposed to userspace
at
> all (your structure?)
> 
> Does this even build properly if you have the header check build option
> enabled?  I would be amazed if it did.
> 
> Anyway, you do not need a uapi .h file from what I can tell, so it
shouldn't be
> needed for your next version.
> 
Yes, Will remove this header file and move all this contents to .c file
itself in the next patch submission.

> thanks,
> 
> greg k-h

Download attachment "smime.p7s" of type "application/pkcs7-signature" (9591 bytes)

Powered by blists - more mailing lists