Thanks Andy. Please see my response and questions on some of the comments below.
Regards, Liming > -----Original Message----- > From: Andy Shevchenko <[email protected]> > Sent: Wednesday, February 13, 2019 1:11 PM > To: Liming Sun <[email protected]> > Cc: David Woods <[email protected]>; Andy Shevchenko <[email protected]>; > Darren Hart <[email protected]>; Vadim > Pasternak <[email protected]>; Linux Kernel Mailing List > <[email protected]>; Platform Driver <platform-driver- > [email protected]> > Subject: Re: [PATCH v9] platform/mellanox: Add TmFifo driver for Mellanox > BlueField Soc > > On Wed, Feb 13, 2019 at 3:27 PM Liming Sun <[email protected]> wrote: > > > ... > > > +/* Use a union struction for 64-bit little/big endian. */ > > What does this mean? > > > +union mlxbf_tmfifo_data_64bit { > > + u64 data; > > + __le64 data_le; > > +}; The purpose is to send 8 bytes into the FIFO without data casting in writeq(). Below is the example with the cast. u64 data = 0x1234; __le64 data_le; data_le = cpu_to_le64(data) writeq(*(u64 *)&data_le, fifo->tx_base + MLXBF_TMFIFO_TX_DATA); Below is the alternative trying to use union to avoid the cast. mlxbf_tmfifo_data_64bit u; u.data = 0x1234; u. data_le = cpu_to_le64(u.data); writeq(u.data_le, fifo->tx_base + MLXBF_TMFIFO_TX_DATA); Which way might be better or any other suggestions? > > + > > +/* Message header used to demux data in the TmFifo. */ > > +union mlxbf_tmfifo_msg_hdr { > > + struct { > > + u8 type; /* message type */ > > + __be16 len; /* payload length */ > > + u8 unused[5]; /* reserved, set to 0 */ > > + } __packed; > > It's already packed. No? The '__packed' is needed here. Without it the compiler will make the structure size exceeding 8 bytes which is not desired. >... > > + if (vdev_id == VIRTIO_ID_CONSOLE) > > > + tm_vdev->tx_buf = devm_kmalloc(dev, > > + > > MLXBF_TMFIFO_CONS_TX_BUF_SIZE, > > + GFP_KERNEL); > > Are you sure devm_ suits here? The 'tx_buf' are normal buffer to hold the console output. It seems ok to use devm_kmalloc so it could automatically freed on driver detach. Please correct me if I am wrong. >>... > > + > > + fifo->tx_base = devm_ioremap(&pdev->dev, tx_res->start, > > + resource_size(tx_res)); > > + if (!fifo->tx_base) > > + return -ENOMEM; > > Switch to devm_ioremap_resource(). > However, I think you probably need memremap(). This is device registers accessed by arm64 core. In arm64/include/asm/io.h, several apis are defined the same. #define ioremap(addr, size) __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE)) #define ioremap_nocache(addr, size) __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE)) #define ioremap_wt(addr, size) __ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE)) How about using devm_ioremap_nocache()? It could take advantage of the devm_xx() api. >... > Is it correct? > > -- > With Best Regards, > Andy Shevchenko
