Re: [PATCH] SPI: SSP SPI Controller driver
On Wed, Jan 9, 2013 at 5:25 AM, Vinod Koul vinod.k...@intel.com wrote: Also I have some questions on this approach. Is this driver for SSP ip or SPI ip, looks like latter. In both the cases there are some existing drivers in kernel and adding one more IMHO doesnt make sense. What we really need a common core for dw IP and SSP IP (i think pxa uses same stuff). That way lot of code will get reduced from driver +1 on this comment, I didn't even notice :-( Linus Walleij -- Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS, MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft MVPs and experts. ON SALE this month only -- learn more at: http://p.sf.net/sfu/learnmore_122712 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Tue, Dec 18, 2012 at 01:47:40PM +0800, chao bi wrote: Dear Linus, Thanks for your kind comments. Seems you were viewing the 1st version, I've submitted 2nd version and to deliver the 3rd version soon, will include you for review. Was the third version posted? Also I have some questions on this approach. Is this driver for SSP ip or SPI ip, looks like latter. In both the cases there are some existing drivers in kernel and adding one more IMHO doesnt make sense. What we really need a common core for dw IP and SSP IP (i think pxa uses same stuff). That way lot of code will get reduced from driver +#define SRAM_BASE_ADDR 0xfffdc000 Should be passed as resource, se above reasoning for the I2C base address. What happens on next ASIC spin when the engineer move this base offset etc, don't you have any system discovery? This is fix value for Moorestown Medfield platforms as what is declared in the file header. If any hardware change, the address should be changed accordantly. Why do you wnat to change this latter, pls add it as a resouce or since this is a PCI device you can use PCI table driver data. Also why would SSP care about SRAM, I am not sure I follow it?? Lastly if you have dedicated SRAM for your use, it should be in PCI BAR and not hard coded like this!! -- ~Vinod -- Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery and much more. Keep your Java skills current with LearnJavaNow - 200+ hours of step-by-step video tutorials by Java experts. SALE $49.99 this month only -- learn more at: http://p.sf.net/sfu/learnmore_122612 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Tue, Dec 18, 2012 at 6:47 AM, chao bi chao...@intel.com wrote:
Thanks for your kind comments. Seems you were viewing the 1st version, I've
submitted 2nd version and to deliver the 3rd version soon, will include you
for review.
OK I'll look over it as it comes along...
+ WARN(i 0, %d words flush occured\n, i);
WARN really? Why not dev_warn()?
It's suppose that SPI FIFO is empty after each transfer, so call flush()
before each time of transfer, if any remain data in SPI FIFO,
It shows some kinds of Error must be happened in last transfer and
deserves a WARN() to record it.
dev_warning() is recorded, I don't get it.
+static int null_writer(struct ssp_driver_context *drv_context)
+static int null_reader(struct ssp_driver_context *drv_context)
+static int u8_writer(struct ssp_driver_context *drv_context)
+static int u8_reader(struct ssp_driver_context *drv_context)
+static int u16_writer(struct ssp_driver_context *drv_context)
+static int u16_reader(struct ssp_driver_context *drv_context)
+static int u32_writer(struct ssp_driver_context *drv_context)
+static int u32_reader(struct ssp_driver_context *drv_context)
These seem to all be designed to return 0 or 1 and should then be
bool. It seems strange actually, you would expect that such a
function returns the number of bytes or words read/written.
these functions are only used by PIO transfer, its name shows how many
bytes to be written/read, and to return whether write/read is success.
If returns bytes of read/written seems make no sense for PIO transfer,
so I think return type of boot is enough, what do you think?
If the return value is succes/error it should return zero for
success and 0 for error. IIRC these returned 1 on error which
is then wrong. Keep them as int but return something negative
when the function fails please.
+ length = TRUNCATE(drv_context-len,
+ drv_context-rx_fifo_threshold *
drv_context-n_bytes);
(...)
It looks very strange.
Isn't this simply an arithmetic soup construction to say:
length = drv_context-len / (drv_context-rx_fifo_threshold *
drv_context-n_bytes);
I think TRUNCATE() is different:
#define TRUNCATE(x, a) ((x) ~((a)-1))
Yes I got it all wrong. I have some problem with this
because of the name of the function I think.
Example for 32bit arithmetic:
TRUNCATE(0x1010, 0x100) ==
(0x1010 ~(0x0100-1)) ==
(0x1010 ~(0x00FF)) ==
(0x1010 0x00) ==
0x1000
It's quite unintuitive to call something that preserve the
upper bits and discards the lower bits to truncate,
usually we use that word for things like removing
trailing zeroes like writing 0x10 instead of 0x0010.
Should it not be named HIGH_N_BITS()
or something like that?
I have considered addin this to linux/bitops.h:
#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
So in this case you would instead of
y = TRUNCATE(x, a);
Use something like:
y = x BITS(a, 31);
And in that case it's also obvious what happens (IMO)
Uusally likely() / unlikely() micro-optimization is discouraged,
do you have specific performance numbers for using it so
much here?
I haven't done any sort of performance test for likely/unlikely, but per
our test on Medfield Platform, it meets our performance request. By the
way, would you please illustrate why likely/unlikely is discouraged. If
it impacts on performance, maybe we could consider to propose another
enhancement patch to optimize performance, but this should based on
test.
Please consult the following:
http://lwn.net/Articles/420019/
http://lwn.net/Articles/70476/
Quoting Rusty Russell:
Sometimes, unlikely()/likely() help code readability. But generally it
should be considered the register keyword of the 2000's: if the case isn't
ABSOLUTELY CRYSTAL CLEAR, or doesn't show up on benchmarks, distain is
appropriate.
+ /* We can fall here when not using DMA mode */
fall? fail?
I think it's fall
Do you mean fall through?
+ if (drv_context-quirks QUIRKS_BIT_BANGING) {
+ /* Bit banging on the clock is done through */
+ /* DFT which is available through I2C. */
+ /* get base address of I2C_Serbus registers */
+ drv_context-I2C_paddr = 0xff12b000;
What on earth is this?
Note the comment says get base address, you're not getting it at
all, you're hardcoding it. Resources like this should be passed in from
the outside.
Ok, the address is fixed for current platform, I'll define the address
at beginning and here just refer to the macro, if other platform, this
address should be defined as other value.
That really does not answer the question why it is not passed as
a resource from the outside.
+#define MAX_TRAILING_BYTE_RETRY 16
+#define MAX_TRAILING_BYTE_LOOP 100
Max iterations?
+#define DELAY_TO_GET_A_WORD 3
+#define DFLT_TIMEOUT_VAL 500
milliseconds?
It depends on
Re: [PATCH] SPI: SSP SPI Controller driver
On Tue, Dec 11, 2012 at 9:58 AM, chao bi chao...@intel.com wrote: 1. I understand the workqueue in spi core is for driving message transfer, so SPI driver should not create new workqueue for this usage. However, the workqueue created here is not for this usage it's to call back to SPI protocol driver (ifx6x60.c) when DMA data transfer is finished, so it seems not conflict with spi core. Am I right? So a single message can contain several transfers, and if this is some per-transfer DMA thing, it could be valid. I need to go in and look closer at the patch. I've considered trying to also generalize parts of the transfer handling but ran out of energy. Yours, Linus Walleij -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Wed, Nov 21, 2012 at 3:16 AM, chao bi chao...@intel.com wrote:
This patch is to implement SSP SPI controller driver, which has been applied
and
validated on intel Moorestown Medfield platform. The patch are originated by
Ken Mills ken.k.mi...@intel.com and Sylvain Centelles
sylvain.centel...@intel.com,
and to be further developed by Channing chao...@intel.com and Chen Jun
jun.d.c...@intel.com according to their integration validation on
Medfield platform.
Signed-off-by: Ken Mills ken.k.mi...@intel.com
Signed-off-by: Sylvain Centelles sylvain.centel...@intel.com
Signed-off-by: channing chao...@intel.com
Signed-off-by: Chen Jun jun.d.c...@intel.com
OK...
+#ifdef DUMP_RX
So since this #define DUMP_RX is not part of this patch and not of the
kernel at large it's basically an #if 0 and all the code within such
defines should be deleted.
But I guess you have this undocumented feature that the developer
is supposed to hack the file and insert #define DUMP_RX to use it.
Idea: if you want to keep this use the kernel verbose debug system.
In drivers/spi/Kconfig we have:
config SPI_DEBUG
boolean Debug support for SPI drivers
depends on DEBUG_KERNEL
help
Say yes to enable debug messaging (like dev_dbg and pr_debug),
sysfs, and debugfs support in SPI controller and protocol drivers.
So insert something like:
config SPI_VERBOSE_DEBUG
boolean Verbose debug support for SPI drivers
depends on SPI_DEBUG
Modify Makefile to contain:
ccflags-$(CONFIG_SPI_VERBOSE_DEBUG) := -DVERBOSE_DEBUG
Then put the above withing #ifdef CONFIG_SPI_VERBOSE_DEBUG
Then you can use the dev_vdgb() and friends from linux/device.h.
Because I think that's what it is essentially: verbose debugging.
+static void dump_trailer(const struct device *dev, char *buf, int len, int
sz)
+{
+ int tlen1 = (len sz ? len : sz);
+ int tlen2 = ((len - sz) sz) ? sz : (len - sz);
+ unsigned char *p;
+ static char msg[MAX_SPI_TRANSFER_SIZE];
+
+ memset(msg, '\0', sizeof(msg));
+ p = buf;
+ while (p buf + tlen1)
+ sprintf(msg, %s%02x, msg, (unsigned int)*p++);
+
+ if (tlen2 0) {
+ sprintf(msg, %s ., msg);
+ p = (buf+len) - tlen2;
+ while (p buf + len)
+ sprintf(msg, %s%02x, msg, (unsigned int)*p++);
+ }
+
+ dev_info(dev, DUMP: %p[0:%d ... %d:%d]:%s, buf, tlen1 - 1,
+ len-tlen2, len - 1, msg);
dev_vdbg().
(...)
+static inline u32 is_tx_fifo_empty(struct ssp_driver_context *drv_context)
Change return type to bool if you're just returning 0 or 1.
+{
+ u32 sssr;
+ sssr = read_SSSR(drv_context-ioaddr);
+ if ((sssr SSSR_TFL_MASK) || (sssr SSSR_TNF) == 0)
+ return 0;
+ else
+ return 1;
+}
return false/true.
+static inline u32 is_rx_fifo_empty(struct ssp_driver_context *drv_context)
+{
+ return ((read_SSSR(drv_context-ioaddr) SSSR_RNE) == 0);
+}
Dito. Here it is even more obvious.
(...)
+static void flush(struct ssp_driver_context *drv_context)
+{
+ void *reg = drv_context-ioaddr;
+ u32 i = 0;
+
+ /* If the transmit fifo is not empty, reset the interface. */
+ if (!is_tx_fifo_empty(drv_context)) {
+ dev_err(drv_context-pdev-dev,
+ TX FIFO not empty. Reset of SPI IF);
+ disable_interface(drv_context);
+ return;
+ }
+
+ dev_dbg(drv_context-pdev-dev, SSSR=%x\r\n, read_SSSR(reg));
+ while (!is_rx_fifo_empty(drv_context) (i SPI_FIFO_SIZE + 1)) {
+ read_SSDR(reg);
+ i++;
+ }
+ WARN(i 0, %d words flush occured\n, i);
WARN really? Why not dev_warn()?
+static int null_writer(struct ssp_driver_context *drv_context)
+static int null_reader(struct ssp_driver_context *drv_context)
+static int u8_writer(struct ssp_driver_context *drv_context)
+static int u8_reader(struct ssp_driver_context *drv_context)
+static int u16_writer(struct ssp_driver_context *drv_context)
+static int u16_reader(struct ssp_driver_context *drv_context)
+static int u32_writer(struct ssp_driver_context *drv_context)
+static int u32_reader(struct ssp_driver_context *drv_context)
These seem to all be designed to return 0 or 1 and should then be
bool. It seems strange actually, you would expect that such a
function returns the number of bytes or words read/written.
+static bool chan_filter(struct dma_chan *chan, void *param)
+static void unmap_dma_buffers(struct ssp_driver_context *drv_context)
+static void intel_mid_ssp_spi_dma_done(void *arg)
+static void intel_mid_ssp_spi_dma_init(struct ssp_driver_context
*drv_context)
+static void intel_mid_ssp_spi_dma_exit(struct ssp_driver_context
*drv_context)
+static void dma_transfer(struct ssp_driver_context
Re: [PATCH] SPI: SSP SPI Controller driver
Dear Linus,
Thanks for your kind comments. Seems you were viewing the 1st version, I've
submitted 2nd version and to deliver the 3rd version soon, will include you
for review.
Please see my comments inline below.
On Mon, 2012-12-17 at 12:23 +0100, Linus Walleij wrote:
On Wed, Nov 21, 2012 at 3:16 AM, chao bi chao...@intel.com wrote:
+#ifdef DUMP_RX
So since this #define DUMP_RX is not part of this patch and not of the
kernel at large it's basically an #if 0 and all the code within such
defines should be deleted.
But I guess you have this undocumented feature that the developer
is supposed to hack the file and insert #define DUMP_RX to use it.
yes, so I'm to deleted it in this patch, and the DUMP feature is to
submitted as another patch, which is referred to your method.
+static inline u32 is_tx_fifo_empty(struct ssp_driver_context *drv_context)
Change return type to bool if you're just returning 0 or 1.
+{
+ u32 sssr;
+ sssr = read_SSSR(drv_context-ioaddr);
+ if ((sssr SSSR_TFL_MASK) || (sssr SSSR_TNF) == 0)
+ return 0;
+ else
+ return 1;
+}
return false/true.
+static inline u32 is_rx_fifo_empty(struct ssp_driver_context *drv_context)
+{
+ return ((read_SSSR(drv_context-ioaddr) SSSR_RNE) == 0);
+}
Dito. Here it is even more obvious.
It's already done in 2nd version.
+static void flush(struct ssp_driver_context *drv_context)
+{
+ void *reg = drv_context-ioaddr;
+ u32 i = 0;
+
+ /* If the transmit fifo is not empty, reset the interface. */
+ if (!is_tx_fifo_empty(drv_context)) {
+ dev_err(drv_context-pdev-dev,
+ TX FIFO not empty. Reset of SPI IF);
+ disable_interface(drv_context);
+ return;
+ }
+
+ dev_dbg(drv_context-pdev-dev, SSSR=%x\r\n, read_SSSR(reg));
+ while (!is_rx_fifo_empty(drv_context) (i SPI_FIFO_SIZE + 1)) {
+ read_SSDR(reg);
+ i++;
+ }
+ WARN(i 0, %d words flush occured\n, i);
WARN really? Why not dev_warn()?
It's suppose that SPI FIFO is empty after each transfer, so call flush()
before each time of transfer, if any remain data in SPI FIFO,
It shows some kinds of Error must be happened in last transfer and
deserves a WARN() to record it.
+static int null_writer(struct ssp_driver_context *drv_context)
+static int null_reader(struct ssp_driver_context *drv_context)
+static int u8_writer(struct ssp_driver_context *drv_context)
+static int u8_reader(struct ssp_driver_context *drv_context)
+static int u16_writer(struct ssp_driver_context *drv_context)
+static int u16_reader(struct ssp_driver_context *drv_context)
+static int u32_writer(struct ssp_driver_context *drv_context)
+static int u32_reader(struct ssp_driver_context *drv_context)
These seem to all be designed to return 0 or 1 and should then be
bool. It seems strange actually, you would expect that such a
function returns the number of bytes or words read/written.
these functions are only used by PIO transfer, its name shows how many
bytes to be written/read, and to return whether write/read is success.
If returns bytes of read/written seems make no sense for PIO transfer,
so I think return type of boot is enough, what do you think?
+/**
+ * sram_to_ddr_cpy() - Copy data from Langwell SDRAM to DDR
+ * @drv_context: Pointer to the private driver context
+ */
+static void sram_to_ddr_cpy(struct ssp_driver_context *drv_context)
+{
+ u32 length = drv_context-len;
+
+ if ((drv_context-quirks QUIRKS_DMA_USE_NO_TRAIL)
+(drv_context-len drv_context-rx_fifo_threshold *
+ drv_context-n_bytes))
+ length = TRUNCATE(drv_context-len,
+ drv_context-rx_fifo_threshold *
drv_context-n_bytes);
TRUNCATE is a too generic name but I'll leave that comment for
the header file where it's defined.
In version 2, most contents have been moved to c file to avoid name
conflicts.
It looks very strange.
Isn't this simply an arithmetic soup construction to say:
length = drv_context-len / (drv_context-rx_fifo_threshold *
drv_context-n_bytes);
I think TRUNCATE() is different:
#define TRUNCATE(x, a) ((x) ~((a)-1))
+static void int_transfer_complete(struct ssp_driver_context *drv_context)
+{
+ void *reg = drv_context-ioaddr;
+ struct spi_message *msg;
+ struct device *dev = drv_context-pdev-dev;
+
+ if (unlikely(drv_context-quirks QUIRKS_USE_PM_QOS))
+ pm_qos_update_request(drv_context-pm_qos_req,
+ PM_QOS_DEFAULT_VALUE);
It's weird that using PM QoS is treated as an unlikely oddity.
Should it not be the other way around?
+
+ if (unlikely(drv_context-quirks QUIRKS_SRAM_ADDITIONAL_CPY))
+
Re: [PATCH] SPI: SSP SPI Controller driver
On Thu, 13 Dec 2012 17:09:34 +0800, chao bi chao...@intel.com wrote: On Tue, 2012-12-11 at 16:46 +, Grant Likely wrote: On Tue, 11 Dec 2012 16:58:31 +0800, chao bi chao...@intel.com wrote: On Thu, 2012-12-06 at 12:38 +, Grant Likely wrote: On Wed, 21 Nov 2012 10:16:43 +0800, chao bi chao...@intel.com wrote: + master-mode_bits = SPI_CPOL | SPI_CPHA; + master-bus_num = SSP_CFG_GET_SPI_BUS_NB(ssp_cfg); + master-num_chipselect = 1; + master-cleanup = cleanup; + master-setup = setup; + master-transfer = transfer; + drv_context-dma_wq = create_workqueue(intel_mid_ssp_spi); + INIT_WORK(drv_context-complete_work, int_transfer_complete_work); Workqueue management is integrated into the core spi infrastructure now. SPI drivers should no longer be creating their own workqueues. Instead, replace the -transfer hook with prepare_transfer_hardware(), unprepare_transfer_hardware() and transfer_one_message(). See Documentation/spi/spi-summary for details. Hi Grant, I'd like to talk about my understanding here, please correct me if I was wrong: 1. I understand the workqueue in spi core is for driving message transfer, so SPI driver should not create new workqueue for this usage. However, the workqueue created here is not for this usage it's to call back to SPI protocol driver (ifx6x60.c) when DMA data transfer is finished, so it seems not conflict with spi core. Am I right? It appears to me like all the stuff in int_transfer_complete() can be performed at interrupt context, or gets removed in moving to the new system. Am I mistaken here? Yes, we can make use of new SPI core interface to callback to protocol driver (through spi_finalize_current_message()), but looks like it's better to call spi_finalize_current_message() inside workqueue than DMA interrupt context, because the callback function for protocol driver would cost much time, it's better to move this part out of interrupt context. Therefore, I prefer to keep the workqueue here if you agree, what's your opinion? It would be better to work within the context of the kthread that is already managing transfers. Otherwise you've got multiple contexts that could be competing. Plus the kthread may be running in realtime context, but that would be useless since the workqueue would never have the same priority. It currently isn't documented whether or not protocol drivers can sleep in the complete callback. I think it is assumed that it cannot, but that should be verified. If it is a problem for the complete callback to requre atomicity, then maybe we should have a separate .complete_atomic hook for those that can handle it, and call .complete() in the kthread context. Linusw, you did a bunch of work on this. What do you think? g. -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Thu, 2012-12-06 at 12:38 +, Grant Likely wrote: On Wed, 21 Nov 2012 10:16:43 +0800, chao bi chao...@intel.com wrote: + master-mode_bits = SPI_CPOL | SPI_CPHA; + master-bus_num = SSP_CFG_GET_SPI_BUS_NB(ssp_cfg); + master-num_chipselect = 1; + master-cleanup = cleanup; + master-setup = setup; + master-transfer = transfer; + drv_context-dma_wq = create_workqueue(intel_mid_ssp_spi); + INIT_WORK(drv_context-complete_work, int_transfer_complete_work); Workqueue management is integrated into the core spi infrastructure now. SPI drivers should no longer be creating their own workqueues. Instead, replace the -transfer hook with prepare_transfer_hardware(), unprepare_transfer_hardware() and transfer_one_message(). See Documentation/spi/spi-summary for details. Hi Grant, I'd like to talk about my understanding here, please correct me if I was wrong: 1. I understand the workqueue in spi core is for driving message transfer, so SPI driver should not create new workqueue for this usage. However, the workqueue created here is not for this usage it's to call back to SPI protocol driver (ifx6x60.c) when DMA data transfer is finished, so it seems not conflict with spi core. Am I right? 2. Currently our Medfield Platform SW is based on linux-3.0, transfer_one_message() is not implemented, so in SPI driver, we're still use -transfer(), this is with long-term validation. If we change to -transfer_one_message() now, it's hardly to do thorough validation on our platform, so shall we complete this part by 2 steps, firstly we implement with -transfer() hoot which can be validation on our hardware platform, next step, when our internal SW version is upgraded to latest Linux version, then we raise a patch to adapt new spi core. what's your opinion? chao -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Thu, 2012-12-06 at 14:19 +, Alan Cox wrote: +late_initcall(intel_mid_ssp_spi_init); Why late_initcall()? module_init() should be sufficient. Or better yet replace the init and exit functions with module_pci_driver() Thats a legacy of the old SPI code not handling bus and device registration in random orders. So it's no longer needed I believe. Alan We use the late_initcall because we want init the spi driver after finished the dma driver with fs_initcall. Now we can not test the module_pci_driver replace the late_initcall because our kernel version is not brach 3.7. So maybe we can use old the late_initcall and update this code in the further, Do you agree? -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Tue, 11 Dec 2012 10:00:16 +0800, chao bi chao...@intel.com wrote:
+static void dump_trailer(const struct device *dev, char *buf, int len,
int sz)
+{
+ int tlen1 = (len sz ? len : sz);
+ int tlen2 = ((len - sz) sz) ? sz : (len - sz);
+ unsigned char *p;
+ static char msg[MAX_SPI_TRANSFER_SIZE];
Is this size a limitation of the hardware, of of the driver?
I think this size is attributed to the DMA controller's maximum block size.
On Medfield platform, the DMA controller used by SSP SPI has defined its
maximum
block size and word width, so SPI transfer size should not exceed the maximum
size that
DMA could transfer in one block.
Typically what a driver should do here is to split up the transfer into
multiple DMA operations. I won't nack the driver over this issue, but
the driver should not have a maximum transfer size limitation in this
way.
g.
--
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
Hi Grant,
Thanks for your comments, please see my answer below..
On Thu, 2012-12-06 at 12:38 +, Grant Likely wrote:
include/linux/spi/spi-intel-mid-ssp.h | 326
Most (if not all) of this header file looks like it needs to be moved
into the .c file. Any symbol that is only used by the driver's .c file
(usually, anything that isn't platform_data) belongs in the .c
Yes, we'll update it.
+#define DRIVER_NAME intel_mid_ssp_spi_unified
This string is used exactly one. Drop the #define.
Yes, we'll update it.
+static const struct pci_device_id pci_ids[];
If you move the pci_ids table up to this point then the forward
declaration can be eliminated.
Also, use a driver-specific prefix on all symbols, even if they are
static. It makes it a lot easier to navigate code when the symbol names
match the driver and it avoids any possibility of conflict with the
global namespace. Something like midssp_ or midssp_spi_.
So, this symbol would be midssp_spi_pci_ids[], and all the static
functions below should be renamed.
Yes, we'll update it.
+static void dump_trailer(const struct device *dev, char *buf, int len, int
sz)
+{
+ int tlen1 = (len sz ? len : sz);
+ int tlen2 = ((len - sz) sz) ? sz : (len - sz);
+ unsigned char *p;
+ static char msg[MAX_SPI_TRANSFER_SIZE];
Is this size a limitation of the hardware, of of the driver?
I think this size is attributed to the DMA controller's maximum block size.
On Medfield platform, the DMA controller used by SSP SPI has defined its
maximum
block size and word width, so SPI transfer size should not exceed the maximum
size that
DMA could transfer in one block.
+
+ memset(msg, '\0', sizeof(msg));
+ p = buf;
+ while (p buf + tlen1)
+ sprintf(msg, %s%02x, msg, (unsigned int)*p++);
+
+ if (tlen2 0) {
+ sprintf(msg, %s ., msg);
+ p = (buf+len) - tlen2;
+ while (p buf + len)
+ sprintf(msg, %s%02x, msg, (unsigned int)*p++);
+ }
This looks like it will overrun the msg buffer. It adds 2 bytes for
every byte of data. It's also kind of sketchy code to sprintf into the
same buffer you're reading from.
You could avoid all these problems by using print_hex_dump() instead.
Yes, we'll update it.
+static inline u32 is_tx_fifo_empty(struct ssp_driver_context *drv_context)
u32 == bool
Yes, we'll update it.
+ u32 sssr;
+ sssr = read_SSSR(drv_context-ioaddr);
+ if ((sssr SSSR_TFL_MASK) || (sssr SSSR_TNF) == 0)
+ return 0;
+ else
+ return 1;
or simply: return (sssr (SSR_TFL_MASK || SSSR_TNF)) != 0;
Yes, that's better.
... Okay, so I just went looking for the read_SSSR() function because I
wanted to know how it was defined. I just discovered that this driver is
the same as drivers/spi/spi-pxa2xx.c with a PCI front end bolted on.
I'm not keen on having two separate drivers for the same logic block.
For SSP SPI driver, read_SSSR() is defined in spi-intel-mid-ssp.h:
#define DEFINE_SSP_REG(reg, off) \
static inline u32 read_##reg(void *p) { return __raw_readl(p + (off)); }
...
DEFINE_SSP_REG(SSSR, 0x08)
+ INIT_WORK(drv_context-complete_work, int_transfer_complete_work);
Workqueue management is integrated into the core spi infrastructure now.
SPI drivers should no longer be creating their own workqueues.
Instead, replace the -transfer hook with prepare_transfer_hardware(),
unprepare_transfer_hardware() and transfer_one_message(). See
Documentation/spi/spi-summary for details.
OK, we'll change according to workqueue management.
+late_initcall(intel_mid_ssp_spi_init);
Why late_initcall()? module_init() should be sufficient. Or better yet
replace the init and exit functions with module_pci_driver()
Yes, module_pci_driver() is good.
As mentioned above, most if not all of the stuff in this file belongs in the
.c.
Thanks for the detailed comments, we're updating per above comments,
after validation is done, we will resubmit patch for you review.
--
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Wed, 21 Nov 2012 10:16:43 +0800, chao bi chao...@intel.com wrote: This patch is to implement SSP SPI controller driver, which has been applied and validated on intel Moorestown Medfield platform. The patch are originated by Ken Mills ken.k.mi...@intel.com and Sylvain Centelles sylvain.centel...@intel.com, and to be further developed by Channing chao...@intel.com and Chen Jun jun.d.c...@intel.com according to their integration validation on Medfield platform. Signed-off-by: Ken Mills ken.k.mi...@intel.com Signed-off-by: Sylvain Centelles sylvain.centel...@intel.com Signed-off-by: channing chao...@intel.com Signed-off-by: Chen Jun jun.d.c...@intel.com Hi Chao, Thanks for the patch, comments below... --- drivers/spi/Kconfig |9 + drivers/spi/Makefile |1 + drivers/spi/spi-intel-mid-ssp.c | 1407 + include/linux/spi/spi-intel-mid-ssp.h | 326 Most (if not all) of this header file looks like it needs to be moved into the .c file. Any symbol that is only used by the driver's .c file (usually, anything that isn't platform_data) belongs in the .c 4 files changed, 1743 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/spi-intel-mid-ssp.c create mode 100644 include/linux/spi/spi-intel-mid-ssp.h diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 1acae35..8b4461b 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -179,6 +179,15 @@ config SPI_IMX This enables using the Freescale i.MX SPI controllers in master mode. +config SPI_INTEL_MID_SSP + tristate SSP SPI controller driver for Intel MID platforms + depends on SPI_MASTER INTEL_MID_DMAC + help + This is the unified SSP SPI master controller driver for + the Intel MID platforms, handling Moorestown Medfield, + master clock mode. + It supports Bulverde SSP core. + I think I've asked this question before, but I can't remember if I've gotten an answer. How is this different from the designware spi controller that is already in the tree for medfield and moorestown MID platforms? (drivers/spi/spi-dw-mid.c). config SPI_LM70_LLP tristate Parallel port adapter for LM70 eval board (DEVELOPMENT) depends on PARPORT EXPERIMENTAL diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index c48df47..83f06d0 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -32,6 +32,7 @@ obj-$(CONFIG_SPI_FSL_ESPI) += spi-fsl-espi.o obj-$(CONFIG_SPI_FSL_SPI)+= spi-fsl-spi.o obj-$(CONFIG_SPI_GPIO) += spi-gpio.o obj-$(CONFIG_SPI_IMX)+= spi-imx.o +obj-$(CONFIG_SPI_INTEL_MID_SSP) += spi-intel-mid-ssp.o obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o obj-$(CONFIG_SPI_MPC512x_PSC)+= spi-mpc512x-psc.o obj-$(CONFIG_SPI_MPC52xx_PSC)+= spi-mpc52xx-psc.o diff --git a/drivers/spi/spi-intel-mid-ssp.c b/drivers/spi/spi-intel-mid-ssp.c new file mode 100644 index 000..8fca48f --- /dev/null +++ b/drivers/spi/spi-intel-mid-ssp.c @@ -0,0 +1,1407 @@ +/* + * spi-intel-mid-ssp.c + * This driver supports Bulverde SSP core used on Intel MID platforms + * It supports SSP of Moorestown Medfield platforms and handles clock + * slave master modes. + * + * Copyright (c) 2010, Intel Corporation. + * Ken Mills ken.k.mi...@intel.com + * Sylvain Centelles sylvain.centel...@intel.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +/* + * Note: + * + * Supports DMA and non-interrupt polled transfers. + * + */ + +#include linux/delay.h +#include linux/interrupt.h +#include linux/highmem.h +#include linux/pci.h +#include linux/init.h +#include linux/interrupt.h +#include linux/dma-mapping.h +#include linux/intel_mid_dma.h +#include linux/pm_qos.h +#include linux/module.h + +#include linux/spi/spi.h +#include linux/spi/spi-intel-mid-ssp.h + +#define DRIVER_NAME intel_mid_ssp_spi_unified This string is used exactly one. Drop the #define. + +MODULE_AUTHOR(Ken Mills); +MODULE_DESCRIPTION(Bulverde SSP core SPI contoller); +MODULE_LICENSE(GPL); + +static const struct pci_device_id pci_ids[]; If you move the pci_ids table up to this point
Re: [PATCH] SPI: SSP SPI Controller driver
I think I've asked this question before, but I can't remember if I've gotten an answer. How is this different from the designware spi controller that is already in the tree for medfield and moorestown MID platforms? (drivers/spi/spi-dw-mid.c). Different devices. ... Okay, so I just went looking for the read_SSSR() function because I wanted to know how it was defined. I just discovered that this driver is the same as drivers/spi/spi-pxa2xx.c with a PCI front end bolted on. Quite possible - I wans't aware of that but they may well come from the same origin. +late_initcall(intel_mid_ssp_spi_init); Why late_initcall()? module_init() should be sufficient. Or better yet replace the init and exit functions with module_pci_driver() Thats a legacy of the old SPI code not handling bus and device registration in random orders. So it's no longer needed I believe. Alan -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
RE: [PATCH] SPI: SSP SPI Controller driver
On Thu, Nov 22, 2012 at 8:56 AM, Bi, Chao chao...@intel.commailto:chao...@intel.com wrote: if (chip_info-enable_loopback) + chip-cr1 |= SSCR1_LBM; Who sets the enable_loopback? [Chao] 'enable_loopback' could be configured by SPI Protocol driver before it setup SPI controller. Generally it is not set by default because it's used for test and validation. Should it not then depend on (spi mode ) SPI_LOOP ? Or am I missing something. [Chao] I think it's up to protocol driver: if protocol driver choose to configure depend on (spi mode)SPI_LOOP, then it should set spi_device.controller_data to make 'enable_loopback' corresponds to (spi mode) SPI_LOOP, anyhow, SSP controller will always judge spi loop only through spi_device.controller_data which may be changed by protocol driver. in spi_intel_mid_ssp.c: /* protocol drivers may change the chip settings, so... */ /* if chip_info exists, use it */ chip_info = spi-controller_data; -- Monitor your physical, virtual and cloud infrastructure from a single web console. Get in-depth insight into apps, servers, databases, vmware, SAP, cloud infrastructure, etc. Download 30-day Free Trial. Pricing starts from $795 for 25 servers or applications! http://p.sf.net/sfu/zoho_dev2dev_nov ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
Thats what I was trying to understand. If I am not wrong the latency is time related. Why only some platforms / modes need it also the value is not speed dependent. Because the problem was fixed in the later devices. Also the spi core today doesnt have slave mode support thats a different discussion altogether may be we can leave it for now. I'd rather we kept the support in the driver ready for that. Alan -- Monitor your physical, virtual and cloud infrastructure from a single web console. Get in-depth insight into apps, servers, databases, vmware, SAP, cloud infrastructure, etc. Download 30-day Free Trial. Pricing starts from $795 for 25 servers or applications! http://p.sf.net/sfu/zoho_dev2dev_nov ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Wed, Nov 21, 2012 at 7:46 AM, chao bi chao...@intel.com wrote:
This patch is to implement SSP SPI controller driver, which has been
applied and
validated on intel Moorestown Medfield platform. The patch are
originated by
Ken Mills ken.k.mi...@intel.com and Sylvain Centelles
sylvain.centel...@intel.com,
and to be further developed by Channing chao...@intel.com and Chen Jun
jun.d.c...@intel.com according to their integration validation on
Medfield platform.
Signed-off-by: Ken Mills ken.k.mi...@intel.com
Signed-off-by: Sylvain Centelles sylvain.centel...@intel.com
Signed-off-by: channing chao...@intel.com
Signed-off-by: Chen Jun jun.d.c...@intel.com
---
drivers/spi/Kconfig |9 +
drivers/spi/Makefile |1 +
drivers/spi/spi-intel-mid-ssp.c | 1407
+
include/linux/spi/spi-intel-mid-ssp.h | 326
4 files changed, 1743 insertions(+), 0 deletions(-)
create mode 100644 drivers/spi/spi-intel-mid-ssp.c
create mode 100644 include/linux/spi/spi-intel-mid-ssp.h
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 1acae35..8b4461b 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -179,6 +179,15 @@ config SPI_IMX
This enables using the Freescale i.MX SPI controllers in master
mode.
+config SPI_INTEL_MID_SSP
+ tristate SSP SPI controller driver for Intel MID platforms
+ depends on SPI_MASTER INTEL_MID_DMAC
+ help
+ This is the unified SSP SPI master controller driver for
+ the Intel MID platforms, handling Moorestown Medfield,
+ master clock mode.
+ It supports Bulverde SSP core.
+
config SPI_LM70_LLP
tristate Parallel port adapter for LM70 eval board (DEVELOPMENT)
depends on PARPORT EXPERIMENTAL
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index c48df47..83f06d0 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_SPI_FSL_ESPI)+= spi-fsl-espi.o
obj-$(CONFIG_SPI_FSL_SPI) += spi-fsl-spi.o
obj-$(CONFIG_SPI_GPIO) += spi-gpio.o
obj-$(CONFIG_SPI_IMX) += spi-imx.o
+obj-$(CONFIG_SPI_INTEL_MID_SSP)+= spi-intel-mid-ssp.o
obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o
obj-$(CONFIG_SPI_MPC512x_PSC) += spi-mpc512x-psc.o
obj-$(CONFIG_SPI_MPC52xx_PSC) += spi-mpc52xx-psc.o
diff --git a/drivers/spi/spi-intel-mid-ssp.c
b/drivers/spi/spi-intel-mid-ssp.c
new file mode 100644
index 000..8fca48f
--- /dev/null
+++ b/drivers/spi/spi-intel-mid-ssp.c
@@ -0,0 +1,1407 @@
+/*
+ * spi-intel-mid-ssp.c
+ * This driver supports Bulverde SSP core used on Intel MID platforms
+ * It supports SSP of Moorestown Medfield platforms and handles clock
+ * slave master modes.
+ *
+ * Copyright (c) 2010, Intel Corporation.
+ * Ken Mills ken.k.mi...@intel.com
+ * Sylvain Centelles sylvain.centel...@intel.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+/*
+ * Note:
+ *
+ * Supports DMA and non-interrupt polled transfers.
+ *
+ */
+
+#include linux/delay.h
+#include linux/interrupt.h
+#include linux/highmem.h
+#include linux/pci.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/dma-mapping.h
+#include linux/intel_mid_dma.h
+#include linux/pm_qos.h
+#include linux/module.h
+
+#include linux/spi/spi.h
+#include linux/spi/spi-intel-mid-ssp.h
+
+#define DRIVER_NAME intel_mid_ssp_spi_unified
+
+MODULE_AUTHOR(Ken Mills);
+MODULE_DESCRIPTION(Bulverde SSP core SPI contoller);
+MODULE_LICENSE(GPL);
+
+static const struct pci_device_id pci_ids[];
+
+#ifdef DUMP_RX
+static void dump_trailer(const struct device *dev, char *buf, int len,
int sz)
+{
+ int tlen1 = (len sz ? len : sz);
+ int tlen2 = ((len - sz) sz) ? sz : (len - sz);
+ unsigned char *p;
+ static char msg[MAX_SPI_TRANSFER_SIZE];
+
+ memset(msg, '\0', sizeof(msg));
+ p = buf;
+ while (p buf + tlen1)
+ sprintf(msg, %s%02x, msg, (unsigned int)*p++);
+
+ if (tlen2 0) {
+ sprintf(msg, %s ., msg);
+ p = (buf+len) - tlen2;
+ while (p buf + len)
+
Re: [PATCH] SPI: SSP SPI Controller driver
On Wed, Nov 21, 2012 at 7:46 AM, chao bi chao...@intel.com wrote: + /* Create the PM_QOS request */ + if (drv_context-quirks QUIRKS_USE_PM_QOS) + pm_qos_add_request(drv_context-pm_qos_req, + PM_QOS_CPU_DMA_LATENCY, + PM_QOS_DEFAULT_VALUE); What happens if the flag is not set if it is absolutely necessary for the driver it should not be a configurable option? -- Monitor your physical, virtual and cloud infrastructure from a single web console. Get in-depth insight into apps, servers, databases, vmware, SAP, cloud infrastructure, etc. Download 30-day Free Trial. Pricing starts from $795 for 25 servers or applications! http://p.sf.net/sfu/zoho_dev2dev_nov ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] SPI: SSP SPI Controller driver
On Wed, 21 Nov 2012 17:44:21 +0530 Shubhrajyoti Datta omaplinuxker...@gmail.com wrote: On Wed, Nov 21, 2012 at 7:46 AM, chao bi chao...@intel.com wrote: + /* Create the PM_QOS request */ + if (drv_context-quirks QUIRKS_USE_PM_QOS) + pm_qos_add_request(drv_context-pm_qos_req, + PM_QOS_CPU_DMA_LATENCY, + PM_QOS_DEFAULT_VALUE); What happens if the flag is not set if it is absolutely necessary for the driver it should not be a configurable option If you read through the code it's set only when the device is Moorestown/Oaktrail based and only in slave mode. It is not necessary in other configurations. Alan -- Monitor your physical, virtual and cloud infrastructure from a single web console. Get in-depth insight into apps, servers, databases, vmware, SAP, cloud infrastructure, etc. Download 30-day Free Trial. Pricing starts from $795 for 25 servers or applications! http://p.sf.net/sfu/zoho_dev2dev_nov ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
RE: [PATCH] SPI: SSP SPI Controller driver
+ if (chip_info-enable_loopback) + chip-cr1 |= SSCR1_LBM; Who sets the enable_loopback? [Chao] 'enable_loopback' could be configured by SPI Protocol driver before it setup SPI controller. Generally it is not set by default because it's used for test and validation. Thanks From: Shubhrajyoti Datta [mailto:omaplinuxker...@gmail.com] Sent: Wednesday, November 21, 2012 8:08 PM To: Bi, Chao Cc: grant.lik...@secretlab.ca; Chen, Jun D; spi-devel-general@lists.sourceforge.net; a...@linux.intel.com; Mills, Ken K; Centelles, Sylvain Subject: Re: [PATCH] SPI: SSP SPI Controller driver On Wed, Nov 21, 2012 at 7:46 AM, chao bi chao...@intel.commailto:chao...@intel.com wrote: This patch is to implement SSP SPI controller driver, which has been applied and validated on intel Moorestown Medfield platform. The patch are originated by Ken Mills ken.k.mi...@intel.commailto:ken.k.mi...@intel.com and Sylvain Centelles sylvain.centel...@intel.commailto:sylvain.centel...@intel.com, and to be further developed by Channing chao...@intel.commailto:chao...@intel.com and Chen Jun jun.d.c...@intel.commailto:jun.d.c...@intel.com according to their integration validation on Medfield platform. Signed-off-by: Ken Mills ken.k.mi...@intel.commailto:ken.k.mi...@intel.com Signed-off-by: Sylvain Centelles sylvain.centel...@intel.commailto:sylvain.centel...@intel.com Signed-off-by: channing chao...@intel.commailto:chao...@intel.com Signed-off-by: Chen Jun jun.d.c...@intel.commailto:jun.d.c...@intel.com --- drivers/spi/Kconfig |9 + drivers/spi/Makefile |1 + drivers/spi/spi-intel-mid-ssp.c | 1407 + include/linux/spi/spi-intel-mid-ssp.h | 326 4 files changed, 1743 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/spi-intel-mid-ssp.c create mode 100644 include/linux/spi/spi-intel-mid-ssp.h diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 1acae35..8b4461b 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -179,6 +179,15 @@ config SPI_IMX This enables using the Freescale i.MX SPI controllers in master mode. +config SPI_INTEL_MID_SSP + tristate SSP SPI controller driver for Intel MID platforms + depends on SPI_MASTER INTEL_MID_DMAC + help + This is the unified SSP SPI master controller driver for + the Intel MID platforms, handling Moorestown Medfield, + master clock mode. + It supports Bulverde SSP core. + config SPI_LM70_LLP tristate Parallel port adapter for LM70 eval board (DEVELOPMENT) depends on PARPORT EXPERIMENTAL diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index c48df47..83f06d0 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -32,6 +32,7 @@ obj-$(CONFIG_SPI_FSL_ESPI)+= spi-fsl-espi.o obj-$(CONFIG_SPI_FSL_SPI) += spi-fsl-spi.o obj-$(CONFIG_SPI_GPIO) += spi-gpio.o obj-$(CONFIG_SPI_IMX) += spi-imx.o +obj-$(CONFIG_SPI_INTEL_MID_SSP)+= spi-intel-mid-ssp.o obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o obj-$(CONFIG_SPI_MPC512x_PSC) += spi-mpc512x-psc.o obj-$(CONFIG_SPI_MPC52xx_PSC) += spi-mpc52xx-psc.o diff --git a/drivers/spi/spi-intel-mid-ssp.c b/drivers/spi/spi-intel-mid-ssp.c new file mode 100644 index 000..8fca48f --- /dev/null +++ b/drivers/spi/spi-intel-mid-ssp.c @@ -0,0 +1,1407 @@ +/* + * spi-intel-mid-ssp.c + * This driver supports Bulverde SSP core used on Intel MID platforms + * It supports SSP of Moorestown Medfield platforms and handles clock + * slave master modes. + * + * Copyright (c) 2010, Intel Corporation. + * Ken Mills ken.k.mi...@intel.commailto:ken.k.mi...@intel.com + * Sylvain Centelles sylvain.centel...@intel.commailto:sylvain.centel...@intel.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +/* + * Note: + * + * Supports DMA and non-interrupt polled transfers. + * + */ + +#include linux/delay.h +#include linux/interrupt.h +#include linux/highmem.h +#include linux/pci.h +#include linux/init.h +#include linux/interrupt.h +#include linux/dma-mapping.h +#include linux/intel_mid_dma.h +#include linux/pm_qos.h +#include linux/module.h + +#include linux/spi/spi.h +#include linux/spi/spi
Re: [PATCH] SPI: SSP SPI Controller driver
On Wed, Nov 21, 2012 at 5:56 PM, Alan Cox a...@linux.intel.com wrote: On Wed, 21 Nov 2012 17:44:21 +0530 Shubhrajyoti Datta omaplinuxker...@gmail.com wrote: On Wed, Nov 21, 2012 at 7:46 AM, chao bi chao...@intel.com wrote: + /* Create the PM_QOS request */ + if (drv_context-quirks QUIRKS_USE_PM_QOS) + pm_qos_add_request(drv_context-pm_qos_req, + PM_QOS_CPU_DMA_LATENCY, + PM_QOS_DEFAULT_VALUE); What happens if the flag is not set if it is absolutely necessary for the driver it should not be a configurable option If you read through the code it's set only when the device is Moorestown/Oaktrail based and only in slave mode. It is not necessary in other configurations. Thats what I was trying to understand. If I am not wrong the latency is time related. Why only some platforms / modes need it also the value is not speed dependent. My doubt is that the time taken for the dma will be more in lower speed so the latency constraint could be relaxed. Also the spi core today doesnt have slave mode support thats a different discussion altogether may be we can leave it for now. Alan -- Monitor your physical, virtual and cloud infrastructure from a single web console. Get in-depth insight into apps, servers, databases, vmware, SAP, cloud infrastructure, etc. Download 30-day Free Trial. Pricing starts from $795 for 25 servers or applications! http://p.sf.net/sfu/zoho_dev2dev_nov ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
