Re: [PATCH] nvmem: core: fix nvmem_cell_write inline function
On 9/9/19 1:18 PM, Sebastian Reichel wrote: Hi, On Mon, Sep 09, 2019 at 12:26:06PM +0300, Nandor Han wrote: On 9/8/19 3:10 PM, Sebastian Reichel wrote: From: Sebastian Reichel nvmem_cell_write's buf argument uses different types based on the configuration of CONFIG_NVMEM. The function prototype for enabled NVMEM uses 'void *' type, but the static dummy function for disabled NVMEM uses 'const char *' instead. Fix the different behaviour by always expecting a 'void *' typed buf argument. Fixes: 7a78a7f7695b ("power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface") Reported-by: kbuild test robot Cc: Han Nandor Cc: Srinivas Kandagatla Cc: linux-kernel@vger.kernel.org Signed-off-by: Sebastian Reichel --- include/linux/nvmem-consumer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h index 8f8be5b00060..5c17cb733224 100644 --- a/include/linux/nvmem-consumer.h +++ b/include/linux/nvmem-consumer.h @@ -118,7 +118,7 @@ static inline void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len) } static inline int nvmem_cell_write(struct nvmem_cell *cell, - const char *buf, size_t len) + void *buf, size_t len) nitpick: alignment issue? This actually fixes the alignment issue as a side-effect. I guess I should have mentioned it in the changelog. Review-By: Han Nandor I suppose you meant to write "Reviewed-by" instead of inventing your own tag? Yes :) Nandor
Re: [PATCH] nvmem: core: fix nvmem_cell_write inline function
On 9/8/19 3:10 PM, Sebastian Reichel wrote: From: Sebastian Reichel nvmem_cell_write's buf argument uses different types based on the configuration of CONFIG_NVMEM. The function prototype for enabled NVMEM uses 'void *' type, but the static dummy function for disabled NVMEM uses 'const char *' instead. Fix the different behaviour by always expecting a 'void *' typed buf argument. Fixes: 7a78a7f7695b ("power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface") Reported-by: kbuild test robot Cc: Han Nandor Cc: Srinivas Kandagatla Cc: linux-kernel@vger.kernel.org Signed-off-by: Sebastian Reichel --- include/linux/nvmem-consumer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h index 8f8be5b00060..5c17cb733224 100644 --- a/include/linux/nvmem-consumer.h +++ b/include/linux/nvmem-consumer.h @@ -118,7 +118,7 @@ static inline void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len) } static inline int nvmem_cell_write(struct nvmem_cell *cell, - const char *buf, size_t len) + void *buf, size_t len) nitpick: alignment issue? Review-By: Han Nandor Nandor
Re: [PATCH] power: reset: make reboot-mode user selectable
On 9/3/19 3:04 AM, Sebastian Reichel wrote: Hi, On Mon, Sep 02, 2019 at 11:16:27PM +0200, Arnd Bergmann wrote: On Mon, Sep 2, 2019 at 10:39 PM Sebastian Reichel wrote: This patch does not look good to me. Better patch would be to allow compiling CONFIG_REBOOT_MODE without CONFIG_OF. Obviously the configuration would not be useful for anything except compile testing, but that is also true for this patch. Ok, I'd suggest we leave it with the bugfix you already applied then. [caa2b55784, power: reset: nvmem-reboot-mode: add CONFIG_OF dependency] That's also fine with me. -- Sebastian Sounds good to me. --- Nandor
Re: [v5] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w
On 8/29/19 5:14 AM, Biwen Li wrote: Issue: - # hwclock -w hwclock: RTC_SET_TIME: Invalid argument Why: - Relative commit: 8b9f9d4dc511309918c4f6793bae7387c0c638af, this patch will always check for unwritable registers, it will compare reg with max_register in regmap_writeable. - The pcf85363/pcf85263 has the capability of address wrapping which means if you access an address outside the allowed range (0x00-0x2f) hardware actually wraps the access to a lower address. The rtc-pcf85363 driver will use this feature to configure the time and execute 2 actions in the same i2c write operation (stopping the clock and configure the time). However the driver has also configured the `regmap maxregister` protection mechanism that will block accessing addresses outside valid range (0x00-0x2f). How: - Split of writing regs to two parts, first part writes control registers about stop_enable and resets, second part writes RTC time and date registers. Signed-off-by: Biwen Li --- Change in v5: - drop robust explanation LGTM +1 Nandor
Re: [v4] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w
On 8/27/19 7:37 AM, Biwen Li wrote: - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS is 0, max_regiter is 0x2f, then reg will be equal to 0x30, '0x30 < 0x2f' is false,so regmap_writeable will return false. - The pcf85363/pcf85263 has the capability of address wrapping which means if you access a continuous address range across a certain boundary(max_register of struct regmap_config) the hardware actually wraps the access to a lower address. But the address violation check of regmap rejects such access. nitpick: This 2 paragraphs could be combined to clear up the issue: ` The pcf85363/pcf85263 has the capability of address wrapping which means if you access an address outside the allowed range (0x00-0x2f) the hardware actually wraps the access to a lower address. The rtc-pf85363 driver will use this feature to configure the time and execute 2 actions in the same i2c write operation (stopping the clock and configure the time). However the driver has also configured the `regmap maxregister` protection mechanism that will block accessing addresses outside valid range (0x00-0x2f). ` nitpick: I would also use separate buffers for this actions. Up to you :) Otherwise LGTM +1 Nandor
Re: [EXT] Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w
On 8/26/19 7:29 AM, Biwen Li wrote: On 8/16/19 10:40 PM, Li Yang wrote: On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni wrote: On 16/08/2019 10:50:49-0500, Li Yang wrote: On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni wrote: On 16/08/2019 10:46:36+0800, Biwen Li wrote: Issue: - # hwclock -w hwclock: RTC_SET_TIME: Invalid argument Why: - Relative patch: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org %2Flkml%2F2019%2F4%2F3%2F55&data=02%7C01%7Cbiwen.li%40nxp. com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99 c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&reserved=0 , this patch will always check for unwritable registers, it will compare reg with max_register in regmap_writeable. - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS is 0, max_regiter is 0x2f, then reg will be equal to 0x30, '0x30 < 0x2f' is false,so regmap_writeable will return false. - Root cause: the buf[] was written to a wrong place in the file drivers/rtc/rtc-pcf85363.c This is not true, the RTC wraps the register accesses properly and this This performance hack probably deserve some explanation in the code comment. :) is probably something that should be handled by regmap_writable. The address wrapping is specific to this RTC chip. Is it also commonly used by other I2C devices? I'm not sure if regmap_writable should handle the wrapping case if it is too special. Most of the i2c RTCs do address wrapping which is sometimes the only way to properly set the time. Adding Mark and Nandor to the loop. Regards, Leo Hi, `regmap` provides couple of ways to validate the registers: max_register, callback function and write table. All of these are optional, so it gives you the freedom to customize it as needed. In this situation probably you could: 1. Avoid using the wrapping feature of pcf85363 (you can just provide separate calls for stop, reset and time confguration). In this way the `max_register` validation method will work fine. Yes, I use this way. Path as follows: Stop and reset - > set time > stop Some of the concerns regarding this method was that it might not be precise enough. That because you need 2 I2C operations (one for stop and one for time configuration). Not sure about your case if this is a problem or not. 2. Replace `max_register` method validation with `callback function` validation method, were you could make your own validation. It is not work, show the code in as follows: bool regmap_writeable(struct regmap *map, unsigned int reg) { if (map->max_register && reg > map->max_register) return false; Callback function (writeable_reg) will not be called. if (map->writeable_reg) return map->writeable_reg(map->dev, reg); Hi Li, If you *replace* the `max_register` method with `callback function` it should work. The code above will use every method *if provided*. In other words if `map->max_register` is 0 will go to the next step and check `map->writeable_reg`. Right? Regards, Nandor
Re: [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w
On 8/16/19 10:40 PM, Li Yang wrote: On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni wrote: On 16/08/2019 10:50:49-0500, Li Yang wrote: On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni wrote: On 16/08/2019 10:46:36+0800, Biwen Li wrote: Issue: - # hwclock -w hwclock: RTC_SET_TIME: Invalid argument Why: - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch will always check for unwritable registers, it will compare reg with max_register in regmap_writeable. - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS is 0, max_regiter is 0x2f, then reg will be equal to 0x30, '0x30 < 0x2f' is false,so regmap_writeable will return false. - Root cause: the buf[] was written to a wrong place in the file drivers/rtc/rtc-pcf85363.c This is not true, the RTC wraps the register accesses properly and this This performance hack probably deserve some explanation in the code comment. :) is probably something that should be handled by regmap_writable. The address wrapping is specific to this RTC chip. Is it also commonly used by other I2C devices? I'm not sure if regmap_writable should handle the wrapping case if it is too special. Most of the i2c RTCs do address wrapping which is sometimes the only way to properly set the time. Adding Mark and Nandor to the loop. Regards, Leo Hi, `regmap` provides couple of ways to validate the registers: max_register, callback function and write table. All of these are optional, so it gives you the freedom to customize it as needed. In this situation probably you could: 1. Avoid using the wrapping feature of pcf85363 (you can just provide separate calls for stop, reset and time confguration). In this way the `max_register` validation method will work fine. 2. Replace `max_register` method validation with `callback function` validation method, were you could make your own validation. Regards, Nandor
Re: drivers/power/reset/nvmem-reboot-mode.c:27:42: error: passing argument 2 of 'nvmem_cell_write' from incompatible pointer type
On 8/11/19 2:43 AM, kbuild test robot wrote: Hi Han, FYI, the error/warning still remains. tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux.git master head: dcbb4a153971ff8646af0c963f5698bf21bfbfdc commit: 7a78a7f7695bf9ef9cef3c06fbc5fa4573fd0eef power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface date: 7 weeks ago config: x86_64-randconfig-d003-201932 (attached as .config) compiler: gcc-7 (Debian 7.4.0-10) 7.4.0 reproduce: git checkout 7a78a7f7695bf9ef9cef3c06fbc5fa4573fd0eef # save the attached .config to linux build tree make ARCH=x86_64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot All errors (new ones prefixed by >>): drivers/power/reset/nvmem-reboot-mode.c: In function 'nvmem_reboot_mode_write': drivers/power/reset/nvmem-reboot-mode.c:27:42: error: passing argument 2 of 'nvmem_cell_write' from incompatible pointer type [-Werror=incompatible-pointer-types] ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic)); ^ In file included from drivers/power/reset/nvmem-reboot-mode.c:10:0: include/linux/nvmem-consumer.h:120:19: note: expected 'const char *' but argument is of type 'unsigned int *' static inline int nvmem_cell_write(struct nvmem_cell *cell, ^~~~ cc1: some warnings being treated as errors vim +/nvmem_cell_write +27 drivers/power/reset/nvmem-reboot-mode.c 18 19 static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot, 20 unsigned int magic) 21 { 22 int ret; 23 struct nvmem_reboot_mode *nvmem_rbm; 24 25 nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot); 26 > 27 ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic)); 28 if (ret < 0) 29 dev_err(reboot->dev, "update reboot mode bits failed\n"); 30 31 return ret; 32 } 33 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation Hi, Seems that `nvmem-consumer.h` declares a different signature for `nvmem_cell_write` method depending on `CONFIG_NVMEM` configuration: #if IS_ENABLED(CONFIG_NVMEM) ... int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len); ... #else ... static inline int nvmem_cell_write(struct nvmem_cell *cell, const char *buf, size_t len) { return -EOPNOTSUPP; } ... #endif /* CONFIG_NVMEM * What's the best approach here? Nandor
Re: [PATCH] power: reset: nvmem-reboot-mode: add CONFIG_OF dependency
On 7/8/19 3:52 PM, Arnd Bergmann wrote: Without CONFIG_OF, we get a build failure in the reboot-mode implementation: drivers/power/reset/reboot-mode.c: In function 'reboot_mode_register': drivers/power/reset/reboot-mode.c:72:2: error: implicit declaration of function 'for_each_property_of_node'; did you mean 'for_each_child_of_node'? [-Werror=implicit-function-declaration] for_each_property_of_node(np, prop) { Add a Kconfig dependency like we have for the other users of CONFIG_REBOOT_MODE. Fixes: 7a78a7f7695b ("power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface") Signed-off-by: Arnd Bergmann --- drivers/power/reset/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig index 8dfb105db391..a564237278ff 100644 --- a/drivers/power/reset/Kconfig +++ b/drivers/power/reset/Kconfig @@ -248,6 +248,7 @@ config POWER_RESET_SC27XX config NVMEM_REBOOT_MODE tristate "Generic NVMEM reboot mode driver" + depends on OF select REBOOT_MODE help Say y here will enable reboot mode driver. This will Wouldn't this be more appropriate to add the "depends on OF" to "config REBOOT_MODE" section, since this is an error to `reboot-mode.c` unit? Nandor
Re: [PATCH v4 0/2] Use NVMEM as reboot-mode write interface
Hi, Changes since v3: - documentation updated according to the comments Thanks, queued. Please fix your git/mail setup, I had to fix the line endings (\r\n -> \n) to apply this. -- Sebastian Ok. Thanks Sebastian. -- Nandor
Re: [PATCH v3 2/2] dt-bindings: power: reset: add document for NVMEM based reboot-mode
On 5/1/19 1:47 AM, Rob Herring wrote: Hi Rob, Thanks for review. @@ -0,0 +1,32 @@ +NVMEM reboot mode driver + +This driver gets reboot mode magic value from reboot-mode driver +and stores it in a NVMEM cell named "reboot-mode". Then the bootloader +can read it and take different action according to the magic +value stored. This is also assuming the nvmem is writeable which is more often not the case. Is your usecase a platform that supports pstore? Adding on to that binding might be a better fit. I'm using an RTC persistent memory for storing this data. The available memory is low and don't think pstore will fit in this case. +The rest of the properties should follow the generic reboot-mode description +found in reboot-mode.txt + +Example: + reboot-mode-nvmem@0 { What's this node for? + compatible = "simple-mfd"; I only see 1 function. No need to this. Will remove Nandor
Re: [PATCH v2 1/2] power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface
On 4/18/19 12:56 AM, Sebastian Reichel wrote: On Thu, Apr 11, 2019 at 05:54:09AM +, Han Nandor wrote: Add a new reboot mode write interface that is using an NVMEM cell to store the reboot mode magic. Signed-off-by: Nandor Han --- +module_platform_driver(nvmem_reboot_mode_driver); + +MODULE_AUTHOR("Nandor Han "); +MODULE_DESCRIPTION("NVMEM reboot mode driver"); +MODULE_LICENSE("GPL v2"); I suppose as of bf7fbeeae6db "GPL v2" is deprecated, before it was often read as GPL v2 only. In both cases it makes sense to just use "GPL". Otherwise the driver looks fine to me, but I'm waiting for Rob's review of the DT bindings before merging this. -- Sebastian Thanks Sebastian. Do you want me to send a new revision with license changed to "GPL"?..or will you change it when it's merged? Nandor
Re: [PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework
On 4/6/19 12:44 AM, Alexandre Belloni wrote: Hi, On 05/04/2019 11:14:35+, Han Nandor wrote: ` # hexdump -n 10 -C /sys/bus/nvmem/devices/ds3232_sram0/nvmem 74 65 73 74 69 6e 67 0a 00 00|testing...| 000a ` Thanks for that nice description! Glad that you like it. :) drivers/rtc/rtc-ds3232.c | 41 ++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c index 7184e5145f12..fe615aedaa9a 100644 --- a/drivers/rtc/rtc-ds3232.c +++ b/drivers/rtc/rtc-ds3232.c @@ -24,6 +24,8 @@ #include #include +#include "rtc-core.h" + The drivers should not have to include that header, see fd5cd21d995e67f87b3eb4adf938be85fe83ef4b (from 4.16). Ahh...right. I did the implementation on 4.14 and it was needed there. I will change the implementation but I'll not be able to fully test it. struct ds3232 { struct device *dev; struct regmap *regmap; int irq; struct rtc_device *rtc; + struct nvmem_config nvmem_cfg; You don't actually need to keep that structure for the whole life of the device as it is copied as soon as it is registered. I usually prefer to have it on the stack. Very true. Done + ds3232->nvmem_cfg.stride = 1; + ds3232->nvmem_cfg.size = DS3232_REG_SRAM_SIZE; + ds3232->nvmem_cfg.word_size = 1; + ds3232->nvmem_cfg.reg_read = ds3232_nvmem_read; + ds3232->nvmem_cfg.reg_write = ds3232_nvmem_write; + ds3232->nvmem_cfg.priv = ds3232; Please also set the type (battery backed). Done. + ds3232->rtc->dev.of_node = dev->of_node; Please don't mess with rtc->dev. In my use-case I do use the device tree to declare a nvmem cell in this RTC node, which is used by another driver. Without this change finding the cell (using `devm_nvmem_cell_get` function) was failing because of missing `of_node` (again this was on 4.14). I can do a bit of digging to see if something was changed, but I will really appreciate your advice, because I'm not be able to test the fix on linux-next. + ds3232->rtc->nvmem_config = &ds3232->nvmem_cfg; + rtc_nvmem_register(ds3232->rtc); This part will not compile on v5.1-rc1. Sorry. I will fix this, and I will at least see that it builds on linux-next. Thanks for review, Nandor
Re: [RFC PATCH 0/1] Verify if register is writeable before a write operation
On 4/2/19 12:06 PM, Mark Brown wrote: On Tue, Apr 02, 2019 at 08:01:21AM +, Han Nandor wrote: Description --- This is an RFC because I don't know if this is a bug or a normal use case. It seems that the function `_regmap_raw_write_impl` from the regmap framework verifies that a register is writable only using the callback function, ignoring the other two (max allowed register, register ranges) Please don't send cover letters for single patches, if there is anything that needs saying put it in the changelog of the patch or after the --- if it's administrative stuff. This reduces mail volume and ensures that any important information is recorded in the changelog rather than being lost. Copy that. If is OK, we can still discuss about this using this patch and I can send a single patch once the things are clear.
Re: EXT: Re: [PATCHv1] Input: atmel_mxt_ts - fix the firmware update
On 23/03/18 21:47, Nick Dyer wrote: On Thu, Mar 22, 2018 at 05:43:30PM +0100, Sebastian Reichel wrote: The automatic update mechanism will trigger an update if the info block CRCs are different between maxtouch configuration file (maxtouch.cfg) and chip. The driver compared the CRCs without retrieving the chip CRC, resulting always in a failure and firmware flashing action triggered. The patch will fix this issue by retrieving the chip info block CRC before the check. Thanks for raising this, I agree it's definitely something we want to fix. However, I'm not convinced you're solving the problem in the best way. You've attached it to the read_t9_resolution() code path, whereas the info block is common between T9 and T100 and works in the same way. Would you mind trying the below patch? I've dusted it off from some work that I did back in 2012 and it should solve your issue. It also has the benefit that by reading the information block and the object table into a contiguous region of memory, we can verify the checksum at probe time. This means we make sure that we are indeed talking to a chip that supports object protocol correctly. Thanks Nick. This looks like a better solution. Sebastian will test this and we can see the results. Nandor
Re: EXT: [PATCH 3/4] gpio: Remove VLA from xra1403 driver
On 10/03/18 02:10, Laura Abbott wrote: The new challenge is to remove VLAs from the kernel (see https://lkml.org/lkml/2018/3/7/621) This patch replaces a VLA with an appropriate call to kmalloc_array. Signed-off-by: Laura Abbott --- This looks good to me. Reviewed-by: Nandor Han Nandor drivers/gpio/gpio-xra1403.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpio/gpio-xra1403.c b/drivers/gpio/gpio-xra1403.c index 0230e4b7a2fb..8d4c8e99b251 100644 --- a/drivers/gpio/gpio-xra1403.c +++ b/drivers/gpio/gpio-xra1403.c @@ -126,11 +126,16 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip) { int reg; struct xra1403 *xra = gpiochip_get_data(chip); - int value[xra1403_regmap_cfg.max_register]; + int *value; int i; unsigned int gcr; unsigned int gsr; + value = kmalloc_array(xra1403_regmap_cfg.max_register, sizeof(*value), + GFP_KERNEL); + if (!value) + return; + seq_puts(s, "xra reg:"); for (reg = 0; reg <= xra1403_regmap_cfg.max_register; reg++) seq_printf(s, " %2.2x", reg); @@ -154,6 +159,7 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip) (gcr & BIT(i)) ? "in" : "out", (gsr & BIT(i)) ? "hi" : "lo"); } + kfree(value); } #else #define xra1403_dbg_show NULL
[PATCH v3 0/2] XRA1403,gpio - add XRA1403 gpio expander driver
The patchset will add a driver to support basic functionality for XRA1403 device. Features supported: - get/set GPIO direction (input, output) - get/set GPIO level (low, high) Documentation: A gpio-xra1403.txt file was added to document the DTS bindings related to driver. Testing: 1.1 XRA1403 connected to iMX53 MCU 1.2 Use the GPIO tools provided by kernel from tools/gpio dir 2.1 `lsgpio` root@csmon ppd:~# lsgpio GPIO chip: gpiochip8, "xra1403", 16 GPIO lines line 0: unnamed unused [output] line 1: unnamed unused [output] line 2: unnamed unused [output] line 3: unnamed unused [output] line 4: unnamed unused [output] line 5: unnamed unused line 6: unnamed unused line 7: unnamed unused [output] line 8: unnamed unused [output] line 9: unnamed unused line 10: unnamed unused [output] line 11: unnamed unused line 12: unnamed unused line 13: unnamed unused line 14: unnamed unused line 15: unnamed unused [output] GPIO chip: gpiochip7, "xra1403", 16 GPIO lines line 0: unnamed unused line 1: unnamed unused line 2: unnamed unused line 3: unnamed unused line 4: unnamed unused line 5: unnamed unused line 6: unnamed unused line 7: unnamed unused line 8: unnamed unused line 9: unnamed unused line 10: unnamed unused line 11: unnamed unused line 12: unnamed unused line 13: unnamed unused line 14: unnamed unused line 15: unnamed unused 3.1 `gpio-hammer` root@csmon ppd:~# gpio-hammer -n gpiochip8 -o0 -o1 -o2 -o3 Hammer lines [0, 1, 2, 3] on gpiochip8, initial states: [0, 0, 0, 0] [\] [0: 0, 1: 0, 2: 0, 3: 0] ... [\] [0: 1, 1: 1, 2: 1, 3: 1] When using `gpio-hammer` I also attached an oscilloscope to one of the pins and I was able to monitor and validate the GPIO status. Changes since v2: - improve the commit message by removing the "sysfs" dependency and be more clear. - include "seq_file.h" header. Note: documentation patches were accepted already. Changes since v1: - use regmap for driver - small changes to documentation Nandor Han (2): gpio - Add EXAR XRA1403 SPI GPIO expander driver Add XRA1403 support to MAINTAINERS file MAINTAINERS | 8 ++ drivers/gpio/Kconfig| 5 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xra1403.c | 237 4 files changed, 251 insertions(+) create mode 100644 drivers/gpio/gpio-xra1403.c -- 2.10.1
[PATCH v3 2/2] Add XRA1403 support to MAINTAINERS file
Add XRA1403 support to MAINTAINERS list. Signed-off-by: Nandor Han --- MAINTAINERS | 8 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index f9deb67..db86335 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14078,6 +14078,14 @@ L: linux-kernel@vger.kernel.org S: Supported F: drivers/char/xillybus/ +XRA1403 GPIO EXPANDER +M: Nandor Han +M: Semi Malinen +L: linux-g...@vger.kernel.org +S: Maintained +F: drivers/gpio/gpio-xra1403.c +F: Documentation/devicetree/bindings/gpio/gpio-xra1403.txt + XTENSA XTFPGA PLATFORM SUPPORT M: Max Filippov L: linux-xte...@linux-xtensa.org -- 2.10.1
[PATCH v3 1/2] gpio - Add EXAR XRA1403 SPI GPIO expander driver
This driver support basic XRA1403 functionalities: - set gpio direction - get gpio direction - set gpio high/low - get gpio status Signed-off-by: Nandor Han Signed-off-by: Semi Malinen --- drivers/gpio/Kconfig| 5 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xra1403.c | 237 3 files changed, 243 insertions(+) create mode 100644 drivers/gpio/gpio-xra1403.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 23ca51e..dbde3ae 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1225,6 +1225,11 @@ config GPIO_PISOSR GPIO driver for SPI compatible parallel-in/serial-out shift registers. These are input only devices. +config GPIO_XRA1403 + tristate "EXAR XRA1403 16-bit GPIO expander" + help + GPIO driver for EXAR XRA1403 16-bit SPI-based GPIO expander. + endmenu menu "SPI or I2C GPIO expanders" diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 68b9627..4d9adc6 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -141,6 +141,7 @@ obj-$(CONFIG_GPIO_XGENE)+= gpio-xgene.o obj-$(CONFIG_GPIO_XGENE_SB)+= gpio-xgene-sb.o obj-$(CONFIG_GPIO_XILINX) += gpio-xilinx.o obj-$(CONFIG_GPIO_XLP) += gpio-xlp.o +obj-$(CONFIG_GPIO_XRA1403) += gpio-xra1403.o obj-$(CONFIG_GPIO_XTENSA) += gpio-xtensa.o obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o obj-$(CONFIG_GPIO_ZYNQ)+= gpio-zynq.o diff --git a/drivers/gpio/gpio-xra1403.c b/drivers/gpio/gpio-xra1403.c new file mode 100644 index 000..0230e4b --- /dev/null +++ b/drivers/gpio/gpio-xra1403.c @@ -0,0 +1,237 @@ +/* + * GPIO driver for EXAR XRA1403 16-bit GPIO expander + * + * Copyright (c) 2017, General Electric Company + * + * 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, see <http://www.gnu.org/licenses/>. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* XRA1403 registers */ +#define XRA_GSR 0x00 /* GPIO State */ +#define XRA_OCR 0x02 /* Output Control */ +#define XRA_PIR 0x04 /* Input Polarity Inversion */ +#define XRA_GCR 0x06 /* GPIO Configuration */ +#define XRA_PUR 0x08 /* Input Internal Pull-up Resistor Enable/Disable */ +#define XRA_IER 0x0A /* Input Interrupt Enable */ +#define XRA_TSCR 0x0C /* Output Three-State Control */ +#define XRA_ISR 0x0E /* Input Interrupt Status */ +#define XRA_REIR 0x10 /* Input Rising Edge Interrupt Enable */ +#define XRA_FEIR 0x12 /* Input Falling Edge Interrupt Enable */ +#define XRA_IFR 0x14 /* Input Filter Enable/Disable */ + +struct xra1403 { + struct gpio_chip chip; + struct regmap *regmap; +}; + +static const struct regmap_config xra1403_regmap_cfg = { + .reg_bits = 7, + .pad_bits = 1, + .val_bits = 8, + + .max_register = XRA_IFR | 0x01, +}; + +static unsigned int to_reg(unsigned int reg, unsigned int offset) +{ + return reg + (offset > 7); +} + +static int xra1403_direction_input(struct gpio_chip *chip, unsigned int offset) +{ + struct xra1403 *xra = gpiochip_get_data(chip); + + return regmap_update_bits(xra->regmap, to_reg(XRA_GCR, offset), + BIT(offset % 8), BIT(offset % 8)); +} + +static int xra1403_direction_output(struct gpio_chip *chip, unsigned int offset, + int value) +{ + int ret; + struct xra1403 *xra = gpiochip_get_data(chip); + + ret = regmap_update_bits(xra->regmap, to_reg(XRA_GCR, offset), + BIT(offset % 8), 0); + if (ret) + return ret; + + ret = regmap_update_bits(xra->regmap, to_reg(XRA_OCR, offset), + BIT(offset % 8), value ? BIT(offset % 8) : 0); + + return ret; +} + +static int xra1403_get_direction(struct gpio_chip *chip, unsigned int offset) +{ + int ret; + unsigned int val; + struct xra1403 *xra = gpiochip_get_data(chip); + + ret = regmap_read(xra->regmap, to_reg(XRA_GCR, offset), &val); + if (ret) + return ret; + + return !!(val & BIT(offset % 8)); +} + +static int xra1403_get(struct gpio_chip *chip, unsigned int offset) +{ + int ret; + unsigned int val; + struct xra1403 *xra = gpiochip_get_data(chip); + + ret = regma
[PATCH v2 4/4] Add XRA1403 support to MAINTAINERS file
Add XRA1403 support to MAINTAINERS list. Signed-off-by: Nandor Han --- MAINTAINERS | 8 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 892e958..0c5b984 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14013,6 +14013,14 @@ L: linux-kernel@vger.kernel.org S: Supported F: drivers/char/xillybus/ +XRA1403 GPIO EXPANDER +M: Nandor Han +M: Semi Malinen +L: linux-g...@vger.kernel.org +S: Maintained +F: drivers/gpio/gpio-xra1403.c +F: Documentation/devicetree/bindings/gpio/gpio-xra1403.txt + XTENSA XTFPGA PLATFORM SUPPORT M: Max Filippov L: linux-xte...@linux-xtensa.org -- 2.10.1
[PATCH v2 2/4] gpio - Add EXAR XRA1403 SPI GPIO expander driver
This is a simple driver that provides a /sys/class/gpio interface for controlling and configuring the GPIO lines. It does not provide support for chip select or interrupts. Signed-off-by: Nandor Han Signed-off-by: Semi Malinen --- drivers/gpio/Kconfig| 5 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xra1403.c | 236 3 files changed, 242 insertions(+) create mode 100644 drivers/gpio/gpio-xra1403.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 63ceed2..53cbe9b 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1214,6 +1214,11 @@ config GPIO_PISOSR GPIO driver for SPI compatible parallel-in/serial-out shift registers. These are input only devices. +config GPIO_XRA1403 + tristate "EXAR XRA1403 16-bit GPIO expander" + help + GPIO driver for EXAR XRA1403 16-bit SPI-based GPIO expander. + endmenu menu "SPI or I2C GPIO expanders" diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 095598e..76dc3d7 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -140,6 +140,7 @@ obj-$(CONFIG_GPIO_XGENE)+= gpio-xgene.o obj-$(CONFIG_GPIO_XGENE_SB)+= gpio-xgene-sb.o obj-$(CONFIG_GPIO_XILINX) += gpio-xilinx.o obj-$(CONFIG_GPIO_XLP) += gpio-xlp.o +obj-$(CONFIG_GPIO_XRA1403) += gpio-xra1403.o obj-$(CONFIG_GPIO_XTENSA) += gpio-xtensa.o obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o obj-$(CONFIG_GPIO_ZYNQ)+= gpio-zynq.o diff --git a/drivers/gpio/gpio-xra1403.c b/drivers/gpio/gpio-xra1403.c new file mode 100644 index 000..e6966d9 --- /dev/null +++ b/drivers/gpio/gpio-xra1403.c @@ -0,0 +1,236 @@ +/* + * GPIO driver for EXAR XRA1403 16-bit GPIO expander + * + * Copyright (c) 2017, General Electric Company + * + * 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, see <http://www.gnu.org/licenses/>. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* XRA1403 registers */ +#define XRA_GSR 0x00 /* GPIO State */ +#define XRA_OCR 0x02 /* Output Control */ +#define XRA_PIR 0x04 /* Input Polarity Inversion */ +#define XRA_GCR 0x06 /* GPIO Configuration */ +#define XRA_PUR 0x08 /* Input Internal Pull-up Resistor Enable/Disable */ +#define XRA_IER 0x0A /* Input Interrupt Enable */ +#define XRA_TSCR 0x0C /* Output Three-State Control */ +#define XRA_ISR 0x0E /* Input Interrupt Status */ +#define XRA_REIR 0x10 /* Input Rising Edge Interrupt Enable */ +#define XRA_FEIR 0x12 /* Input Falling Edge Interrupt Enable */ +#define XRA_IFR 0x14 /* Input Filter Enable/Disable */ + +struct xra1403 { + struct gpio_chip chip; + struct regmap *regmap; +}; + +static const struct regmap_config xra1403_regmap_cfg = { + .reg_bits = 7, + .pad_bits = 1, + .val_bits = 8, + + .max_register = XRA_IFR | 0x01, +}; + +static unsigned int to_reg(unsigned int reg, unsigned int offset) +{ + return reg + (offset > 7); +} + +static int xra1403_direction_input(struct gpio_chip *chip, unsigned int offset) +{ + struct xra1403 *xra = gpiochip_get_data(chip); + + return regmap_update_bits(xra->regmap, to_reg(XRA_GCR, offset), + BIT(offset % 8), BIT(offset % 8)); +} + +static int xra1403_direction_output(struct gpio_chip *chip, unsigned int offset, + int value) +{ + int ret; + struct xra1403 *xra = gpiochip_get_data(chip); + + ret = regmap_update_bits(xra->regmap, to_reg(XRA_GCR, offset), + BIT(offset % 8), 0); + if (ret) + return ret; + + ret = regmap_update_bits(xra->regmap, to_reg(XRA_OCR, offset), + BIT(offset % 8), value ? BIT(offset % 8) : 0); + + return ret; +} + +static int xra1403_get_direction(struct gpio_chip *chip, unsigned int offset) +{ + int ret; + unsigned int val; + struct xra1403 *xra = gpiochip_get_data(chip); + + ret = regmap_read(xra->regmap, to_reg(XRA_GCR, offset), &val); + if (ret) + return ret; + + return !!(val & BIT(offset % 8)); +} + +static int xra1403_get(struct gpio_chip *chip, unsigned int offset) +{ + int ret; + unsigned int val; + struct xra1403 *xra = gpiochip_get_data(chip); + +
[PATCH v2 0/4] XRA1403,gpio - add XRA1403 gpio expander driver
The patchset will add a driver to support basic functionality for XRA1403 device. Features supported: - get/set GPIO direction (input, output) - get/set GPIO level (low, high) Documentation: A gpio-xra1403.txt file was added to document the DTS bindings related to driver. Testing: 1.1 XRA1403 connected to iMX53 MCU 1.2 Use the GPIO tools provided by kernel from tools/gpio dir 2.1 `lsgpio` root@csmon ppd:~# lsgpio GPIO chip: gpiochip8, "xra1403", 16 GPIO lines line 0: unnamed unused [output] line 1: unnamed unused [output] line 2: unnamed unused [output] line 3: unnamed unused [output] line 4: unnamed unused [output] line 5: unnamed unused line 6: unnamed unused line 7: unnamed unused [output] line 8: unnamed unused [output] line 9: unnamed unused line 10: unnamed unused [output] line 11: unnamed unused line 12: unnamed unused line 13: unnamed unused line 14: unnamed unused line 15: unnamed unused [output] GPIO chip: gpiochip7, "xra1403", 16 GPIO lines line 0: unnamed unused line 1: unnamed unused line 2: unnamed unused line 3: unnamed unused line 4: unnamed unused line 5: unnamed unused line 6: unnamed unused line 7: unnamed unused line 8: unnamed unused line 9: unnamed unused line 10: unnamed unused line 11: unnamed unused line 12: unnamed unused line 13: unnamed unused line 14: unnamed unused line 15: unnamed unused 3.1 `gpio-hammer` root@csmon ppd:~# gpio-hammer -n gpiochip8 -o0 -o1 -o2 -o3 Hammer lines [0, 1, 2, 3] on gpiochip8, initial states: [0, 0, 0, 0] [\] [0: 0, 1: 0, 2: 0, 3: 0] ... [\] [0: 1, 1: 1, 2: 1, 3: 1] When using `gpio-hammer` I also attached an oscilloscope to one of the pins and I was able to monitor and validate the GPIO status. Nandor Han (4): dt-bindings: gpio - add exar to vendor prefixes list gpio - Add EXAR XRA1403 SPI GPIO expander driver doc,dts - add XRA1403 DTS binding documentation Add XRA1403 support to MAINTAINERS file .../devicetree/bindings/gpio/gpio-xra1403.txt | 46 .../devicetree/bindings/vendor-prefixes.txt| 1 + MAINTAINERS| 8 + drivers/gpio/Kconfig | 5 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xra1403.c| 236 + 6 files changed, 297 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xra1403.txt create mode 100644 drivers/gpio/gpio-xra1403.c -- 2.10.1
[PATCH v2 1/4] dt-bindings: gpio - add exar to vendor prefixes list
Add Exar Corporation to vendors list. Signed-off-by: Nandor Han --- Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index c7d1b72..ba9058c 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -104,6 +104,7 @@ ettus NI Ettus Research eukrea Eukréa Electromatique everestEverest Semiconductor Co. Ltd. everspin Everspin Technologies, Inc. +exar Exar Corporation excito Excito ezchip EZchip Semiconductor faradayFaraday Technology Corporation -- 2.10.1
[PATCH v2 3/4] doc,dts - add XRA1403 DTS binding documentation
Add the XRA1403 DTS binding documentation. Signed-off-by: Nandor Han --- .../devicetree/bindings/gpio/gpio-xra1403.txt | 46 ++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xra1403.txt diff --git a/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt b/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt new file mode 100644 index 000..e13cc39 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt @@ -0,0 +1,46 @@ +GPIO Driver for XRA1403 16-BIT GPIO Expander With Reset Input from EXAR + +The XRA1403 is an 16-bit GPIO expander with an SPI interface. Features available: + - Individually programmable inputs: + - Internal pull-up resistors + - Polarity inversion + - Individual interrupt enable + - Rising edge and/or Falling edge interrupt + - Input filter + - Individually programmable outputs + - Output Level Control + - Output Three-State Control + +Properties +-- +Check documentation for SPI and GPIO controllers regarding properties needed to configure the node. + + - compatible = "exar,xra1403". + - reg - SPI id of the device. + - gpio-controller - marks the node as gpio. + - #gpio-cells - should be two where the first cell is the pin number + and the second one is used for optional parameters. + +Optional properties: +--- + - reset-gpios: in case available used to control the device reset line. + - interrupt-controller - marks the node as interrupt controller. + - #interrupt-cells - should be two and represents the number of cells + needed to encode interrupt source. + +Example + + + gpioxra0: gpio@2 { + compatible = "exar,xra1403"; + reg = <2>; + + gpio-controller; + #gpio-cells = <2>; + + interrupt-controller; + #interrupt-cells = <2>; + + reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; + spi-max-frequency = <100>; + }; -- 2.10.1
[PATCH 2/3] doc,dts - add XRA1403 DTS binding documentation
Add the XRA1403 DTS binding documentation. Signed-off-by: Nandor Han --- .../devicetree/bindings/gpio/gpio-xra1403.txt | 37 ++ 1 file changed, 37 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xra1403.txt diff --git a/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt b/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt new file mode 100644 index 000..ccf5337 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt @@ -0,0 +1,37 @@ +GPIO Driver for XRA1403 16-BIT GPIO Expander With Reset Input from EXAR + +The XRA1403 is an 16-bit GPIO expander with an SPI interface. Features available: + - Individually programmable inputs: + - Internal pull-up resistors + - Polarity inversion + - Individual interrupt enable + - Rising edge and/or Falling edge interrupt + - Input filter + - Individually programmable outputs + - Output Level Control + - Output Three-State Control + +Properties +-- +Check documentation for SPI and GPIO controllers regarding properties needed to configure the node. + + - compatible = "exar,xra1403". + - reg = SPI id of the device. + - gpio-controller: mark the node as gpio. + +Optional properties: +--- + - reset-gpios: in case available used to control the device reset line. + +Example + + + gpioxra0: gpio@2 { + compatible = "exar,xra1403"; + reg = <2>; + gpio-controller; + #gpio-cells = <2>; + reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; + spi-max-frequency = <100>; + status = "okay"; + }; -- 2.10.1
[PATCH 0/3] XRA1403,gpio - add XRA1403 gpio expander driver
The patchset will add a driver to support basic functionality for XRA1403 device. Features supported: - configure gpin as input/out - get/set gpio status Documentation: A gpio-xra1403.txt file was added to document the DTS bindings related to driver. Testing: 1. XRA1403 connected to iMX53 MCU 2. Export gpio from userspace 3. Verify that corresponding gpio directories are created in `/sys/class/gpio/gpioXX` 4. Export gpios from first and second bank as output 5. Set the output gpio pin to high/low and verify with the oscilloscope that gpio status is according with the configured value. Nandor Han (3): gpio - Add EXAR XRA1403 SPI GPIO expander driver doc,dts - add XRA1403 DTS binding documentation Add XRA1403 support to MAINTAINERS file .../devicetree/bindings/gpio/gpio-xra1403.txt | 37 +++ .../devicetree/bindings/vendor-prefixes.txt| 1 + MAINTAINERS| 8 + drivers/gpio/Kconfig | 5 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xra1403.c| 252 + 6 files changed, 304 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xra1403.txt create mode 100644 drivers/gpio/gpio-xra1403.c -- 2.10.1
[PATCH 3/3] Add XRA1403 support to MAINTAINERS file
Add the XRA1403 support to MAINTAINERS list. Signed-off-by: Nandor Han --- MAINTAINERS | 8 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 58b3a22..539c88c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13903,6 +13903,14 @@ L: linux-kernel@vger.kernel.org S: Supported F: drivers/char/xillybus/ +XRA1403 GPIO EXPANDER +M: Nandor Han +M: Semi Malinen +L: linux-g...@vger.kernel.org +S: Maintained +F: drivers/gpio/gpio-xra1403.c +F: Documentation/devicetree/bindings/gpio/gpio-xra1403.txt + XTENSA XTFPGA PLATFORM SUPPORT M: Max Filippov L: linux-xte...@linux-xtensa.org -- 2.10.1
[PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
This is a simple driver that provides a /sys/class/gpio interface for controlling and configuring the GPIO lines. It does not provide support for chip select or interrupts. Signed-off-by: Nandor Han Signed-off-by: Semi Malinen --- .../devicetree/bindings/vendor-prefixes.txt| 1 + drivers/gpio/Kconfig | 5 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-xra1403.c| 252 + 4 files changed, 259 insertions(+) create mode 100644 drivers/gpio/gpio-xra1403.c diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 0ad67d5..7ca9d41 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -103,6 +103,7 @@ ettus NI Ettus Research eukrea Eukréa Electromatique everestEverest Semiconductor Co. Ltd. everspin Everspin Technologies, Inc. +exar Exar Corporation excito Excito ezchip EZchip Semiconductor faradayFaraday Technology Corporation diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index d6e3cfd..3a6c9a3 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1208,6 +1208,11 @@ config GPIO_PISOSR GPIO driver for SPI compatible parallel-in/serial-out shift registers. These are input only devices. +config GPIO_XRA1403 + tristate "EXAR XRA1403 16-bit GPIO expander" + help + GPIO driver for EXAR XRA1403 16-bit SPI-based GPIO expander. + endmenu menu "SPI or I2C GPIO expanders" diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index bd995dc..8f50844 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -139,6 +139,7 @@ obj-$(CONFIG_GPIO_XGENE)+= gpio-xgene.o obj-$(CONFIG_GPIO_XGENE_SB)+= gpio-xgene-sb.o obj-$(CONFIG_GPIO_XILINX) += gpio-xilinx.o obj-$(CONFIG_GPIO_XLP) += gpio-xlp.o +obj-$(CONFIG_GPIO_XRA1403) += gpio-xra1403.o obj-$(CONFIG_GPIO_XTENSA) += gpio-xtensa.o obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o obj-$(CONFIG_GPIO_ZYNQ)+= gpio-zynq.o diff --git a/drivers/gpio/gpio-xra1403.c b/drivers/gpio/gpio-xra1403.c new file mode 100644 index 000..1b7138a8 --- /dev/null +++ b/drivers/gpio/gpio-xra1403.c @@ -0,0 +1,252 @@ +/* + * GPIO driver for EXAR XRA1403 16-bit GPIO expander + * + * Copyright (c) 2017, General Electric Company + * + * 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, see <http://www.gnu.org/licenses/>. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +/* XRA1403 registers */ +#define XRA_GSR 0x00 /* GPIO State */ +#define XRA_OCR 0x02 /* Output Control */ +#define XRA_GCR 0x06 /* GPIO Configuration */ + +/* SPI headers */ +#define XRA_READ 0x80 /* read bit of the SPI command byte */ + +struct xra1403 { + struct mutex lock; + struct gpio_chip chip; + struct spi_device *spi; +}; + +static int xra1403_get_byte(struct xra1403 *xra, unsigned int addr) +{ + return spi_w8r8(xra->spi, XRA_READ | (addr << 1)); +} + +static int xra1403_get_bit(struct xra1403 *xra, unsigned int addr, + unsigned int bit) +{ + int ret; + + ret = xra1403_get_byte(xra, addr + (bit > 7)); + if (ret < 0) + return ret; + + return !!(ret & BIT(bit % 8)); +} + +static int xra1403_set_bit(struct xra1403 *xra, unsigned int addr, + unsigned int bit, int value) +{ + int ret; + u8 mask; + u8 tx[2]; + + addr += bit > 7; + + mutex_lock(&xra->lock); + + ret = xra1403_get_byte(xra, addr); + if (ret < 0) + goto out_unlock; + + mask = BIT(bit % 8); + if (value) + value = ret | mask; + else + value = ret & ~mask; + + if (value != ret) { + tx[0] = addr << 1; + tx[1] = value; + ret = spi_write(xra->spi, tx, sizeof(tx)); + } else { + ret = 0; + } + +out_unlock: + mutex_unlock(&xra->lock); + + return ret; +} + +static int xra1403_direction_input(struct gpio_chip *chip, unsigned int offset) +{ + return xra1403_set_bit(gpiochip_get_data(chip), XRA_GCR, offset, 1); +} + +st
[PATCH 1/1] dmaengine: imx-sdma - correct the dma transfer residue calculation
The residue calculation was taking in consideration that dma transaction status will be always retrieved in the dma callback used to inform that dma transfer is complete. However this is not the case for all subsystems that use dma. Some subsystems use a timer to check the dma status periodically. Therefore the calculation was updated and residue is calculated accordingly by a) update the residue calculation taking in consideration the last used buffer index by using *buf_ptail* variable and b) chn_real_count (number of bytes transferred) is initialized to zero, when dma channel is created, to avoid using an uninitialized value in residue calculation when dma status is checked without waiting dma complete event. Signed-off-by: Nandor Han --- drivers/dma/imx-sdma.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index b9629b2..d1651a5 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -298,6 +298,7 @@ struct sdma_engine; * @event_id1 for channels that use 2 events * @word_size peripheral access size * @buf_tail ID of the buffer that was processed + * @buf_ptail ID of the previous buffer that was processed * @num_bd max NUM_BD. number of descriptors currently handling */ struct sdma_channel { @@ -309,6 +310,7 @@ struct sdma_channel { unsigned intevent_id1; enum dma_slave_buswidth word_size; unsigned intbuf_tail; + unsigned intbuf_ptail; unsigned intnum_bd; unsigned intperiod_len; struct sdma_buffer_descriptor *bd; @@ -700,6 +702,8 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) sdmac->chn_real_count = bd->mode.count; bd->mode.status |= BD_DONE; bd->mode.count = sdmac->period_len; + sdmac->buf_ptail = sdmac->buf_tail; + sdmac->buf_tail = (sdmac->buf_tail + 1) % sdmac->num_bd; /* * The callback is called from the interrupt context in order @@ -710,9 +714,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) dmaengine_desc_get_callback_invoke(&sdmac->desc, NULL); - sdmac->buf_tail++; - sdmac->buf_tail %= sdmac->num_bd; - if (error) sdmac->status = old_status; } @@ -1186,6 +1187,8 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg( sdmac->flags = 0; sdmac->buf_tail = 0; + sdmac->buf_ptail = 0; + sdmac->chn_real_count = 0; dev_dbg(sdma->dev, "setting up %d entries for channel %d.\n", sg_len, channel); @@ -1288,6 +1291,8 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic( sdmac->status = DMA_IN_PROGRESS; sdmac->buf_tail = 0; + sdmac->buf_ptail = 0; + sdmac->chn_real_count = 0; sdmac->period_len = period_len; sdmac->flags |= IMX_DMA_SG_LOOP; @@ -1385,7 +1390,7 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan, u32 residue; if (sdmac->flags & IMX_DMA_SG_LOOP) - residue = (sdmac->num_bd - sdmac->buf_tail) * + residue = (sdmac->num_bd - sdmac->buf_ptail) * sdmac->period_len - sdmac->chn_real_count; else residue = sdmac->chn_count - sdmac->chn_real_count; -- 2.7.1
[PATCH 0/1] dmaengine: imx-sdma - fix the dma residue calculation
The patch will correct the calculation of the dma residue taking in consideration that some subsystem can check the dma status without waiting the dma complete event. Affected devices: imx based device using sdma module with cyclic channels. Tested on imx6 and imx53 by playing wav files using `speaker-test`application, part of alsa-utils package, and verify that the sound plays smoothly without interruptions. Verified also the serial communication by transferring data over the serial line using various packet sizes and checked the logs and the serial port status (`cat /proc/tty/driver/IMX-uart`) that no data is lost. Nandor Han (1): dmaengine: imx-sdma - correct the dma transfer residue calculation drivers/dma/imx-sdma.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) -- 2.7.1
[PATCH 4/4] serial: imx-serial - update RX error counters when DMA is used
Update error counters when DMA is used for receiving data. Do this by using DMA transaction error event instead error interrupts to reduce interrupt load. Tested-by: Peter Senna Tschudin Acked-by: Peter Senna Tschudin Signed-off-by: Nandor Han --- drivers/tty/serial/imx.c | 32 1 file changed, 32 insertions(+) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 1912136..08ccfe1 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -704,6 +704,7 @@ out: return IRQ_HANDLED; } +static void clear_rx_errors(struct imx_port *sport); static int start_rx_dma(struct imx_port *sport); /* * If the RXFIFO is filled with some data, and then we @@ -729,6 +730,11 @@ static void imx_dma_rxint(struct imx_port *sport) temp &= ~(UCR2_ATEN); writel(temp, sport->port.membase + UCR2); + /* disable the rx errors interrupts */ + temp = readl(sport->port.membase + UCR4); + temp &= ~UCR4_OREN; + writel(temp, sport->port.membase + UCR4); + /* tell the DMA to receive the data. */ start_rx_dma(sport); } @@ -961,6 +967,7 @@ static void dma_rx_callback(void *data) if (status == DMA_ERROR) { dev_err(sport->port.dev, "DMA transaction error.\n"); + clear_rx_errors(sport); return; } @@ -1057,6 +1064,31 @@ static int start_rx_dma(struct imx_port *sport) return 0; } +static void clear_rx_errors(struct imx_port *sport) +{ + unsigned int status_usr1, status_usr2; + + status_usr1 = readl(sport->port.membase + USR1); + status_usr2 = readl(sport->port.membase + USR2); + + if (status_usr2 & USR2_BRCD) { + sport->port.icount.brk++; + writel(USR2_BRCD, sport->port.membase + USR2); + } else if (status_usr1 & USR1_FRAMERR) { + sport->port.icount.frame++; + writel(USR1_FRAMERR, sport->port.membase + USR1); + } else if (status_usr1 & USR1_PARITYERR) { + sport->port.icount.parity++; + writel(USR1_PARITYERR, sport->port.membase + USR1); + } + + if (status_usr2 & USR2_ORE) { + sport->port.icount.overrun++; + writel(USR2_ORE, sport->port.membase + USR2); + } + +} + #define TXTL_DEFAULT 2 /* reset default */ #define RXTL_DEFAULT 1 /* reset default */ #define TXTL_DMA 8 /* DMA burst setting */ -- 2.8.3
[PATCH 2/4] dmaengine: imx-sdma - update the residue calculation for cyclic channels
The calculation of the DMA transaction residue supports only fixed size data transfers. This implementation is not covering all operations (e.g. data receiving) when we need to know the exact amount of bytes transferred. The loop channels handling was changed to clear the buffer descriptor errors and use the bd->mode.count to calculate the residue. Tested-by: Peter Senna Tschudin Acked-by: Peter Senna Tschudin Reviewed-by: Vinod Koul Signed-off-by: Nandor Han --- drivers/dma/imx-sdma.c | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index aa35a77..3cb4738 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -651,6 +651,8 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event) static void sdma_update_channel_loop(struct sdma_channel *sdmac) { struct sdma_buffer_descriptor *bd; + int error = 0; + enum dma_status old_status = sdmac->status; /* * loop mode. Iterate over descriptors, re-setup them and @@ -662,10 +664,20 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) if (bd->mode.status & BD_DONE) break; - if (bd->mode.status & BD_RROR) + if (bd->mode.status & BD_RROR) { + bd->mode.status &= ~BD_RROR; sdmac->status = DMA_ERROR; + error = -EIO; + } + /* + * We use bd->mode.count to calculate the residue, since contains + * the number of bytes present in the current buffer descriptor. + */ + + sdmac->chn_real_count = bd->mode.count; bd->mode.status |= BD_DONE; + bd->mode.count = sdmac->period_len; /* * The callback is called from the interrupt context in order @@ -679,6 +691,9 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) sdmac->buf_tail++; sdmac->buf_tail %= sdmac->num_bd; + + if (error) + sdmac->status = old_status; } } @@ -1349,7 +1364,8 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan, u32 residue; if (sdmac->flags & IMX_DMA_SG_LOOP) - residue = (sdmac->num_bd - sdmac->buf_tail) * sdmac->period_len; + residue = (sdmac->num_bd - sdmac->buf_tail) * + sdmac->period_len - sdmac->chn_real_count; else residue = sdmac->chn_count - sdmac->chn_real_count; -- 2.8.3
[PATCH 1/4] dmaengine: imx-sdma - reduce transfer latency for DMA cyclic clients
Having the SDMA driver use a tasklet for running the clients callback introduce some issues: - probability to have desynchronized data because of the race condition created since the DMA transaction status is retrieved only when the callback is executed, leaving plenty of time for transaction status to get altered. - inter-transfer latency which can leave channels idle. Move the callback execution, for cyclic channels, to SDMA interrupt (as advised in `Documentation/dmaengine/provider.txt`) to (a)reduce the inter-transfer latency and (b) eliminate the race condition possibility where DMA transaction status might be changed by the time is read. The responsibility of the SDMA interrupt latency is moved to the SDMA clients which case by case should defer the work to bottom-halves when needed. Tested-by: Peter Senna Tschudin Acked-by: Peter Senna Tschudin Reviewed-by: Vinod Koul Signed-off-by: Nandor Han --- drivers/dma/imx-sdma.c | 36 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 03ec76f..aa35a77 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -648,12 +648,6 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event) writel_relaxed(val, sdma->regs + chnenbl); } -static void sdma_handle_channel_loop(struct sdma_channel *sdmac) -{ - if (sdmac->desc.callback) - sdmac->desc.callback(sdmac->desc.callback_param); -} - static void sdma_update_channel_loop(struct sdma_channel *sdmac) { struct sdma_buffer_descriptor *bd; @@ -672,13 +666,25 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) sdmac->status = DMA_ERROR; bd->mode.status |= BD_DONE; + + /* +* The callback is called from the interrupt context in order +* to reduce latency and to avoid the risk of altering the +* SDMA transaction status by the time the client tasklet is +* executed. +*/ + + if (sdmac->desc.callback) + sdmac->desc.callback(sdmac->desc.callback_param); + sdmac->buf_tail++; sdmac->buf_tail %= sdmac->num_bd; } } -static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac) +static void mxc_sdma_handle_channel_normal(unsigned long data) { + struct sdma_channel *sdmac = (struct sdma_channel *) data; struct sdma_buffer_descriptor *bd; int i, error = 0; @@ -705,16 +711,6 @@ static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac) sdmac->desc.callback(sdmac->desc.callback_param); } -static void sdma_tasklet(unsigned long data) -{ - struct sdma_channel *sdmac = (struct sdma_channel *) data; - - if (sdmac->flags & IMX_DMA_SG_LOOP) - sdma_handle_channel_loop(sdmac); - else - mxc_sdma_handle_channel_normal(sdmac); -} - static irqreturn_t sdma_int_handler(int irq, void *dev_id) { struct sdma_engine *sdma = dev_id; @@ -731,8 +727,8 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id) if (sdmac->flags & IMX_DMA_SG_LOOP) sdma_update_channel_loop(sdmac); - - tasklet_schedule(&sdmac->tasklet); + else + tasklet_schedule(&sdmac->tasklet); __clear_bit(channel, &stat); } @@ -1732,7 +1728,7 @@ static int sdma_probe(struct platform_device *pdev) dma_cookie_init(&sdmac->chan); sdmac->channel = i; - tasklet_init(&sdmac->tasklet, sdma_tasklet, + tasklet_init(&sdmac->tasklet, mxc_sdma_handle_channel_normal, (unsigned long) sdmac); /* * Add the channel to the DMAC list. Do not add channel 0 though -- 2.8.3
[PATCH 0/4] serial,dmaengine: use DMA cyclic for IMX UART driver
Update the IMX UART driver to use cyclic DMA in order to avoid losing serial data because of DMA start latency. Support variable data length transfers for cyclic channels in IMX SDMA(Smart DMA) driver and, as specified in "Documentation/dmaengine/provider.txt" section "General Design Notes", run the SDMA channel callback directly from the SDMA interrupt context. Every commit message will explain the changes. Impacted devices: IMX6i,IMX53 CPU: - clients of the IMX UART that have DMA enabled. - cyclic SDMA clients (Note:Not able to find others than IMX UART driver) Functionality tested by connecting the m53evk to my laptop using a USB-serial converter and start sending, from my laptop 800 bytes long packets at 8ms interval. On the m53evk with interrupt load, generated by connecting/disconnecting at 3s interval a USB HUB having some devices connected (~4), I receive serial data in a loop and check that no overruns are created using command: `watch -n1 cat /proc/tty/driver/IMX-uart` Also done tests were I check how the data is received when sending packets bigger than a buffer descriptor size (1024). In my tests I have sent 4 packets (1028, 2048, 2048, 2048) and check that 7172 bytes were received using the command: `watch -n1 cat /proc/tty/driver/IMX-uart` Result: serinfo:1.0 driver revision: 2: uart:IMX mmio:0x5000C000 irq:49 tx:0 rx:7172 RTS|CTS|DTR|DSR|CD Nandor Han (4): dmaengine: imx-sdma - reduce transfer latency for DMA cyclic clients dmaengine: imx-sdma - update the residue calculation for cyclic channels serial: imx-serial - update UART IMX driver to use cyclic DMA serial: imx-serial - update RX error counters when DMA is used drivers/dma/imx-sdma.c | 56 +-- drivers/tty/serial/imx.c | 173 ++- 2 files changed, 144 insertions(+), 85 deletions(-) -- 2.8.3
[PATCH 3/4] serial: imx-serial - update UART IMX driver to use cyclic DMA
The IMX UART has a 32 bytes HW buffer which can be filled up in 2777us at 115200 baud or 80us at 4Mbaud (supported by IMX53). Taking this in consideration there is a good probability to lose data because of the DMA startup latency. Our tests (explained below) indicates a latency up to 4400us when creating interrupt load and ~70us without. When creating interrupt load I was able to see continuous overrun errors by checking serial driver statistics using the command: `cat /proc/tty/driver/IMX-uart`. Replace manual restart of DMA with cyclic DMA to eliminate data loss due to DMA engine startup latency (similar approch to atmel_serial.c driver). As result the DMA engine will start on the first serial data transfer and stops only when serial port is closed. Tests environment: Using the m53evk board I have used a GPIO for profiling the IMX serial driver. - The RX line and GPIO were connected to oscilloscope. - Run a small test program on the m53evk board that will only open and read data from ttymxc2 port. - Connect the ttymxc2 port to my laptop using a USB serial converter where another test program is running, able to send configurable packet lengths and intervals. - Serial ports configured at 115200 8N1. - Interrupts load created by disconnecting/connecting (3s interval) a USB hub, using a digital switch, with 4 USB devices (USB-Serial converter, USB SD card, etc) connected. (around 160 interrupts/second generated) - The GPIO was toggled HI in the `imx_int` when USR1_RRDY or USR1_AGTIM events are received and toggled back, once the DMA configuration is finalized, at the end of `imx_dma_rxint`. Measurements: The measurements were done from the end of the last byte (RX line) until the GPIO was toggled back LOW. Note: The GPIO toggling was done using `gpiod_set_value` method. Tests performed: 1. Sending 9 bytes packets at 8ms interval. Having the 9 bytes packets will activate the RRDY threshold event and IMX serial interrupt called. Results: - DMA start latency (interrupt start latency + DMA configuration) consistently 70us when system not loaded. - DMA start latency up to 4400us when system loaded. 2. Sending 40 bytes packet at 8mS interval. Results with load: - Able to observe overruns by running: `watch -n1 cat /proc/tty/driver/IMX-uart` Tested-by: Peter Senna Tschudin Acked-by: Peter Senna Tschudin Signed-off-by: Nandor Han --- drivers/tty/serial/imx.c | 141 ++- 1 file changed, 78 insertions(+), 63 deletions(-) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 0df2b1c..1912136 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -222,6 +222,9 @@ struct imx_port { struct dma_chan *dma_chan_rx, *dma_chan_tx; struct scatterlist rx_sgl, tx_sgl[2]; void*rx_buf; + struct circ_buf rx_ring; + unsigned intrx_periods; + dma_cookie_trx_cookie; unsigned inttx_bytes; unsigned intdma_tx_nents; wait_queue_head_t dma_wait; @@ -932,30 +935,6 @@ static void imx_timeout(unsigned long data) } #define RX_BUF_SIZE(PAGE_SIZE) -static void imx_rx_dma_done(struct imx_port *sport) -{ - unsigned long temp; - unsigned long flags; - - spin_lock_irqsave(&sport->port.lock, flags); - - /* re-enable interrupts to get notified when new symbols are incoming */ - temp = readl(sport->port.membase + UCR1); - temp |= UCR1_RRDYEN; - writel(temp, sport->port.membase + UCR1); - - temp = readl(sport->port.membase + UCR2); - temp |= UCR2_ATEN; - writel(temp, sport->port.membase + UCR2); - - sport->dma_is_rxing = 0; - - /* Is the shutdown waiting for us? */ - if (waitqueue_active(&sport->dma_wait)) - wake_up(&sport->dma_wait); - - spin_unlock_irqrestore(&sport->port.lock, flags); -} /* * There are two kinds of RX DMA interrupts(such as in the MX6Q): @@ -972,43 +951,75 @@ static void dma_rx_callback(void *data) struct scatterlist *sgl = &sport->rx_sgl; struct tty_port *port = &sport->port.state->port; struct dma_tx_state state; + struct circ_buf *rx_ring = &sport->rx_ring; enum dma_status status; - unsigned int count; - - /* unmap it first */ - dma_unmap_sg(sport->port.dev, sgl, 1, DMA_FROM_DEVICE); + unsigned int w_bytes = 0; + unsigned int r_bytes; + unsigned int bd_size; status = dmaengine_tx_status(chan, (dma_cookie_t)0, &state); - count = RX_BUF_SIZE - state.residue; - dev_dbg(sport->port.dev, "We get %d bytes.\n", count); + if (status == DMA_ERROR) { +
Re: EXT: Re: [PATCH RFC 1/4] dma: imx-sdma - reduce transfer latency for DMA cyclic clients
On 28/06/16 17:34, Vinod Koul wrote: On Thu, Jun 09, 2016 at 03:16:30PM +0300, Nandor Han wrote: Having the SDMA driver use a tasklet for running the clients callback introduce some issues: - probability to have desynchronized data because of the race condition created since the DMA transaction status is retrieved only when the callback is executed, leaving plenty of time for transaction status to get altered. - inter-transfer latency which can leave channels idle. Move the callback execution, for cyclic channels, to SDMA interrupt (as advised in `Documentation/dmaengine/provider.txt`) to (a)reduce the inter-transfer latency and (b) eliminate the race condition possibility where DMA transaction status might be changed by the time is read. The responsibility of the SDMA interrupt latency is moved to the SDMA clients which case by case should defer the work to bottom-halves when needed. Both of these look fine. Please change the patch titles to dmaengine: Are these going to be merged thru dmaengine tree or serial one? I will send soon a V2 where I will fix the titles. If you are OK with all the patchset it can be merged to dmaengine tree, otherwise probably goes to serial one. Let me know which is the best option. Thanks, Nandor Signed-off-by: Nandor Han --- drivers/dma/imx-sdma.c | 36 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 0f6fd42..e497847 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -654,12 +654,6 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event) writel_relaxed(val, sdma->regs + chnenbl); } -static void sdma_handle_channel_loop(struct sdma_channel *sdmac) -{ - if (sdmac->desc.callback) - sdmac->desc.callback(sdmac->desc.callback_param); -} - static void sdma_update_channel_loop(struct sdma_channel *sdmac) { struct sdma_buffer_descriptor *bd; @@ -678,13 +672,25 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) sdmac->status = DMA_ERROR; bd->mode.status |= BD_DONE; + + /* +* The callback is called from the interrupt context in order +* to reduce latency and to avoid the risk of altering the +* SDMA transaction status by the time the client tasklet is +* executed. +*/ + + if (sdmac->desc.callback) + sdmac->desc.callback(sdmac->desc.callback_param); + sdmac->buf_tail++; sdmac->buf_tail %= sdmac->num_bd; } } -static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac) +static void mxc_sdma_handle_channel_normal(unsigned long data) { + struct sdma_channel *sdmac = (struct sdma_channel *) data; struct sdma_buffer_descriptor *bd; int i, error = 0; @@ -711,16 +717,6 @@ static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac) sdmac->desc.callback(sdmac->desc.callback_param); } -static void sdma_tasklet(unsigned long data) -{ - struct sdma_channel *sdmac = (struct sdma_channel *) data; - - if (sdmac->flags & IMX_DMA_SG_LOOP) - sdma_handle_channel_loop(sdmac); - else - mxc_sdma_handle_channel_normal(sdmac); -} - static irqreturn_t sdma_int_handler(int irq, void *dev_id) { struct sdma_engine *sdma = dev_id; @@ -737,8 +733,8 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id) if (sdmac->flags & IMX_DMA_SG_LOOP) sdma_update_channel_loop(sdmac); - - tasklet_schedule(&sdmac->tasklet); + else + tasklet_schedule(&sdmac->tasklet); __clear_bit(channel, &stat); } @@ -1739,7 +1735,7 @@ static int sdma_probe(struct platform_device *pdev) dma_cookie_init(&sdmac->chan); sdmac->channel = i; - tasklet_init(&sdmac->tasklet, sdma_tasklet, + tasklet_init(&sdmac->tasklet, mxc_sdma_handle_channel_normal, (unsigned long) sdmac); /* * Add the channel to the DMAC list. Do not add channel 0 though -- 2.8.3
[PATCH RFC 1/4] dma: imx-sdma - reduce transfer latency for DMA cyclic clients
Having the SDMA driver use a tasklet for running the clients callback introduce some issues: - probability to have desynchronized data because of the race condition created since the DMA transaction status is retrieved only when the callback is executed, leaving plenty of time for transaction status to get altered. - inter-transfer latency which can leave channels idle. Move the callback execution, for cyclic channels, to SDMA interrupt (as advised in `Documentation/dmaengine/provider.txt`) to (a)reduce the inter-transfer latency and (b) eliminate the race condition possibility where DMA transaction status might be changed by the time is read. The responsibility of the SDMA interrupt latency is moved to the SDMA clients which case by case should defer the work to bottom-halves when needed. Signed-off-by: Nandor Han --- drivers/dma/imx-sdma.c | 36 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 0f6fd42..e497847 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -654,12 +654,6 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event) writel_relaxed(val, sdma->regs + chnenbl); } -static void sdma_handle_channel_loop(struct sdma_channel *sdmac) -{ - if (sdmac->desc.callback) - sdmac->desc.callback(sdmac->desc.callback_param); -} - static void sdma_update_channel_loop(struct sdma_channel *sdmac) { struct sdma_buffer_descriptor *bd; @@ -678,13 +672,25 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) sdmac->status = DMA_ERROR; bd->mode.status |= BD_DONE; + + /* +* The callback is called from the interrupt context in order +* to reduce latency and to avoid the risk of altering the +* SDMA transaction status by the time the client tasklet is +* executed. +*/ + + if (sdmac->desc.callback) + sdmac->desc.callback(sdmac->desc.callback_param); + sdmac->buf_tail++; sdmac->buf_tail %= sdmac->num_bd; } } -static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac) +static void mxc_sdma_handle_channel_normal(unsigned long data) { + struct sdma_channel *sdmac = (struct sdma_channel *) data; struct sdma_buffer_descriptor *bd; int i, error = 0; @@ -711,16 +717,6 @@ static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac) sdmac->desc.callback(sdmac->desc.callback_param); } -static void sdma_tasklet(unsigned long data) -{ - struct sdma_channel *sdmac = (struct sdma_channel *) data; - - if (sdmac->flags & IMX_DMA_SG_LOOP) - sdma_handle_channel_loop(sdmac); - else - mxc_sdma_handle_channel_normal(sdmac); -} - static irqreturn_t sdma_int_handler(int irq, void *dev_id) { struct sdma_engine *sdma = dev_id; @@ -737,8 +733,8 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id) if (sdmac->flags & IMX_DMA_SG_LOOP) sdma_update_channel_loop(sdmac); - - tasklet_schedule(&sdmac->tasklet); + else + tasklet_schedule(&sdmac->tasklet); __clear_bit(channel, &stat); } @@ -1739,7 +1735,7 @@ static int sdma_probe(struct platform_device *pdev) dma_cookie_init(&sdmac->chan); sdmac->channel = i; - tasklet_init(&sdmac->tasklet, sdma_tasklet, + tasklet_init(&sdmac->tasklet, mxc_sdma_handle_channel_normal, (unsigned long) sdmac); /* * Add the channel to the DMAC list. Do not add channel 0 though -- 2.8.3
[PATCH RFC 0/4] serial,dma: use DMA cyclic for IMX UART driver
Update the IMX UART driver to use cyclic DMA in order to avoid losing serial data because of DMA start latency. Support variable data length transfers for cyclic channels in IMX SDMA(Smart DMA) driver and, as specified in "Documentation/dmaengine/provider.txt" section "General Design Notes", run the SDMA channel callback directly from the SDMA interrupt context. Every commit message will explain the changes. Impacted devices: IMX6i,IMX53 CPU: - clients of the IMX UART that have DMA enabled. - cyclic SDMA clients (Note:Not able to find others than IMX UART driver) Functionality tested by connecting the m53evk to my laptop using a USB-serial converter and start sending, from my laptop 800 bytes long packets at 8ms interval. On the m53evk with interrupt load, generated by connecting/disconnecting at 3s interval a USB HUB having some devices connected (~4), I receive serial data in a loop and check that no overruns are created using command: `watch -n1 cat /proc/tty/driver/IMX-uart` Also done tests were I check how the data is received when sending packets bigger than a buffer descriptor size (1024). In my tests I have sent 4 packets (1028, 2048, 2048, 2048) and check that 7172 bytes were received using the command: `watch -n1 cat /proc/tty/driver/IMX-uart` Result: serinfo:1.0 driver revision: 2: uart:IMX mmio:0x5000C000 irq:49 tx:0 rx:7172 RTS|CTS|DTR|DSR|CD Nandor Han (4): dma: imx-sdma - reduce transfer latency for DMA cyclic clients dma: imx-sdma - update the residue calculation for cyclic channels serial: imx-serial - update UART IMX driver to use cyclic DMA serial: imx-serial - update RX error counters when DMA is used drivers/dma/imx-sdma.c | 56 +-- drivers/tty/serial/imx.c | 173 ++- 2 files changed, 144 insertions(+), 85 deletions(-) -- 2.8.3
[PATCH RFC 2/4] dma: imx-sdma - update the residue calculation for cyclic channels
The calculation of the DMA transaction residue supports only fixed size data transfers. This implementation is not covering all operations (e.g. data receiving) when we need to know the exact amount of bytes transferred. The loop channels handling was changed to clear the buffer descriptor errors and use the bd->mode.count to calculate the residue. Signed-off-by: Nandor Han --- drivers/dma/imx-sdma.c | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index e497847..9da258a 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -657,6 +657,8 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event) static void sdma_update_channel_loop(struct sdma_channel *sdmac) { struct sdma_buffer_descriptor *bd; + int error = 0; + enum dma_status old_status = sdmac->status; /* * loop mode. Iterate over descriptors, re-setup them and @@ -668,10 +670,20 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) if (bd->mode.status & BD_DONE) break; - if (bd->mode.status & BD_RROR) + if (bd->mode.status & BD_RROR) { + bd->mode.status &= ~BD_RROR; sdmac->status = DMA_ERROR; + error = -EIO; + } + /* + * We use bd->mode.count to calculate the residue, since contains + * the number of bytes present in the current buffer descriptor. + */ + + sdmac->chn_real_count = bd->mode.count; bd->mode.status |= BD_DONE; + bd->mode.count = sdmac->period_len; /* * The callback is called from the interrupt context in order @@ -685,6 +697,9 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac) sdmac->buf_tail++; sdmac->buf_tail %= sdmac->num_bd; + + if (error) + sdmac->status = old_status; } } @@ -1358,7 +1373,8 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan, u32 residue; if (sdmac->flags & IMX_DMA_SG_LOOP) - residue = (sdmac->num_bd - sdmac->buf_tail) * sdmac->period_len; + residue = (sdmac->num_bd - sdmac->buf_tail) * + sdmac->period_len - sdmac->chn_real_count; else residue = sdmac->chn_count - sdmac->chn_real_count; -- 2.8.3
[PATCH RFC 4/4] serial: imx-serial - update RX error counters when DMA is used
Update error couters when DMA is used for receiving data. Do this by using DMA transaction error event instead error interrupts to reduce interrupt load. Signed-off-by: Nandor Han --- drivers/tty/serial/imx.c | 32 1 file changed, 32 insertions(+) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 1912136..08ccfe1 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -704,6 +704,7 @@ out: return IRQ_HANDLED; } +static void clear_rx_errors(struct imx_port *sport); static int start_rx_dma(struct imx_port *sport); /* * If the RXFIFO is filled with some data, and then we @@ -729,6 +730,11 @@ static void imx_dma_rxint(struct imx_port *sport) temp &= ~(UCR2_ATEN); writel(temp, sport->port.membase + UCR2); + /* disable the rx errors interrupts */ + temp = readl(sport->port.membase + UCR4); + temp &= ~UCR4_OREN; + writel(temp, sport->port.membase + UCR4); + /* tell the DMA to receive the data. */ start_rx_dma(sport); } @@ -961,6 +967,7 @@ static void dma_rx_callback(void *data) if (status == DMA_ERROR) { dev_err(sport->port.dev, "DMA transaction error.\n"); + clear_rx_errors(sport); return; } @@ -1057,6 +1064,31 @@ static int start_rx_dma(struct imx_port *sport) return 0; } +static void clear_rx_errors(struct imx_port *sport) +{ + unsigned int status_usr1, status_usr2; + + status_usr1 = readl(sport->port.membase + USR1); + status_usr2 = readl(sport->port.membase + USR2); + + if (status_usr2 & USR2_BRCD) { + sport->port.icount.brk++; + writel(USR2_BRCD, sport->port.membase + USR2); + } else if (status_usr1 & USR1_FRAMERR) { + sport->port.icount.frame++; + writel(USR1_FRAMERR, sport->port.membase + USR1); + } else if (status_usr1 & USR1_PARITYERR) { + sport->port.icount.parity++; + writel(USR1_PARITYERR, sport->port.membase + USR1); + } + + if (status_usr2 & USR2_ORE) { + sport->port.icount.overrun++; + writel(USR2_ORE, sport->port.membase + USR2); + } + +} + #define TXTL_DEFAULT 2 /* reset default */ #define RXTL_DEFAULT 1 /* reset default */ #define TXTL_DMA 8 /* DMA burst setting */ -- 2.8.3
[PATCH RFC 3/4] serial: imx-serial - update UART IMX driver to use cyclic DMA
The IMX UART has a 32 bytes HW buffer which can be filled up in 2777us at 115200 baud or 80us at 4Mbaud (supported by IMX53). Taking this in consideration there is a good probability to lose data because of the DMA startup latency. Our tests (explained below) indicates a latency up to 4400us when creating interrupt load and ~70us without. When creating interrupt load I was able to see continuous overrun errors by checking serial driver statistics using the command: `cat /proc/tty/driver/IMX-uart`. Replace manual restart of DMA with cyclic DMA to eliminate data loss due to DMA engine startup latency (similar approch to atmel_serial.c driver). As result the DMA engine will start on the first serial data transfer and stops only when serial port is closed. Tests environment: Using the m53evk board I have used a GPIO for profiling the IMX serial driver. - The RX line and GPIO were connected to oscilloscope. - Run a small test program on the m53evk board that will only open and read data from ttymxc2 port. - Connect the ttymxc2 port to my laptop using a USB serial converter where another test program is running, able to send configurable packet lengths and intervals. - Serial ports configured at 115200 8N1. - Interrupts load created by disconnecting/connecting (3s interval) a USB hub, using a digital switch, with 4 USB devices (USB-Serial converter, USB SD card, etc) connected. (around 160 interrupts/second generated) - The GPIO was toggled HI in the `imx_int` when USR1_RRDY or USR1_AGTIM events are received and toggled back, once the DMA configuration is finalized, at the end of `imx_dma_rxint`. Measurements: The measurements were done from the end of the last byte (RX line) until the GPIO was toggled back LOW. Note: The GPIO toggling was done using `gpiod_set_value` method. Tests performed: 1. Sending 9 bytes packets at 8ms interval. Having the 9 bytes packets will activate the RRDY threshold event and IMX serial interrupt called. Results: - DMA start latency (interrupt start latency + DMA configuration) consistently 70us when system not loaded. - DMA start latency up to 4400us when system loaded. 2. Sending 40 bytes packet at 8mS interval. Results with load: - Able to observe overruns by running: `watch -n1 cat /proc/tty/driver/IMX-uart` Signed-off-by: Nandor Han --- drivers/tty/serial/imx.c | 141 ++- 1 file changed, 78 insertions(+), 63 deletions(-) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 0df2b1c..1912136 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -222,6 +222,9 @@ struct imx_port { struct dma_chan *dma_chan_rx, *dma_chan_tx; struct scatterlist rx_sgl, tx_sgl[2]; void*rx_buf; + struct circ_buf rx_ring; + unsigned intrx_periods; + dma_cookie_trx_cookie; unsigned inttx_bytes; unsigned intdma_tx_nents; wait_queue_head_t dma_wait; @@ -932,30 +935,6 @@ static void imx_timeout(unsigned long data) } #define RX_BUF_SIZE(PAGE_SIZE) -static void imx_rx_dma_done(struct imx_port *sport) -{ - unsigned long temp; - unsigned long flags; - - spin_lock_irqsave(&sport->port.lock, flags); - - /* re-enable interrupts to get notified when new symbols are incoming */ - temp = readl(sport->port.membase + UCR1); - temp |= UCR1_RRDYEN; - writel(temp, sport->port.membase + UCR1); - - temp = readl(sport->port.membase + UCR2); - temp |= UCR2_ATEN; - writel(temp, sport->port.membase + UCR2); - - sport->dma_is_rxing = 0; - - /* Is the shutdown waiting for us? */ - if (waitqueue_active(&sport->dma_wait)) - wake_up(&sport->dma_wait); - - spin_unlock_irqrestore(&sport->port.lock, flags); -} /* * There are two kinds of RX DMA interrupts(such as in the MX6Q): @@ -972,43 +951,75 @@ static void dma_rx_callback(void *data) struct scatterlist *sgl = &sport->rx_sgl; struct tty_port *port = &sport->port.state->port; struct dma_tx_state state; + struct circ_buf *rx_ring = &sport->rx_ring; enum dma_status status; - unsigned int count; - - /* unmap it first */ - dma_unmap_sg(sport->port.dev, sgl, 1, DMA_FROM_DEVICE); + unsigned int w_bytes = 0; + unsigned int r_bytes; + unsigned int bd_size; status = dmaengine_tx_status(chan, (dma_cookie_t)0, &state); - count = RX_BUF_SIZE - state.residue; - dev_dbg(sport->port.dev, "We get %d bytes.\n", count); + if (status == DMA_ERROR) { + dev_err(sport->port.dev, "DMA transaction error.\n")