* 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 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 0/6] fbdev/ssd1307fb: Some changes and improvement
From: Jyri Sarha @ 2017-01-12 11:27 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ
Cc: tomi.valkeinen-l0cyMroinI0,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <cover.1484219137.git.jsarha-l0cyMroinI0@public.gmane.org>
I did not notice the latest update on MAINTAINERS when sending this
series. Added Bartlomiej Zolnierkiewicz <b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> to loop.
On 01/12/17 13:16, Jyri Sarha wrote:
> We needed these changes for couple of our evm. Everything else is quite
> straight forward, but removing the reset-active-low dts property is a
> questionable at least in theory. However, in practice if anyone was
> using the property in their device-tree blobs, they have never been
> able to get desired effect with mainline kernel.
>
> Jyri Sarha (3):
> fbdev: ssd1307fb: Start to use gpiod API for reset gpio
> fbdev: ssd1307fb: Remove reset-active-low from the DT binding document
> fbdev: ssd1307fb: Make reset gpio devicetree property optional
>
> Tomi Valkeinen (3):
> fbdev/ssd1307fb: add support to enable VBAT
> fbdev/ssd1307fb: clear screen in probe
> fbdev/ssd1307fb: add support to enable VBAT
>
> .../devicetree/bindings/display/ssd1307fb.txt | 5 +-
> drivers/video/fbdev/ssd1307fb.c | 55 ++++++++++++++--------
> 2 files changed, 38 insertions(+), 22 deletions(-)
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 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 6/6] fbdev/ssd1307fb: add support to enable VBAT
From: Tomi Valkeinen @ 2017-01-12 12:05 UTC (permalink / raw)
To: Jyri Sarha, linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <c394e52447ed45fff9672eb3e3bb6729bea87ee7.1484219137.git.jsarha-l0cyMroinI0@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1547 bytes --]
On 12/01/17 13:16, Jyri Sarha wrote:
> From: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
>
> SSD1306 needs VBAT when it is wired in charge pump configuration. This
> patch adds support to the driver to enable VBAT regulator at init time.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
> Reviewed-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
> ---
> Documentation/devicetree/bindings/display/ssd1307fb.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/display/ssd1307fb.txt b/Documentation/devicetree/bindings/display/ssd1307fb.txt
> index 6617df6..209d931 100644
> --- a/Documentation/devicetree/bindings/display/ssd1307fb.txt
> +++ b/Documentation/devicetree/bindings/display/ssd1307fb.txt
> @@ -16,6 +16,7 @@ Required properties:
> Optional properties:
> - reset-gpios: The GPIO used to reset the OLED display, if available. See
> Documentation/devicetree/bindings/gpio/gpio.txt for details.
> + - vbat-supply: The supply for VBAT
> - solomon,segment-no-remap: Display needs normal (non-inverted) data column
> to segment mapping
> - solomon,com-seq: Display uses sequential COM pin configuration
>
It's rather confusing to have two patches with identical subjects and
descs (this and 4). And there's even an unrelated patch in between this
and the driver change...
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv2] dt: bindings: Add support for CSI1 bus
From: Baruch Siach @ 2017-01-12 12:06 UTC (permalink / raw)
To: Pavel Machek
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
ivo.g.dimitrov.75-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-X3B1VOXEql0, sre-DgEjT+Ai2ygdnm+yROfE0A,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
linux-media-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170111225335.GA21553@amd>
Hi Pavel,
On Wed, Jan 11, 2017 at 11:53:35PM +0100, Pavel Machek wrote:
> From: Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
>
> In the vast majority of cases the bus type is known to the driver(s)
> since a receiver or transmitter can only support a single one. There
> are cases however where different options are possible.
>
> The existing V4L2 OF support tries to figure out the bus type and
> parse the bus parameters based on that. This does not scale too well
> as there are multiple serial busses that share common properties.
>
> Some hardware also supports multiple types of busses on the same
> interfaces.
>
> Document the CSI1/CCP2 property strobe. It signifies the clock or
> strobe mode.
>
> Signed-off-by: Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
>
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..08c4498 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -76,6 +76,11 @@ Optional endpoint properties
> mode horizontal and vertical synchronization signals are provided to the
> slave device (data source) by the master device (data sink). In the master
> mode the data source device is also the source of the synchronization signals.
> +- bus-type: data bus type. Possible values are:
> + 0 - MIPI CSI2
> + 1 - parallel / Bt656
Why not have separate values for parallel and BT.656?
baruch
> + 2 - MIPI CSI1
> + 3 - CCP2
> - bus-width: number of data lines actively used, valid for the parallel busses.
> - data-shift: on the parallel data busses, if bus-width is used to specify the
> number of data lines, data-shift can be used to specify which data lines are
> @@ -112,7 +117,8 @@ Optional endpoint properties
> should be the combined length of data-lanes and clock-lanes properties.
> If the lane-polarities property is omitted, the value must be interpreted
> as 0 (normal). This property is valid for serial busses only.
> -
> +- strobe: Whether the clock signal is used as clock or strobe. Used
> + with CCP2, for instance.
>
> Example
> -------
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.52.368.4656, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v7 0/4] arm64: arch_timer: Add workaround for hisilicon-161601 erratum
From: Ding Tianhong @ 2017-01-12 13:24 UTC (permalink / raw)
To: Marc Zyngier, catalin.marinas, will.deacon, mark.rutland, oss,
devicetree, shawnguo, stuart.yoder, linux-arm-kernel, linuxarm
In-Reply-To: <ff9c81c2-90ef-a5c2-6f0b-c6cf5d0d11a9@arm.com>
On 2017/1/12 17:11, Marc Zyngier wrote:
> On 12/01/17 04:23, Ding Tianhong wrote:
>> Hi Marc:
>>
>> How about this v7, if any suggestions very grateful.
>
> It's been less than 5 days since you posted this. I'll get to it once I
> finish reviewing all the other patches that are sitting in the queue
> right before yours.
>
Ok and sorry for the noisy.
Thanks
Ding
> Thanks,
>
> M.
>
^ permalink raw reply
* Re: [PATCH 1/2][UPDATE] of: base: add support to get the number of cache levels
From: Rob Herring @ 2017-01-12 13:24 UTC (permalink / raw)
To: Sudeep Holla
Cc: Mark Rutland, devicetree@vger.kernel.org, Catalin Marinas,
Will Deacon, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Tan Xiaojun
In-Reply-To: <1484049616-22388-1-git-send-email-sudeep.holla@arm.com>
On Tue, Jan 10, 2017 at 6:00 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> It is useful to have helper function just to get the number of cache
> levels for a given logical cpu. This patch adds the support for the
> same.
>
> It will be used on ARM64 platform where the device tree provides the
> information for the additional non-architected/transparent/external
> last level caches that are not integrated with the processors.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/of/base.c | 23 +++++++++++++++++++++++
> include/linux/of.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4bea3c797d6..80e557eca858 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -25,6 +25,7 @@
> #include <linux/cpu.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/of_graph.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> @@ -2268,6 +2269,28 @@ struct device_node *of_find_next_cache_node(const struct device_node *np)
> }
>
> /**
> + * of_count_cache_levels - Find the total number of cache levels for the
> + * given logical cpu
> + *
> + * @cpu: cpu number(logical index) for which cache levels is being counted
> + *
> + * Returns the total number of cache levels for the given logical cpu
> + */
> +int of_count_cache_levels(unsigned int cpu)
> +{
> + int level = 0;
> + struct device_node *np = of_cpu_device_node_get(cpu);
> +
> + while (np) {
> + level++;
This will return 1 if you have a cpu node and no cache nodes. Are you
assuming the cpu has a cache?
Perhaps you should just find the last level cache node and then just
read "cache-level".
Rob
^ permalink raw reply
* Re: [PATCH 6/6] fbdev/ssd1307fb: add support to enable VBAT
From: Jyri Sarha @ 2017-01-12 13:46 UTC (permalink / raw)
To: Tomi Valkeinen, linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <d06c3a64-7627-2553-84e5-491a5acbe0de-l0cyMroinI0@public.gmane.org>
On 01/12/17 14:05, Tomi Valkeinen wrote:
> On 12/01/17 13:16, Jyri Sarha wrote:
>> From: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
>>
>> SSD1306 needs VBAT when it is wired in charge pump configuration. This
>> patch adds support to the driver to enable VBAT regulator at init time.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
>> Reviewed-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
>> Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
>> ---
>> Documentation/devicetree/bindings/display/ssd1307fb.txt | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ssd1307fb.txt b/Documentation/devicetree/bindings/display/ssd1307fb.txt
>> index 6617df6..209d931 100644
>> --- a/Documentation/devicetree/bindings/display/ssd1307fb.txt
>> +++ b/Documentation/devicetree/bindings/display/ssd1307fb.txt
>> @@ -16,6 +16,7 @@ Required properties:
>> Optional properties:
>> - reset-gpios: The GPIO used to reset the OLED display, if available. See
>> Documentation/devicetree/bindings/gpio/gpio.txt for details.
>> + - vbat-supply: The supply for VBAT
>> - solomon,segment-no-remap: Display needs normal (non-inverted) data column
>> to segment mapping
>> - solomon,com-seq: Display uses sequential COM pin configuration
>>
>
> It's rather confusing to have two patches with identical subjects and
> descs (this and 4). And there's even an unrelated patch in between this
> and the driver change...
>
Argh. I wonder how did this even happen. Something weird must have been
going on... there is only one such patch in branch I cherry-picked these
patches from. These last patches did not give me any errors and compiled
fine, so did not look too carefully at the results. Anyway, I'll squash
this into the earlier patch with the same subject in version 2 series.
Best regards,
Jyri
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V2 1/2] clk: vc5: Add bindings for IDT VersaClock 5P49V5923 and 5P49V5933
From: Marek Vasut @ 2017-01-12 13:47 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-clk, Michael Turquette, Stephen Boyd, Laurent Pinchart,
Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux-Renesas
In-Reply-To: <CAMuHMdWTGyRc82fPZ-k28=-2PzpkQooKPcB9OF0BctDuw7qbZA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 01/12/2017 09:25 AM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi!
> On Thu, Jan 12, 2017 at 2:03 AM, Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Add bindings for IDT VersaClock 5 5P49V5923 and 5P49V5933 chips.
>> These are I2C clock generators with optional clock source from
>> either XTal or dedicated clock generator and, depending on the
>> model, two or more clock outputs.
>>
>> Signed-off-by: Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Michael Turquette <mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> Cc: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Cc: linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> ---
>> V2: Add mapping between the clock specifier and physical pins of the chip
>> ---
>> .../devicetree/bindings/clock/idt,versaclock5.txt | 65 ++++++++++++++++++++++
>> 1 file changed, 65 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/idt,versaclock5.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt
>> new file mode 100644
>> index 000000000000..87e9c47a89a3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt
>> @@ -0,0 +1,65 @@
>
>> +==Mapping between clock specifier and physical pins==
>> +
>> +When referencing the provided clock in the DT using phandle and
>> +clock specifier, the following mapping applies:
>> +
>> +5P49V5923:
>> + 0 -- OUT0_SEL_I2CB
>> + 1 -- OUT1
>> + 2 -- OUT2
>> +
>> +5P49V5933:
>> + 0 -- OUT0_SEL_I2CB
>> + 1 -- OUT1
>> + 2 -- OUT4
>
> I'm a bit puzzled by the use of "OUT4".
You're not the only one, but at least you're not the one banging your
head against the desk while glaring into the scope and seeing nonsense
on one of the channels :-)
> According to the datasheets, both '5923 and '5933 have OUT1 and OUT2.
> The '5933 datasheet has a single reference to OUT4 ("The OUT1 to OUT4 clock
> outputs"), but that may be a copy and paste error from a datasheet for a part
> with 4 outputs.
The physical VC5 5P49V5933 devkit has OUT0 , OUT1 and OUT4 . At first, I
thought it was laziness in the PCB design -- because there are VC5s with
up to 4 outputs and the devkit PCB is identical for all of them,
except some components are not populated -- but that is not correct.
They actually use (inside of the chip) output1 with fod1 and output4
with fod4 for the 5933 and they use output1 with fod1 and output2 with
fod2 on the 5923 . This became obvious after I took a look into the IDT
clock commander software, which has a visualization of the chip and
there are two "unpopulated" positions between out1 and out4 in case of
the 5933 while on 5923 the unpopulated positions are below out2 .
I have no explanation as for why they did this, it's just unsystematic.
> Apart from that:
> Reviewed-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 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 v5 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario
From: Appana Durga Kedareswara Rao @ 2017-01-12 14:19 UTC (permalink / raw)
To: Vinod Koul
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
dan.j.williams@intel.com, michal.simek@xilinx.com,
Soren Brinkmann, moritz.fischer@ettus.com,
laurent.pinchart@ideasonboard.com, luis@debethencourt.com,
Jose.Abreu@synopsys.com, dmaengine@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <20170110084917.GA3573@localhost>
Hi Vinod,
Thanks for the review...
> On Sat, Jan 07, 2017 at 12:15:30PM +0530, Kedareswara rao Appana wrote:
> > When driver is handling AXI DMA SoftIP When user submits multiple
> > descriptors back to back on the S2MM(recv) side with the current
> > driver flow the last buffer descriptor next bd points to a invalid
> > location resulting the invalid data or errors in the DMA engine.
>
> Can you rephrase this, it a bit hard to understand.
When DMA is receiving packets h/w expects the descriptors
Should be in the form of a ring (I mean h/w buffer descriptor
Next descriptor field should always point to valid address
So that when DMA engine go and fetch that next descriptor it always
Sees a valid address).
But with the current driver implementation when user queues
Multiple descriptors the last descriptor next descriptor field
Pointing to an invalid location causing data corruption or
Errors from the DMA h/w engine...
To avoid this issue creating a Buffer descriptor Chain during
Channel allocation and using those buffer descriptors for processing
User requested data.
Please let me know if the above explanation is not clear will explain in detail....
>
> >
> > This patch fixes this issue by creating a BD Chain during
>
> whats a BD?
Buffer descriptor.
>
> > channel allocation itself and use those BD's.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> >
> > drivers/dma/xilinx/xilinx_dma.c | 133
> > +++++++++++++++++++++++++---------------
> > 1 file changed, 83 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > b/drivers/dma/xilinx/xilinx_dma.c index 0e9c02e..af2159d 100644
> > --- a/drivers/dma/xilinx/xilinx_dma.c
> > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > @@ -163,6 +163,7 @@
> > #define XILINX_DMA_BD_SOP BIT(27)
> > #define XILINX_DMA_BD_EOP BIT(26)
> > #define XILINX_DMA_COALESCE_MAX 255
> > +#define XILINX_DMA_NUM_DESCS 255
>
> why 255?
It is not an h/w limitation
Allocating 255 descriptors (Each descriptor is capable of sending 7MB data)
So roughly using allocated descriptors DMA engine can transfer 1GB data
And in the driver we are reusing the allocated descriptors when they are free.
Regards,
Kedar.
^ permalink raw reply
* RE: [PATCH v5 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma
From: Appana Durga Kedareswara Rao @ 2017-01-12 14:19 UTC (permalink / raw)
To: Vinod Koul
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org,
michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org,
Soren Brinkmann,
moritz.fischer-+aYTwkv1SeIAvxtiuMwx3w@public.gmane.org,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org,
luis-HiykPkW1eAzzDCI4PIEvbQC/G2K4zDHf@public.gmane.org,
Jose.Abreu-HKixBCOQz3hWk0Htik3J/w@public.gmane.org,
dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170110062620.GZ3573@localhost>
Hi Vinod,
Thanks for the review...
>
> On Sat, Jan 07, 2017 at 12:15:29PM +0530, Kedareswara rao Appana wrote:
> > When VDMA is configured for more than one frame in the h/w for example
> > h/w is configured for n number of frames and user Submits n number of
> > frames and triggered the DMA using issue_pending API.
>
> title case in middle if sentence, no commas, can you make it easier to read
> please..
Ok sure will fix commit message in the next version.
>
> > In the current driver flow we are submitting one frame at a time but
> > we should submit all the n number of frames at one time as the h/w Is
> > configured for n number of frames.
>
> s/Is/is
Ok sure will fix in the next version....
Regards,
Kedar.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/4] ARM: dts: sun8i: add OTG function to Lichee Pi Zero
From: Bin Liu @ 2017-01-12 14:50 UTC (permalink / raw)
To: Maxime Ripard
Cc: devicetree@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, Kishon Vijay Abraham I,
Chen-Yu Tsai, Icenowy Zheng, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20170111210638.ppdnpzdd2l6x4lyo@lukather>
On Wed, Jan 11, 2017 at 10:06:38PM +0100, Maxime Ripard wrote:
> On Wed, Jan 11, 2017 at 02:08:11PM -0600, Bin Liu wrote:
> > On Thu, Jan 12, 2017 at 03:55:33AM +0800, Icenowy Zheng wrote:
> > >
> > >
> > > 11.01.2017, 04:24, "Bin Liu" <b-liu@ti.com>:
> > > > On Tue, Jan 03, 2017 at 11:25:34PM +0800, Icenowy Zheng wrote:
> > > >> Lichee Pi Zero features a USB OTG port.
> > > >>
> > > >> Add support for it.
> > > >>
> > > >> Note: in order to use the Host mode, the board must be powered via the
> > > >> +5V and GND pins.
> > > >>
> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> > > >> ---
> > > >> arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts | 10 ++++++++++
> > > >> 1 file changed, 10 insertions(+)
> > > >>
> > > >> diff --git a/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts b/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > >> index 0099affc6ce3..3d9168cbaeca 100644
> > > >> --- a/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > >> +++ b/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > >> @@ -71,3 +71,13 @@
> > > >> pinctrl-names = "default";
> > > >> status = "okay";
> > > >> };
> > > >> +
> > > >> +&usb_otg {
> > > >> + dr_mode = "otg";
> > > >
> > > > Why not set this default mode in dtsi instead?
> > > >
> > > > Regards,
> > > > -Bin.
> > >
> > > There's possibly boards which do not have OTG functions.
> >
> > That is board specific.
>
> Exactly, and this is why it should be done in the board DT.
I am just suggesting based on the common practice. If a .dtsi exists for
a family, the .dtsi describes the device and common properties for all
possible boards, and each board .dts adds or overrides its specific
implementation. Kernel has many devices/boards done in this way - define
the default dr_mode in .dtsi.
In this case, I suggest to set the common dr_mode in .dtsi, then each
board .dts only overrides it if the implementation is different.
>
> The controller in the Allwinner SoCs do not handle directly the ID pin
> and VBUS, but rather rely on a GPIO to do so.
>
> So boards with OTG will need setup anyway, at least to tell which
> GPIOs are used. There's no point in enforcing a default if it doesn't
> work by default.
Then define a default which supposes to work for most boards.
Why I suggest this, is because defining a default dr_mode which works
for most cases in dtsi could prevent a little surprise in MUSB function.
If someone designs a new board but forgets to define dr_mode in the new
board DT, the MUSB driver will default to org mode, which might not be
intended.
Regards,
-Bin.
^ permalink raw reply
* Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
From: Will Deacon @ 2017-01-12 15:17 UTC (permalink / raw)
To: Rob Clark
Cc: iommu@lists.linux-foundation.org, linux-arm-msm, Sricharan R,
Jordan Crouse, Mark Rutland, Rob Herring,
devicetree@vger.kernel.org
In-Reply-To: <CAF6AEGvGWu+OMED52_Mphpd1K=2QAeeuPV8RaRzL9S+N4YOh2Q@mail.gmail.com>
On Wed, Jan 11, 2017 at 03:59:30PM -0500, Rob Clark wrote:
> On Wed, Jan 11, 2017 at 4:36 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jan 10, 2017 at 02:20:13PM -0500, Rob Clark wrote:
> >> On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote:
> >> >> Hmm, well we install the fault handler on the iommu_domain.. perhaps
> >> >> maybe a combo of dts property (or deciding based on more specific
> >> >> compat string), plus extra param passed in to
> >> >> iommu_set_fault_hander(). The dts property or compat string to
> >> >> indicate whether the iommu (and how it is wired up) can handle stalls,
> >> >> and enable_stall param when fault handler is registered to indicate
> >> >> whether the device itself can cope.. if either can't do stalling, then
> >> >> don't set CFCFG.
> >> >
> >> > I thought about this some more, and I think you're right. Having
> >> > iommu_set_fault_handler take a flags parameter indicating that, for example,
> >> > the fault handler can deal with paging, is all we need to implement the
> >> > per-master opt-in functionality for stalling faults. There's no real
> >> > requirement to standardise a generic firmware property for that (but
> >> > we still need *something* that says stalling is usable on the SMMU --
> >> > perhaps just the compatible string is ok).
> >>
> >> btw, it occurred to me that maybe it should be flags param to
> >> iommu_attach_device() (just in case fault handler not installed?)
> >> otoh stalling without a fault handler is silly, but I guess we need it
> >> to infer whether stalling can be supported by other devices on same
> >> iommu.. tbh I'm on a bit shaky ground when it comes to multiple
> >> devices per iommu since the SoC's I'm familiar with do it the other
> >> way around. But I guess you have thought more about the multi-device
> >> case, so figured I should suggest it..
> >
> > I don't think it works at attach time, because the stalling property belongs
> > to the domain, rather than the individual devices within it. Similarly, I
> > don't think we should allow this property to be toggled once devices have
> > been attached.
> >
>
> hmm, I was more thinking of cases where drivers for particular devices
> need some work (ie. like potentially disabling hw hang detect during
> faults).. I guess we could have three levels, that all have to be true
> in order to enable stall: smmu, domain (pass flags in to
> iommu_domain_alloc()??), and device (iommu_attach_device())?
Hooking iommu_set_fault_handler, as you originally suggested, is the best
way to set the flag on the domain. I think we just need to enforce that
iommu_set_fault_handler is called prior to attaching devices to a domain,
so that the IOMMU driver can configure the domain appropriately on the
first attach.
Will
^ permalink raw reply
* Re: [PATCH v3 2/4] dt-bindings: Add TI SCI PM Domains
From: Dave Gerlach @ 2017-01-12 15:27 UTC (permalink / raw)
To: Rob Herring
Cc: Ulf Hansson, Rafael J . Wysocki, Kevin Hilman,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Nishanth Menon, Keerthy, Russell King, Tero Kristo, Sudeep Holla,
Santosh Shilimkar, Lokesh Vutla
In-Reply-To: <CAL_JsqKxU4c=KSn0=xX9kmbk498fhqpiXMeGjcoe8Ah48FAzUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Rob,
On 01/11/2017 03:34 PM, Rob Herring wrote:
> On Mon, Jan 9, 2017 at 11:57 AM, Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> wrote:
>> Rob,
>>
>> On 01/09/2017 11:50 AM, Rob Herring wrote:
>>>
>>> On Wed, Jan 04, 2017 at 02:55:34PM -0600, Dave Gerlach wrote:
>>>>
>>>> Add a generic power domain implementation, TI SCI PM Domains, that
>>>> will hook into the genpd framework and allow the TI SCI protocol to
>>>> control device power states.
>>>>
>>>> Also, provide macros representing each device index as understood
>>>> by TI SCI to be used in the device node power-domain references.
>>>> These are identifiers for the K2G devices managed by the PMMC.
>>>>
>>>> Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
>>>> Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
>>>> ---
>>>> v2->v3:
>>>> Update k2g_pds node docs to show it should be a child of pmmc
>>>> node.
>>>> In early versions a phandle was used to point to pmmc and docs
>>>> still
>>>> incorrectly showed this.
>>>>
>>>> .../devicetree/bindings/soc/ti/sci-pm-domain.txt | 59 ++++++++++++++
>>>> MAINTAINERS | 2 +
>>>> include/dt-bindings/genpd/k2g.h | 90
>>>> ++++++++++++++++++++++
>>>> 3 files changed, 151 insertions(+)
>>>> create mode 100644
>>>> Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>> create mode 100644 include/dt-bindings/genpd/k2g.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>> b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>> new file mode 100644
>>>> index 000000000000..4c9064e512cb
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>> @@ -0,0 +1,59 @@
>>>> +Texas Instruments TI-SCI Generic Power Domain
>>>> +---------------------------------------------
>>>> +
>>>> +Some TI SoCs contain a system controller (like the PMMC, etc...) that is
>>>> +responsible for controlling the state of the IPs that are present.
>>>> +Communication between the host processor running an OS and the system
>>>> +controller happens through a protocol known as TI-SCI [1]. This pm
>>>> domain
>>>> +implementation plugs into the generic pm domain framework and makes use
>>>> of
>>>> +the TI SCI protocol power on and off each device when needed.
>>>> +
>>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>>> +
>>>> +PM Domain Node
>>>> +==============
>>>> +The PM domain node represents the global PM domain managed by the PMMC,
>>>> +which in this case is the single implementation as documented by the
>>>> generic
>>>> +PM domain bindings in
>>>> Documentation/devicetree/bindings/power/power_domain.txt.
>>>> +Because this relies on the TI SCI protocol to communicate with the PMMC
>>>> it
>>>> +must be a child of the pmmc node.
>>>> +
>>>> +Required Properties:
>>>> +--------------------
>>>> +- compatible: should be "ti,sci-pm-domain"
>>>> +- #power-domain-cells: Must be 0.
>>>> +
>>>> +Example (K2G):
>>>> +-------------
>>>> + pmmc: pmmc {
>>>> + compatible = "ti,k2g-sci";
>>>> + ...
>>>> +
>>>> + k2g_pds: k2g_pds {
>>>> + compatible = "ti,sci-pm-domain";
>>>> + #power-domain-cells = <0>;
>>>> + };
>>>> + };
>>>> +
>>>> +PM Domain Consumers
>>>> +===================
>>>> +Hardware blocks that require SCI control over their state must provide
>>>> +a reference to the sci-pm-domain they are part of and a unique device
>>>> +specific ID that identifies the device.
>>>> +
>>>> +Required Properties:
>>>> +--------------------
>>>> +- power-domains: phandle pointing to the corresponding PM domain node.
>>>> +- ti,sci-id: index representing the device id to be passed oevr SCI to
>>>> + be used for device control.
>>>
>>>
>>> As I've already stated before, this goes in power-domain cells. When you
>>> have a single thing (i.e. node) that controls multiple things, then you
>>> you need to specify the ID for each of them in phandle args. This is how
>>> irqs, gpio, clocks, *everything* in DT works.
>>
>>
>> You think the reasoning for doing it this way provided by both Ulf and
>> myself on v2 [1] is not valid then?
>>
>> From Ulf:
>>
>> To me, the TI SCI ID, is similar to a "conid" for any another "device
>> resource" (like clock, pinctrl, regulator etc) which we can describe
>> in DT and assign to a device node. The only difference here, is that
>> we don't have common API to fetch the resource (like clk_get(),
>> regulator_get()), but instead we fetches the device's resource from
>> SoC specific code, via genpd's device ->attach() callback.
>
> Sorry, but that sounds like a kernel problem to me and has nothing to
> do with DT bindings.
>
>> From me:
>>
>> Yes, you've pretty much hit it on the head. It is not an index into a list
>> of genpds but rather identifies the device *within* a single genpd. It is a
>> property specific to each device that resides in a ti-sci-genpd, not a
>> mapping describing which genpd the device belongs to. The generic power
>> domain binding is concerned with mapping the device to a specific genpd,
>> which is does fine for us, but we have a sub mapping for devices that exist
>> inside a genpd which, we must describe as well, hence the ti,sci-id.
>>
>>
>> So to summarize, the genpd framework does interpret the phandle arg as an
>> index into multiple genpds, just as you've said other frameworks do, but
>> this is not what I am trying to do, we have multiple devices within this
>> *single* genpd, hence the need for the ti,sci-id property.
>
> Fix the genpd framework rather than work around it in DT.
I still disagree that this has nothing to do with DT bindings, as the
current DT binding represents something different already. I am trying
to extend it to give me additional information needed for our platforms.
Are you saying that we should break what the current DT binding already
represents to mean something else?
Regards,
Dave
>
> Rob
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Benjamin Herrenschmidt @ 2017-01-12 15:27 UTC (permalink / raw)
To: Greg KH, Cyril Bur
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
joel-U3u1mxZcP9KHXe+LvDLADg, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
andrew-zrmu5oMJ5Fs, xow-hpIqsD4AKlfQT0dZR+AlfA,
jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <20170112103038.GA19239-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Thu, 2017-01-12 at 11:30 +0100, Greg KH wrote:
> > > And don't call access_ok(), it's racy and no driver should ever do that.
> > >
> >
> > Oh, duly noted. I'll be sure to check out how and why. Perhaps it
> > would be wise that no driver actually do that, I'm quite sure I used
> > other drivers as examples of best practice.
>
> You did? Please point me at that code so we can fix them up properly.
> Cargo-cult coding is not a good thing, but it happens, so if we can at
> least provide clean code to fixate on, it's good overall for everyone.
How so ? I mean, access_ok followed by __get/__put_user is still a
classic, what's wrong with it ?
The test performed by access_ok tests whether the address hits the
kernel/userspace limit, that limit doesn't change afaik, so there is no
race there, and avoids repeatedly testing it for series of subsequent
__get_user or __put_user.
What's wrong with them ?
Cheers,
Ben.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2][UPDATE] of: base: add support to get the number of cache levels
From: Sudeep Holla @ 2017-01-12 15:28 UTC (permalink / raw)
To: Rob Herring
Cc: Sudeep Holla,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Catalin Marinas, Will Deacon,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tan Xiaojun,
Mark Rutland
In-Reply-To: <CAL_Jsq+wawaXeFPnmYw++YMiMzMm+9nSn498Dj7KsGzkfndfog-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 12/01/17 13:24, Rob Herring wrote:
> On Tue, Jan 10, 2017 at 6:00 AM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
>> It is useful to have helper function just to get the number of cache
>> levels for a given logical cpu. This patch adds the support for the
>> same.
>>
>> It will be used on ARM64 platform where the device tree provides the
>> information for the additional non-architected/transparent/external
>> last level caches that are not integrated with the processors.
>>
>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>> ---
>> drivers/of/base.c | 23 +++++++++++++++++++++++
>> include/linux/of.h | 1 +
>> 2 files changed, 24 insertions(+)
>>
[...]
>> +int of_count_cache_levels(unsigned int cpu)
>> +{
>> + int level = 0;
>> + struct device_node *np = of_cpu_device_node_get(cpu);
>> +
>> + while (np) {
>> + level++;
>
> This will return 1 if you have a cpu node and no cache nodes. Are you
> assuming the cpu has a cache?
>
Ah right, that's completely wrong assumption.
> Perhaps you should just find the last level cache node and then just
> read "cache-level".
>
Yes, sounds better. I will update accordingly. Thanks for the suggestion.
--
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 0/3] Adding DT support for TI HECC module
From: Yegor Yefremov @ 2017-01-12 15:32 UTC (permalink / raw)
To: Tony Lindgren
Cc: linux-can-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
Andrey Skvortsov, hs-ynQEQJNshbs, Marc Kleine-Budde
In-Reply-To: <CAGm1_kvNcmpayN-=mMmkCn1=wXaykhENUrHK2-MVmZLC+Cca0Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Jan 12, 2017 at 8:59 AM, Yegor Yefremov
<yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> On Thu, Jan 12, 2017 at 1:47 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> * Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> [170111 13:52]:
>>> On Wed, Jan 11, 2017 at 3:05 PM, <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>> > From: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>> >
>>> > This is an attempt to revive DT support for TI HECC that was started in 2015.
>>> >
>>> > I haven't changed much because not all questions could be fully answered:
>>> >
>>> > * Should HECC use "am3505" as compatible?
>>>
>>> I mean "ti,am3505-hecc"
>>
>> Yeah it should use the device name for the driver.
I've looked at drivers/net/ethernet/ti/davinci_emac.c and there
"ti,am3517-emac" will be used. Should we stick to am3517? am3505 won't
be used in any dts[i] file.
>>> > * What should be done to the offsets (ti,scc-ram-offset, ti,hecc-ram-offset, ti,mbx-offset)?
>>
>> The devicetree maintainers need to ack the binding doc. Maybe
>> send that as a first patch?
>
> The question is whether to place these settings into dtsi (as it was
> done in the original patch) or in the driver itself.
Yegor
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Benjamin Herrenschmidt @ 2017-01-12 15:35 UTC (permalink / raw)
To: Cyril Bur, Greg KH
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
joel-U3u1mxZcP9KHXe+LvDLADg, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
andrew-zrmu5oMJ5Fs, xow-hpIqsD4AKlfQT0dZR+AlfA,
jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <1484216163.11416.8.camel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Thu, 2017-01-12 at 21:16 +1100, Cyril Bur wrote:
> My aim here was to only have one process playing with the LPC mapping
> registers at a time.
>
> > Again, use UIO, it will save you from yourself...
> >
>
> Thank-you! This is the first I've heard of UIO and I'll investigate
> furthur!
Greg, I don't think UIO is the answer here either. Note, this isn't an
exploit so much as root shooting itself in the foot as this driver
should never be accessed by anybody but root, but see below.
This is a BMC, ie, the system controller of a x86 or POWER based
system.
The LPC controller controls the LPC bus which is mastered by the "host"
(ie. the x86 or PPC) and acts as a slave on the BMC side.
It has a bunch of registers that need to be configured in more/less
system specific ways by the BMC, but more so, it has a pair of
registers that allow "mapping" of a region of the BMC physical address
space into the host address space.
This is by definition dangerous to configure since it gives you a
window to any part of the BMC, kernel space, any IOs, etc... however it
needs to be configured by a userspace daemon which communicates with
the host via a mailbox in order to map either different portions of the
system flash controller address space or reserved memory.
So in fact it should be done by the kernel, not userspace.
What Cyril needs to do to make it more secure is:
- For random register accesses, white list what registers
specifically are allowed (and if necessary filter values). These
registers aren't dangerous from the BMC perspective and need to be set
appropriately for the host to operate correctly.
- For the mapping of the LPC FW space <-> BMC space, use ioctl's to
explicit establish the mapping to a portion of the flash (and nowhere
else) or one of the known reserved memory areas. IE, dont have
userspace just pass raw physical addresses through, but tell the kernel
driver what portion (offset/size) of what area (flash space or reserved
memory region) to configure the HW window for.
Cheers,
Ben.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Benjamin Herrenschmidt @ 2017-01-12 15:36 UTC (permalink / raw)
To: Greg KH, Cyril Bur
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
joel-U3u1mxZcP9KHXe+LvDLADg, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
andrew-zrmu5oMJ5Fs, xow-hpIqsD4AKlfQT0dZR+AlfA,
jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <20170112074312.GA23943-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Thu, 2017-01-12 at 08:43 +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > This driver exposes a reserved chunk of BMC ram to userspace as
> > well as
> > an ioctl interface to control the BMC<->HOST mapping of the LPC
> > bus.
> > This allows for a communication channel between the BMC and the
> > host
>
> Why not make this a UIO driver? Why does it have to be a character
> device?
See my other email (looks like I'm getting things in reverse order
today ;-), basically it should just be some ioctl's, but what they do
should really be done by the kernel as it controls external access to
part of the chip physical address space.
Cheers,
Ben.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v29 9/9] Documentation: dt: chosen properties for arm64 kdump
From: Mark Rutland @ 2017-01-12 15:39 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, james.morse-5wv7dgnIgG8,
geoff-wEGCiKHe2LqWVfeAwA7xHQ,
bauerman-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
dyoung-H+wXaHxf7aLQT0dZR+AlfA,
kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161228043734.27535-1-takahiro.akashi-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Hi,
On Wed, Dec 28, 2016 at 01:37:34PM +0900, AKASHI Takahiro wrote:
> From: James Morse <james.morse-5wv7dgnIgG8@public.gmane.org>
>
> Add documentation for
> linux,crashkernel-base and crashkernel-size,
> linux,usable-memory-range
> linux,elfcorehdr
> used by arm64 kdump to decribe the kdump reserved area, and
> the elfcorehdr's location within it.
>
> Signed-off-by: James Morse <james.morse-5wv7dgnIgG8@public.gmane.org>
> [takahiro.akashi-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org: added "linux,crashkernel-base" and "-size" ]
> Signed-off-by: AKASHI Takahiro <takahiro.akashi-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> ---
> Documentation/devicetree/bindings/chosen.txt | 50 ++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> index 6ae9d82d4c37..7b115165e9ec 100644
> --- a/Documentation/devicetree/bindings/chosen.txt
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -52,3 +52,53 @@ This property is set (currently only on PowerPC, and only needed on
> book3e) by some versions of kexec-tools to tell the new kernel that it
> is being booted by kexec, as the booting environment may differ (e.g.
> a different secondary CPU release mechanism)
> +
> +linux,crashkernel-base
> +linux,crashkernel-size
> +----------------------
> +
> +These properties (currently used on PowerPC and arm64) indicates
> +the base address and the size, respectively, of the reserved memory
> +range for crash dump kernel.
>From this description, it's not clear to me what the (expected)
consumers of this property are, nor what is expected to provide it.
In previous rounds of review, I had assumed that this was used to
describe a preference to the first kernel as to what region of memory
should be used for a subsequent kdump kernel. Looking around, I'm not
sure if I was correct in that assessment.
I see that arch/powerpc seems to consume this property to configure
crashk_res, but it also rewrites it based on crashk_res, presumably for
the benefit of userspace. It's not clear to me how on powerpc the kdump
kernel knows its memory range -- is more DT modification done in the
kernel and/or userspace?
I disagree with modifying this property to expose it to userspace. For
arm64 we should either ensure that /proc/iomem is consistently usable
(and have userspace consistently use it), or we should expose a new file
specifically to expose this information.
Further, I do not think we need this property. It makes more sense to me
for the preference of a a region to be described to the *first* kernel
using the command line consistently.
So I think we should drop this property, and not use it on arm64. Please
document this as powerpc only.
> +e.g.
> +
> +/ {
> + chosen {
> + linux,crashkernel-base = <0x9 0xf0000000>;
> + linux,crashkernel-size = <0x0 0x10000000>;
> + };
> +};
> +
> +linux,usable-memory-range
> +-------------------------
> +
> +This property (currently used only on arm64) holds the memory range,
> +the base address and the size, which can be used as system ram on
> +the *current* kernel. Note that, if this property is present, any memory
> +regions under "memory" nodes in DT blob or ones marked as "conventional
> +memory" in EFI memory map should be ignored.
Could you please replace this with:
This property (arm64 only) holds a base address and size, describing a
limited region in which memory may be considered available for use by
the kernel. Memory outside of this range is not available for use.
This property describes a limitation: memory within this range is only
valid when also described through another mechanism that the kernel
would otherwise use to determine available memory (e.g. memory nodes
or the EFI memory map). Valid memory may be sparse within the range.
To clarify why we need this, given by above comments w.r.r. the
linux,crashkernel-* properties:
* It preserves all the original memory map information (e.g. memory
nodes and/or EFI memory map)
* It works consistently, regardless of how the kdump kernel would
otherwise determine which memory to use (memory nodes, EFI, etc).
* It will be simply and reliable for an in-kernel purgatory to insert,
if we need a kexec_file_load()-based kdump (e.g. without requiring
memory map rewrites, and avoiding clashes with command line
parameters). For a first kernel, this is not as big a concern.
> +linux,elfcorehdr
> +----------------
> +
> +This property (currently used only on arm64) holds the memory range,
> +the address and the size, of the elf core header which mainly describes
> +the panicked kernel's memory layout as PT_LOAD segments of elf format.
> +e.g.
> +
> +/ {
> + chosen {
> + linux,elfcorehdr = <0x9 0xfffff000 0x0 0x800>;
> + };
> +};
This property looks fine to me.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 0/3] Adding DT support for TI HECC module
From: Tony Lindgren @ 2017-01-12 15:41 UTC (permalink / raw)
To: Yegor Yefremov
Cc: linux-can-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
Andrey Skvortsov, hs-ynQEQJNshbs, Marc Kleine-Budde
In-Reply-To: <CAGm1_kvNcmpayN-=mMmkCn1=wXaykhENUrHK2-MVmZLC+Cca0Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
* Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> [170112 00:00]:
> On Thu, Jan 12, 2017 at 1:47 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> [170111 13:52]:
> >> On Wed, Jan 11, 2017 at 3:05 PM, <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> >> > From: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> >> >
> >> > This is an attempt to revive DT support for TI HECC that was started in 2015.
> >> >
> >> > I haven't changed much because not all questions could be fully answered:
> >> >
> >> > * Should HECC use "am3505" as compatible?
> >>
> >> I mean "ti,am3505-hecc"
> >
> > Yeah it should use the device name for the driver.
> >
> >> > * What should be done to the offsets (ti,scc-ram-offset, ti,hecc-ram-offset, ti,mbx-offset)?
> >
> > The devicetree maintainers need to ack the binding doc. Maybe
> > send that as a first patch?
>
> The question is whether to place these settings into dtsi (as it was
> done in the original patch) or in the driver itself.
Well where are they on the SoC? Each driver should only access registers
that belong to the driver module.
If the ti,scc-ram-offset and ti,hecc-ram-offset are not within the ECC
driver module, probably you should use a separate driver for them
such as drivers/misc/sram.c.
Also, sounds like the ti,mbx-offset should just be using the mailbox
framework like remoteproc is doing with include/linux/omap-mailbox.h?
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 0/3] Adding DT support for TI HECC module
From: Tony Lindgren @ 2017-01-12 15:44 UTC (permalink / raw)
To: Yegor Yefremov
Cc: linux-can-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
Andrey Skvortsov, hs-ynQEQJNshbs, Marc Kleine-Budde
In-Reply-To: <CAGm1_ksOZ591TQHVo5u0MHP_H5fzPX3ip5Jf3eunfT2OOW7fZw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
* Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> [170112 07:33]:
> On Thu, Jan 12, 2017 at 8:59 AM, Yegor Yefremov
> <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> > On Thu, Jan 12, 2017 at 1:47 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> >> * Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> [170111 13:52]:
> >>> On Wed, Jan 11, 2017 at 3:05 PM, <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> >>> > From: Yegor Yefremov <yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> >>> >
> >>> > This is an attempt to revive DT support for TI HECC that was started in 2015.
> >>> >
> >>> > I haven't changed much because not all questions could be fully answered:
> >>> >
> >>> > * Should HECC use "am3505" as compatible?
> >>>
> >>> I mean "ti,am3505-hecc"
> >>
> >> Yeah it should use the device name for the driver.
>
> I've looked at drivers/net/ethernet/ti/davinci_emac.c and there
> "ti,am3517-emac" will be used. Should we stick to am3517? am3505 won't
> be used in any dts[i] file.
If they are the same, then usually the earliest hardware naming can be
used for later versions too. I don't know which was first am3517 or
3505. If you think "ti,am3517-hecc" makes sense I'm fine with that.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] pinctrl: core: Fix regression caused by delayed work for hogs
From: Tony Lindgren @ 2017-01-12 15:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linus Walleij, Haojian Zhuang, Masahiro Yamada, Grygorii Strashko,
Nishanth Menon, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-omap@vger.kernel.org, Gary Bisson, Linux-Renesas
In-Reply-To: <CAMuHMdXGaDoBbNGkS8oYRiNUNbtUNand6qkNY-s4QNSt5Fj0ZQ@mail.gmail.com>
* Geert Uytterhoeven <geert@linux-m68k.org> [170112 00:44]:
> Hi Tony,
>
> On Wed, Jan 11, 2017 at 11:13 PM, Tony Lindgren <tony@atomide.com> wrote:
> > Commit df61b366af26 ("pinctrl: core: Use delayed work for hogs") caused a
> > regression at least with sh-pfc that is also a GPIO controller as
> > noted by Geert Uytterhoeven <geert@linux-m68k.org>.
> >
> > As the original pinctrl_register() has issues calling pin controller
> > driver functions early before the controller has finished registering,
> > we can't just revert commit df61b366af26. That would break the drivers
> > using GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS.
> >
> > So let's fix the issue with the following steps as a single patch:
> >
> > 1. Revert the late_init parts of commit df61b366af26.
> >
> > The late_init clearly won't work and we have to just give up
> > on fixing pinctrl_register() for GENERIC_PINCTRL_GROUPS and
> > GENERIC_PINMUX_FUNCTIONS.
> >
> > 2. Split pinctrl_register() into two parts
> >
> > By splitting pinctrl_register() into pinctrl_init_controller()
> > and pinctrl_create_and_start() we have better control over when
> > it's safe to call pinctrl_create().
> >
> > 3. Introduce a new pinctrl_register_and_init() function
> >
> > As suggested by Linus Walleij <linus.walleij@linaro.org>, we
> > can just introduce a new function for the controllers that need
> > pinctrl_create() called later.
> >
> > 4. Convert the four known problem cases to use new function
> >
> > Let's convert pinctrl-imx, pinctrl-single, sh-pfc and ti-iodelay
> > to use the new function to fix the issues. The rest of the drivers
> > can be converted later. Let's also update Documentation/pinctrl.txt
> > accordingly because of the known issues with pinctrl_register().
> >
> > Fixes: df61b366af26 ("pinctrl: core: Use delayed work for hogs")
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Gary Bisson <gary.bisson@boundarydevices.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
>
> Thanks, this fixes r8a7740/armadillo.
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
OK thanks for testing, good to hear it works for you.
Thanks,
Tony
^ 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