* [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 v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-12 13:47 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: <20170112120309.fmrt2lwz3vklqmti@pengutronix.de>
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.
After receiving the ACK, the device could send the second data byte (Data1).
When this byte is received in the shift register, an ACK is
automatically generated.
As the first data byte has not been read yet in DR, the BTF (Byte
Transfer Finihed) flag is set and an interrupt is raised.
So, in that case, the SCL line is also streched low as long as the
data register has not been read.
In the routine that handle BTF event, we enable NACK in order to
generate this pulse as soon as the last data byte will be received and
then we read DR register ([Count])
At this moment, SCL line is released and the device could send the
last data byte.
After receiving the ACK, the device could send the third and last data
byte (Data2)
When this byte is received in the shift register, a NACK is
automatically generated as we enable it as explained above.
As the second data byte (Data1) has not been read yet in DR, the BTF
flag is set again and an interrupt is raised.
The SCL line is stretched low and in that way we could set the STOP
bit to generate this pulse.
Then we run 2 consecutives read of DR to retrieve [Data1] and [Data2]
and to set SCL high.
So, thanks to SCL stretching, it seems that NA and P will be generated
at the right time.
Please let me know if it is not correct.
Best regards,
Cedric
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-12 12:03 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: <CAOAejn2pW20VPP_yGtvJ_ufvj6Xj1poBiiA2WqkALiaLyyONug@mail.gmail.com>
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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-12 11:25 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: <20170111154200.xcxdlnnakyttumzw@pengutronix.de>
Hi Uwe,
2017-01-11 16:42 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Cedric,
>
> On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
>> >
>> >> + */
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> >
>> > You could get rid of this, when caching the value of CR1. Would save two
>> > register reads here. This doesn't work for all registers, but it should
>> > be possible to apply for most of them, maybe enough to get rid of the
>> > clr_bits and set_bits function.
>>
>> I agree at many places I could save registers read by not using
>> clr_bits and set_bits function when the registers in question has been
>> already read.
>> But it is not enough to get rid of the clr_bits and set_bits function.
>> For example when calling stm32f4_i2c_terminate_xfer(), the CR1
>> register is never read before so set_bits function is useful.
>
> I didn't double check the manual, but I would expect that CR1 isn't
> modified by hardware. So you can cache the result in the driver data
> structure and do the necessary modifications with that one.
CR1 is modified by hardware to clear some bits set by software during
the communication.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Best regards,
Cedric
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-12 11:23 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: <20170111153940.dtxzvtdici3r7l54@pengutronix.de>
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:
>> Hi Uwe,
>>
>> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > Hello Cedric,
>> >
>> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>> >> +/*
>> >> + * In standard mode:
>> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
>> >> + *
>> >> + * In fast mode:
>> >> + * If Duty = 0; SCL high period = 1 * CCR * I2C parent clk period
> ^^
>> >> + * SCL low period = 2 * CCR * I2C parent clk period
> ^^
>> >> + * If Duty = 1; SCL high period = 9 * CCR * I2C parent clk period
> ^^
>> >> + * SCL low period = 16 * CCR * I2C parent clk period
>
>> > s/ \*/ */ several times
>>
>> Sorry but I don't see where is the issue as the style for multi-line
>> comments seems ok.
>> Could you please clarify that point if possible ? Thanks in advance
>
> There are several places with double spaces before * marked above.
Ok I see thanks.
>
>> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
>> >> + * Duty = 1
>> >> + *
>> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> > s/mode/Mode/
>>
>> ok thanks
>>
>> >
>> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>> >> + * constraints defined by i2c bus specification
>> >
>> > 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.
>
>> >> + */
>> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> >> [...]
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_hw_config() - Prepare I2C block
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + int ret = 0;
>> >> +
>> >> + /* Disable I2C */
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> >> +
>> >> + ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + stm32f4_i2c_set_rise_time(i2c_dev);
>> >> +
>> >> + stm32f4_i2c_set_speed_mode(i2c_dev);
>> >> +
>> >> + stm32f4_i2c_set_filter(i2c_dev);
>> >> +
>> >> + /* Enable I2C */
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>> >
>> > This function is called after a hw reset, so there should be no need to
>> > use clr_bits and set_bits because the value read from hw should be
>> > known.
>>
>> ok thanks
>>
>> >
>> >> + return ret;
>> >
>> > return 0;
>>
>> ok thanks
>>
>> >
>> >> +}
>> >> +
>> >> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> + u32 status;
>> >> + int ret;
>> >> +
>> >> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> >> + status,
>> >> + !(status & STM32F4_I2C_SR2_BUSY),
>> >> + 10, 1000);
>> >> + if (ret) {
>> >> + dev_dbg(i2c_dev->dev, "bus not free\n");
>> >> + ret = -EBUSY;
>> >> + }
>> >> +
>> >> + return ret;
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>> >> + * @i2c_dev: Controller's private data
>> >> + * @byte: Data to write in the register
>> >> + */
>> >> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>> >> +{
>> >> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>> >> + * @i2c_dev: Controller's private data
>> >> + *
>> >> + * This function fills the data register with I2C transfer buffer
>> >> + */
>> >> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> +
>> >> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>> >> + msg->count--;
>> >> +}
>> >> +
>> >> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> + u32 rbuf;
>> >> +
>> >> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> >> + *msg->buf++ = rbuf & 0xff;
>> >
>> > This is unnecessary. buf has an 8 bit wide type so
>> >
>> > *msg->buf++ = rbuf;
>> >
>> > has the same effect. (ISTR this is something I already pointed out
>> > earlier?)
>>
>> Yes you are right.
>>
>> >
>> >> + msg->count--;
>> >> +}
>> >> +
>> >> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> >> +
>> >> + stm32f4_i2c_disable_irq(i2c_dev);
>> >> +
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + if (msg->stop)
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> >> + else
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> >> +
>> >> + complete(&i2c_dev->complete);
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> >> +
>> >> + if (msg->count) {
>> >> + stm32f4_i2c_write_msg(i2c_dev);
>> >> + if (!msg->count) {
>> >> + /* Disable buffer interrupts for RXNE/TXE events */
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> >> + }
>> >> + } else {
>> >> + stm32f4_i2c_terminate_xfer(i2c_dev);
>> >
>> > Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
>> > yes, is it then right to set STM32F4_I2C_CR1_STOP or
>> > STM32F4_I2C_CR1_START?
>>
>> If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
>> In that case, we return -EAGAIN and i2c-core will retry by calling
>> stm32f4_i2c_xfer()
>>
>> >
>> >> + }
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> >> +
>> >> + switch (msg->count) {
>> >> + case 1:
>> >> + stm32f4_i2c_disable_irq(i2c_dev);
>> >> + stm32f4_i2c_read_msg(i2c_dev);
>> >> + complete(&i2c_dev->complete);
>> >> + break;
>> >> + /*
>> >> + * For 2 or 3-byte reception, we do not have to read the data register
>> >> + * when RXNE occurs as we have to wait for byte transferred finished
>> >
>> > it's hard to understand because if you don't know the hardware the
>> > meaning of RXNE is unknown.
>>
>> Ok I will replace RXNE by RX not empty in that comment
>>
>> >
>> >> + * event before reading data. So, here we just disable buffer
>> >> + * interrupt in order to avoid another system preemption due to RXNE
>> >> + * event
>> >> + */
>> >> + case 2:
>> >> + case 3:
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> >> + break;
>> >> + /* For N byte reception with N > 3 we directly read data register */
>> >> + default:
>> >> + stm32f4_i2c_read_msg(i2c_dev);
>> >> + }
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>> >> + * in case of read
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >
>> > btf is a hw-related name. Maybe better use _done which is easier to
>> > understand?
>>
>> OK
>>
>> >
>> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> + void __iomem *reg;
>> >> + u32 mask;
>> >> + int i;
>> >> +
>> >> + switch (msg->count) {
>> >> + case 2:
>> >> + /*
>> >> + * In order to correctly send the Stop or Repeated Start
>> >> + * condition on the I2C bus, the STOP/START bit has to be set
>> >> + * before reading the last two bytes.
>> >> + * After that, we could read the last two bytes, disable
>> >> + * remaining interrupts and notify the end of xfer to the
>> >> + * client
>> >
>> > 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.
>
>> > How does this work, when I only want to read a single
>> > byte? Same problem for ACK below.
>>
>> For a single reception, we enable NACK and STOP or Repeatead START
>> bits during address match.
>> The NACK and STOP/START pulses are sent as soon as the data is
>> received in the shift register.
>> Please note that in that case, we don't have to wait BTF event to read the data.
>> Data is read as soon as RXNE event occurs.
>>
>> >
>> >> + */
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + if (msg->stop)
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> >> + else
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> >> +
>> >> + for (i = 2; i > 0; i--)
>> >> + stm32f4_i2c_read_msg(i2c_dev);
>> >> +
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>> >> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> >> + stm32f4_i2c_clr_bits(reg, mask);
>> >> +
>> >> + complete(&i2c_dev->complete);
>> >> + break;
>> >> + case 3:
>> >> + /*
>> >> + * In order to correctly send the ACK on the I2C bus for the
>> >> + * last two bytes, we have to set ACK bit before reading the
>> >> + * third last data byte
>> >> + */
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> + stm32f4_i2c_read_msg(i2c_dev);
>> >> + break;
>> >> + default:
>> >> + stm32f4_i2c_read_msg(i2c_dev);
>> >> + }
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>> >> + * master receiver
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> + void __iomem *reg;
>> >> +
>> >> + switch (msg->count) {
>> >> + case 0:
>> >> + stm32f4_i2c_terminate_xfer(i2c_dev);
>> >> + /* Clear ADDR flag */
>> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> >> + break;
>> >> + case 1:
>> >> + /*
>> >> + * Single byte reception:
>> >
>> > This also happens for the last byte of a 5 byte transfer, right?
>>
>> For a 5 byte transfer the behavior is different:
>> We have to read data from DR (data register) as soon as the RXNE (RX
>> not empty) event occurs for data1, data2 and data3 (until N-2 data for
>> a more generic case)
>> The ACK is automatically sent as soon as the data is received in the
>> shift register as the I2C controller was configured to do that during
>> adress match phase.
>>
>> For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
>> in order to set NACK before reading DR.
>> This event occurs when a new data has been received in shift register
>> (in our case data4 or N-1 data) but the prevoius data in DR (in our
>> case data3 or N-2 data) has not been read yet.
>> In that way, the NACK pulse will be correctly generated after the last
>> received data byte.
>>
>> For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
>> and data5 or N data in shift register), set STOP or repeated Start in
>> order to correctly sent the right pulse after the last received data
>> byte and run 2 consecutives read of DR.
>
> So "Single byte reception" above is wrong, as this case is also used for
> longer transfers and should be updated accordingly.
I don't think so.
stm32f4_i2c_handle_rx_addr() is called once during adress match phase.
It is used to configure the I2C controller according to the number of
data to be received as it has to be done in a different way according
to the number of data to received:
- single byte reception
- 2-byte reception
- N-byte reception
Then, as soon as, the controller is correctly configured, for each
byte to be received, we use stm32f4_i2c_handle_read() or
stm32f4_i2c_handle_rx_done().
stm32f4_i2c_handle_read() is used to read data for a single byte
reception or until N-2 data for N-byte reception
stm32f4_i2c_handle_rx_done() is used to read data for a 2-byte
reception, or data N-2, N-1 and N for a N-byte reception.
So, single-reception and longer transfer have been clearly managed in
a different way.
>
>> >> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>> >> + */
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> >> + if (msg->stop)
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> >> + else
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> >> + break;
>> >> + case 2:
>> >> + /*
>> >> + * 2-byte reception:
>> >> + * Enable NACK and set POS
>> >
>> > What is POS?
>> POS is used to define the position of the (N)ACK pulse
>> 0: ACK is generated when the current is being received in the shift register
>> 1: ACK is generated when the next byte which will be received in the
>> shift register (used for 2-byte reception)
>
> Can you please put this into the comment. "POS" isn't much helpful
> there.
Ok I will add a comment for that.
>
>>
>> >
>> >> + */
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> >
>> > You could get rid of this, when caching the value of CR1. Would save two
>> > register reads here. This doesn't work for all registers, but it should
>> > be possible to apply for most of them, maybe enough to get rid of the
>> > clr_bits and set_bits function.
>> >
>> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> >> + break;
>> >> +
>> >> + default:
>> >> + /* N-byte reception: Enable ACK */
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>> >
>> > Do you need to set ACK for each byte transferred?
>> I need to do that in order to be SMBus compatible and the ACK/NACK
>> seems to be used by default in Documentation/i2c/i2c-protocol file.
>
> Yeah, protocol wise you need to ack each byte. I just wondered if you
> need to set the hardware bit for each byte or if it is retained in
> hardware until unset by a register write.
ACK bit is set in stm32f4_i2c_handle_rx_addr().
As explained above, this function is called once during address match phase.
So, this bit is set only once just before receiving the first data byte.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Best regards,
Cedric
^ permalink raw reply
* Re: [PATCH] i2c: piix4: Avoid race conditions with IMC
From: Wolfram Sang @ 2017-01-12 10:00 UTC (permalink / raw)
To: Jean Delvare
Cc: Andy Shevchenko, Ricardo Ribalda Delgado, linux-i2c, linux-kernel
In-Reply-To: <20170112104006.4dfed9b7@endymion>
[-- Attachment #1: Type: text/plain, Size: 885 bytes --]
> > > + 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH linux v2 0/6] drivers: hwmon: Add On-Chip Controller driver
From: Wolfram Sang @ 2017-01-12 9:56 UTC (permalink / raw)
To: eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-0h96xk9xTtrk1uMJSBkQmQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
jdelvare-IBi9RG/b67k, corbet-T1hC0tSOHrs,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, andrew-zrmu5oMJ5Fs,
joel-U3u1mxZcP9KHXe+LvDLADg,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r, Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 444 bytes --]
> This patchset adds a hwmon driver to support the OCC (On-Chip Controller)
> on the IBM POWER8 and POWER9 processors, from a BMC (Baseboard Management
> Controller). The OCC is an embedded processor that provides real time
> power and thermal monitoring.
Please don't cc the I2C list for I2C client drivers unless you have a
specific question about the I2C framework. Same as you usually don't add
the PCI list for every PCI (client) card.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: piix4: Avoid race conditions with IMC
From: Jean Delvare @ 2017-01-12 9:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Ricardo Ribalda Delgado, Wolfram Sang, linux-i2c, linux-kernel
In-Reply-To: <CAHp75VdnoW84sQh-vf57NSC6_tXGqvb_36KCPpi-XFbktWiKfw@mail.gmail.com>
On Wed, 11 Jan 2017 03:49:21 +0200, Andy Shevchenko wrote:
> On Tue, Jan 10, 2017 at 2:16 PM, Ricardo Ribalda Delgado
> > --- a/drivers/i2c/busses/i2c-piix4.c
> > +++ b/drivers/i2c/busses/i2c-piix4.c
> > @@ -585,9 +585,28 @@ 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;
> > 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.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: [PATCH linux v2 2/6] hwmon: occ: Add sysfs interface
From: kbuild test robot @ 2017-01-12 7:39 UTC (permalink / raw)
To: eajames.ibm
Cc: kbuild-all, linux, devicetree, jdelvare, corbet, linux-doc,
linux-hwmon, linux-i2c, linux-kernel, mark.rutland, robh+dt, wsa,
andrew, joel, benh, Edward A. James
In-Reply-To: <1484158237-10014-3-git-send-email-eajames.ibm@gmail.com>
Hi Edward,
[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v4.10-rc3 next-20170111]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/eajames-ibm-gmail-com/hwmon-Add-core-On-Chip-Controller-support-for-POWER-CPUs/20170112-150440
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/hwmon/occ/occ_sysfs.c:265:9-16: WARNING: ERR_CAST can be used with hwmon -> dev
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* [PATCH] hwmon: occ: fix err_cast.cocci warnings
From: kbuild test robot @ 2017-01-12 7:39 UTC (permalink / raw)
To: eajames.ibm
Cc: kbuild-all, linux, devicetree, jdelvare, corbet, linux-doc,
linux-hwmon, linux-i2c, linux-kernel, mark.rutland, robh+dt, wsa,
andrew, joel, benh, Edward A. James
In-Reply-To: <1484158237-10014-3-git-send-email-eajames.ibm@gmail.com>
drivers/hwmon/occ/occ_sysfs.c:265:9-16: WARNING: ERR_CAST can be used with hwmon -> dev
Use ERR_CAST inlined function instead of ERR_PTR(PTR_ERR(...))
Generated by: scripts/coccinelle/api/err_cast.cocci
CC: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
occ_sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/hwmon/occ/occ_sysfs.c
+++ b/drivers/hwmon/occ/occ_sysfs.c
@@ -262,7 +262,7 @@ struct occ_sysfs *occ_sysfs_start(struct
if (IS_ERR(hwmon->dev)) {
dev_err(dev, "cannot register hwmon device %s: %ld\n",
hwmon->hwmon_name, PTR_ERR(hwmon->dev));
- return ERR_PTR(PTR_ERR(hwmon->dev));
+ return ERR_CAST(hwmon->dev);
}
return hwmon;
^ permalink raw reply
* [PATCH linux v2 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: eajames.ibm @ 2017-01-11 18:10 UTC (permalink / raw)
To: linux
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm@gmail.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as
well as probe the entire driver from the I2C bus. I2C is the communication
method between the BMC and the P8 OCC.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Documentation/devicetree/bindings/hwmon/occ.txt | 13 +++
drivers/hwmon/occ/Kconfig | 14 +++
drivers/hwmon/occ/Makefile | 1 +
drivers/hwmon/occ/p8_occ_i2c.c | 123 ++++++++++++++++++++++++
4 files changed, 151 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
diff --git a/Documentation/devicetree/bindings/hwmon/occ.txt b/Documentation/devicetree/bindings/hwmon/occ.txt
new file mode 100644
index 0000000..b0d2b36
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/occ.txt
@@ -0,0 +1,13 @@
+HWMON I2C driver for IBM POWER CPU OCC (On Chip Controller)
+
+Required properties:
+ - compatible: must be "ibm,p8-occ-i2c"
+ - reg: physical address
+
+Example:
+i2c3: i2c-bus@100 {
+ occ@50 {
+ compatible = "ibm,p8-occ-i2c";
+ reg = <0x50>;
+ };
+};
diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
index cdb64a7..3a5188f 100644
--- a/drivers/hwmon/occ/Kconfig
+++ b/drivers/hwmon/occ/Kconfig
@@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC
This driver can also be built as a module. If so, the module
will be called occ.
+
+if SENSORS_PPC_OCC
+
+config SENSORS_PPC_OCC_P8_I2C
+ tristate "POWER8 OCC hwmon support"
+ depends on I2C
+ help
+ Provide a hwmon sysfs interface for the POWER8 On-Chip Controller,
+ exposing temperature, frequency and power measurements.
+
+ This driver can also be built as a module. If so, the module will be
+ called p8-occ-i2c.
+
+endif
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
index a6881f9..9294b58 100644
--- a/drivers/hwmon/occ/Makefile
+++ b/drivers/hwmon/occ/Makefile
@@ -1 +1,2 @@
obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
+obj-$(CONFIG_SENSORS_PPC_OCC_P8_I2C) += occ_scom_i2c.o occ_p8.o p8_occ_i2c.o
diff --git a/drivers/hwmon/occ/p8_occ_i2c.c b/drivers/hwmon/occ/p8_occ_i2c.c
new file mode 100644
index 0000000..4515c68
--- /dev/null
+++ b/drivers/hwmon/occ/p8_occ_i2c.c
@@ -0,0 +1,123 @@
+/*
+ * p8_occ_i2c.c - hwmon OCC driver
+ *
+ * This file contains the i2c layer for accessing the P8 OCC over i2c bus.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "scom.h"
+#include "occ_scom_i2c.h"
+#include "occ_p8.h"
+#include "occ_sysfs.h"
+
+#define P8_OCC_I2C_NAME "p8-occ-i2c"
+
+int p8_i2c_getscom(void *bus, u32 address, u64 *data)
+{
+ /* P8 i2c slave requires address to be shifted by 1 */
+ address = address << 1;
+
+ return occ_i2c_getscom(bus, address, data);
+}
+
+int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+ /* P8 i2c slave requires address to be shifted by 1 */
+ address = address << 1;
+
+ return occ_i2c_putscom(bus, address, data0, data1);
+}
+
+static struct occ_bus_ops p8_bus_ops = {
+ .getscom = p8_i2c_getscom,
+ .putscom = p8_i2c_putscom,
+};
+
+static int p8_occ_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct occ *occ;
+ struct occ_sysfs *hwmon;
+ const u32 *sensor_hwmon_configs = p8_get_sensor_hwmon_configs();
+
+ occ = p8_occ_start(&client->dev, client, &p8_bus_ops);
+ if (IS_ERR(occ))
+ return PTR_ERR(occ);
+
+ hwmon = occ_sysfs_start(&client->dev, occ, sensor_hwmon_configs,
+ P8_OCC_I2C_NAME);
+ if (IS_ERR(hwmon))
+ return PTR_ERR(hwmon);
+
+ i2c_set_clientdata(client, occ);
+
+ return 0;
+}
+
+static int p8_occ_remove(struct i2c_client *client)
+{
+ struct occ *occ = i2c_get_clientdata(client);
+
+ return p8_occ_stop(occ);
+}
+
+/* used by old-style board info. */
+static const struct i2c_device_id occ_ids[] = {
+ { P8_OCC_I2C_NAME, 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, occ_ids);
+
+/* used by device table */
+static const struct of_device_id occ_of_match[] = {
+ { .compatible = "ibm,p8-occ-i2c" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, occ_of_match);
+
+/*
+ * i2c-core uses i2c-detect() to detect device in below address list.
+ * If exists, address will be assigned to client.
+ * It is also possible to read address from device table.
+ */
+static const unsigned short normal_i2c[] = {0x50, 0x51, I2C_CLIENT_END };
+
+static struct i2c_driver p8_occ_driver = {
+ .class = I2C_CLASS_HWMON,
+ .driver = {
+ .name = P8_OCC_I2C_NAME,
+ .pm = NULL,
+ .of_match_table = occ_of_match,
+ },
+ .probe = p8_occ_probe,
+ .remove = p8_occ_remove,
+ .id_table = occ_ids,
+ .address_list = normal_i2c,
+};
+
+module_i2c_driver(p8_occ_driver);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("BMC P8 OCC hwmon driver");
+MODULE_LICENSE("GPL");
--
1.9.1
^ permalink raw reply related
* [PATCH linux v2 0/6] drivers: hwmon: Add On-Chip Controller driver
From: eajames.ibm @ 2017-01-11 18:10 UTC (permalink / raw)
To: linux
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
This patchset adds a hwmon driver to support the OCC (On-Chip Controller)
on the IBM POWER8 and POWER9 processors, from a BMC (Baseboard Management
Controller). The OCC is an embedded processor that provides real time
power and thermal monitoring.
The driver provides an interface on a BMC to poll OCC sensor data, set
user power caps, and perform some basic OCC error handling. It interfaces
with userspace through hwmon.
The driver is currently functional only for the OCC on POWER8 chips.
Communicating with the POWER9 OCC requries FSI support.
Edward A. James (6):
hwmon: Add core On-Chip Controller support for POWER CPUs
hwmon: occ: Add sysfs interface
hwmon: occ: Add I2C transport implementation for SCOM operations
hwmon: occ: Add callbacks for parsing P8 OCC datastructures
hwmon: occ: Add hwmon implementation for the P8 OCC
hwmon: occ: Add callbacks for parsing P9 OCC datastructures
Documentation/devicetree/bindings/hwmon/occ.txt | 13 +
Documentation/hwmon/occ | 114 +++++
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 29 ++
drivers/hwmon/occ/Makefile | 2 +
drivers/hwmon/occ/occ.c | 533 ++++++++++++++++++++++++
drivers/hwmon/occ/occ.h | 83 ++++
drivers/hwmon/occ/occ_p8.c | 254 +++++++++++
drivers/hwmon/occ/occ_p8.h | 31 ++
drivers/hwmon/occ/occ_p9.c | 314 ++++++++++++++
drivers/hwmon/occ/occ_p9.h | 31 ++
drivers/hwmon/occ/occ_scom_i2c.c | 73 ++++
drivers/hwmon/occ/occ_scom_i2c.h | 26 ++
drivers/hwmon/occ/occ_sysfs.c | 274 ++++++++++++
drivers/hwmon/occ/occ_sysfs.h | 44 ++
drivers/hwmon/occ/p8_occ_i2c.c | 123 ++++++
drivers/hwmon/occ/scom.h | 47 +++
18 files changed, 1994 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
create mode 100644 Documentation/hwmon/occ
create mode 100644 drivers/hwmon/occ/Kconfig
create mode 100644 drivers/hwmon/occ/Makefile
create mode 100644 drivers/hwmon/occ/occ.c
create mode 100644 drivers/hwmon/occ/occ.h
create mode 100644 drivers/hwmon/occ/occ_p8.c
create mode 100644 drivers/hwmon/occ/occ_p8.h
create mode 100644 drivers/hwmon/occ/occ_p9.c
create mode 100644 drivers/hwmon/occ/occ_p9.h
create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
create mode 100644 drivers/hwmon/occ/occ_sysfs.c
create mode 100644 drivers/hwmon/occ/occ_sysfs.h
create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
create mode 100644 drivers/hwmon/occ/scom.h
--
1.9.1
^ permalink raw reply
* [PATCH linux v2 6/6] hwmon: occ: Add callbacks for parsing P9 OCC datastructures
From: eajames.ibm @ 2017-01-11 18:10 UTC (permalink / raw)
To: linux
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm@gmail.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add functions to parse the data structures that are specific to the OCC on
the POWER9 processor. These are the sensor data structures, including
temperature, frequency, power, and "caps."
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Documentation/hwmon/occ | 3 +
drivers/hwmon/occ/occ_p9.c | 314 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ_p9.h | 31 +++++
3 files changed, 348 insertions(+)
create mode 100644 drivers/hwmon/occ/occ_p9.c
create mode 100644 drivers/hwmon/occ/occ_p9.h
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index 143951e..6cea853 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -34,6 +34,9 @@ number of data structures, such as command format, response headers, and the
like, are also defined in this specification, and are common to both POWER8 and
POWER9 OCCs.
+There is currently no public P9 OCC specification, and the data structures
+defined in the POWER9 OCC driver are subject to change.
+
sysfs Entries
-------------
diff --git a/drivers/hwmon/occ/occ_p9.c b/drivers/hwmon/occ/occ_p9.c
new file mode 100644
index 0000000..8b351a0
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p9.c
@@ -0,0 +1,314 @@
+/*
+ * occ_p9.c - OCC hwmon driver
+ *
+ * This file contains the Power9-specific methods and data structures for
+ * the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <asm/unaligned.h>
+
+#include "occ.h"
+#include "occ_p9.h"
+
+/* P9 OCC sensor data format */
+struct p9_temp_sensor {
+ u32 sensor_id;
+ u8 fru_type;
+ u8 value;
+};
+
+struct p9_freq_sensor {
+ u32 sensor_id;
+ u16 value;
+};
+
+struct p9_power_sensor {
+ u32 sensor_id;
+ u8 function_id;
+ u8 apss_channel;
+ u16 reserved;
+ u32 update_tag;
+ u64 accumulator;
+ u16 value;
+};
+
+struct p9_caps_sensor {
+ u16 curr_powercap;
+ u16 curr_powerreading;
+ u16 norm_powercap;
+ u16 max_powercap;
+ u16 min_powercap;
+ u16 user_powerlimit;
+ u8 user_powerlimit_source;
+};
+
+static const u32 p9_sensor_hwmon_configs[MAX_OCC_SENSOR_TYPE] = {
+ HWMON_I_INPUT | HWMON_I_LABEL, /* freq: value | label */
+ /* temp: value | label | fru_type */
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_TYPE,
+ /* power: value | label | accum[0] | accum[1] | update_tag |
+ * (function_id | (apss_channel << 8))
+ */
+ HWMON_P_INPUT | HWMON_P_LABEL | HWMON_P_AVERAGE_MIN |
+ HWMON_P_AVERAGE_MAX | HWMON_P_AVERAGE_INTERVAL |
+ HWMON_P_RESET_HISTORY,
+ /* caps: curr | max | min | norm | user | source */
+ HWMON_P_CAP | HWMON_P_CAP_MAX | HWMON_P_CAP_MIN | HWMON_P_MAX |
+ HWMON_P_ALARM | HWMON_P_CAP_ALARM,
+};
+
+void p9_parse_sensor(u8 *data, void *sensor, int sensor_type, int off,
+ int snum)
+{
+ switch (sensor_type) {
+ case FREQ:
+ {
+ struct p9_freq_sensor *fs =
+ &(((struct p9_freq_sensor *)sensor)[snum]);
+
+ fs->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+ fs->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+ }
+ break;
+ case TEMP:
+ {
+ struct p9_temp_sensor *ts =
+ &(((struct p9_temp_sensor *)sensor)[snum]);
+
+ ts->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+ fs->fru_type = data[off + 4];
+ fs->value = data[off + 5];
+ }
+ break;
+ case POWER:
+ {
+ struct p9_power_sensor *ps =
+ &(((struct p9_power_sensor *)sensor)[snum]);
+
+ ps->sensor_id = be32_to_cpu(get_unaligned((u32 *)&data[off]));
+ ps->function_id = data[off + 4];
+ ps->apss_channel = data[off + 5];
+ ps->update_tag =
+ be32_to_cpu(get_unaligned((u32 *)&data[off + 8]));
+ ps->accumulator =
+ be64_to_cpu(get_unaligned((u64 *)&data[off + 12]));
+ ps->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 20]));
+ }
+ break;
+ case CAPS:
+ {
+ struct p9_caps_sensor *cs =
+ &(((struct p9_caps_sensor *)sensor)[snum]);
+
+ cs->curr_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off]));
+ cs->curr_powerreading =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+ cs->norm_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+ cs->max_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 6]));
+ cs->min_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 8]));
+ cs->user_powerlimit =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+ cs->user_powerlimit_source = data[off + 12];
+ }
+ break;
+ };
+}
+
+void *p9_alloc_sensor(int sensor_type, int num_sensors)
+{
+ switch (sensor_type) {
+ case FREQ:
+ return kcalloc(num_sensors, sizeof(struct p9_freq_sensor),
+ GFP_KERNEL);
+ case TEMP:
+ return kcalloc(num_sensors, sizeof(struct p9_temp_sensor),
+ GFP_KERNEL);
+ case POWER:
+ return kcalloc(num_sensors, sizeof(struct p9_power_sensor),
+ GFP_KERNEL);
+ case CAPS:
+ return kcalloc(num_sensors, sizeof(struct p9_caps_sensor),
+ GFP_KERNEL);
+ default:
+ return NULL;
+ }
+}
+
+int p9_get_sensor(struct occ *driver, int sensor_type, int sensor_num,
+ u32 hwmon, long *val)
+{
+ int rc = 0;
+ void *sensor;
+
+ if (sensor_type == POWER) {
+ if (hwmon == hwmon_power_cap || hwmon == hwmon_power_cap_max ||
+ hwmon == hwmon_power_cap_min || hwmon == hwmon_power_max ||
+ hwmon == hwmon_power_alarm ||
+ hwmon == hwmon_power_cap_alarm)
+ sensor_type = CAPS;
+ }
+
+ sensor = occ_get_sensor(driver, sensor_type);
+ if (!sensor)
+ return -ENODEV;
+
+ switch (sensor_type) {
+ case FREQ:
+ {
+ struct p9_freq_sensor *fs =
+ &(((struct p9_freq_sensor *)sensor)[snum]);
+
+ switch (hwmon) {
+ case hwmon_in_input:
+ *val = fs->value;
+ break;
+ case hwmon_in_label:
+ *val = fs->sensor_id;
+ break;
+ default:
+ rc = -EOPNOTSUPP;
+ }
+ }
+ break;
+ case TEMP:
+ {
+ struct p9_temp_sensor *ts =
+ &(((struct p9_temp_sensor *)sensor)[snum]);
+
+ switch (hwmon) {
+ case hwmon_temp_input:
+ *val = ts->value;
+ break;
+ case hwmon_temp_type:
+ *val = ts->fru_type;
+ break;
+ case hwmon_temp_label:
+ *val = ts->sensor_id;
+ break;
+ default:
+ rc = -EOPNOTSUPP;
+ }
+ }
+ break;
+ case POWER:
+ {
+ struct p9_power_sensor *ps =
+ &(((struct p9_power_sensor *)sensor)[snum]);
+
+ switch (hwmon) {
+ case hwmon_power_input:
+ *val = ps->value;
+ break;
+ case hwmon_power_label:
+ *val = ps->sensor_id;
+ break;
+ case hwmon_power_average_min:
+ *val = ((u32 *)(&ps->accumulator))[0];
+ break;
+ case hwmon_power_average_max:
+ *val = ((u32 *)(&ps->accumulator))[1];
+ break;
+ case hwmon_power_average_interval:
+ *val = ps->update_tag;
+ break;
+ case hwmon_power_reset_history:
+ *val = ps->function_id | (ps->apss_channel << 8);
+ break;
+ default:
+ rc = -EOPNOTSUPP;
+ }
+ }
+ break;
+ case CAPS:
+ {
+ struct p9_caps_sensor *cs =
+ &(((struct p9_caps_sensor *)sensor)[snum]);
+
+ switch (hwmon) {
+ case hwmon_power_cap:
+ *val = cs->curr_powercap;
+ break;
+ case hwmon_power_cap_max:
+ *val = cs->max_powercap;
+ break;
+ case hwmon_power_cap_min:
+ *val = cs->min_powercap;
+ break;
+ case hwmon_power_max:
+ *val = cs->norm_powercap;
+ break;
+ case hwmon_power_alarm:
+ *val = cs->user_powerlimit;
+ break;
+ case hwmon_power_cap_alarm:
+ *val = cs->user_powerlimit_source;
+ break;
+ default:
+ rc = -EOPNOTSUPP;
+ }
+ }
+ break;
+ default:
+ rc = -EINVAL;
+ }
+
+ return rc;
+}
+
+static const struct occ_ops p9_ops = {
+ .parse_sensor = p9_parse_sensor,
+ .alloc_sensor = p9_alloc_sensor,
+ .get_sensor = p9_get_sensor,
+};
+
+static const struct occ_config p9_config = {
+ .command_addr = 0xFFFBE000,
+ .response_addr = 0xFFFBF000,
+};
+
+const u32 *p9_get_sensor_hwmon_configs()
+{
+ return p9_sensor_hwmon_configs;
+}
+EXPORT_SYMBOL(p9_get_sensor_hwmon_configs);
+
+struct occ *p9_occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops)
+{
+ return occ_start(dev, bus, bus_ops, &p9_ops, &p9_config);
+}
+EXPORT_SYMBOL(p9_occ_start);
+
+int p9_occ_stop(struct occ *occ)
+{
+ return occ_stop(occ);
+}
+EXPORT_SYMBOL(p9_occ_stop);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("P9 OCC sensors");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_p9.h b/drivers/hwmon/occ/occ_p9.h
new file mode 100644
index 0000000..b1c01d6
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p9.h
@@ -0,0 +1,31 @@
+/*
+ * occ_p9.h - OCC hwmon driver
+ *
+ * This file contains Power9-specific function prototypes
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#ifndef __OCC_P9_H__
+#define __OCC_P9_H__
+
+#include "scom.h"
+
+struct device;
+
+const u32 *p9_get_sensor_hwmon_configs(void);
+struct occ *p9_occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops);
+int p9_occ_stop(struct occ *occ);
+
+#endif /* __OCC_P9_H__ */
--
1.9.1
^ permalink raw reply related
* [PATCH linux v2 4/6] hwmon: occ: Add callbacks for parsing P8 OCC datastructures
From: eajames.ibm @ 2017-01-11 18:10 UTC (permalink / raw)
To: linux
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm@gmail.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add functions to parse the data structures that are specific to the OCC on
the POWER8 processor. These are the sensor data structures, including
temperature, frequency, power, and "caps."
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Documentation/hwmon/occ | 9 ++
drivers/hwmon/occ/occ_p8.c | 254 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ_p8.h | 31 ++++++
3 files changed, 294 insertions(+)
create mode 100644 drivers/hwmon/occ/occ_p8.c
create mode 100644 drivers/hwmon/occ/occ_p8.h
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index d0bdf06..143951e 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -25,6 +25,15 @@ Currently, all versions of the OCC support four types of sensor data: power,
temperature, frequency, and "caps," which indicate limits and thresholds used
internally on the OCC.
+The format for the POWER8 OCC sensor data can be found in the P8 OCC
+specification:
+github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf
+This document provides the details of the OCC sensors: power, frequency,
+temperature, and caps. These sensor formats are specific to the POWER8 OCC. A
+number of data structures, such as command format, response headers, and the
+like, are also defined in this specification, and are common to both POWER8 and
+POWER9 OCCs.
+
sysfs Entries
-------------
diff --git a/drivers/hwmon/occ/occ_p8.c b/drivers/hwmon/occ/occ_p8.c
new file mode 100644
index 0000000..6673da2
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p8.c
@@ -0,0 +1,254 @@
+/*
+ * occ_p8.c - OCC hwmon driver
+ *
+ * This file contains the Power8-specific methods and data structures for
+ * the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <asm/unaligned.h>
+#include <linux/hwmon.h>
+
+#include "occ.h"
+#include "occ_p8.h"
+
+/* P8 OCC sensor data format */
+struct p8_occ_sensor {
+ u16 sensor_id;
+ u16 value;
+};
+
+struct p8_power_sensor {
+ u16 sensor_id;
+ u32 update_tag;
+ u32 accumulator;
+ u16 value;
+};
+
+struct p8_caps_sensor {
+ u16 curr_powercap;
+ u16 curr_powerreading;
+ u16 norm_powercap;
+ u16 max_powercap;
+ u16 min_powercap;
+ u16 user_powerlimit;
+};
+
+static const u32 p8_sensor_hwmon_configs[MAX_OCC_SENSOR_TYPE] = {
+ HWMON_I_INPUT | HWMON_I_LABEL, /* freq: value | label */
+ HWMON_T_INPUT | HWMON_T_LABEL, /* temp: value | label */
+ /* power: value | label | accumulator | update_tag */
+ HWMON_P_INPUT | HWMON_P_LABEL | HWMON_P_AVERAGE |
+ HWMON_P_AVERAGE_INTERVAL,
+ /* caps: curr | max | min | norm | user */
+ HWMON_P_CAP | HWMON_P_CAP_MAX | HWMON_P_CAP_MIN | HWMON_P_MAX |
+ HWMON_P_ALARM,
+};
+
+void p8_parse_sensor(u8 *data, void *sensor, int sensor_type, int off,
+ int snum)
+{
+ switch (sensor_type) {
+ case FREQ:
+ case TEMP:
+ {
+ struct p8_occ_sensor *os =
+ &(((struct p8_occ_sensor *)sensor)[snum]);
+
+ os->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off]));
+ os->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+ }
+ break;
+ case POWER:
+ {
+ struct p8_power_sensor *ps =
+ &(((struct p8_power_sensor *)sensor)[snum]);
+
+ ps->sensor_id = be16_to_cpu(get_unaligned((u16 *)&data[off]));
+ ps->update_tag =
+ be32_to_cpu(get_unaligned((u32 *)&data[off + 2]));
+ ps->accumulator =
+ be32_to_cpu(get_unaligned((u32 *)&data[off + 6]));
+ ps->value = be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+ }
+ break;
+ case CAPS:
+ {
+ struct p8_caps_sensor *cs =
+ &(((struct p8_caps_sensor *)sensor)[snum]);
+
+ cs->curr_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off]));
+ cs->curr_powerreading =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 2]));
+ cs->norm_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 4]));
+ cs->max_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 6]));
+ cs->min_powercap =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 8]));
+ cs->user_powerlimit =
+ be16_to_cpu(get_unaligned((u16 *)&data[off + 10]));
+ }
+ break;
+ };
+}
+
+void *p8_alloc_sensor(int sensor_type, int num_sensors)
+{
+ switch (sensor_type) {
+ case FREQ:
+ case TEMP:
+ return kcalloc(num_sensors, sizeof(struct p8_occ_sensor),
+ GFP_KERNEL);
+ case POWER:
+ return kcalloc(num_sensors, sizeof(struct p8_power_sensor),
+ GFP_KERNEL);
+ case CAPS:
+ return kcalloc(num_sensors, sizeof(struct p8_caps_sensor),
+ GFP_KERNEL);
+ default:
+ return NULL;
+ }
+}
+
+int p8_get_sensor(struct occ *driver, int sensor_type, int sensor_num,
+ u32 hwmon, long *val)
+{
+ int rc = 0;
+ void *sensor;
+
+ if (sensor_type == POWER) {
+ if (hwmon == hwmon_power_cap || hwmon == hwmon_power_cap_max ||
+ hwmon == hwmon_power_cap_min || hwmon == hwmon_power_max ||
+ hwmon == hwmon_power_alarm)
+ sensor_type = CAPS;
+ }
+
+ sensor = occ_get_sensor(driver, sensor_type);
+ if (!sensor)
+ return -ENODEV;
+
+ switch (sensor_type) {
+ case FREQ:
+ case TEMP:
+ {
+ struct p8_occ_sensor *os =
+ &(((struct p8_occ_sensor *)sensor)[sensor_num]);
+
+ if (hwmon == hwmon_in_input || hwmon == hwmon_temp_input)
+ *val = os->value;
+ else if (hwmon == hwmon_in_label || hwmon == hwmon_temp_label)
+ *val = os->sensor_id;
+ else
+ rc = -EOPNOTSUPP;
+ }
+ break;
+ case POWER:
+ {
+ struct p8_power_sensor *ps =
+ &(((struct p8_power_sensor *)sensor)[sensor_num]);
+
+ switch (hwmon) {
+ case hwmon_power_input:
+ *val = ps->value;
+ break;
+ case hwmon_power_label:
+ *val = ps->sensor_id;
+ break;
+ case hwmon_power_average:
+ *val = ps->accumulator;
+ break;
+ case hwmon_power_average_interval:
+ *val = ps->update_tag;
+ break;
+ default:
+ rc = -EOPNOTSUPP;
+ }
+ }
+ break;
+ case CAPS:
+ {
+ struct p8_caps_sensor *cs =
+ &(((struct p8_caps_sensor *)sensor)[sensor_num]);
+
+ switch (hwmon) {
+ case hwmon_power_cap:
+ *val = cs->curr_powercap;
+ break;
+ case hwmon_power_cap_max:
+ *val = cs->max_powercap;
+ break;
+ case hwmon_power_cap_min:
+ *val = cs->min_powercap;
+ break;
+ case hwmon_power_max:
+ *val = cs->norm_powercap;
+ break;
+ case hwmon_power_alarm:
+ *val = cs->user_powerlimit;
+ break;
+ default:
+ rc = -EOPNOTSUPP;
+ }
+ }
+ break;
+ default:
+ rc = -EINVAL;
+ }
+
+ return rc;
+}
+
+static const struct occ_ops p8_ops = {
+ .parse_sensor = p8_parse_sensor,
+ .alloc_sensor = p8_alloc_sensor,
+ .get_sensor = p8_get_sensor,
+};
+
+static const struct occ_config p8_config = {
+ .command_addr = 0xFFFF6000,
+ .response_addr = 0xFFFF7000,
+};
+
+const u32 *p8_get_sensor_hwmon_configs()
+{
+ return p8_sensor_hwmon_configs;
+}
+EXPORT_SYMBOL(p8_get_sensor_hwmon_configs);
+
+struct occ *p8_occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops)
+{
+ return occ_start(dev, bus, bus_ops, &p8_ops, &p8_config);
+}
+EXPORT_SYMBOL(p8_occ_start);
+
+int p8_occ_stop(struct occ *occ)
+{
+ return occ_stop(occ);
+}
+EXPORT_SYMBOL(p8_occ_stop);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("P8 OCC sensors");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_p8.h b/drivers/hwmon/occ/occ_p8.h
new file mode 100644
index 0000000..54ce907
--- /dev/null
+++ b/drivers/hwmon/occ/occ_p8.h
@@ -0,0 +1,31 @@
+/*
+ * occ_p8.h - OCC hwmon driver
+ *
+ * This file contains Power8-specific function prototypes
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#ifndef __OCC_P8_H__
+#define __OCC_P8_H__
+
+#include "scom.h"
+
+struct device;
+
+const u32 *p8_get_sensor_hwmon_configs(void);
+struct occ *p8_occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops);
+int p8_occ_stop(struct occ *occ);
+
+#endif /* __OCC_P8_H__ */
--
1.9.1
^ permalink raw reply related
* [PATCH linux v2 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations
From: eajames.ibm @ 2017-01-11 18:10 UTC (permalink / raw)
To: linux
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm@gmail.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add functions to send SCOM operations over I2C bus. The BMC can
communicate with the Power8 host processor over I2C, but needs to use SCOM
operations in order to access the OCC register space.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
drivers/hwmon/occ/occ_scom_i2c.c | 73 ++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++
2 files changed, 99 insertions(+)
create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
diff --git a/drivers/hwmon/occ/occ_scom_i2c.c b/drivers/hwmon/occ/occ_scom_i2c.c
new file mode 100644
index 0000000..a922f83
--- /dev/null
+++ b/drivers/hwmon/occ/occ_scom_i2c.c
@@ -0,0 +1,73 @@
+/*
+ * occ_scom_i2c.c - hwmon OCC driver
+ *
+ * This file contains the functions for performing SCOM operations over I2C bus
+ * to access the OCC.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "scom.h"
+#include "occ_scom_i2c.h"
+
+int occ_i2c_getscom(void *bus, u32 address, u64 *data)
+{
+ ssize_t rc;
+ u64 buf;
+ struct i2c_client *client = bus;
+
+ rc = i2c_master_send(client, (const char *)&address, sizeof(u32));
+ if (rc < 0)
+ return rc;
+ else if (rc != sizeof(u32))
+ return -EIO;
+
+ rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
+ if (rc < 0)
+ return rc;
+ else if (rc != sizeof(u64))
+ return -EIO;
+
+ *data = be64_to_cpu(buf);
+
+ return 0;
+}
+EXPORT_SYMBOL(occ_i2c_getscom);
+
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+ u32 buf[3];
+ ssize_t rc;
+ struct i2c_client *client = bus;
+
+ buf[0] = address;
+ buf[1] = data1;
+ buf[2] = data0;
+
+ rc = i2c_master_send(client, (const char *)buf, sizeof(u32) * 3);
+ if (rc < 0)
+ return rc;
+ else if (rc != sizeof(u32) * 3)
+ return -EIO;
+
+ return 0;
+}
+EXPORT_SYMBOL(occ_i2c_putscom);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("I2C OCC SCOM transport");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_scom_i2c.h b/drivers/hwmon/occ/occ_scom_i2c.h
new file mode 100644
index 0000000..945739c
--- /dev/null
+++ b/drivers/hwmon/occ/occ_scom_i2c.h
@@ -0,0 +1,26 @@
+/*
+ * occ_scom_i2c.h - hwmon OCC driver
+ *
+ * This file contains function protoypes for peforming SCOM operations over I2C
+ * bus to access the OCC.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#ifndef __OCC_SCOM_I2C_H__
+#define __OCC_SCOM_I2C_H__
+
+int occ_i2c_getscom(void *bus, u32 address, u64 *data);
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
+
+#endif /* __OCC_SCOM_I2C_H__ */
--
1.9.1
^ permalink raw reply related
* [PATCH linux v2 2/6] hwmon: occ: Add sysfs interface
From: eajames.ibm @ 2017-01-11 18:10 UTC (permalink / raw)
To: linux
Cc: devicetree, jdelvare, corbet, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, mark.rutland, robh+dt, wsa, andrew, joel, benh,
Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm@gmail.com>
From: "Edward A. James" <eajames@us.ibm.com>
Add a generic mechanism to expose the sensors provided by the OCC in
sysfs.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Documentation/hwmon/occ | 62 ++++++++++
drivers/hwmon/occ/Makefile | 2 +-
drivers/hwmon/occ/occ_sysfs.c | 274 ++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ_sysfs.h | 44 +++++++
4 files changed, 381 insertions(+), 1 deletion(-)
create mode 100644 drivers/hwmon/occ/occ_sysfs.c
create mode 100644 drivers/hwmon/occ/occ_sysfs.h
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
index 79d1642..d0bdf06 100644
--- a/Documentation/hwmon/occ
+++ b/Documentation/hwmon/occ
@@ -25,6 +25,68 @@ Currently, all versions of the OCC support four types of sensor data: power,
temperature, frequency, and "caps," which indicate limits and thresholds used
internally on the OCC.
+sysfs Entries
+-------------
+
+The OCC driver uses the hwmon sysfs framework to provide data to userspace.
+
+The driver exports a number of sysfs files for each type of sensor. The
+sensor-specific files vary depending on the processor type, though many of the
+attributes are common for both the POWER8 and POWER9.
+
+The hwmon interface cannot define every type of sensor that may be used.
+Therefore, the frequency sensor on the OCC uses the "input" type sensor defined
+by the hwmon interface, rather than defining a new type of custom sensor.
+
+Below are detailed the names and meaning of each sensor file for both types of
+processors. All sensors are read-only unless otherwise specified. <x> indicates
+the hwmon index. sensor id indicates the unique internal OCC identifer. Please
+see the POWER OCC specification for details on all these sensor values.
+
+frequency:
+ all processors:
+ in<x>_input - frequency value
+ in<x>_label - sensor id
+temperature:
+ POWER8:
+ temp<x>_input - temperature value
+ temp<x>_label - sensor id
+ POWER9 (in addition to above):
+ temp<x>_type - FRU type
+
+power:
+ POWER8:
+ power<x>_input - power value
+ power<x>_label - sensor id
+ power<x>_average - accumulator
+ power<x>_average_interval - update tag (number of samples in
+ accumulator)
+ POWER9:
+ power<x>_input - power value
+ power<x>_label - sensor id
+ power<x>_average_min - accumulator[0]
+ power<x>_average_max - accumulator[1] (64 bits total)
+ power<x>_average_interval - update tag
+ power<x>_reset_history - (function_id | (apss_channel << 8)
+
+caps:
+ POWER8:
+ power<x>_cap - current powercap
+ power<x>_cap_max - max powercap
+ power<x>_cap_min - min powercap
+ power<x>_max - normal powercap
+ power<x>_alarm - user powercap, r/w
+ POWER9:
+ power<x>_cap_alarm - user powercap source
+
+The driver also provides two sysfs entries through hwmon to better
+control the driver and monitor the master OCC. Though there may be multiple
+OCCs present on the system, these two files are only present for the "master"
+OCC.
+ name - read the name of the driver
+ update_interval - read or write the minimum interval for polling the
+ OCC.
+
BMC - Host Communications
-------------------------
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
index 93cb52f..a6881f9 100644
--- a/drivers/hwmon/occ/Makefile
+++ b/drivers/hwmon/occ/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
diff --git a/drivers/hwmon/occ/occ_sysfs.c b/drivers/hwmon/occ/occ_sysfs.c
new file mode 100644
index 0000000..e846b0c
--- /dev/null
+++ b/drivers/hwmon/occ/occ_sysfs.c
@@ -0,0 +1,274 @@
+/*
+ * occ_sysfs.c - OCC sysfs interface
+ *
+ * This file contains the methods and data structures for implementing the OCC
+ * hwmon sysfs entries.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "occ.h"
+#include "occ_sysfs.h"
+
+#define RESP_RETURN_CMD_INVAL 0x13
+
+static int occ_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ int rc = 0;
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+ struct occ *occ = driver->occ;
+
+ switch (type) {
+ case hwmon_in:
+ rc = occ_get_sensor_value(occ, FREQ, channel, attr, val);
+ break;
+ case hwmon_temp:
+ rc = occ_get_sensor_value(occ, TEMP, channel, attr, val);
+ break;
+ case hwmon_power:
+ rc = occ_get_sensor_value(occ, POWER, channel, attr, val);
+ break;
+ default:
+ rc = -EOPNOTSUPP;
+ }
+
+ return rc;
+}
+
+static int occ_hwmon_read_string(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, char **str)
+{
+ int rc;
+ unsigned long val = 0;
+
+ if (!((type == hwmon_in && attr == hwmon_in_label) ||
+ (type == hwmon_temp && attr == hwmon_temp_label) ||
+ (type == hwmon_power && attr == hwmon_power_label)))
+ return -EOPNOTSUPP;
+
+ rc = occ_hwmon_read(dev, type, attr, channel, &val);
+ if (rc < 0)
+ return rc;
+
+ rc = snprintf(*str, PAGE_SIZE - 1, "%ld", val);
+ if (rc > 0)
+ rc = 0;
+
+ return rc;
+}
+
+static int occ_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ int rc = 0;
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+ if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
+ occ_set_update_interval(driver->occ, val);
+ return 0;
+ } else if (type == hwmon_power && attr == hwmon_power_alarm) {
+ rc = occ_set_user_powercap(driver->occ, val);
+ if (rc) {
+ if (rc == RESP_RETURN_CMD_INVAL) {
+ dev_err(dev,
+ "set invalid powercap value: %ld\n",
+ val);
+ return -EINVAL;
+ }
+
+ dev_err(dev, "set user powercap failed: 0x:%x\n", rc);
+ return rc;
+ }
+
+ driver->user_powercap = val;
+
+ return rc;
+ }
+
+ return -EOPNOTSUPP;
+}
+
+static umode_t occ_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ const struct occ_sysfs *driver = data;
+
+ switch (type) {
+ case hwmon_chip:
+ if (attr == hwmon_chip_update_interval)
+ return S_IRUGO | S_IWUSR;
+ break;
+ case hwmon_in:
+ if (BIT(attr) & driver->sensor_hwmon_configs[0])
+ return S_IRUGO;
+ break;
+ case hwmon_temp:
+ if (BIT(attr) & driver->sensor_hwmon_configs[1])
+ return S_IRUGO;
+ break;
+ case hwmon_power:
+ /* user power limit */
+ if (attr == hwmon_power_alarm)
+ return S_IRUGO | S_IWUSR;
+ else if ((BIT(attr) & driver->sensor_hwmon_configs[2]) ||
+ (BIT(attr) & driver->sensor_hwmon_configs[3]))
+ return S_IRUGO;
+ break;
+ default:
+ return 0;
+ }
+
+ return 0;
+}
+
+static const struct hwmon_ops occ_hwmon_ops = {
+ .is_visible = occ_is_visible,
+ .read = occ_hwmon_read,
+ .read_string = occ_hwmon_read_string,
+ .write = occ_hwmon_write,
+};
+
+static const u32 occ_chip_config[] = {
+ HWMON_C_UPDATE_INTERVAL,
+ 0
+};
+
+static const struct hwmon_channel_info occ_chip = {
+ .type = hwmon_chip,
+ .config = occ_chip_config
+};
+
+static const enum hwmon_sensor_types occ_sensor_types[MAX_OCC_SENSOR_TYPE] = {
+ hwmon_in,
+ hwmon_temp,
+ hwmon_power,
+ hwmon_power
+};
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+ const u32 *sensor_hwmon_configs,
+ const char *name)
+{
+ bool master_occ = false;
+ int rc, i, j, sensor_num, index = 0, id;
+ char *brk;
+ struct occ_blocks *resp = NULL;
+ u32 *sensor_config;
+ struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
+ GFP_KERNEL);
+ if (!hwmon)
+ return ERR_PTR(-ENOMEM);
+
+ /* need space for null-termination and occ chip */
+ hwmon->occ_sensors =
+ devm_kzalloc(dev, sizeof(struct hwmon_channel_info *) *
+ (MAX_OCC_SENSOR_TYPE + 2), GFP_KERNEL);
+ if (!hwmon->occ_sensors)
+ return ERR_PTR(-ENOMEM);
+
+ hwmon->occ = occ;
+ hwmon->sensor_hwmon_configs = (u32 *)sensor_hwmon_configs;
+ hwmon->occ_info.ops = &occ_hwmon_ops;
+ hwmon->occ_info.info =
+ (const struct hwmon_channel_info **)hwmon->occ_sensors;
+
+ dev_set_drvdata(dev, hwmon);
+
+ occ_get_response_blocks(occ, &resp);
+
+ for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
+ resp->sensor_block_id[i] = -1;
+
+ /* read sensor data from occ */
+ rc = occ_update_device(occ);
+ if (rc) {
+ dev_err(dev, "cannot get occ sensor data: %d\n", rc);
+ return ERR_PTR(rc);
+ }
+ if (!resp->blocks)
+ return ERR_PTR(-ENOMEM);
+
+ master_occ = resp->sensor_block_id[CAPS] >= 0;
+
+ for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
+ id = resp->sensor_block_id[i];
+ if (id < 0)
+ continue;
+
+ sensor_num = resp->blocks[id].header.sensor_num;
+ /* need null-termination */
+ sensor_config = devm_kzalloc(dev,
+ sizeof(u32) * (sensor_num + 1),
+ GFP_KERNEL);
+ if (!sensor_config)
+ return ERR_PTR(-ENOMEM);
+
+ for (j = 0; j < sensor_num; j++)
+ sensor_config[j] = sensor_hwmon_configs[i];
+
+ hwmon->occ_sensors[index] =
+ devm_kzalloc(dev, sizeof(struct hwmon_channel_info),
+ GFP_KERNEL);
+ if (!hwmon->occ_sensors[index])
+ return ERR_PTR(-ENOMEM);
+
+ hwmon->occ_sensors[index]->type = occ_sensor_types[i];
+ hwmon->occ_sensors[index]->config = sensor_config;
+ index++;
+ }
+
+ /* only need one of these for any number of occs */
+ if (master_occ)
+ hwmon->occ_sensors[index] =
+ (struct hwmon_channel_info *)&occ_chip;
+
+ /* search for bad chars */
+ strncpy(hwmon->hwmon_name, name, OCC_HWMON_NAME_LENGTH);
+ brk = strpbrk(hwmon->hwmon_name, "-* \t\n");
+ while (brk) {
+ *brk = '_';
+ brk = strpbrk(brk, "-* \t\n");
+ }
+
+ hwmon->dev = devm_hwmon_device_register_with_info(dev,
+ hwmon->hwmon_name,
+ hwmon,
+ &hwmon->occ_info,
+ NULL);
+ if (IS_ERR(hwmon->dev)) {
+ dev_err(dev, "cannot register hwmon device %s: %ld\n",
+ hwmon->hwmon_name, PTR_ERR(hwmon->dev));
+ return ERR_PTR(PTR_ERR(hwmon->dev));
+ }
+
+ return hwmon;
+}
+EXPORT_SYMBOL(occ_sysfs_start);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("OCC sysfs driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_sysfs.h b/drivers/hwmon/occ/occ_sysfs.h
new file mode 100644
index 0000000..7de92e7
--- /dev/null
+++ b/drivers/hwmon/occ/occ_sysfs.h
@@ -0,0 +1,44 @@
+/*
+ * occ_sysfs.h - OCC sysfs interface
+ *
+ * This file contains the data structures and function prototypes for the OCC
+ * hwmon sysfs entries.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#ifndef __OCC_SYSFS_H__
+#define __OCC_SYSFS_H__
+
+#include <linux/hwmon.h>
+
+struct occ;
+struct device;
+
+#define OCC_HWMON_NAME_LENGTH 32
+
+struct occ_sysfs {
+ struct device *dev;
+ struct occ *occ;
+
+ char hwmon_name[OCC_HWMON_NAME_LENGTH + 1];
+ u32 *sensor_hwmon_configs;
+ struct hwmon_channel_info **occ_sensors;
+ struct hwmon_chip_info occ_info;
+ u16 user_powercap;
+};
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+ const u32 *sensor_hwmon_configs,
+ const char *name);
+#endif /* __OCC_SYSFS_H__ */
--
1.9.1
^ permalink raw reply related
* [PATCH linux v2 1/6] hwmon: Add core On-Chip Controller support for POWER CPUs
From: eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w @ 2017-01-11 18:10 UTC (permalink / raw)
To: linux-0h96xk9xTtrk1uMJSBkQmQ
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, jdelvare-IBi9RG/b67k,
corbet-T1hC0tSOHrs, linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, wsa-z923LK4zBo2bacvFa/9K2g,
andrew-zrmu5oMJ5Fs, joel-U3u1mxZcP9KHXe+LvDLADg,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r, Edward A. James
In-Reply-To: <1484158237-10014-1-git-send-email-eajames.ibm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: "Edward A. James" <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Add core support for polling the OCC for it's sensor data and parsing that
data into sensor-specific information.
Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>
---
Documentation/hwmon/occ | 40 ++++
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 15 ++
drivers/hwmon/occ/Makefile | 1 +
drivers/hwmon/occ/occ.c | 533 +++++++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/occ/occ.h | 83 +++++++
drivers/hwmon/occ/scom.h | 47 ++++
8 files changed, 722 insertions(+)
create mode 100644 Documentation/hwmon/occ
create mode 100644 drivers/hwmon/occ/Kconfig
create mode 100644 drivers/hwmon/occ/Makefile
create mode 100644 drivers/hwmon/occ/occ.c
create mode 100644 drivers/hwmon/occ/occ.h
create mode 100644 drivers/hwmon/occ/scom.h
diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
new file mode 100644
index 0000000..79d1642
--- /dev/null
+++ b/Documentation/hwmon/occ
@@ -0,0 +1,40 @@
+Kernel driver occ
+=================
+
+Supported chips:
+ * ASPEED AST2400
+ * ASPEED AST2500
+
+Please note that the chip must be connected to a POWER8 or POWER9 processor
+(see the BMC - Host Communications section).
+
+Author: Eddie James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+
+Description
+-----------
+
+This driver implements support for the OCC (On-Chip Controller) on the IBM
+POWER8 and POWER9 processors, from a BMC (Baseboard Management Controller). The
+OCC is an embedded processor that provides real time power and thermal
+monitoring.
+
+This driver provides an interface on a BMC to poll OCC sensor data, set user
+power caps, and perform some basic OCC error handling.
+
+Currently, all versions of the OCC support four types of sensor data: power,
+temperature, frequency, and "caps," which indicate limits and thresholds used
+internally on the OCC.
+
+BMC - Host Communications
+-------------------------
+
+For the POWER8 application, the BMC can communicate with the P8 over I2C bus.
+However, to access the OCC register space, any data transfer must use a SCOM
+operation. SCOM is a procedure to initiate a data transfer, typically of 8
+bytes. SCOMs consist of writing a 32-bit command register and then
+reading/writing two 32-bit data registers. This driver implements these
+SCOM operations over I2C bus in order to communicate with the OCC.
+
+For the POWER9 application, the BMC can communicate with the P9 over FSI bus
+and SBE engine. Once again, SCOM operations are required. This driver will
+implement SCOM ops over FSI/SBE. This will require the FSI driver.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 190d270..e80ca81 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1240,6 +1240,8 @@ config SENSORS_NSA320
This driver can also be built as a module. If so, the module
will be called nsa320-hwmon.
+source drivers/hwmon/occ/Kconfig
+
config SENSORS_PCF8591
tristate "Philips PCF8591 ADC/DAC"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d2cb7e8..c7ec5d4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -169,6 +169,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ/
obj-$(CONFIG_PMBUS) += pmbus/
ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
new file mode 100644
index 0000000..cdb64a7
--- /dev/null
+++ b/drivers/hwmon/occ/Kconfig
@@ -0,0 +1,15 @@
+#
+# On Chip Controller configuration
+#
+
+menuconfig SENSORS_PPC_OCC
+ bool "PPC On-Chip Controller"
+ help
+ If you say yes here you get support to monitor Power CPU
+ sensors via the On-Chip Controller (OCC).
+
+ Generally this is used by management controllers such as a BMC
+ on an OpenPower system.
+
+ This driver can also be built as a module. If so, the module
+ will be called occ.
diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
new file mode 100644
index 0000000..93cb52f
--- /dev/null
+++ b/drivers/hwmon/occ/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
diff --git a/drivers/hwmon/occ/occ.c b/drivers/hwmon/occ/occ.c
new file mode 100644
index 0000000..31e6164
--- /dev/null
+++ b/drivers/hwmon/occ/occ.c
@@ -0,0 +1,533 @@
+/*
+ * occ.c - OCC hwmon driver
+ *
+ * This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <asm/unaligned.h>
+
+#include "occ.h"
+
+#define OCC_DATA_MAX 4096
+#define OCC_BMC_TIMEOUT_MS 20000
+
+/* To generate attn to OCC */
+#define ATTN_DATA 0x0006B035
+
+/* For BMC to read/write SRAM */
+#define OCB_ADDRESS 0x0006B070
+#define OCB_DATA 0x0006B075
+#define OCB_STATUS_CONTROL_AND 0x0006B072
+#define OCB_STATUS_CONTROL_OR 0x0006B073
+
+/* To init OCB */
+#define OCB_AND_INIT0 0xFBFFFFFF
+#define OCB_AND_INIT1 0xFFFFFFFF
+#define OCB_OR_INIT0 0x08000000
+#define OCB_OR_INIT1 0x00000000
+
+/* To generate attention on OCC */
+#define ATTN0 0x01010000
+#define ATTN1 0x00000000
+
+/* OCC return status */
+#define RESP_RETURN_CMD_IN_PRG 0xFF
+#define RESP_RETURN_SUCCESS 0
+#define RESP_RETURN_CMD_INVAL 0x11
+#define RESP_RETURN_CMD_LEN 0x12
+#define RESP_RETURN_DATA_INVAL 0x13
+#define RESP_RETURN_CHKSUM 0x14
+#define RESP_RETURN_OCC_ERR 0x15
+#define RESP_RETURN_STATE 0x16
+
+/* time interval to retry on "command in progress" return status */
+#define CMD_IN_PRG_INT_MS 100
+#define CMD_IN_PRG_RETRIES (OCC_BMC_TIMEOUT_MS / CMD_IN_PRG_INT_MS)
+
+/* OCC command definitions */
+#define OCC_POLL 0
+#define OCC_SET_USER_POWR_CAP 0x22
+
+/* OCC poll command data */
+#define OCC_POLL_STAT_SENSOR 0x10
+
+/* OCC response data offsets */
+#define RESP_RETURN_STATUS 2
+#define RESP_DATA_LENGTH 3
+#define RESP_HEADER_OFFSET 5
+#define SENSOR_STR_OFFSET 37
+#define SENSOR_BLOCK_NUM_OFFSET 43
+#define SENSOR_BLOCK_OFFSET 45
+
+/* occ_poll_header
+ * structure to match the raw occ poll response data
+ */
+struct occ_poll_header {
+ u8 status;
+ u8 ext_status;
+ u8 occs_present;
+ u8 config;
+ u8 occ_state;
+ u8 mode;
+ u8 ips_status;
+ u8 error_log_id;
+ u32 error_log_addr_start;
+ u16 error_log_length;
+ u8 reserved2;
+ u8 reserved3;
+ u8 occ_code_level[16];
+ u8 sensor_eye_catcher[6];
+ u8 sensor_block_num;
+ u8 sensor_data_version;
+} __attribute__((packed, aligned(4)));
+
+struct occ_response {
+ struct occ_poll_header header;
+ struct occ_blocks data;
+};
+
+struct occ {
+ struct device *dev;
+ void *bus;
+ struct occ_bus_ops bus_ops;
+ struct occ_ops ops;
+ struct occ_config config;
+ unsigned long update_interval;
+ unsigned long last_updated;
+ struct mutex update_lock;
+ struct occ_response response;
+ bool valid;
+};
+
+static void deinit_occ_resp_buf(struct occ_response *resp)
+{
+ int i;
+
+ if (!resp)
+ return;
+
+ if (!resp->data.blocks)
+ return;
+
+ for (i = 0; i < resp->header.sensor_block_num; ++i)
+ kfree(resp->data.blocks[i].sensors);
+
+ kfree(resp->data.blocks);
+
+ memset(resp, 0, sizeof(struct occ_response));
+
+ for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
+ resp->data.sensor_block_id[i] = -1;
+}
+
+static void *occ_get_sensor_by_type(struct occ_response *resp,
+ enum sensor_type t)
+{
+ if (!resp->data.blocks)
+ return NULL;
+
+ if (resp->data.sensor_block_id[t] == -1)
+ return NULL;
+
+ return resp->data.blocks[resp->data.sensor_block_id[t]].sensors;
+}
+
+static int occ_check_sensor(struct occ *driver, u8 sensor_length,
+ u8 sensor_num, enum sensor_type t, int block)
+{
+ void *sensor;
+ int type_block_id;
+ struct occ_response *resp = &driver->response;
+
+ sensor = occ_get_sensor_by_type(resp, t);
+
+ /* empty sensor block, release older sensor data */
+ if (sensor_num == 0 || sensor_length == 0) {
+ kfree(sensor);
+ dev_err(driver->dev, "no sensor blocks available\n");
+ return -ENODATA;
+ }
+
+ type_block_id = resp->data.sensor_block_id[t];
+ if (!sensor || sensor_num !=
+ resp->data.blocks[type_block_id].header.sensor_num) {
+ kfree(sensor);
+ resp->data.blocks[block].sensors =
+ driver->ops.alloc_sensor(t, sensor_num);
+ if (!resp->data.blocks[block].sensors)
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int parse_occ_response(struct occ *driver, u8 *data,
+ struct occ_response *resp)
+{
+ int b;
+ int s;
+ int rc;
+ int offset = SENSOR_BLOCK_OFFSET;
+ int sensor_type;
+ u8 sensor_block_num;
+ char sensor_type_string[5] = { 0 };
+ struct sensor_data_block_header *block;
+ struct device *dev = driver->dev;
+
+ /* check if the data is valid */
+ if (strncmp(&data[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {
+ dev_err(dev, "no SENSOR string in response\n");
+ rc = -ENODATA;
+ goto err;
+ }
+
+ sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET];
+ if (sensor_block_num == 0) {
+ dev_err(dev, "no sensor blocks available\n");
+ rc = -ENODATA;
+ goto err;
+ }
+
+ /* if number of sensor block has changed, re-malloc */
+ if (sensor_block_num != resp->header.sensor_block_num) {
+ deinit_occ_resp_buf(resp);
+ resp->data.blocks = kcalloc(sensor_block_num,
+ sizeof(struct sensor_data_block),
+ GFP_KERNEL);
+ if (!resp->data.blocks)
+ return -ENOMEM;
+ }
+
+ memcpy(&resp->header, &data[RESP_HEADER_OFFSET],
+ sizeof(struct occ_poll_header));
+ resp->header.error_log_addr_start =
+ be32_to_cpu(resp->header.error_log_addr_start);
+ resp->header.error_log_length =
+ be16_to_cpu(resp->header.error_log_length);
+
+ dev_dbg(dev, "Reading %d sensor blocks\n",
+ resp->header.sensor_block_num);
+ for (b = 0; b < sensor_block_num; b++) {
+ block = (struct sensor_data_block_header *)&data[offset];
+ /* copy to a null terminated string */
+ strncpy(sensor_type_string, block->sensor_type, 4);
+ offset += 8;
+
+ dev_dbg(dev, "sensor block[%d]: type: %s, sensor_num: %d\n", b,
+ sensor_type_string, block->sensor_num);
+
+ if (strncmp(block->sensor_type, "FREQ", 4) == 0)
+ sensor_type = FREQ;
+ else if (strncmp(block->sensor_type, "TEMP", 4) == 0)
+ sensor_type = TEMP;
+ else if (strncmp(block->sensor_type, "POWR", 4) == 0)
+ sensor_type = POWER;
+ else if (strncmp(block->sensor_type, "CAPS", 4) == 0)
+ sensor_type = CAPS;
+ else {
+ dev_err(dev, "sensor type not supported %s\n",
+ sensor_type_string);
+ continue;
+ }
+
+ rc = occ_check_sensor(driver, block->sensor_length,
+ block->sensor_num, sensor_type, b);
+ if (rc == -ENOMEM)
+ goto err;
+ else if (rc)
+ continue;
+
+ resp->data.sensor_block_id[sensor_type] = b;
+ for (s = 0; s < block->sensor_num; s++) {
+ driver->ops.parse_sensor(data,
+ resp->data.blocks[b].sensors,
+ sensor_type, offset, s);
+ offset += block->sensor_length;
+ }
+
+ /* copy block data over to response pointer */
+ resp->data.blocks[b].header = *block;
+ }
+
+ return 0;
+err:
+ deinit_occ_resp_buf(resp);
+ return rc;
+}
+
+static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,
+ const u8 *data, u8 *resp)
+{
+ u32 cmd1, cmd2;
+ u16 checksum = 0;
+ u16 length_le = cpu_to_le16(length);
+ bool retry = 0;
+ int i, rc, tries = 0;
+
+ cmd1 = (seq << 24) | (type << 16) | length_le;
+ memcpy(&cmd2, data, length);
+ cmd2 <<= ((4 - length) * 8);
+
+ /* checksum: sum of every bytes of cmd1, cmd2 */
+ for (i = 0; i < 4; i++) {
+ checksum += (cmd1 >> (i * 8)) & 0xFF;
+ checksum += (cmd2 >> (i * 8)) & 0xFF;
+ }
+
+ cmd2 |= checksum << ((2 - length) * 8);
+
+ /* Init OCB */
+ rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_OR,
+ OCB_OR_INIT0, OCB_OR_INIT1);
+ if (rc)
+ goto err;
+
+ rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_AND,
+ OCB_AND_INIT0, OCB_AND_INIT1);
+ if (rc)
+ goto err;
+
+ /* Send command, 2nd half of the 64-bit addr is unused (write 0) */
+ rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
+ driver->config.command_addr, 0);
+ if (rc)
+ goto err;
+
+ rc = driver->bus_ops.putscom(driver->bus, OCB_DATA, cmd1, cmd2);
+ if (rc)
+ goto err;
+
+ /* Trigger attention */
+ rc = driver->bus_ops.putscom(driver->bus, ATTN_DATA, ATTN0, ATTN1);
+ if (rc)
+ goto err;
+
+ /* Get response data */
+ rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
+ driver->config.response_addr, 0);
+ if (rc)
+ goto err;
+
+ do {
+ if (retry) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS));
+ }
+
+ rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
+ (u64 *)resp);
+ if (rc)
+ goto err;
+
+ /* retry if we get "command in progress" return status */
+ retry = (resp[RESP_RETURN_STATUS] == RESP_RETURN_CMD_IN_PRG) &&
+ (tries++ < CMD_IN_PRG_RETRIES);
+ } while (retry);
+
+ switch (resp[RESP_RETURN_STATUS]) {
+ case RESP_RETURN_CMD_IN_PRG:
+ rc = -EALREADY;
+ break;
+ case RESP_RETURN_SUCCESS:
+ rc = 0;
+ break;
+ case RESP_RETURN_CMD_INVAL:
+ case RESP_RETURN_CMD_LEN:
+ case RESP_RETURN_DATA_INVAL:
+ case RESP_RETURN_CHKSUM:
+ rc = -EINVAL;
+ break;
+ case RESP_RETURN_OCC_ERR:
+ rc = -EREMOTE;
+ break;
+ default:
+ rc = -EFAULT;
+ }
+
+ return rc;
+
+err:
+ dev_err(driver->dev, "scom op failed rc:%d\n", rc);
+ return rc;
+}
+
+static int occ_get_all(struct occ *driver)
+{
+ int i = 0, rc;
+ u8 *occ_data;
+ u16 num_bytes;
+ const u8 poll_cmd_data = OCC_POLL_STAT_SENSOR;
+ struct device *dev = driver->dev;
+ struct occ_response *resp = &driver->response;
+
+ occ_data = devm_kzalloc(dev, OCC_DATA_MAX, GFP_KERNEL);
+ if (!occ_data)
+ return -ENOMEM;
+
+ rc = occ_send_cmd(driver, 0, OCC_POLL, 1, &poll_cmd_data, occ_data);
+ if (rc) {
+ dev_err(dev, "OCC poll failed: %d\n", rc);
+ goto out;
+ }
+
+ num_bytes = get_unaligned((u16 *)&occ_data[RESP_DATA_LENGTH]);
+ num_bytes = be16_to_cpu(num_bytes);
+ dev_dbg(dev, "OCC data length: %d\n", num_bytes);
+
+ if (num_bytes > OCC_DATA_MAX) {
+ dev_err(dev, "OCC data length must be < 4KB\n");
+ rc = -EINVAL;
+ goto out;
+ }
+
+ if (num_bytes <= 0) {
+ dev_err(dev, "OCC data length is zero\n");
+ rc = -EINVAL;
+ goto out;
+ }
+
+ /* read remaining data */
+ for (i = 8; i < num_bytes + 8; i += 8) {
+ rc = driver->bus_ops.getscom(driver->bus, OCB_DATA,
+ (u64 *)&occ_data[i]);
+ if (rc) {
+ dev_err(dev, "scom op failed rc:%d\n", rc);
+ goto out;
+ }
+ }
+
+ /* don't need more sanity checks; buffer is alloc'd for max response
+ * size so we just check for valid data in parse_occ_response
+ */
+ rc = parse_occ_response(driver, occ_data, resp);
+
+out:
+ devm_kfree(dev, occ_data);
+ return rc;
+}
+
+int occ_update_device(struct occ *driver)
+{
+ int rc = 0;
+
+ mutex_lock(&driver->update_lock);
+
+ if (time_after(jiffies, driver->last_updated + driver->update_interval)
+ || !driver->valid) {
+ driver->valid = 1;
+
+ rc = occ_get_all(driver);
+ if (rc)
+ driver->valid = 0;
+
+ driver->last_updated = jiffies;
+ }
+
+ mutex_unlock(&driver->update_lock);
+
+ return rc;
+}
+EXPORT_SYMBOL(occ_update_device);
+
+void *occ_get_sensor(struct occ *driver, int sensor_type)
+{
+ int rc;
+
+ /* occ_update_device locks the update lock */
+ rc = occ_update_device(driver);
+ if (rc) {
+ dev_err(driver->dev, "cannot get occ sensor data: %d\n",
+ rc);
+ return NULL;
+ }
+
+ return occ_get_sensor_by_type(&driver->response, sensor_type);
+}
+EXPORT_SYMBOL(occ_get_sensor);
+
+int occ_get_sensor_value(struct occ *occ, int sensor_type, int sensor_num,
+ u32 hwmon, long *val)
+{
+ return occ->ops.get_sensor(occ, sensor_type, sensor_num, hwmon, val);
+}
+EXPORT_SYMBOL(occ_get_sensor_value);
+
+void occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks)
+{
+ *blocks = &occ->response.data;
+}
+EXPORT_SYMBOL(occ_get_response_blocks);
+
+void occ_set_update_interval(struct occ *occ, unsigned long interval)
+{
+ occ->update_interval = msecs_to_jiffies(interval);
+}
+EXPORT_SYMBOL(occ_set_update_interval);
+
+int occ_set_user_powercap(struct occ *occ, u16 cap)
+{
+ u8 resp[8];
+
+ cap = cpu_to_be16(cap);
+
+ return occ_send_cmd(occ, 0, OCC_SET_USER_POWR_CAP, 2, (const u8 *)&cap,
+ resp);
+}
+EXPORT_SYMBOL(occ_set_user_powercap);
+
+struct occ *occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
+ const struct occ_config *config)
+{
+ struct occ *driver = devm_kzalloc(dev, sizeof(struct occ), GFP_KERNEL);
+
+ if (!driver)
+ return ERR_PTR(-ENOMEM);
+
+ driver->dev = dev;
+ driver->bus = bus;
+ driver->bus_ops = *bus_ops;
+ driver->ops = *ops;
+ driver->config = *config;
+
+ driver->update_interval = HZ;
+ mutex_init(&driver->update_lock);
+
+ return driver;
+}
+EXPORT_SYMBOL(occ_start);
+
+int occ_stop(struct occ *occ)
+{
+ devm_kfree(occ->dev, occ);
+
+ return 0;
+}
+EXPORT_SYMBOL(occ_stop);
+
+MODULE_AUTHOR("Eddie James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("OCC hwmon core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ.h b/drivers/hwmon/occ/occ.h
new file mode 100644
index 0000000..be869bc
--- /dev/null
+++ b/drivers/hwmon/occ/occ.h
@@ -0,0 +1,83 @@
+/*
+ * occ.h - hwmon OCC driver
+ *
+ * This file contains data structures and function prototypes for common access
+ * between different bus protocols and host systems.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#ifndef __OCC_H__
+#define __OCC_H__
+
+#include "scom.h"
+
+struct device;
+struct occ;
+
+/* sensor_data_block_header
+ * structure to match the raw occ sensor block header
+ */
+struct sensor_data_block_header {
+ u8 sensor_type[4];
+ u8 reserved0;
+ u8 sensor_format;
+ u8 sensor_length;
+ u8 sensor_num;
+} __attribute__((packed, aligned(4)));
+
+struct sensor_data_block {
+ struct sensor_data_block_header header;
+ void *sensors;
+};
+
+enum sensor_type {
+ FREQ = 0,
+ TEMP,
+ POWER,
+ CAPS,
+ MAX_OCC_SENSOR_TYPE
+};
+
+struct occ_ops {
+ void (*parse_sensor)(u8 *data, void *sensor, int sensor_type, int off,
+ int snum);
+ void *(*alloc_sensor)(int sensor_type, int num_sensors);
+ int (*get_sensor)(struct occ *driver, int sensor_type, int sensor_num,
+ u32 hwmon, long *val);
+};
+
+struct occ_config {
+ u32 command_addr;
+ u32 response_addr;
+};
+
+struct occ_blocks {
+ int sensor_block_id[MAX_OCC_SENSOR_TYPE];
+ struct sensor_data_block *blocks;
+};
+
+struct occ *occ_start(struct device *dev, void *bus,
+ struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
+ const struct occ_config *config);
+int occ_stop(struct occ *occ);
+
+void *occ_get_sensor(struct occ *occ, int sensor_type);
+int occ_get_sensor_value(struct occ *occ, int sensor_type, int sensor_num,
+ u32 hwmon, long *val);
+void occ_get_response_blocks(struct occ *occ, struct occ_blocks **blocks);
+int occ_update_device(struct occ *driver);
+void occ_set_update_interval(struct occ *occ, unsigned long interval);
+int occ_set_user_powercap(struct occ *occ, u16 cap);
+
+#endif /* __OCC_H__ */
diff --git a/drivers/hwmon/occ/scom.h b/drivers/hwmon/occ/scom.h
new file mode 100644
index 0000000..c1da645
--- /dev/null
+++ b/drivers/hwmon/occ/scom.h
@@ -0,0 +1,47 @@
+/*
+ * scom.h - hwmon OCC driver
+ *
+ * This file contains data structures for scom operations to the OCC
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#ifndef __SCOM_H__
+#define __SCOM_H__
+
+/*
+ * occ_bus_ops - represent the low-level transfer methods to communicate with
+ * the OCC.
+ *
+ * getscom - OCC scom read
+ * @bus: handle to slave device
+ * @address: address
+ * @data: where to store data read from slave; buffer size must be at least
+ * eight bytes.
+ *
+ * Returns 0 on success or a negative errno on error
+ *
+ * putscom - OCC scom write
+ * @bus: handle to slave device
+ * @address: address
+ * @data0: first data byte to write
+ * @data1: second data byte to write
+ *
+ * Returns 0 on success or a negative errno on error
+ */
+struct occ_bus_ops {
+ int (*getscom)(void *bus, u32 address, u64 *data);
+ int (*putscom)(void *bus, u32 address, u32 data0, u32 data1);
+};
+
+#endif /* __SCOM_H__ */
--
1.9.1
--
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 related
* Re: [PATCH v2] i2c: piix4: Avoid race conditions with IMC
From: Andy Shevchenko @ 2017-01-11 16:39 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Jean Delvare, Wolfram Sang, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <20170111091144.6881-1-ricardo.ribalda@gmail.com>
On Wed, Jan 11, 2017 at 11:11 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> 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.
>
After addressing below comment
FWIW: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>:
> 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.
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>
> 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 f0563f7ce01b..81d06be0a72d 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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-11 15:42 UTC (permalink / raw)
To: M'boumba Cedric Madianga
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: <CAOAejn2cDzPgqvWZf7ASvtST+aDeaAebf=1aMWA9Zd8CDg4pmA@mail.gmail.com>
Hello Cedric,
On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
> >
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> >
> > You could get rid of this, when caching the value of CR1. Would save two
> > register reads here. This doesn't work for all registers, but it should
> > be possible to apply for most of them, maybe enough to get rid of the
> > clr_bits and set_bits function.
>
> I agree at many places I could save registers read by not using
> clr_bits and set_bits function when the registers in question has been
> already read.
> But it is not enough to get rid of the clr_bits and set_bits function.
> For example when calling stm32f4_i2c_terminate_xfer(), the CR1
> register is never read before so set_bits function is useful.
I didn't double check the manual, but I would expect that CR1 isn't
modified by hardware. So you can cache the result in the driver data
structure and do the necessary modifications with that one.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-11 15:39 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: <CAOAejn2eOy2sn1VkE979ne23Sj9L6+kaQDNpL1EUKb2m=6sGXw@mail.gmail.com>
On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> Hi Uwe,
>
> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Hello Cedric,
> >
> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> >> +/*
> >> + * In standard mode:
> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
> >> + *
> >> + * In fast mode:
> >> + * If Duty = 0; SCL high period = 1 * CCR * I2C parent clk period
^^
> >> + * SCL low period = 2 * CCR * I2C parent clk period
^^
> >> + * If Duty = 1; SCL high period = 9 * CCR * I2C parent clk period
^^
> >> + * SCL low period = 16 * CCR * I2C parent clk period
> > s/ \*/ */ several times
>
> Sorry but I don't see where is the issue as the style for multi-line
> comments seems ok.
> Could you please clarify that point if possible ? Thanks in advance
There are several places with double spaces before * marked above.
> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
> >> + * Duty = 1
> >> + *
> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> > s/mode/Mode/
>
> ok thanks
>
> >
> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
> >> + * constraints defined by i2c bus specification
> >
> > 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.
> >> + */
> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
> >> [...]
> >> +
> >> +/**
> >> + * stm32f4_i2c_hw_config() - Prepare I2C block
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + int ret = 0;
> >> +
> >> + /* Disable I2C */
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> >> +
> >> + ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + stm32f4_i2c_set_rise_time(i2c_dev);
> >> +
> >> + stm32f4_i2c_set_speed_mode(i2c_dev);
> >> +
> >> + stm32f4_i2c_set_filter(i2c_dev);
> >> +
> >> + /* Enable I2C */
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
> >
> > This function is called after a hw reset, so there should be no need to
> > use clr_bits and set_bits because the value read from hw should be
> > known.
>
> ok thanks
>
> >
> >> + return ret;
> >
> > return 0;
>
> ok thanks
>
> >
> >> +}
> >> +
> >> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + u32 status;
> >> + int ret;
> >> +
> >> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> >> + status,
> >> + !(status & STM32F4_I2C_SR2_BUSY),
> >> + 10, 1000);
> >> + if (ret) {
> >> + dev_dbg(i2c_dev->dev, "bus not free\n");
> >> + ret = -EBUSY;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
> >> + * @i2c_dev: Controller's private data
> >> + * @byte: Data to write in the register
> >> + */
> >> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
> >> +{
> >> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
> >> + * @i2c_dev: Controller's private data
> >> + *
> >> + * This function fills the data register with I2C transfer buffer
> >> + */
> >> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +
> >> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
> >> + msg->count--;
> >> +}
> >> +
> >> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + u32 rbuf;
> >> +
> >> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> >> + *msg->buf++ = rbuf & 0xff;
> >
> > This is unnecessary. buf has an 8 bit wide type so
> >
> > *msg->buf++ = rbuf;
> >
> > has the same effect. (ISTR this is something I already pointed out
> > earlier?)
>
> Yes you are right.
>
> >
> >> + msg->count--;
> >> +}
> >> +
> >> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> + stm32f4_i2c_disable_irq(i2c_dev);
> >> +
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + if (msg->stop)
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> + else
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> +
> >> + complete(&i2c_dev->complete);
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> + if (msg->count) {
> >> + stm32f4_i2c_write_msg(i2c_dev);
> >> + if (!msg->count) {
> >> + /* Disable buffer interrupts for RXNE/TXE events */
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> >> + }
> >> + } else {
> >> + stm32f4_i2c_terminate_xfer(i2c_dev);
> >
> > Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
> > yes, is it then right to set STM32F4_I2C_CR1_STOP or
> > STM32F4_I2C_CR1_START?
>
> If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
> In that case, we return -EAGAIN and i2c-core will retry by calling
> stm32f4_i2c_xfer()
>
> >
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> + switch (msg->count) {
> >> + case 1:
> >> + stm32f4_i2c_disable_irq(i2c_dev);
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> + complete(&i2c_dev->complete);
> >> + break;
> >> + /*
> >> + * For 2 or 3-byte reception, we do not have to read the data register
> >> + * when RXNE occurs as we have to wait for byte transferred finished
> >
> > it's hard to understand because if you don't know the hardware the
> > meaning of RXNE is unknown.
>
> Ok I will replace RXNE by RX not empty in that comment
>
> >
> >> + * event before reading data. So, here we just disable buffer
> >> + * interrupt in order to avoid another system preemption due to RXNE
> >> + * event
> >> + */
> >> + case 2:
> >> + case 3:
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> >> + break;
> >> + /* For N byte reception with N > 3 we directly read data register */
> >> + default:
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
> >> + * in case of read
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >
> > btf is a hw-related name. Maybe better use _done which is easier to
> > understand?
>
> OK
>
> >
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg;
> >> + u32 mask;
> >> + int i;
> >> +
> >> + switch (msg->count) {
> >> + case 2:
> >> + /*
> >> + * In order to correctly send the Stop or Repeated Start
> >> + * condition on the I2C bus, the STOP/START bit has to be set
> >> + * before reading the last two bytes.
> >> + * After that, we could read the last two bytes, disable
> >> + * remaining interrupts and notify the end of xfer to the
> >> + * client
> >
> > 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).
> > How does this work, when I only want to read a single
> > byte? Same problem for ACK below.
>
> For a single reception, we enable NACK and STOP or Repeatead START
> bits during address match.
> The NACK and STOP/START pulses are sent as soon as the data is
> received in the shift register.
> Please note that in that case, we don't have to wait BTF event to read the data.
> Data is read as soon as RXNE event occurs.
>
> >
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + if (msg->stop)
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> + else
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> +
> >> + for (i = 2; i > 0; i--)
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> +
> >> + reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> >> + stm32f4_i2c_clr_bits(reg, mask);
> >> +
> >> + complete(&i2c_dev->complete);
> >> + break;
> >> + case 3:
> >> + /*
> >> + * In order to correctly send the ACK on the I2C bus for the
> >> + * last two bytes, we have to set ACK bit before reading the
> >> + * third last data byte
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> + break;
> >> + default:
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
> >> + * master receiver
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg;
> >> +
> >> + switch (msg->count) {
> >> + case 0:
> >> + stm32f4_i2c_terminate_xfer(i2c_dev);
> >> + /* Clear ADDR flag */
> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> + break;
> >> + case 1:
> >> + /*
> >> + * Single byte reception:
> >
> > This also happens for the last byte of a 5 byte transfer, right?
>
> For a 5 byte transfer the behavior is different:
> We have to read data from DR (data register) as soon as the RXNE (RX
> not empty) event occurs for data1, data2 and data3 (until N-2 data for
> a more generic case)
> The ACK is automatically sent as soon as the data is received in the
> shift register as the I2C controller was configured to do that during
> adress match phase.
>
> For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
> in order to set NACK before reading DR.
> This event occurs when a new data has been received in shift register
> (in our case data4 or N-1 data) but the prevoius data in DR (in our
> case data3 or N-2 data) has not been read yet.
> In that way, the NACK pulse will be correctly generated after the last
> received data byte.
>
> For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
> and data5 or N data in shift register), set STOP or repeated Start in
> order to correctly sent the right pulse after the last received data
> byte and run 2 consecutives read of DR.
So "Single byte reception" above is wrong, as this case is also used for
longer transfers and should be updated accordingly.
> >> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> + if (msg->stop)
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> + else
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> + break;
> >> + case 2:
> >> + /*
> >> + * 2-byte reception:
> >> + * Enable NACK and set POS
> >
> > What is POS?
> POS is used to define the position of the (N)ACK pulse
> 0: ACK is generated when the current is being received in the shift register
> 1: ACK is generated when the next byte which will be received in the
> shift register (used for 2-byte reception)
Can you please put this into the comment. "POS" isn't much helpful
there.
>
> >
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> >
> > You could get rid of this, when caching the value of CR1. Would save two
> > register reads here. This doesn't work for all registers, but it should
> > be possible to apply for most of them, maybe enough to get rid of the
> > clr_bits and set_bits function.
> >
> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> + break;
> >> +
> >> + default:
> >> + /* N-byte reception: Enable ACK */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
> >
> > Do you need to set ACK for each byte transferred?
> I need to do that in order to be SMBus compatible and the ACK/NACK
> seems to be used by default in Documentation/i2c/i2c-protocol file.
Yeah, protocol wise you need to ack each byte. I just wondered if you
need to set the hardware bit for each byte or if it is retained in
hardware until unset by a register write.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-11 14:20 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: <CAOAejn2eOy2sn1VkE979ne23Sj9L6+kaQDNpL1EUKb2m=6sGXw@mail.gmail.com>
>
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>
> You could get rid of this, when caching the value of CR1. Would save two
> register reads here. This doesn't work for all registers, but it should
> be possible to apply for most of them, maybe enough to get rid of the
> clr_bits and set_bits function.
I agree at many places I could save registers read by not using
clr_bits and set_bits function when the registers in question has been
already read.
But it is not enough to get rid of the clr_bits and set_bits function.
For example when calling stm32f4_i2c_terminate_xfer(), the CR1
register is never read before so set_bits function is useful.
Another example, when stm32f4_i2c_handle_rx_done(), the CR1 register
is also never read before so clr_bits finction is again useful.
2017-01-11 14:58 GMT+01:00 M'boumba Cedric Madianga <cedric.madianga@gmail.com>:
> Hi Uwe,
>
> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> Hello Cedric,
>>
>> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>>> +/*
>>> + * In standard mode:
>>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
>>> + *
>>> + * In fast mode:
>>> + * If Duty = 0; SCL high period = 1 * CCR * I2C parent clk period
>>> + * SCL low period = 2 * CCR * I2C parent clk period
>>> + * If Duty = 1; SCL high period = 9 * CCR * I2C parent clk period
>>> + * SCL low period = 16 * CCR * I2C parent clk period
>> s/ \*/ */ several times
>
> Sorry but I don't see where is the issue as the style for multi-line
> comments seems ok.
> Could you please clarify that point if possible ? Thanks in advance
>
>>
>>> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
>>> + * Duty = 1
>>> + *
>>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> s/mode/Mode/
>
> ok thanks
>
>>
>>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>>> + * constraints defined by i2c bus specification
>>
>> 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
>
>>
>>> + */
>>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>>> [...]
>>> +
>>> +/**
>>> + * stm32f4_i2c_hw_config() - Prepare I2C block
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> + int ret = 0;
>>> +
>>> + /* Disable I2C */
>>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>>> +
>>> + ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + stm32f4_i2c_set_rise_time(i2c_dev);
>>> +
>>> + stm32f4_i2c_set_speed_mode(i2c_dev);
>>> +
>>> + stm32f4_i2c_set_filter(i2c_dev);
>>> +
>>> + /* Enable I2C */
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>>
>> This function is called after a hw reset, so there should be no need to
>> use clr_bits and set_bits because the value read from hw should be
>> known.
>
> ok thanks
>
>>
>>> + return ret;
>>
>> return 0;
>
> ok thanks
>
>>
>>> +}
>>> +
>>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> + u32 status;
>>> + int ret;
>>> +
>>> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>>> + status,
>>> + !(status & STM32F4_I2C_SR2_BUSY),
>>> + 10, 1000);
>>> + if (ret) {
>>> + dev_dbg(i2c_dev->dev, "bus not free\n");
>>> + ret = -EBUSY;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>>> + * @i2c_dev: Controller's private data
>>> + * @byte: Data to write in the register
>>> + */
>>> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>>> +{
>>> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>>> + * @i2c_dev: Controller's private data
>>> + *
>>> + * This function fills the data register with I2C transfer buffer
>>> + */
>>> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +
>>> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>>> + msg->count--;
>>> +}
>>> +
>>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> + u32 rbuf;
>>> +
>>> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>>> + *msg->buf++ = rbuf & 0xff;
>>
>> This is unnecessary. buf has an 8 bit wide type so
>>
>> *msg->buf++ = rbuf;
>>
>> has the same effect. (ISTR this is something I already pointed out
>> earlier?)
>
> Yes you are right.
>
>>
>>> + msg->count--;
>>> +}
>>> +
>>> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> + stm32f4_i2c_disable_irq(i2c_dev);
>>> +
>>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> + if (msg->stop)
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> + else
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> +
>>> + complete(&i2c_dev->complete);
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> + if (msg->count) {
>>> + stm32f4_i2c_write_msg(i2c_dev);
>>> + if (!msg->count) {
>>> + /* Disable buffer interrupts for RXNE/TXE events */
>>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>>> + }
>>> + } else {
>>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>>
>> Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
>> yes, is it then right to set STM32F4_I2C_CR1_STOP or
>> STM32F4_I2C_CR1_START?
>
> If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
> In that case, we return -EAGAIN and i2c-core will retry by calling
> stm32f4_i2c_xfer()
>
>>
>>> + }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> + switch (msg->count) {
>>> + case 1:
>>> + stm32f4_i2c_disable_irq(i2c_dev);
>>> + stm32f4_i2c_read_msg(i2c_dev);
>>> + complete(&i2c_dev->complete);
>>> + break;
>>> + /*
>>> + * For 2 or 3-byte reception, we do not have to read the data register
>>> + * when RXNE occurs as we have to wait for byte transferred finished
>>
>> it's hard to understand because if you don't know the hardware the
>> meaning of RXNE is unknown.
>
> Ok I will replace RXNE by RX not empty in that comment
>
>>
>>> + * event before reading data. So, here we just disable buffer
>>> + * interrupt in order to avoid another system preemption due to RXNE
>>> + * event
>>> + */
>>> + case 2:
>>> + case 3:
>>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>>> + break;
>>> + /* For N byte reception with N > 3 we directly read data register */
>>> + default:
>>> + stm32f4_i2c_read_msg(i2c_dev);
>>> + }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>>> + * in case of read
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>
>> btf is a hw-related name. Maybe better use _done which is easier to
>> understand?
>
> OK
>
>>
>>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> + void __iomem *reg;
>>> + u32 mask;
>>> + int i;
>>> +
>>> + switch (msg->count) {
>>> + case 2:
>>> + /*
>>> + * In order to correctly send the Stop or Repeated Start
>>> + * condition on the I2C bus, the STOP/START bit has to be set
>>> + * before reading the last two bytes.
>>> + * After that, we could read the last two bytes, disable
>>> + * remaining interrupts and notify the end of xfer to the
>>> + * client
>>
>> 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.
>
>>How does this work, when I only want to read a single
>> byte? Same problem for ACK below.
>
> For a single reception, we enable NACK and STOP or Repeatead START
> bits during address match.
> The NACK and STOP/START pulses are sent as soon as the data is
> received in the shift register.
> Please note that in that case, we don't have to wait BTF event to read the data.
> Data is read as soon as RXNE event occurs.
>
>>
>>> + */
>>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> + if (msg->stop)
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> + else
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> +
>>> + for (i = 2; i > 0; i--)
>>> + stm32f4_i2c_read_msg(i2c_dev);
>>> +
>>> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>>> + stm32f4_i2c_clr_bits(reg, mask);
>>> +
>>> + complete(&i2c_dev->complete);
>>> + break;
>>> + case 3:
>>> + /*
>>> + * In order to correctly send the ACK on the I2C bus for the
>>> + * last two bytes, we have to set ACK bit before reading the
>>> + * third last data byte
>>> + */
>>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> + stm32f4_i2c_read_msg(i2c_dev);
>>> + break;
>>> + default:
>>> + stm32f4_i2c_read_msg(i2c_dev);
>>> + }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>>> + * master receiver
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> + void __iomem *reg;
>>> +
>>> + switch (msg->count) {
>>> + case 0:
>>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>>> + /* Clear ADDR flag */
>>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> + break;
>>> + case 1:
>>> + /*
>>> + * Single byte reception:
>>
>> This also happens for the last byte of a 5 byte transfer, right?
>
> For a 5 byte transfer the behavior is different:
> We have to read data from DR (data register) as soon as the RXNE (RX
> not empty) event occurs for data1, data2 and data3 (until N-2 data for
> a more generic case)
> The ACK is automatically sent as soon as the data is received in the
> shift register as the I2C controller was configured to do that during
> adress match phase.
>
> For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
> in order to set NACK before reading DR.
> This event occurs when a new data has been received in shift register
> (in our case data4 or N-1 data) but the prevoius data in DR (in our
> case data3 or N-2 data) has not been read yet.
> In that way, the NACK pulse will be correctly generated after the last
> received data byte.
>
> For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
> and data5 or N data in shift register), set STOP or repeated Start in
> order to correctly sent the right pulse after the last received data
> byte and run 2 consecutives read of DR.
>
>>
>>> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>>> + */
>>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> + if (msg->stop)
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> + else
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> + break;
>>> + case 2:
>>> + /*
>>> + * 2-byte reception:
>>> + * Enable NACK and set POS
>>
>> What is POS?
> POS is used to define the position of the (N)ACK pulse
> 0: ACK is generated when the current is being received in the shift register
> 1: ACK is generated when the next byte which will be received in the
> shift register (used for 2-byte reception)
>
>>
>>> + */
>>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>>
>> You could get rid of this, when caching the value of CR1. Would save two
>> register reads here. This doesn't work for all registers, but it should
>> be possible to apply for most of them, maybe enough to get rid of the
>> clr_bits and set_bits function.
>>
>>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> + break;
>>> +
>>> + default:
>>> + /* N-byte reception: Enable ACK */
>>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>>
>> Do you need to set ACK for each byte transferred?
> I need to do that in order to be SMBus compatible and the ACK/NACK
> seems to be used by default in Documentation/i2c/i2c-protocol file.
>
>>
>> I stopp reviewing here because of -ENOTIME on my side but don't want to
>> delay discussion, so sent my comments up to here already now.
>>
>> Best regards
>> Uwe
>>
>> --
>> Pengutronix e.K. | Uwe Kleine-König |
>> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-11 13: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: <20170111082208.vzu7xgpd4eakyldl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hi Uwe,
2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Cedric,
>
> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>> +/*
>> + * In standard mode:
>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
>> + *
>> + * In fast mode:
>> + * If Duty = 0; SCL high period = 1 * CCR * I2C parent clk period
>> + * SCL low period = 2 * CCR * I2C parent clk period
>> + * If Duty = 1; SCL high period = 9 * CCR * I2C parent clk period
>> + * SCL low period = 16 * CCR * I2C parent clk period
> s/ \*/ */ several times
Sorry but I don't see where is the issue as the style for multi-line
comments seems ok.
Could you please clarify that point if possible ? Thanks in advance
>
>> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
>> + * Duty = 1
>> + *
>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> s/mode/Mode/
ok thanks
>
>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>> + * constraints defined by i2c bus specification
>
> 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
>
>> + */
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> [...]
>> +
>> +/**
>> + * stm32f4_i2c_hw_config() - Prepare I2C block
>> + * @i2c_dev: Controller's private data
>> + */
>> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + int ret = 0;
>> +
>> + /* Disable I2C */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> +
>> + ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>> + if (ret)
>> + return ret;
>> +
>> + stm32f4_i2c_set_rise_time(i2c_dev);
>> +
>> + stm32f4_i2c_set_speed_mode(i2c_dev);
>> +
>> + stm32f4_i2c_set_filter(i2c_dev);
>> +
>> + /* Enable I2C */
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>
> This function is called after a hw reset, so there should be no need to
> use clr_bits and set_bits because the value read from hw should be
> known.
ok thanks
>
>> + return ret;
>
> return 0;
ok thanks
>
>> +}
>> +
>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 status;
>> + int ret;
>> +
>> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> + status,
>> + !(status & STM32F4_I2C_SR2_BUSY),
>> + 10, 1000);
>> + if (ret) {
>> + dev_dbg(i2c_dev->dev, "bus not free\n");
>> + ret = -EBUSY;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>> + * @i2c_dev: Controller's private data
>> + * @byte: Data to write in the register
>> + */
>> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>> +{
>> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>> + * @i2c_dev: Controller's private data
>> + *
>> + * This function fills the data register with I2C transfer buffer
>> + */
>> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +
>> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>> + msg->count--;
>> +}
>> +
>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + u32 rbuf;
>> +
>> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> + *msg->buf++ = rbuf & 0xff;
>
> This is unnecessary. buf has an 8 bit wide type so
>
> *msg->buf++ = rbuf;
>
> has the same effect. (ISTR this is something I already pointed out
> earlier?)
Yes you are right.
>
>> + msg->count--;
>> +}
>> +
>> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> +
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> + complete(&i2c_dev->complete);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + if (msg->count) {
>> + stm32f4_i2c_write_msg(i2c_dev);
>> + if (!msg->count) {
>> + /* Disable buffer interrupts for RXNE/TXE events */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + }
>> + } else {
>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>
> Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
> yes, is it then right to set STM32F4_I2C_CR1_STOP or
> STM32F4_I2C_CR1_START?
If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
In that case, we return -EAGAIN and i2c-core will retry by calling
stm32f4_i2c_xfer()
>
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + switch (msg->count) {
>> + case 1:
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + complete(&i2c_dev->complete);
>> + break;
>> + /*
>> + * For 2 or 3-byte reception, we do not have to read the data register
>> + * when RXNE occurs as we have to wait for byte transferred finished
>
> it's hard to understand because if you don't know the hardware the
> meaning of RXNE is unknown.
Ok I will replace RXNE by RX not empty in that comment
>
>> + * event before reading data. So, here we just disable buffer
>> + * interrupt in order to avoid another system preemption due to RXNE
>> + * event
>> + */
>> + case 2:
>> + case 3:
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + break;
>> + /* For N byte reception with N > 3 we directly read data register */
>> + default:
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>> + * in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>
> btf is a hw-related name. Maybe better use _done which is easier to
> understand?
OK
>
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> + u32 mask;
>> + int i;
>> +
>> + switch (msg->count) {
>> + case 2:
>> + /*
>> + * In order to correctly send the Stop or Repeated Start
>> + * condition on the I2C bus, the STOP/START bit has to be set
>> + * before reading the last two bytes.
>> + * After that, we could read the last two bytes, disable
>> + * remaining interrupts and notify the end of xfer to the
>> + * client
>
> 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.
>How does this work, when I only want to read a single
> byte? Same problem for ACK below.
For a single reception, we enable NACK and STOP or Repeatead START
bits during address match.
The NACK and STOP/START pulses are sent as soon as the data is
received in the shift register.
Please note that in that case, we don't have to wait BTF event to read the data.
Data is read as soon as RXNE event occurs.
>
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> + for (i = 2; i > 0; i--)
>> + stm32f4_i2c_read_msg(i2c_dev);
>> +
>> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> + stm32f4_i2c_clr_bits(reg, mask);
>> +
>> + complete(&i2c_dev->complete);
>> + break;
>> + case 3:
>> + /*
>> + * In order to correctly send the ACK on the I2C bus for the
>> + * last two bytes, we have to set ACK bit before reading the
>> + * third last data byte
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + break;
>> + default:
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>> + * master receiver
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> +
>> + switch (msg->count) {
>> + case 0:
>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>> + /* Clear ADDR flag */
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> + case 1:
>> + /*
>> + * Single byte reception:
>
> This also happens for the last byte of a 5 byte transfer, right?
For a 5 byte transfer the behavior is different:
We have to read data from DR (data register) as soon as the RXNE (RX
not empty) event occurs for data1, data2 and data3 (until N-2 data for
a more generic case)
The ACK is automatically sent as soon as the data is received in the
shift register as the I2C controller was configured to do that during
adress match phase.
For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
in order to set NACK before reading DR.
This event occurs when a new data has been received in shift register
(in our case data4 or N-1 data) but the prevoius data in DR (in our
case data3 or N-2 data) has not been read yet.
In that way, the NACK pulse will be correctly generated after the last
received data byte.
For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
and data5 or N data in shift register), set STOP or repeated Start in
order to correctly sent the right pulse after the last received data
byte and run 2 consecutives read of DR.
>
>> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> + break;
>> + case 2:
>> + /*
>> + * 2-byte reception:
>> + * Enable NACK and set POS
>
> What is POS?
POS is used to define the position of the (N)ACK pulse
0: ACK is generated when the current is being received in the shift register
1: ACK is generated when the next byte which will be received in the
shift register (used for 2-byte reception)
>
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>
> You could get rid of this, when caching the value of CR1. Would save two
> register reads here. This doesn't work for all registers, but it should
> be possible to apply for most of them, maybe enough to get rid of the
> clr_bits and set_bits function.
>
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> +
>> + default:
>> + /* N-byte reception: Enable ACK */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>
> Do you need to set ACK for each byte transferred?
I need to do that in order to be SMBus compatible and the ACK/NACK
seems to be used by default in Documentation/i2c/i2c-protocol file.
>
> I stopp reviewing here because of -ENOTIME on my side but don't want to
> delay discussion, so sent my comments up to here already now.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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
* [PATCH v2] i2c: piix4: Avoid race conditions with IMC
From: Ricardo Ribalda Delgado @ 2017-01-11 9:11 UTC (permalink / raw)
To: Andy Shevchenko, Jean Delvare, Wolfram Sang, linux-i2c,
linux-kernel@vger.kernel.org
Cc: Ricardo Ribalda Delgado
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.
Credit-to: Alexandre Desnoyers <alex@qtec.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
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 f0563f7ce01b..81d06be0a72d 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] i2c: piix4: Avoid race conditions with IMC
From: Ricardo Ribalda Delgado @ 2017-01-11 9:11 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jean Delvare, Wolfram Sang, linux-i2c,
linux-kernel@vger.kernel.org
In-Reply-To: <CAHp75VdnoW84sQh-vf57NSC6_tXGqvb_36KCPpi-XFbktWiKfw@mail.gmail.com>
Hi Andy
Thanks for your review!
On Wed, Jan 11, 2017 at 2:49 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jan 10, 2017 at 2:16 PM, Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
>> 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)
>
> It would be nice to understand what kind of devices are accessing and to where.
On my platform I found out that the IMC is polling address 0x98 and
0x99 every 14 and 177 msec.
Check out:
https://postimg.org/image/bssxhlg9d
https://postimg.org/image/g37ld6lch
But be aware that the firmware on the IMC might be different on other
platforms so the addresses and interval will be different.
>
> Hans seems discovered one pretty nice issue on Intel
> BayTrail/CherryTrail platforms where I2C semaphore is used to prevent
> simultaneous access to P-Unit, but we have two paths there which are
> not synchronized (yet). It brings a set of interesting (and
> unfortunately "famous") bugs.
AFAIK on AMD the smbus is just used on this driver.
>
>>
>> Without this patch, many access to the SMBus end with an invalid
>> transaction or even with the bus stalled.
>>
>
>> Credit-to: Alexandre Desnoyers <alex@qtec.com>
>
> Never saw before. Did he suggested the solution or what?
He is the hardware engineer where I work (qtec), when I showed him the
the logic analyzer output and told him that I was pretty sure that the
kernel/userpace was not doing those transactions he came up with the
theory of the IMC. He found up the semaphore on the documentation
also, so he deserves a lot of credit :).
>
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -585,9 +585,28 @@ 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;
>> u8 smba_en_lo;
>> u8 port;
>> int retval;
>> + int timeout = 0;
>> + int smbslvcnt;
>
>> while (++timeout < MAX_TIMEOUT) {
>
> Usual pattern is countdown.
I was trying to follow the pattern on the file.
>
>> + /* SMBus is still owned by the IMC, we give up */
>> + if (timeout == MAX_TIMEOUT)
>> + return -EBUSY;
>
> Would caller do it again? Perhaps -EAGAIN?
I think in this case we should return -EBUSY. If after 500 attempts
the bus is still hold by the IMC the bus is stalled or the IMC is
crashed, we should not retry. It is also the same errcode returned by
piix4_transaction().
>
> Since the returned value is not -ETIMEDOUT, I suppose the name of
> counter variable is a bit confusing. Basically it's amount of attempts
> with some gap between them. Though, it's up to you and maintainer.
>
>> + /* Release the semaphore */
>> + outb_p(smbslvcnt | 0x20, SMBSLVCNT);
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks again, I will send a v2 with your comments!
--
Ricardo Ribalda
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-11 8:22 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
alexandre.torgue-qxv4g6HH51o,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A, patrice.chotard-qxv4g6HH51o,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483607246-14771-3-git-send-email-cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hello Cedric,
On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> +/*
> + * In standard mode:
> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
> + *
> + * In fast mode:
> + * If Duty = 0; SCL high period = 1 * CCR * I2C parent clk period
> + * SCL low period = 2 * CCR * I2C parent clk period
> + * If Duty = 1; SCL high period = 9 * CCR * I2C parent clk period
> + * SCL low period = 16 * CCR * I2C parent clk period
s/ \*/ */ several times
> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
> + * Duty = 1
> + *
> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
s/mode/Mode/
> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
> + * constraints defined by i2c bus specification
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?
> + */
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> [...]
> +
> +/**
> + * stm32f4_i2c_hw_config() - Prepare I2C block
> + * @i2c_dev: Controller's private data
> + */
> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> + int ret = 0;
> +
> + /* Disable I2C */
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> +
> + ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
> + if (ret)
> + return ret;
> +
> + stm32f4_i2c_set_rise_time(i2c_dev);
> +
> + stm32f4_i2c_set_speed_mode(i2c_dev);
> +
> + stm32f4_i2c_set_filter(i2c_dev);
> +
> + /* Enable I2C */
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
This function is called after a hw reset, so there should be no need to
use clr_bits and set_bits because the value read from hw should be
known.
> + return ret;
return 0;
> +}
> +
> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 status;
> + int ret;
> +
> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> + status,
> + !(status & STM32F4_I2C_SR2_BUSY),
> + 10, 1000);
> + if (ret) {
> + dev_dbg(i2c_dev->dev, "bus not free\n");
> + ret = -EBUSY;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
> + * @i2c_dev: Controller's private data
> + * @byte: Data to write in the register
> + */
> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
> +{
> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
> +}
> +
> +/**
> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
> + * @i2c_dev: Controller's private data
> + *
> + * This function fills the data register with I2C transfer buffer
> + */
> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +
> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
> + msg->count--;
> +}
> +
> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + u32 rbuf;
> +
> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> + *msg->buf++ = rbuf & 0xff;
This is unnecessary. buf has an 8 bit wide type so
*msg->buf++ = rbuf;
has the same effect. (ISTR this is something I already pointed out
earlier?)
> + msg->count--;
> +}
> +
> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + stm32f4_i2c_disable_irq(i2c_dev);
> +
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + if (msg->stop)
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + else
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +
> + complete(&i2c_dev->complete);
> +}
> +
> +/**
> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + if (msg->count) {
> + stm32f4_i2c_write_msg(i2c_dev);
> + if (!msg->count) {
> + /* Disable buffer interrupts for RXNE/TXE events */
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> + }
> + } else {
> + stm32f4_i2c_terminate_xfer(i2c_dev);
Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
yes, is it then right to set STM32F4_I2C_CR1_STOP or
STM32F4_I2C_CR1_START?
> + }
> +}
> +
> +/**
> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + switch (msg->count) {
> + case 1:
> + stm32f4_i2c_disable_irq(i2c_dev);
> + stm32f4_i2c_read_msg(i2c_dev);
> + complete(&i2c_dev->complete);
> + break;
> + /*
> + * For 2 or 3-byte reception, we do not have to read the data register
> + * when RXNE occurs as we have to wait for byte transferred finished
it's hard to understand because if you don't know the hardware the
meaning of RXNE is unknown.
> + * event before reading data. So, here we just disable buffer
> + * interrupt in order to avoid another system preemption due to RXNE
> + * event
> + */
> + case 2:
> + case 3:
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> + break;
> + /* For N byte reception with N > 3 we directly read data register */
> + default:
> + stm32f4_i2c_read_msg(i2c_dev);
> + }
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
> + * in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
> +{
btf is a hw-related name. Maybe better use _done which is easier to
understand?
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg;
> + u32 mask;
> + int i;
> +
> + switch (msg->count) {
> + case 2:
> + /*
> + * In order to correctly send the Stop or Repeated Start
> + * condition on the I2C bus, the STOP/START bit has to be set
> + * before reading the last two bytes.
> + * After that, we could read the last two bytes, disable
> + * remaining interrupts and notify the end of xfer to the
> + * client
This is surprising. I didn't recheck the manual, but that looks very
uncomfortable. How does this work, when I only want to read a single
byte? Same problem for ACK below.
> + */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + if (msg->stop)
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + else
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +
> + for (i = 2; i > 0; i--)
> + stm32f4_i2c_read_msg(i2c_dev);
> +
> + reg = i2c_dev->base + STM32F4_I2C_CR2;
> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> + stm32f4_i2c_clr_bits(reg, mask);
> +
> + complete(&i2c_dev->complete);
> + break;
> + case 3:
> + /*
> + * In order to correctly send the ACK on the I2C bus for the
> + * last two bytes, we have to set ACK bit before reading the
> + * third last data byte
> + */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + stm32f4_i2c_read_msg(i2c_dev);
> + break;
> + default:
> + stm32f4_i2c_read_msg(i2c_dev);
> + }
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
> + * master receiver
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg;
> +
> + switch (msg->count) {
> + case 0:
> + stm32f4_i2c_terminate_xfer(i2c_dev);
> + /* Clear ADDR flag */
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + break;
> + case 1:
> + /*
> + * Single byte reception:
This also happens for the last byte of a 5 byte transfer, right?
> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
> + */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + if (msg->stop)
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + else
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> + break;
> + case 2:
> + /*
> + * 2-byte reception:
> + * Enable NACK and set POS
What is POS?
> + */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
You could get rid of this, when caching the value of CR1. Would save two
register reads here. This doesn't work for all registers, but it should
be possible to apply for most of them, maybe enough to get rid of the
clr_bits and set_bits function.
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + break;
> +
> + default:
> + /* N-byte reception: Enable ACK */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
Do you need to set ACK for each byte transferred?
I stopp reviewing here because of -ENOTIME on my side but don't want to
delay discussion, so sent my comments up to here already now.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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