* [PATCH -next] i2c: mux: mlxcpld: remove unused including <linux/version.h>
From: Wei Yongjun @ 2017-01-12 14:29 UTC (permalink / raw)
To: Vadim Pasternak, Michael Shych, Wolfram Sang, Peter Rosin
Cc: Wei Yongjun, linux-i2c
From: Wei Yongjun <weiyongjun1@huawei.com>
Remove including <linux/version.h> that don't need it.
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
drivers/i2c/muxes/i2c-mux-mlxcpld.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
index b7ca249..e53f2ab 100644
--- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
+++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
@@ -40,7 +40,6 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
-#include <linux/version.h>
#include <linux/i2c/mlxcpld.h>
#define CPLD_MUX_MAX_NCHANS 8
^ permalink raw reply related
* Re: [PATCH -next] i2c: mux: mlxcpld: remove unused including <linux/version.h>
From: Peter Rosin @ 2017-01-12 14:36 UTC (permalink / raw)
To: Wei Yongjun, Vadim Pasternak, Michael Shych, Wolfram Sang
Cc: Wei Yongjun, linux-i2c
In-Reply-To: <20170112142904.17582-1-weiyj.lk@gmail.com>
On 2017-01-12 15:29, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> Remove including <linux/version.h> that don't need it.
s/don't need it/is not needed/
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/i2c/muxes/i2c-mux-mlxcpld.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> index b7ca249..e53f2ab 100644
> --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> @@ -40,7 +40,6 @@
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
I don't see a reason to keep slab.h either?
Cheers,
peda
> -#include <linux/version.h>
> #include <linux/i2c/mlxcpld.h>
>
> #define CPLD_MUX_MAX_NCHANS 8
>
>
>
^ permalink raw reply
* RE: [PATCH -next] i2c: mux: mlxcpld: remove unused including <linux/version.h>
From: Vadim Pasternak @ 2017-01-12 15:40 UTC (permalink / raw)
To: Wei Yongjun, Michael Shych, Wolfram Sang, Peter Rosin
Cc: Wei Yongjun, linux-i2c@vger.kernel.org
In-Reply-To: <20170112142904.17582-1-weiyj.lk@gmail.com>
> -----Original Message-----
> From: Wei Yongjun [mailto:weiyj.lk@gmail.com]
> Sent: Thursday, January 12, 2017 4:29 PM
> To: Vadim Pasternak <vadimp@mellanox.com>; Michael Shych
> <michaelsh@mellanox.com>; Wolfram Sang <wsa@the-dreams.de>; Peter
> Rosin <peda@axentia.se>
> Cc: Wei Yongjun <weiyongjun1@huawei.com>; linux-i2c@vger.kernel.org
> Subject: [PATCH -next] i2c: mux: mlxcpld: remove unused including
> <linux/version.h>
>
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> Remove including <linux/version.h> that don't need it.
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
Acked-by: Vadim Pasternak <vadimp@mellanox.com>
> drivers/i2c/muxes/i2c-mux-mlxcpld.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-
> mux-mlxcpld.c
> index b7ca249..e53f2ab 100644
> --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> @@ -40,7 +40,6 @@
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> -#include <linux/version.h>
> #include <linux/i2c/mlxcpld.h>
>
> #define CPLD_MUX_MAX_NCHANS 8
>
>
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-12 16:17 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, Russell King, linux-i2c,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <CAOAejn2pW20VPP_yGtvJ_ufvj6Xj1poBiiA2WqkALiaLyyONug@mail.gmail.com>
>>> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>>> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>>> > somewhere?
>>>
>>> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
>>> 2Mhz and SCL_period = 1 we have:
>>> CCR = 1 * 2Mhz = 2.
>>> But to compute, scl_low and scl_high in Fast mode, we have to do the
>>> following thing as Duty=1:
>>> scl_high = 9 * CCR * I2C parent clk period
>>> scl_low = 16 * CCR * I2C parent clk period
>>> In our example:
>>> scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 µs
>>> scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 µs
>>> So low + high = 27 µs > 2,5 µs
>>
>> For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
>> if there is a factor 10 missing somewhere.
>
> Hum ok. I am going to double-check what is wrong because when I check
> with the scope I always reach 400Khz for SCL.
> I will let you know.
There is one point I miss here that is described in the reference manual:
To reach the 400 kHz maximum I²C fast mode clock, the I2C parent rate
must be a multiple of 10 MHz.
So, contrary to what we said in a previous thread, 400 kHz could not
be reached with low frequencies.
In that way, we could compute CCR with duty = 0 by default.
So, I find another formula very close to the first one I pushed in the
first version:
In fast mode, we compute CCR with duty = 0:
t_scl_high = CCR * I2C parent clk period
t_scl_low = 2 *CCR * I2C parent clk period
So, CCR = I2C parent rate / 400 kHz / 3
For example with parent rate = 40 MHz:
CCR = 40000000 / 400000 / 3 = 33.333333333 = 33
t_scl_high = 33 * (1 / 2000000) = 825 ns > 600 ns
t_scl_low = 2 * 16 * (1 / 2000000) = 1650 ns > 1300 ns
It seems ok now.
Best regards,
Cedric
^ permalink raw reply
* Re: [PATCH] i2c: core: helper function to detect slave mode
From: Andy Shevchenko @ 2017-01-12 17:01 UTC (permalink / raw)
To: Vladimir Zapolskiy, Andy Shevchenko
Cc: Luis Oliveira, Wolfram Sang, Rob Herring, Mark Rutland,
Jarkko Nikula, Mika Westerberg, linux-i2c, devicetree,
linux-kernel@vger.kernel.org, Ramiro.Oliveira, Joao Pinto,
CARLOS.PALMINHA
In-Reply-To: <3748130b-5321-12eb-ec75-e2637dd9fc54@mleia.com>
On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote:
> On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
> > On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@mleia.com>
> > wrote:
> > > On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
> > > > + }
> > > > > > + } else if (IS_BUILTIN(CONFIG_ACPI) &&
> > > > > > ACPI_HANDLE(dev)) {
> > > > > > + dev_dbg(dev, "ACPI slave is not supported
> > > > > > yet\n");
> > > > > > + }
> > > > >
> > > > > If so, then it might be better to drop else-if stub for now.
> > > >
> > > > Please, don't.
> > > >
> > >
> > > Why do you ask for this stub to be added?
> >
> > 1. Exactly the reason you asked above. Here is the code which has
> > built differently on different platforms. x86 usually is not using
> > CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
> > existing examples.
>
> From the context by the stub I mean dev_dbg() in
> i2c_slave_mode_detect()
> function, I don't see a connection to GPIO library, please clarify.
I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro.
> > 2. We might add that support later, but here is again, just no-op.
> >
> > So, what is your strong argument here against that?
>
> When the support is ready for ACPI case, you'll remove the added
> dev_dbg(), and I don't see a good point by adding it temporarily.
It would remind me to look at it at some point.
> What is wrong with the approach of adding the ACPI case handling
> branch when it is ready and remove any kind of stubs right now?
I will not object. Here is maintainer, let him speak.
> On ACPI platforms the function returns 'false' always, will the
> function work correctly (= corresponding to its description) as is?
Yes.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-12 17:49 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: devicetree, Alexandre Torgue, Wolfram Sang, linux-kernel,
Linus Walleij, Patrice Chotard, Russell King, Rob Herring,
linux-i2c, Maxime Coquelin, linux-arm-kernel
In-Reply-To: <CAOAejn289GLOSP-nPJnO_VpXLpyhTsF1bWQ7Ns9OfgPQCa8YTw@mail.gmail.com>
On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Hello Cedric,
> >
> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> > This is surprising. I didn't recheck the manual, but that looks very
> >> >> > uncomfortable.
> >> >>
> >> >> I agree but this exactly the hardware way of working described in the
> >> >> reference manual.
> >> >
> >> > IMHO that's a hw bug. This makes it for example impossible to implement
> >> > SMBus block transfers (I think).
> >>
> >> This is not correct.
> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
> >> Here we have just to prepare the hardware for the 2 next pulse but the
> >> STOP/START/ACK pulse will be generated at the right time as required
> >> by I2C specification.
> >> So SMBus block transfer will be possible.
> >
> > A block transfer consists of a byte that specifies the count of bytes
> > yet to come. So the device sends for example:
> >
> > 0x01 0xab
> >
> > So when you read the 1 in the first byte it's already too late to set
> > STOP to get it after the 2nd byte.
> >
> > Not sure I got all the required details right, though.
>
> Ok I understand your use case but I always think that the harware manages it.
> If I take the above example, the I2C SMBus block read transaction will
> be as below:
> S Addr Wr [A] Comm [A]
> S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
>
> The first message is a single byte-transmission so there is no problem.
>
> The second message is a N-byte reception with N = 3
>
> When the I2C controller has finished to send the device address (S
> Addr Rd), the ADDR flag is set and an interrupt is raised.
> In the routine that handles ADDR event, we set ACK bit in order to
> generate ACK pulse as soon as a data byte is received in the shift
> register and then we clear the ADDR flag.
> Please note that the SCL line is stretched low until ADDR flag is cleared.
> So, as far I understand, the device could not sent any data as long as
> the SCL line is stretched low. Right ?
>
> Then, as soon as the SCL line is high, the device could send the first
> data byte (Count).
> When this byte is received in the shift register, an ACK is
> automatically generated as defined during adress match phase and the
> data byte is pushed in DR (data register).
> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
> In the routine that handles RXNE event, as N=3, we just clear all
> buffer interrupts in order to avoid another system preemption due to
> RXNE event but we does not read the data in DR.
In my example I want to receive a block of length 1, so only two bytes
are read, a 1 (the length) and the data byte (0xab in my example). I
think that as soon as you read the 1 it's already to late to schedule
the NA after the next byte?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH] drivers/i2c/i2c-dev: Fix kernel memory disclosure
From: Wolfram Sang @ 2017-01-12 18:36 UTC (permalink / raw)
To: Vlad Tsyrklevich; +Cc: linux-i2c
In-Reply-To: <1483977216-7623-1-git-send-email-vlad@tsyrklevich.net>
On Mon, Jan 09, 2017 at 10:53:36PM +0700, Vlad Tsyrklevich wrote:
> i2c_smbus_xfer() does not always fill an entire block, allowing
> kernel stack memory disclosure through the temp variable. Clear
> it before it's read to.
>
> Signed-off-by: Vlad Tsyrklevich <vlad@tsyrklevich.net>
Applied to for-current, thanks!
^ permalink raw reply
* Re: [PATCH v2] i2c: do not enable fall back to Host Notify by default
From: Wolfram Sang @ 2017-01-12 18:41 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Benjamin Tissoires, Pali Rohár,
Michał Kępień, Jean Delvare, Takashi Iwai,
linux-i2c, devicetree, linux-kernel
In-Reply-To: <20170105045722.GA17958@dtor-ws>
On Wed, Jan 04, 2017 at 08:57:22PM -0800, Dmitry Torokhov wrote:
> Falling back unconditionally to HostNotify as primary client's interrupt
> breaks some drivers which alter their functionality depending on whether
> interrupt is present or not, so let's introduce a board flag telling I2C
> core explicitly if we want wired interrupt or HostNotify-based one:
> I2C_CLIENT_HOST_NOTIFY.
>
> For DT-based systems we introduce "host-notify" property that we convert
> to I2C_CLIENT_HOST_NOTIFY board flag.
>
> Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Applied to for-current, thanks!
How do we handle driver fixes? Shall I take them via I2C to have the
dependency clear? Or can they go seperately?
^ permalink raw reply
* Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Wolfram Sang @ 2017-01-12 18:45 UTC (permalink / raw)
To: Hans de Goede
Cc: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Len Brown, Andy Shevchenko, intel-gfx, dri-devel,
Mika Westerberg, Takashi Iwai, russianneuromancer @ ya . ru,
linux-i2c
In-Reply-To: <20170108134427.8392-4-hdegoede@redhat.com>
On Sun, Jan 08, 2017 at 02:44:23PM +0100, Hans de Goede wrote:
> Take the punit lock to stop others from accessing the punit while the
> pmic i2c bus is in use. This is necessary because accessing the punit
> from the kernel may result in the punit trying to access the pmic i2c
> bus, which results in a hang when it happens while we own the pmic i2c
> bus semaphore.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
I don't think the I2C patches need to go via I2C tree, so:
Acked-by: Wolfram Sang <wsa@the-dreams.de>
^ permalink raw reply
* Re: [PATCH 4/7] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain
From: Wolfram Sang @ 2017-01-12 18:45 UTC (permalink / raw)
To: Hans de Goede
Cc: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Len Brown, Andy Shevchenko, intel-gfx, dri-devel,
Mika Westerberg, Takashi Iwai, russianneuromancer @ ya . ru,
linux-i2c
In-Reply-To: <20170108134427.8392-5-hdegoede@redhat.com>
On Sun, Jan 08, 2017 at 02:44:24PM +0100, Hans de Goede wrote:
> Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire / release.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
^ permalink raw reply
* Re: [PATCH v3 resend] i2c: designware: add reset interface
From: Wolfram Sang @ 2017-01-12 18:54 UTC (permalink / raw)
To: Zhangfei Gao
Cc: andriy.shevchenko, mika.westerberg, jarkko.nikula, p.zabel,
linux-arm-kernel, linux-i2c
In-Reply-To: <1482848560-3752-1-git-send-email-zhangfei.gao@linaro.org>
On Tue, Dec 27, 2016 at 10:22:40PM +0800, Zhangfei Gao wrote:
> Some platforms like hi3660 need do reset first to allow accessing registers
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Ramiro Oliveira <ramiro.oliveira@synopsys.com>
Applied to for-next, thanks!
^ permalink raw reply
* Re: [PATCH] i2c: print correct device invalid address
From: Wolfram Sang @ 2017-01-12 19:07 UTC (permalink / raw)
To: John Garry; +Cc: linux-i2c, linux-kernel, linuxarm
In-Reply-To: <1483700577-60130-1-git-send-email-john.garry@huawei.com>
On Fri, Jan 06, 2017 at 07:02:57PM +0800, John Garry wrote:
> In of_i2c_register_device(), when the check for
> device address validity fails we print the info.addr,
> which has not been assigned properly.
>
> Fix this by printing the actual invalid address.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
Applied to for-current, thanks!
^ permalink raw reply
* Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Wolfram Sang @ 2017-01-12 19:15 UTC (permalink / raw)
To: Phil Reid
Cc: peda-koto5C5qi+TLoDKTGw+V6w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483952576-5308-4-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
On Mon, Jan 09, 2017 at 05:02:54PM +0800, Phil Reid wrote:
> Various muxes can aggregate multiple interrupts from each i2c bus.
> All of the muxes with interrupt support combine the active low irq lines
> using an internal 'and' function and generate a combined active low
> output. The muxes do provide the ability to read a control register to
> determine which irq is active. By making the mux an irq controller isr
> latency can potentially be reduced by reading the status register and
> then only calling the registered isr on that bus segment.
>
> As there is no irq masking on the mux irq are disabled until irq_unmask is
> called at least once.
>
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
Is the ack from Peda here forgotten or still missing?
@peda: Once you are happy, do you want to take these patches via your
shiny new mux-tree or do you prefer if I pick them?
Regards,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] i2c: i2c-cadence: Don't register the adapter until it's ready
From: Wolfram Sang @ 2017-01-12 19:22 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Mike Looijmans, linux-i2c, linux-kernel, linux-arm-kernel,
soren.brinkmann, michal.simek
In-Reply-To: <428e8fc8-8b23-8e2c-763e-d977aaa98691@mleia.com>
> Because the adapter registration i2c_add_adapter() can fail, information
> about the adapter initialization would be expected only in case of
> successful registration.
Exactly.
>
> The information sent to the kernel log buffer here is quite trivial,
> probably dev_info() can be just removed, but in any case it should be
> a separate change.
I am not sure I get you here, but to not have false positive success
messages, I'd think that should be all in one patch.
Regards,
Wolfram
^ permalink raw reply
* Re: [PATCH] i2c: fix spelling mistake: "insufficent" -> "insufficient"
From: Wolfram Sang @ 2017-01-12 19:25 UTC (permalink / raw)
To: Colin King; +Cc: linux-i2c, linux-kernel
In-Reply-To: <20161229222733.30284-1-colin.king@canonical.com>
On Thu, Dec 29, 2016 at 10:27:33PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Trivial fix to spelling mistake in WARN message, insufficient has
> an insufficient number of i's in the spelling.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
:)
Applied to for-current, thanks!
^ permalink raw reply
* Re: [PATCH v2] i2c: piix4: Avoid race conditions with IMC
From: Wolfram Sang @ 2017-01-12 19:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Ricardo Ribalda Delgado, Jean Delvare, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <CAHp75VeV2TfgfaYni7oS4ax4H87euJa9ReWX+vYjZ4b5nYRsMg@mail.gmail.com>
> After addressing below comment
> FWIW: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>:
@andy: Please don't prefix tags. Patchwork doesn't seem to find them like this :/
> > Credit-to: Alexandre Desnoyers <alex@qtec.com>
>
> It would be nice to put in plain test what you tell in the discussion
> here instead of non-standard (see submitting-patches.rst) tag.
@ricardo: I agree. Is this Reported-by maybe?
^ permalink raw reply
* Re: [PATCH v2] i2c: piix4: Avoid race conditions with IMC
From: Andy Shevchenko @ 2017-01-12 19:36 UTC (permalink / raw)
To: Wolfram Sang
Cc: Ricardo Ribalda Delgado, Jean Delvare, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <20170112192908.zvi7puq72rnfcj6j@ninjato>
On Thu, Jan 12, 2017 at 9:29 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> After addressing below comment
>> FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>:
> @andy: Please don't prefix tags. Patchwork doesn't seem to find them like this :/
Oops, sorry, didn't notice that. I usually use a new line for them.
>> > Credit-to: Alexandre Desnoyers <alex@qtec.com>
>>
>> It would be nice to put in plain test what you tell in the discussion
>> here instead of non-standard (see submitting-patches.rst) tag.
>
> @ricardo: I agree. Is this Reported-by maybe?
Yeah, since it was mentioned IIRC that guy is hardware engineer I
don't think Suggested-by would work here better.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] i2c: piix4: Avoid race conditions with IMC
From: Andy Shevchenko @ 2017-01-12 19:37 UTC (permalink / raw)
To: Wolfram Sang
Cc: Jean Delvare, Ricardo Ribalda Delgado, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <20170112100046.GB1477@katana>
On Thu, Jan 12, 2017 at 12:00 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > > + unsigned short piix4_smba = adapdata->smba;
>> > > u8 smba_en_lo;
>> > > u8 port;
>> > > int retval;
>> > > + int timeout = 0;
>> > > + int smbslvcnt;
>> >
>> > Keep them just after your another added variable.
>>
>> FWIW, I don't think this makes sense as a general rule. I'd rather have
>> the variables in an order which makes sense (for human readers or for
>> stack size optimization - unless gcc does it for us?), rather than
>> always adding at the same place. Is there a rationale for doing that? I
>> don't think shrinking the patch size is good enough a reason.
>
> Not really. Some say "Reorder to save bytes", some say "reorder to
> utilize cache lines most". Unless I get some numbers showing the desired
> effect,
> I go for "most readable" approach which is subjective, of
> course. I'd be totally fine with the above.
My motivation was pure readability.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] i2c: piix4: Avoid race conditions with IMC
From: Wolfram Sang @ 2017-01-12 19:54 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Ricardo Ribalda Delgado, Jean Delvare, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <CAHp75VfiwtAY6=9-Pz-NNzcYaJaHmxAB3vacMB=XMwb5wFLBkg@mail.gmail.com>
> > @andy: Please don't prefix tags. Patchwork doesn't seem to find them like this :/
>
> Oops, sorry, didn't notice that. I usually use a new line for them.
Thanks for fixing it :)
> > @ricardo: I agree. Is this Reported-by maybe?
>
> Yeah, since it was mentioned IIRC that guy is hardware engineer I
> don't think Suggested-by would work here better.
So, I changed that and applied to for-current so it can be in my next
pull-request.
Thanks!
^ permalink raw reply
* Re: [PATCH v2] i2c: piix4: Avoid race conditions with IMC
From: Ricardo Ribalda Delgado @ 2017-01-12 20:02 UTC (permalink / raw)
To: Wolfram Sang
Cc: Andy Shevchenko, Jean Delvare, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <20170112195424.mprdpyjscnj6vq6j@ninjato>
Hi Wolfram and Andy,
I have already sent a v3 with the latest suggested changes.
On Thu, Jan 12, 2017 at 8:54 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > @andy: Please don't prefix tags. Patchwork doesn't seem to find them like this :/
>>
>> Oops, sorry, didn't notice that. I usually use a new line for them.
>
> Thanks for fixing it :)
>
>> > @ricardo: I agree. Is this Reported-by maybe?
I still believe that we miss a Credit-to: or Co-author: tag ;). Most
times we do not give enough credit to the hardware engineers. I would
have taken me a long time through dark AMD doc to figure this out. But
for the meantime Reported-by will do the job.
Thanks all of you for your fast review!
--
Ricardo Ribalda
^ permalink raw reply
* [PATCH v3] i2c: piix4: Avoid race conditions with IMC
From: Ricardo Ribalda Delgado @ 2017-01-12 19:49 UTC (permalink / raw)
To: Wolfram Sang, Andy Shevchenko, Ricardo Ribalda Delgado,
Jean Delvare, linux-i2c, linux-kernel
From: Ricardo Ribalda <ricardo.ribalda@gmail.com>
On AMD's SB800 and upwards, the SMBus is shared with the Integrated
Micro Controller (IMC).
The platform provides a hardware semaphore to avoid race conditions
among them. (Check page 288 of the SB800-Series Southbridges Register
Reference Guide http://support.amd.com/TechDocs/45482.pdf)
Without this patch, many access to the SMBus end with an invalid
transaction or even with the bus stalled.
Reported-by: Alexandre Desnoyers <alex@qtec.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v3: Suggestions by Andy Shevchenko <andy.shevchenko@gmail.com> and
Wolfram Sang <wsa@the-dreams.de>:
-Use Reported-by instead of Credit-to
v2: Suggestions by Andy Shevchenko <andy.shevchenko@gmail.com>:
-Rename timeout to retries
-Use do {} while(--retries) pattern
-Group new variables
drivers/i2c/busses/i2c-piix4.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c2268cdf38e8..e34d82e79b98 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -585,10 +585,29 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
u8 command, int size, union i2c_smbus_data *data)
{
struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
+ unsigned short piix4_smba = adapdata->smba;
+ int retries = MAX_TIMEOUT;
+ int smbslvcnt;
u8 smba_en_lo;
u8 port;
int retval;
+ /* Request the SMBUS semaphore, avoid conflicts with the IMC */
+ smbslvcnt = inb_p(SMBSLVCNT);
+ do {
+ outb_p(smbslvcnt | 0x10, SMBSLVCNT);
+
+ /* Check the semaphore status */
+ smbslvcnt = inb_p(SMBSLVCNT);
+ if (smbslvcnt & 0x10)
+ break;
+
+ usleep_range(1000, 2000);
+ } while (--retries);
+ /* SMBus is still owned by the IMC, we give up */
+ if (!retries)
+ return -EBUSY;
+
mutex_lock(&piix4_mutex_sb800);
outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
@@ -606,6 +625,9 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
mutex_unlock(&piix4_mutex_sb800);
+ /* Release the semaphore */
+ outb_p(smbslvcnt | 0x20, SMBSLVCNT);
+
return retval;
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2] i2c: piix4: Avoid race conditions with IMC
From: Wolfram Sang @ 2017-01-12 20:27 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Andy Shevchenko, Jean Delvare, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <CAPybu_36pu5SDhcxnqmFwviQ4NiEH9cAx0mr7bUF6_oaGfL_Mg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 315 bytes --]
> I still believe that we miss a Credit-to: or Co-author: tag ;). Most
Co-author is just a second signed-off (in my book at least).
> times we do not give enough credit to the hardware engineers.
True. Yet again, I'd favor simply multiple SoB here.
> Thanks all of you for your fast review!
You are welcome!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2] i2c: do not enable fall back to Host Notify by default
From: Dmitry Torokhov @ 2017-01-12 20:33 UTC (permalink / raw)
To: Wolfram Sang
Cc: Rob Herring, Benjamin Tissoires, Pali Rohár,
Michał Kępień, Jean Delvare, Takashi Iwai,
linux-i2c, devicetree, linux-kernel
In-Reply-To: <20170112184101.slxulrvreq7zl2pc@ninjato>
On Thu, Jan 12, 2017 at 07:41:01PM +0100, Wolfram Sang wrote:
> On Wed, Jan 04, 2017 at 08:57:22PM -0800, Dmitry Torokhov wrote:
> > Falling back unconditionally to HostNotify as primary client's interrupt
> > breaks some drivers which alter their functionality depending on whether
> > interrupt is present or not, so let's introduce a board flag telling I2C
> > core explicitly if we want wired interrupt or HostNotify-based one:
> > I2C_CLIENT_HOST_NOTIFY.
> >
> > For DT-based systems we introduce "host-notify" property that we convert
> > to I2C_CLIENT_HOST_NOTIFY board flag.
> >
> > Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Applied to for-current, thanks!
>
> How do we handle driver fixes? Shall I take them via I2C to have the
> dependency clear? Or can they go seperately?
The drivers that need this will go [hopefully] into next so they should
be OK to go through my tree.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] i2c: piix4: Avoid race conditions with IMC
From: Andy Shevchenko @ 2017-01-12 20:42 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Wolfram Sang, Jean Delvare, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <CAPybu_36pu5SDhcxnqmFwviQ4NiEH9cAx0mr7bUF6_oaGfL_Mg@mail.gmail.com>
On Thu, Jan 12, 2017 at 10:02 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
>>> > @ricardo: I agree. Is this Reported-by maybe?
>
> I still believe that we miss a Credit-to: or Co-author: tag ;)
We have more than enough of tags.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-12 20:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, Russell King,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170112174902.j52foglkdouyz36n-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
>> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > Hello Cedric,
>> >
>> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
>> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> >> > This is surprising. I didn't recheck the manual, but that looks very
>> >> >> > uncomfortable.
>> >> >>
>> >> >> I agree but this exactly the hardware way of working described in the
>> >> >> reference manual.
>> >> >
>> >> > IMHO that's a hw bug. This makes it for example impossible to implement
>> >> > SMBus block transfers (I think).
>> >>
>> >> This is not correct.
>> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
>> >> Here we have just to prepare the hardware for the 2 next pulse but the
>> >> STOP/START/ACK pulse will be generated at the right time as required
>> >> by I2C specification.
>> >> So SMBus block transfer will be possible.
>> >
>> > A block transfer consists of a byte that specifies the count of bytes
>> > yet to come. So the device sends for example:
>> >
>> > 0x01 0xab
>> >
>> > So when you read the 1 in the first byte it's already too late to set
>> > STOP to get it after the 2nd byte.
>> >
>> > Not sure I got all the required details right, though.
>>
>> Ok I understand your use case but I always think that the harware manages it.
>> If I take the above example, the I2C SMBus block read transaction will
>> be as below:
>> S Addr Wr [A] Comm [A]
>> S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
>>
>> The first message is a single byte-transmission so there is no problem.
>>
>> The second message is a N-byte reception with N = 3
>>
>> When the I2C controller has finished to send the device address (S
>> Addr Rd), the ADDR flag is set and an interrupt is raised.
>> In the routine that handles ADDR event, we set ACK bit in order to
>> generate ACK pulse as soon as a data byte is received in the shift
>> register and then we clear the ADDR flag.
>> Please note that the SCL line is stretched low until ADDR flag is cleared.
>> So, as far I understand, the device could not sent any data as long as
>> the SCL line is stretched low. Right ?
>>
>> Then, as soon as the SCL line is high, the device could send the first
>> data byte (Count).
>> When this byte is received in the shift register, an ACK is
>> automatically generated as defined during adress match phase and the
>> data byte is pushed in DR (data register).
>> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
>> In the routine that handles RXNE event, as N=3, we just clear all
>> buffer interrupts in order to avoid another system preemption due to
>> RXNE event but we does not read the data in DR.
>
> In my example I want to receive a block of length 1, so only two bytes
> are read, a 1 (the length) and the data byte (0xab in my example). I
> think that as soon as you read the 1 it's already to late to schedule
> the NA after the next byte?
Not really. This 2-byte reception is also correctly managed.
Indeed, in this case, when the controller has sent the device address,
the ADDR flag is set and an interrupt is raised.
So, as long as the ADDR flag is not cleared, the SCL line is stretched
low and the device could not send any data.
During this address match phase, for a 2-byte reception, we enable
NACK and set POS bit (ACK/NACK position).
As POS=1, the NACK will be sent for the next byte which will be
received in the shift register instead of the current one.
So in this example, the next byte will be the last one.
After that, we clear the ADDR flag and the device is allowed to send data.
When the first data is received in the shift register, the RXNE flag
is set and an interrupt is raised.
As it is a 2-byte reception, we just clear all interrupts buffer to
avoid another preemption due to RXNE but we does not read DR.
Then, the second and last byte is received in the shift register.
The NACK is automatically sent by I2C controller as it was configured
to do that in the address match phase described above.
Moereover, as the first byte has not been read in DR, the BTF event
flag is set and an interrupt is raised.
Again, the SCL line is stretching low as long as data register has not
been read.
In the meantime, we set STOP bit to generate the pulse and we launch 2
consecutive read of DR to retrieve the 2 data bytes and release SCL
stretching.
In that way, NA and STOP are generated as expected even for a 2-byte reception.
Best regards,
Cedric
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox