| Message ID | 1370950268-7224-2-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sebastian, On Tue, Jun 11, 2013 at 01:30:47PM +0200, Sebastian Andrzej Siewior wrote: > The MFD part uses regmap without caching and is the only user of the > regmap. The child drivers, that is input(touch) and iio(adc), use direct > reg access. > There is a patch which converts them to use regmap as well but I see no > benefit at all doing this. There is a direct MMIO bus access with no > need to cache values like for I2C or SPI devices. Furthermore, most (if > not all) of the access done by the touch & ADC driver read volatile > register. > Therefore this patch removes regmap part of the driver. NAK. Using regmap is better than open coding your register accesses, and the children not using this API is not a reason for the MFD driver to do the same. Cheers, Samuel.
On 06/11/2013 04:23 PM, Samuel Ortiz wrote: > Hi Sebastian, Hi Samuel, >> Therefore this patch removes regmap part of the driver. > NAK. Using regmap is better than open coding your register accesses, and > the children not using this API is not a reason for the MFD driver to do > the same. There is no advantage over using regmap in the first place. It goes through a few layers, uses no caching because almost all registers are volatile and this is a direct bus. In the end it complicates more than it helps. > > Cheers, > Samuel. > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 11, 2013 at 04:34:53PM +0200, Sebastian Andrzej Siewior wrote: > >> Therefore this patch removes regmap part of the driver. > > NAK. Using regmap is better than open coding your register accesses, and > > the children not using this API is not a reason for the MFD driver to do > > the same. > There is no advantage over using regmap in the first place. It goes > through a few layers, uses no caching because almost all registers are > volatile and this is a direct bus. In the end it complicates more than > it helps. It does give you tracepoints and debugfs. If it's making things at all complicated we need to look at why that is and figure out how to fix that since it's probably an issue for other users.
On 06/14/2013 03:53 PM, Mark Brown wrote: > On Tue, Jun 11, 2013 at 04:34:53PM +0200, Sebastian Andrzej Siewior > wrote: > > It does give you tracepoints and debugfs. If it's making things at > all complicated we need to look at why that is and figure out how > to fix that since it's probably an issue for other users. debugfs are tracepoints is our offer? Let me check the price one more time. A simply mmio read does right now: - lock + unlock. each time you chase another pointer plus enable/disable interrupts plus you have to save flags in another structure. - _regmap_read() We check a few variables and then we go after reg_read and we end up in _regmap_raw_read(). Here we call _regmap_range_lookup() which should return NULL. Next thing we invoke map->format.format_reg(). Finally we can call map->bus->read() which brings us to regmap_mmio_read(). At the end we invoke map->format.parse_val(). write looks most likely the same. A simple register read invokes 5 functions pointers. I am not counting the function arguments in between and I am also not counting the number of arguments which are involved and take pointer as well. This is a lot of stuff that is done for a simple read of an mmio. I understand that most of this may not be expensive in total if it comes to SPI or I2C and all the goodies like reg caching and one interface which deals with SPI and I2C. I also understand that some chips have a non standard interface and are either BE or LE. We have similar things on USB where people wired the BUS wrongly either at the BUS level or at the SoC level so some PowerPC have an in-core ehci controller but its registers are BE and not LE like it should be. The solution here was variable check a simple swap() in that case. I like abstractions but this gone a little too far I think. This is a lot of for a simple mmio access. In terms of performance: If I add a trace point to my read and write I have still less code which is called and it can be disabled. This regmap overhead is always there chasing pointers. As I said before: I doubt that you get this regmap access in an one GiB network driver and in turn remove their trace points to register access. Please don't get me wrong: It is still nice for slow buses and this ADC driver isn't anything close to high performance like a 1GiB network driver but it adds a lot of unwanted overhead which I prefer not to have. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 17, 2013 at 01:41:40PM +0200, Sebastian Andrzej Siewior wrote: > On 06/14/2013 03:53 PM, Mark Brown wrote: > > It does give you tracepoints and debugfs. If it's making things at > > all complicated we need to look at why that is and figure out how > > to fix that since it's probably an issue for other users. > debugfs are tracepoints is our offer? Let me check the price one more > time. OK... > This is a lot of for a simple mmio access. In terms of performance: If > I add a trace point to my read and write I have still less code which > is called and it can be disabled. This regmap overhead is always there > chasing pointers. Equally well what you're implementing here is support for something that's typically implemented with an I2C or SPI control interface so you're already going to be quite a way ahead of the norm. This is part of what's confusing me, usually for this application things aren't that bad performance wise even on a massively slower bus. If all you're saying here is that there's some overhead that's fine if a bit surprising, but the way you're talking made it sound like there was some issue that made the API actually unusable. > As I said before: I doubt that you get this regmap access in an one GiB > network driver and in turn remove their trace points to register access. Well, of course for that sort of thing the general trick is not to talk to the hardware at all and do as much as possible with memory that the hardware can DMA - there's actually still non-trivial costs in talking over the buses. > Please don't get me wrong: It is still nice for slow buses and this ADC > driver isn't anything close to high performance like a 1GiB network > driver but it adds a lot of unwanted overhead which I prefer not to > have. OK, but equally well remember that from a subsystem maintainer point of view having things factored out is a win in itself; for example with the MFDs locking on the register I/O has been a persistent issue in the past.
Sorry for the long pause… On 06/17/2013 06:03 PM, Mark Brown wrote: >> This is a lot of for a simple mmio access. In terms of >> performance: If I add a trace point to my read and write I have >> still less code which is called and it can be disabled. This >> regmap overhead is always there chasing pointers. > > Equally well what you're implementing here is support for > something that's typically implemented with an I2C or SPI control > interface so you're already going to be quite a way ahead of the > norm. This is part of what's confusing me, usually for this > application things aren't that bad performance wise even on a > massively slower bus. > > If all you're saying here is that there's some overhead that's fine > if a bit surprising, but the way you're talking made it sound like > there was some issue that made the API actually unusable. No, sorry for that confusion. I had a problem with the locking in irq context, changed the driver & pointed out the problem, been told that there is a fix, applied it, never complained again. This is simply about I am forced to use regmap and I don't agree with it. >> As I said before: I doubt that you get this regmap access in an >> one GiB network driver and in turn remove their trace points to >> register access. > > Well, of course for that sort of thing the general trick is not to > talk to the hardware at all and do as much as possible with memory > that the hardware can DMA - there's actually still non-trivial > costs in talking over the buses. That is true but even while doing DMA you have enough handshake with the HW to trigger the transfer and if you are lucky your DMA descriptors are in cached memory. >> Please don't get me wrong: It is still nice for slow buses and >> this ADC driver isn't anything close to high performance like a >> 1GiB network driver but it adds a lot of unwanted overhead which >> I prefer not to have. > > OK, but equally well remember that from a subsystem maintainer > point of view having things factored out is a win in itself; for > example with the MFDs locking on the register I/O has been a > persistent issue in the past. I agree with that. But: The driver here does not use atomic updates but read followed by write so your locking here is futile. So the API/regmap alone does not make it right. And look: the MFD part uses regmap. Its children (IIO & input) do not use it. After I told this Samuel he said that it is okay. So here I am. Using regmap in MFD which is only used once on init and never again. It has regmap. The register access in both child driver is split making sure they do not use the same ones. I added one function which writes a common register. That one is reset after an operation and needs to be written by the currently active child so the HW continues to work. Haven't seen anything close to it in regmap. I ask, politely I hope, to get this patch in and remove regmap since none of its features are used in this driver. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 04, 2013 at 11:02:41AM +0200, Sebastian Andrzej Siewior wrote: > The driver here does not use atomic updates but read followed by write > so your locking here is futile. So the API/regmap alone does not make Doesn't that sound like the driver ought to be using a r/m/w primitive though? > it right. And look: the MFD part uses regmap. Its children (IIO & > input) do not use it. After I told this Samuel he said that it is okay. Again I think the point here was that they probably ought to do so. But I guess if you're saying there's no problem that's fine...
On 07/04/2013 12:45 PM, Mark Brown wrote: > On Thu, Jul 04, 2013 at 11:02:41AM +0200, Sebastian Andrzej Siewior > wrote: > >> The driver here does not use atomic updates but read followed by >> write so your locking here is futile. So the API/regmap alone >> does not make > > Doesn't that sound like the driver ought to be using a r/m/w > primitive though? It does this in the init phase before the child devices are created so no harm is done. I just wanted to say that regmap alone does not help as long as the use simply replaces all reads & writes with regmap reads & writes. > >> it right. And look: the MFD part uses regmap. Its children (IIO >> & input) do not use it. After I told this Samuel he said that it >> is okay. > > Again I think the point here was that they probably ought to do > so. It didn't sound that way. > But I guess if you're saying there's no problem that's fine... Thank you. Samuel, is it okay if I repost the patch? It can wait till past -rc1 if you are willing to take it. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c index e9f3fb5..804e61e 100644 --- a/drivers/mfd/ti_am335x_tscadc.c +++ b/drivers/mfd/ti_am335x_tscadc.c @@ -19,7 +19,6 @@ #include <linux/err.h> #include <linux/io.h> #include <linux/clk.h> -#include <linux/regmap.h> #include <linux/mfd/core.h> #include <linux/pm_runtime.h> @@ -29,25 +28,15 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg) { - unsigned int val; - - regmap_read(tsadc->regmap_tscadc, reg, &val); - return val; + return readl(tsadc->tscadc_base + reg); } static void tscadc_writel(struct ti_tscadc_dev *tsadc, unsigned int reg, unsigned int val) { - regmap_write(tsadc->regmap_tscadc, reg, val); + writel(val, tsadc->tscadc_base + reg); } -static const struct regmap_config tscadc_regmap_config = { - .name = "ti_tscadc", - .reg_bits = 32, - .reg_stride = 4, - .val_bits = 32, -}; - static void tscadc_idle_config(struct ti_tscadc_dev *config) { unsigned int idleconfig; @@ -121,14 +110,6 @@ static int ti_tscadc_probe(struct platform_device *pdev) return -ENOMEM; } - tscadc->regmap_tscadc = devm_regmap_init_mmio(&pdev->dev, - tscadc->tscadc_base, &tscadc_regmap_config); - if (IS_ERR(tscadc->regmap_tscadc)) { - dev_err(&pdev->dev, "regmap init failed\n"); - err = PTR_ERR(tscadc->regmap_tscadc); - goto ret; - } - pm_runtime_enable(&pdev->dev); pm_runtime_get_sync(&pdev->dev); diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h index c79ad5d..dc75c34 100644 --- a/include/linux/mfd/ti_am335x_tscadc.h +++ b/include/linux/mfd/ti_am335x_tscadc.h @@ -137,7 +137,6 @@ struct mfd_tscadc_board { struct ti_tscadc_dev { struct device *dev; - struct regmap *regmap_tscadc; void __iomem *tscadc_base; int irq; struct mfd_cell cells[TSCADC_CELLS];
The MFD part uses regmap without caching and is the only user of the regmap. The child drivers, that is input(touch) and iio(adc), use direct reg access. There is a patch which converts them to use regmap as well but I see no benefit at all doing this. There is a direct MMIO bus access with no need to cache values like for I2C or SPI devices. Furthermore, most (if not all) of the access done by the touch & ADC driver read volatile register. Therefore this patch removes regmap part of the driver. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/mfd/ti_am335x_tscadc.c | 23 ++--------------------- include/linux/mfd/ti_am335x_tscadc.h | 1 - 2 files changed, 2 insertions(+), 22 deletions(-)