On Tue, Feb 5, 2013 at 6:32 PM, Grant Likely <[email protected]> wrote: > On Fri, 18 Jan 2013 10:03:28 +0100, Thomas De Schampheleire > <[email protected]> wrote: >> Hi, >> >> The Freescale eSPI controller driver is broken in several ways. I already >> attempted to fix this with a patch many months back. The patch works for >> me, but never got feedback from the original author. >> (see https://patchwork.kernel.org/patch/988802/) >> >> The same problems are still present on the current 3.x version of the >> driver. I will now re-investigate the problem, and submit a revised patch >> later. >> >> I have some queries though: >> - When reading from memory devices, the first bytes received may not yet be >> the real contents of memory, but rather to-be-ignored bytes caused by the >> full-duplex nature of the transaction while sending the command and address >> bytes. >> Am I correct in understanding that these to-be-ignored bytes are to be >> transparently passed through to the requester of the transaction (i.e. a >> protocol driver or a userspace application through spidev) and that it's >> the requestor's responsability to know that these bytes are to be ignored? > > Yes > >> >> - While investigating the various problems of the driver, I am adding print >> statements throughout the code, displaying certain variables and buffer >> contents. What should I do with these when the problems are fixed? Remove >> all such statements, or rather, use a mechanism like dev_dbg (or similar) >> and thus keep them in the code? What is your preference? > > It is common to use dev_dbg(). They are turned off by removing #define > DEBUG from the top of the file. As long as they don't impair readability > of the code I would keep them in. > >> - One of the aspects that seems to be broken in the driver is accesses that >> do not have either 8-bits-per-word or 16-bits-per-word, e.g. 7 bpw or 12 >> bpw. The hardware defaults to sending least-significant-bits first, and the >> setting to change this is only allowed for 8 or 16 bpw: >> >> -------- (datasheet) >> Reverse data mode. Determines the receive and transmit character bit order. >> 0 lsb of the character sent and received first >> 1 msb of the character sent and received first-for 8/16 bits data character >> only >> ------- >> >> However, the driver sets this bit (CSMODE_REV) by default, except when the >> mode SPI_LSB_FIRST is explicitly set. This means that a protocol driver >> that requests a 12bpw transaction, will inadvertently cause an illegal mode >> setting in the hardware. It doesn't and shouldn't know of this hardware >> restriction. Is this analysis correct? > > Correct. The master driver needs to work around limitations of the > hardware, or reject them as unsupported. > >> What is the correct way of handling this? Clear the bit in case of a sub-8 >> or sub-16 bpw transaction, and do the bit swapping in software if >> SPI_LSB_FIRST is not explicitly set? > > Probably > >> Or force SPI_LSB_FIRST in these cases, leaving the bit swapping to the >> protocol driver? > > No. it should be abstracted > >> Or disable support for sub-8 and sub-16 bpw transactions? > > Only if the first option isn't feasable.
Thanks Grant for your responses. I'm planning to fixing the driver, I'll send it when I'm done. Best regards, Thomas ------------------------------------------------------------------------------ Free Next-Gen Firewall Hardware Offer Buy your Sophos next-gen firewall before the end March 2013 and get the hardware for free! Learn more. http://p.sf.net/sfu/sophos-d2d-feb _______________________________________________ spi-devel-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/spi-devel-general
