[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-i2c
Subject:    Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support
From:       Dmitry Osipenko <digetx () gmail ! com>
Date:       2019-01-31 19:02:55
Message-ID: 365fe7b3-7206-cefa-f10a-06b5db42c761 () gmail ! com
[Download RAW message or body]

31.01.2019 19:27, Dmitry Osipenko пишет:
> 31.01.2019 19:01, Thierry Reding пишет:
> > On Thu, Jan 31, 2019 at 06:02:45PM +0300, Dmitry Osipenko wrote:
> > > 31.01.2019 17:43, Thierry Reding пишет:
> > > > On Thu, Jan 31, 2019 at 05:06:18PM +0300, Dmitry Osipenko wrote:
> > > > > 31.01.2019 15:06, Thierry Reding пишет:
> > > > > > On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
> > > > > > > 30.01.2019 19:01, Sowjanya Komatineni пишет:
> > > > > > [...]
> > > > > > > > diff --git a/drivers/i2c/busses/i2c-tegra.c \
> > > > > > > > b/drivers/i2c/busses/i2c-tegra.c
> > > > > > [...]
> > > > > > > > +		return -EIO;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	dma_desc->callback = tegra_i2c_dma_complete;
> > > > > > > > +	dma_desc->callback_param = i2c_dev;
> > > > > > > > +	dmaengine_submit(dma_desc);
> > > > > > > > +	dma_async_issue_pending(chan);
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev,
> > > > > > > > +				    bool dma_to_memory)
> > > > > > > > +{
> > > > > > > > +	struct dma_chan *dma_chan;
> > > > > > > > +	u32 *dma_buf;
> > > > > > > > +	dma_addr_t dma_phys;
> > > > > > > > +	int ret;
> > > > > > > > +	const char *chan_name = dma_to_memory ? "rx" : "tx";
> > > > > > > 
> > > > > > > What about to move out chan_name into the function arguments?
> > > > > > 
> > > > > > That opens up the possibility of passing dma_to_memory = true and
> > > > > > chan_name as "tx" and create an inconsistency.
> > > > > > 
> > > > > > > > @@ -884,6 +1187,8 @@ static void tegra_i2c_parse_dt(struct \
> > > > > > > > tegra_i2c_dev *i2c_dev) 
> > > > > > > > 	i2c_dev->is_multimaster_mode = of_property_read_bool(np,
> > > > > > > > 			"multi-master");
> > > > > > > > +
> > > > > > > > +	i2c_dev->has_dma = of_property_read_bool(np, "dmas");
> > > > > > > 
> > > > > > > Not only the existence of "dmas" property defines whether DMA is \
> > > > > > > available. DMA subsystem could be disabled in the kernels \
> > > > > > > configuration. 
> > > > > > > Hence there is a need to check for DMA driver presence in the code:
> > > > > > > 
> > > > > > > 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
> > > > > > > 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
> > > > > > 
> > > > > > Do we even need the ->has_dma at all? We can just go ahead and request
> > > > > > the channels at probe time and respond accordingly. If there's no dmas
> > > > > > property in DT, dma_request_slave_channel_reason() returns an error so
> > > > > > we can just deal with it at that time.
> > > > > > 
> > > > > > So if we get -EPROBE_DEFER we can propagate that, for any other errors
> > > > > > we can simply fallback to PIO. Or perhaps we want to restrict fallback
> > > > > > to PIO for -ENODEV?
> > > > > > 
> > > > > > I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
> > > > > > The purpose of these subsystems it to abstract all of that away.
> > > > > > Otherwise we could just as well use custom APIs, if we're tying together
> > > > > > drivers in this way anyway.
> > > > > 
> > > > > DMA API doesn't fully abstract the dependencies between drivers, hence
> > > > > I disagree.
> > > > 
> > > > Why not? The dependency we're talking about here is a runtime dependency
> > > > rather than a build time dependency. Kconfig is really all about build-
> > > > time dependencies.
> > > 
> > > My understanding is that Kconfig is also about runtime dependencies,
> > > do you know if it's explicitly documented anywhere?
> > 
> > I don't think it's explicitly documented, just a common practice that
> > I've seen applied multiple times over the years. A quick grep through
> > the drivers/ subdirectory confirms that it's not typical to have this
> > sort of dependency in the code.
> > 
> > Similarly, Kconfig uses select primarily to pull in dependencies that
> > are in the form of helper libraries and such. Occasionally you'll have
> > some ARCH_* option select a couple of features, or even drivers, but
> > that is mostly a shortcut to explicitly having to list the essentials
> > in a defconfig.
> > 
> > Another reason why it's not good to model these runtime dependencies in
> > Kconfig is because they unnecessarily restrict the driver. For example,
> > if you want to build a specialized Linux binary for Tegra186, you will
> > certainly want to include the i2c-tegra driver. At the same time you
> > won't want to include the APB DMA driver because it doesn't exist on
> > Tegra186. Instead you'd want the (non-existent) GPC DMA driver. select
> > on the APB DMA driver will unconditionally pull in the driver, depends
> > will only allow you to build i2c-tegra if the APB DMA driver is also
> > enabled and the conditional in the code may lead to not using DMA
> > because the APB DMA driver is not available. So you'd have to modify the
> > i2c-tegra driver to take into account the GPC DMA driver.
> > 
> > > > > > > Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
> > > > > > > driver built-in when I2C driver is built-in:
> > > > > > 
> > > > > > I don't think there's a requirement for that. The only dependency we
> > > > > > really have here is the one on the DMA engine API. Since dmaengine.h
> > > > > > already provides dummy implementations, there's really no need for
> > > > > > us to have the dependency. If the DMA engine API is completely disabled,
> > > > > > a call to dma_request_slave_channel_reason() will return -ENODEV and we
> > > > > > should just deal with that the same way we would if there was no "dmas"
> > > > > > property present.
> > > > > 
> > > > > In my opinion it is much better to avoid I2C driver probe failing with
> > > > > -EPROBE_DEFER if we could. It's just one line in code and one in
> > > > > Kconfig.. really. 
> > > > 
> > > > The problem is that from a theoretical point of view we don't know that
> > > > APB DMA is the provider for the DMA channels. This provider could be a
> > > > completely different device on a different Tegra generation (in fact,
> > > > the DMA engine on Tegra186 is a different one, so we'd have to add that
> > > > to the list of checks to make sure we don't disable DMA there). And the
> > > > fact that we're tightly integrated is really only by accident. We could
> > > > have the same situation on a SoC that incorporates IP from multiple
> > > > different sources and multiple combinations thereof as well, so how
> > > > would you want to deal with those cases?
> > > > 
> > > > Agreed, failing with -EPROBE_DEFER is suboptimal in that case, but that
> > > > is really more of an integration problem. Ideally of course there'd be
> > > > some way for the DMA engine subsystem to know that the provider for the
> > > > given device node will never show up and give us -ENODEV instead, but,
> > > > alas, I don't even think that would be possible. That's the price to pay
> > > > for abstraction.
> > > 
> > > It's not a big problem to solve for this case, there is
> > > of_machine_is_compatible(). To me it's more a question about the will
> > > to invest some extra effort to support all of possible combinations.
> > > If there is no such will, then at least those unpopular combinations
> > > shouldn't hurt and thus it should make sense to add an explicit
> > > build-dependency on the DMA drivers.
> > 
> > I think we're arguing about the same thing, only coming at it from
> > different angles. For me "all possible combinations" also includes the
> > case where you want to be able to run the driver with DMA if the APB DMA
> > is not enabled. And I similarly want to be able to run without DMA if
> > the APB DMA is enabled (by explicitly removing dmas from DT for
> > example). It just seems that we can't have it both ways.
> > 
> > Also the i2c-tegra driver can perfectly well function without DMA
> > support (it's done so ever since it was first merged). Keeping existing
> > functionality shouldn't require the addition of another driver.
> > 
> > Given the deadlock, I think I'd prefer the option of adding the
> > conditional in the code. I think that's the most accurate description of
> > the dependency, even though ideally it would be handled transparently by
> > the DMA engine API. Would that be an acceptable compromise?
> 
> Adding conditional to the code is not enough. Tegra I2C driver could be built-in, \
> while APB DMA driver is a loadable module, hence Tegra I2C will fail to probe with \
> -EPROBE_DEFER. Tegra I2C must select all of the relevant DMA drivers to avoid that \
> situation. Later on it shouldn't be a problem to add .has_gpc_dma to the \
> tegra_i2c_hw_feature and then check in the code whether corresponding DMA driver is \
> enabled or not in the kernel's config. 
> Combining the code checking with the Kconfig selection that I'm suggesting covers \
> all of possible combinations, otherwise please give me an explicit example when it \
> could fail. 

Hm, oddly TEGRA20_APB_DMA driver can't be built as a module because it is a bool in \
Kconfig, but the code has the remove function. Maybe someday somebody will clean out \
all that mess :) Hence, Thierry, your point make sense. Still could be good to make \
DMA support explicitly optional for the I2C driver by adding Kconfig option for it, \
given that it adds some extra complexity and not well tested, up to you to decide.


[prev in list] [next in list] [prev in thread] [next in thread]