Grant Likely wrote:
> Oops, I replied to the original version, but missed the subsequent
> versions. Looks like some of my comments still apply though.
> Overall, the patch changes too many things all at once. You should
> look at splitting it up. At the very least the io accessor changes
> should be done in a separate patch.
>
> On Mon, Sep 28, 2009 at 7:22 AM, Richard Röjfors
> <richard.rojf...@mocean-labs.com> wrote:
>> @@ -227,6 +227,21 @@ config SPI_XILINX
>> See the "OPB Serial Peripheral Interface (SPI) (v1.00e)"
>> Product Specification document (DS464) for hardware details.
>>
>> + Or for the DS570, see "XPS Serial Peripheral Interface (SPI)
>> (v2.00b)"
>> +
>> +config SPI_XILINX_OF
>> + tristate "Xilinx SPI controller OF device"
>> + depends on SPI_XILINX && XILINX_VIRTEX
>> + help
>> + This is the OF driver for the SPI controller IP from the Xilinx
>> EDK.
>> +
>> +config SPI_XILINX_PLTFM
>> + tristate "Xilinx SPI controller platform device"
>> + depends on SPI_XILINX
>> + help
>> + This is the platform driver for the SPI controller IP
>> + from the Xilinx EDK.
>> +
>
> Personally, I don't think it is necessary to present these options to
> the user. I think they can be auto-selected depending on CONFIG_OF
> and CONFIG_PLATFORM.
Sure that can be changed, I prefer to to that in an incremental patch after
this one.
>
>> +struct xilinx_spi {
>> + /* bitbang has to be first */
>> + struct spi_bitbang bitbang;
>> + struct completion done;
>> + struct resource mem; /* phys mem */
>> + void __iomem *regs; /* virt. address of the control registers */
>> + u32 irq;
>> + u8 *rx_ptr; /* pointer in the Tx buffer */
>> + const u8 *tx_ptr; /* pointer in the Rx buffer */
>> + int remaining_bytes; /* the number of bytes left to transfer */
>> + /* offset to the XSPI regs, these might vary... */
>> + u8 bits_per_word;
>> + bool big_endian; /* The device could be accessed big or little
>> + * endian
>> + */
>> +};
>> +
>
> Why is the definition of xilinx_spi moved?
I liked the idea of heaving the struct defined in the top of the file.
>> /* Register definitions as per "OPB Serial Peripheral Interface (SPI)
>> (v1.00e)
>> * Product Specification", DS464
>> */
>> -#define XSPI_CR_OFFSET 0x62 /* 16-bit Control Register */
>> +#define XSPI_CR_OFFSET 0x60 /* Control Register */
>>
>> #define XSPI_CR_ENABLE 0x02
>> #define XSPI_CR_MASTER_MODE 0x04
>> @@ -40,8 +53,9 @@
>> #define XSPI_CR_RXFIFO_RESET 0x40
>> #define XSPI_CR_MANUAL_SSELECT 0x80
>> #define XSPI_CR_TRANS_INHIBIT 0x100
>> +#define XSPI_CR_LSB_FIRST 0x200
>>
>> -#define XSPI_SR_OFFSET 0x67 /* 8-bit Status Register */
>> +#define XSPI_SR_OFFSET 0x64 /* Status Register */
>>
>> #define XSPI_SR_RX_EMPTY_MASK 0x01 /* Receive FIFO is empty */
>> #define XSPI_SR_RX_FULL_MASK 0x02 /* Receive FIFO is full */
>> @@ -49,8 +63,8 @@
>> #define XSPI_SR_TX_FULL_MASK 0x08 /* Transmit FIFO is full */
>> #define XSPI_SR_MODE_FAULT_MASK 0x10 /* Mode fault error */
>>
>> -#define XSPI_TXD_OFFSET 0x6b /* 8-bit Data Transmit
>> Register */
>> -#define XSPI_RXD_OFFSET 0x6f /* 8-bit Data Receive
>> Register */
>> +#define XSPI_TXD_OFFSET 0x68 /* Data Transmit Register */
>> +#define XSPI_RXD_OFFSET 0x6C /* Data Receive Register */
>>
>> #define XSPI_SSR_OFFSET 0x70 /* 32-bit Slave Select
>> Register */
>>
>> @@ -70,43 +84,72 @@
>> #define XSPI_INTR_TX_UNDERRUN 0x08 /* TxFIFO was underrun */
>> #define XSPI_INTR_RX_FULL 0x10 /* RxFIFO is full */
>> #define XSPI_INTR_RX_OVERRUN 0x20 /* RxFIFO was overrun */
>> +#define XSPI_INTR_TX_HALF_EMPTY 0x40 /* TxFIFO is half
>> empty */
>>
>> #define XIPIF_V123B_RESETR_OFFSET 0x40 /* IPIF reset register */
>> #define XIPIF_V123B_RESET_MASK 0x0a /* the value to write */
>>
>> -struct xilinx_spi {
>> - /* bitbang has to be first */
>> - struct spi_bitbang bitbang;
>> - struct completion done;
>> +/* to follow are some functions that does little of big endian read and
>> + * write depending on the config of the device.
>> + */
>> +static inline void xspi_write8(struct xilinx_spi *xspi, u32 offs, u8 val)
>> +{
>> + iowrite8(val, xspi->regs + offs + ((xspi->big_endian) ? 3 : 0));
>> +}
>>
>> - void __iomem *regs; /* virt. address of the control registers */
>> +static inline void xspi_write16(struct xilinx_spi *xspi, u32 offs, u16 val)
>> +{
>> + if (xspi->big_endian)
>> + iowrite16be(val, xspi->regs + offs + 2);
>> + else
>> + iowrite16(val, xspi->regs + offs);
>> +}
>>
>> - u32 irq;
>> +static inline void xspi_write32(struct xilinx_spi *xspi, u32 offs, u32 val)
>> +{
>> + if (xspi->big_endian)
>> + iowrite32be(val, xspi->regs + offs);
>> + else
>> + iowrite32(val, xspi->regs + offs);
>> +}
>>
>> - u32 speed_hz; /* SCK has a fixed frequency of speed_hz
>> Hz */
>> +static inline u8 xspi_read8(struct xilinx_spi *xspi, u32 offs)
>> +{
>> + return ioread8(xspi->regs + offs + ((xspi->big_endian) ? 3 : 0));
>> +}
>>
>> - u8 *rx_ptr; /* pointer in the Tx buffer */
>> - const u8 *tx_ptr; /* pointer in the Rx buffer */
>> - int remaining_bytes; /* the number of bytes left to transfer */
>> -};
>> +static inline u16 xspi_read16(struct xilinx_spi *xspi, u32 offs)
>> +{
>> + if (xspi->big_endian)
>> + return ioread16be(xspi->regs + offs + 2);
>> + else
>> + return ioread16(xspi->regs + offs);
>> +}
>> +
>> +static inline u32 xspi_read32(struct xilinx_spi *xspi, u32 offs)
>> +{
>> + if (xspi->big_endian)
>> + return ioread32be(xspi->regs + offs);
>> + else
>> + return ioread32(xspi->regs + offs);
>> +}
>
> Ah, you changed these to functions instead of macros. I like.
> However, as you suggested elsewhere in this thread, you could change
> these to callbacks and then eliminate the if/else statements. I think
> that is the approach you should use. I don't think you need to worry
> about it being slower. Any extra cycles for jumping to a callback
> will be far dwarfed by the number of cycles it takes to complete an
> SPI transfer.
Sure that can be updated. I prefer to do that in an incremental patch, would be
great to get this
big one merged first.
--Richard
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
