Re: [PATCH 1/2] mtd: nand: force NAND_CMD_READID onto 8-bit bus

[Brian Norris <computersforpeace@gmail.com>]

On Thu, Jan 30, 2014 at 04:51:08PM -0300, Ezequiel Garcia wrote:
> On Thu, Jan 30, 2014 at 07:17:29PM +0000, Gupta, Pekon wrote:
> > >From: Brian Norris [mailto:computersforpeace@gmail.com]
> > >diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > >index a719686c9cce..c034dc4224cb 100644
> > >--- a/include/linux/mtd/nand.h
> > >+++ b/include/linux/mtd/nand.h
> > >@@ -832,4 +832,14 @@ static inline bool nand_is_slc(struct nand_chip *chip)
> > > {
> > > 	return chip->bits_per_cell == 1;
> > > }
> > >+
> > >+/**
> > >+ * Check if the opcode's address should be sent only on the lower 8 bits
> > >+ * @command: opcode to check
> > >+ */
> > >+static inline int nand_opcode_8bits(unsigned int command)
> > >+{
> > >+	return command == NAND_CMD_READID;
> > >+}
> > >+
> > >
> > Can 'nand_opcode_8bits, made a macro instead of inline function ?
> > #define IS_8BIT_CMD(cmd)  (unlikely(cmd == NAND_CMD_READID))
> > 
> > Because 'nand_opcode_8bits' is used in nand_command() and nand_command_lp()
> > which is performance critical code (chip->cmd is called multiple times for fetching
> > page data and OOB).

First of all, I really don't think micro-architectural optimizations are
significant at this point. The overhead (if any exists) is likely very
small, especially relative to other sorts of optimization obstacles
(e.g., use of function pointers). But in any case, optimization must be
informed by data, not simply speculation.

> > Though we should expect compiler to treat this inline function
> > same as macro here, But just to be doubly sure for future changes also.
> > 
> 
> I'm not a compiler guru, but I'd be very surprised if the compiler would make
> a difference here, given the function is static, inline and small. Besides,
> an inline function is more readable and type-aware. I'd say it's the Right
> Thing to do.

I agree that, unless there is a significant demonstrable benefit to do
otherwise, static inline is the way to go (for reasons of type safety,
for one).

> Nevertheless, I did a small check on my platform (handbuilt ARM GCC 4.7.2) and
> both macro and inline is compiled into a "cmp" instruction. The unlikely
> doesn't seem to have any effect.

And so we have data. I'm sure there are other data points to consider
(different compilers, different ARCH, nand_opcode_8bits() used in
different contexts, etc.), but it's not worth it IMO.

Thanks,
Brian