* 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 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: [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 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: 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
* [PATCH 6/6] fbdev/ssd1307fb: add support to enable VBAT
From: Jyri Sarha @ 2017-01-12 11:16 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: tomi.valkeinen-l0cyMroinI0,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Jyri Sarha
In-Reply-To: <cover.1484219137.git.jsarha-l0cyMroinI0@public.gmane.org>
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
--
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
* [PATCH 5/6] fbdev/ssd1307fb: clear screen in probe
From: Jyri Sarha @ 2017-01-12 11:16 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: tomi.valkeinen-l0cyMroinI0,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Jyri Sarha
In-Reply-To: <cover.1484219137.git.jsarha-l0cyMroinI0@public.gmane.org>
From: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
SSD1306 does not clear the panel's framebuffer automatically, even if a
HW reset happens, so we need to do that at probe time before enabling
the panel.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
drivers/video/fbdev/ssd1307fb.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 616a6a3..5c87ae4 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -441,9 +441,8 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
- /* Clear the screen if we could not give reset at probe time */
- if (!par->reset)
- ssd1307fb_update_display(par);
+ /* Clear the screen */
+ ssd1307fb_update_display(par);
/* Turn on the display */
ret = ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
--
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
* [PATCH 4/6] fbdev/ssd1307fb: add support to enable VBAT
From: Jyri Sarha @ 2017-01-12 11:16 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: tomi.valkeinen-l0cyMroinI0,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Jyri Sarha
In-Reply-To: <cover.1484219137.git.jsarha-l0cyMroinI0@public.gmane.org>
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>
Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
drivers/video/fbdev/ssd1307fb.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 89372af..616a6a3 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -16,6 +16,7 @@
#include <linux/of_gpio.h>
#include <linux/pwm.h>
#include <linux/uaccess.h>
+#include <linux/regulator/consumer.h>
#define SSD1307FB_DATA 0x40
#define SSD1307FB_COMMAND 0x80
@@ -74,6 +75,7 @@ struct ssd1307fb_par {
struct pwm_device *pwm;
u32 pwm_period;
struct gpio_desc *reset;
+ struct regulator *vbat_reg;
u32 seg_remap;
u32 vcomh;
u32 width;
@@ -574,6 +576,14 @@ static int ssd1307fb_probe(struct i2c_client *client,
goto fb_alloc_error;
}
+ par->vbat_reg = devm_regulator_get_optional(&client->dev, "vbat");
+ if (IS_ERR(par->vbat_reg)) {
+ dev_err(&client->dev, "failed to get VBAT regulator: %ld\n",
+ PTR_ERR(par->vbat_reg));
+ ret = PTR_ERR(par->vbat_reg);
+ goto fb_alloc_error;
+ }
+
if (of_property_read_u32(node, "solomon,width", &par->width))
par->width = 96;
@@ -658,9 +668,15 @@ static int ssd1307fb_probe(struct i2c_client *client,
udelay(4);
}
+ ret = regulator_enable(par->vbat_reg);
+ if (ret) {
+ dev_err(&client->dev, "failed to enable VBAT: %d\n", ret);
+ goto reset_oled_error;
+ }
+
ret = ssd1307fb_init(par);
if (ret)
- goto reset_oled_error;
+ goto regulator_enable_error;
ret = register_framebuffer(info);
if (ret) {
@@ -693,6 +709,8 @@ static int ssd1307fb_probe(struct i2c_client *client,
pwm_disable(par->pwm);
pwm_put(par->pwm);
};
+regulator_enable_error:
+ regulator_disable(par->vbat_reg);
reset_oled_error:
fb_deferred_io_cleanup(info);
fb_alloc_error:
--
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
* [PATCH 3/6] fbdev: ssd1307fb: Make reset gpio devicetree property optional
From: Jyri Sarha @ 2017-01-12 11:16 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: tomi.valkeinen-l0cyMroinI0,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Jyri Sarha
In-Reply-To: <cover.1484219137.git.jsarha-l0cyMroinI0@public.gmane.org>
Make reset gpio devicetree property optional. Depending on the board
designing there may not be a dedicated gpio for resetting the
display. Without a proper reset there may be some junk in the display
memory at probe time, so in such a case the display memory is cleared
before turning it on. The devicetree binding document is also updated.
Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
.../devicetree/bindings/display/ssd1307fb.txt | 4 ++--
drivers/video/fbdev/ssd1307fb.c | 19 +++++++++++++------
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/ssd1307fb.txt b/Documentation/devicetree/bindings/display/ssd1307fb.txt
index 4aee67f..6617df6 100644
--- a/Documentation/devicetree/bindings/display/ssd1307fb.txt
+++ b/Documentation/devicetree/bindings/display/ssd1307fb.txt
@@ -8,14 +8,14 @@ Required properties:
0x3c or 0x3d
- pwm: Should contain the pwm to use according to the OF device tree PWM
specification [0]. Only required for the ssd1307.
- - reset-gpios: Should contain the GPIO used to reset the OLED display. See
- Documentation/devicetree/bindings/gpio/gpio.txt for details.
- solomon,height: Height in pixel of the screen driven by the controller
- solomon,width: Width in pixel of the screen driven by the controller
- solomon,page-offset: Offset of pages (band of 8 pixels) that the screen is
mapped to.
Optional properties:
+ - reset-gpios: The GPIO used to reset the OLED display, if available. See
+ Documentation/devicetree/bindings/gpio/gpio.txt for details.
- solomon,segment-no-remap: Display needs normal (non-inverted) data column
to segment mapping
- solomon,com-seq: Display uses sequential COM pin configuration
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 8ffaaee..89372af 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -439,6 +439,10 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
+ /* Clear the screen if we could not give reset at probe time */
+ if (!par->reset)
+ ssd1307fb_update_display(par);
+
/* Turn on the display */
ret = ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
if (ret < 0)
@@ -561,7 +565,8 @@ static int ssd1307fb_probe(struct i2c_client *client,
par->device_info = of_device_get_match_data(&client->dev);
- par->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
+ par->reset = devm_gpiod_get_optional(&client->dev, "reset",
+ GPIOD_OUT_LOW);
if (IS_ERR(par->reset)) {
dev_err(&client->dev, "failed to get reset gpio: %ld\n",
PTR_ERR(par->reset));
@@ -645,11 +650,13 @@ static int ssd1307fb_probe(struct i2c_client *client,
i2c_set_clientdata(client, info);
- /* Reset the screen */
- gpiod_set_value(par->reset, 0);
- udelay(4);
- gpiod_set_value(par->reset, 1);
- udelay(4);
+ if (par->reset) {
+ /* Reset the screen */
+ gpiod_set_value(par->reset, 0);
+ udelay(4);
+ gpiod_set_value(par->reset, 1);
+ udelay(4);
+ }
ret = ssd1307fb_init(par);
if (ret)
--
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
* [PATCH 2/6] fbdev: ssd1307fb: Remove reset-active-low from the DT binding document
From: Jyri Sarha @ 2017-01-12 11:16 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: tomi.valkeinen-l0cyMroinI0,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Jyri Sarha
In-Reply-To: <cover.1484219137.git.jsarha-l0cyMroinI0@public.gmane.org>
Remove reset-active-low from the devicetree binding document. The actual
implementation has never been there in the driver code and there is no
reason to add it because the gpiod API supports gpio flags, including
GPIO_ACTIVE_LOW, directly trough its own devicetree binding.
Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
Documentation/devicetree/bindings/display/ssd1307fb.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/ssd1307fb.txt b/Documentation/devicetree/bindings/display/ssd1307fb.txt
index eb31ed4..4aee67f 100644
--- a/Documentation/devicetree/bindings/display/ssd1307fb.txt
+++ b/Documentation/devicetree/bindings/display/ssd1307fb.txt
@@ -8,14 +8,14 @@ Required properties:
0x3c or 0x3d
- pwm: Should contain the pwm to use according to the OF device tree PWM
specification [0]. Only required for the ssd1307.
- - reset-gpios: Should contain the GPIO used to reset the OLED display
+ - reset-gpios: Should contain the GPIO used to reset the OLED display. See
+ Documentation/devicetree/bindings/gpio/gpio.txt for details.
- solomon,height: Height in pixel of the screen driven by the controller
- solomon,width: Width in pixel of the screen driven by the controller
- solomon,page-offset: Offset of pages (band of 8 pixels) that the screen is
mapped to.
Optional properties:
- - reset-active-low: Is the reset gpio is active on physical low?
- solomon,segment-no-remap: Display needs normal (non-inverted) data column
to segment mapping
- solomon,com-seq: Display uses sequential COM pin configuration
--
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
* [PATCH 1/6] fbdev: ssd1307fb: Start to use gpiod API for reset gpio
From: Jyri Sarha @ 2017-01-12 11:16 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: tomi.valkeinen-l0cyMroinI0,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Jyri Sarha
In-Reply-To: <cover.1484219137.git.jsarha-l0cyMroinI0@public.gmane.org>
Start to use gpiod API for reset gpio.
Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
drivers/video/fbdev/ssd1307fb.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 2925d5c..8ffaaee 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -73,7 +73,7 @@ struct ssd1307fb_par {
u32 prechargep2;
struct pwm_device *pwm;
u32 pwm_period;
- int reset;
+ struct gpio_desc *reset;
u32 seg_remap;
u32 vcomh;
u32 width;
@@ -561,10 +561,11 @@ static int ssd1307fb_probe(struct i2c_client *client,
par->device_info = of_device_get_match_data(&client->dev);
- par->reset = of_get_named_gpio(client->dev.of_node,
- "reset-gpios", 0);
- if (!gpio_is_valid(par->reset)) {
- ret = -EINVAL;
+ par->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(par->reset)) {
+ dev_err(&client->dev, "failed to get reset gpio: %ld\n",
+ PTR_ERR(par->reset));
+ ret = PTR_ERR(par->reset);
goto fb_alloc_error;
}
@@ -642,22 +643,12 @@ static int ssd1307fb_probe(struct i2c_client *client,
fb_deferred_io_init(info);
- ret = devm_gpio_request_one(&client->dev, par->reset,
- GPIOF_OUT_INIT_HIGH,
- "oled-reset");
- if (ret) {
- dev_err(&client->dev,
- "failed to request gpio %d: %d\n",
- par->reset, ret);
- goto reset_oled_error;
- }
-
i2c_set_clientdata(client, info);
/* Reset the screen */
- gpio_set_value(par->reset, 0);
+ gpiod_set_value(par->reset, 0);
udelay(4);
- gpio_set_value(par->reset, 1);
+ gpiod_set_value(par->reset, 1);
udelay(4);
ret = ssd1307fb_init(par);
--
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
* [PATCH 0/6] fbdev/ssd1307fb: Some changes and improvement
From: Jyri Sarha @ 2017-01-12 11:16 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
plagnioj-sclMFOaUSTBWk0Htik3J/w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: tomi.valkeinen-l0cyMroinI0,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Jyri Sarha
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(-)
--
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
* Re: [PATCH v2] Documentation: dt: reset: Revise typos in TI syscon reset example
From: Philipp Zabel @ 2017-01-12 10:54 UTC (permalink / raw)
To: Suman Anna
Cc: devicetree, linux-kernel, Andrew Davis, Rob Herring,
Santosh Shilimkar, linux-arm-kernel
In-Reply-To: <20170112012217.18115-1-s-anna@ti.com>
Am Mittwoch, den 11.01.2017, 19:22 -0600 schrieb Suman Anna:
> Fix couple of typos in the example given in the TI syscon reset
> binding. The ti,reset-bits used for DSP0 are corrected to match
> the values that will be used in the actual DT node.
>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: Address Rob Herring's comment to change the reset node name
> from "psc-reset" to "reset-controller"
I've applied the patch, thank you.
regards
Philipp
^ permalink raw reply
* Re: [PATCH 1/2] mmc: mediatek: Use data tune for CMD line tune
From: Ulf Hansson @ 2017-01-12 10:39 UTC (permalink / raw)
To: Yong Mao
Cc: Mark Rutland, devicetree@vger.kernel.org, Nicolas Boichat,
srv_heupstream, Javier Martinez Canillas, Catalin Marinas,
Will Deacon, Douglas Anderson, linux-kernel@vger.kernel.org,
Chunfeng Yun, Rob Herring, linux-mediatek,
linux-arm-kernel@lists.infradead.org, Philipp Zabel,
Greg Kroah-Hartman, Matthias Brugger, linux-mmc@vger.kernel.org,
Eddie Huang, Chaotian Jing
In-Reply-To: <1484215490-7494-2-git-send-email-yong.mao@mediatek.com>
On 12 January 2017 at 11:04, Yong Mao <yong.mao@mediatek.com> wrote:
> From: yong mao <yong.mao@mediatek.com>
>
> CMD response CRC error may cause cannot boot up
> Change to use data tune for CMD line
> Separate cmd internal delay for HS200/HS400 mode
Please try to work a little bit on improving the change log. Moreover
as this is a fix for a regression (it seems like so?), please try to
make that clear.
>
> Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
> arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 3 +
Changes to the DTS files should be a separate change. Please split it
into its own patch.
> drivers/mmc/host/mtk-sd.c | 169 +++++++++++++++++++++++----
> 2 files changed, 149 insertions(+), 23 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> index 0ecaad4..29c3100 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> @@ -134,6 +134,9 @@
> bus-width = <8>;
> max-frequency = <50000000>;
> cap-mmc-highspeed;
> + hs200-cmd-int-delay = <26>;
> + hs400-cmd-int-delay = <14>;
> + cmd-resp-sel = <0>; /* 0: rising, 1: falling */
> vmmc-supply = <&mt6397_vemc_3v3_reg>;
> vqmmc-supply = <&mt6397_vio18_reg>;
> non-removable;
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 80ba034..93eb395 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -75,6 +75,7 @@
> #define MSDC_PATCH_BIT1 0xb4
> #define MSDC_PAD_TUNE 0xec
> #define PAD_DS_TUNE 0x188
> +#define PAD_CMD_TUNE 0x18c
> #define EMMC50_CFG0 0x208
>
> /*--------------------------------------------------------------------------*/
> @@ -210,12 +211,17 @@
> #define MSDC_PATCH_BIT_SPCPUSH (0x1 << 29) /* RW */
> #define MSDC_PATCH_BIT_DECRCTMO (0x1 << 30) /* RW */
>
> -#define MSDC_PAD_TUNE_DATRRDLY (0x1f << 8) /* RW */
> -#define MSDC_PAD_TUNE_CMDRDLY (0x1f << 16) /* RW */
> +#define MSDC_PAD_TUNE_DATWRDLY (0x1f << 0) /* RW */
> +#define MSDC_PAD_TUNE_DATRRDLY (0x1f << 8) /* RW */
> +#define MSDC_PAD_TUNE_CMDRDLY (0x1f << 16) /* RW */
> +#define MSDC_PAD_TUNE_CMDRRDLY (0x1f << 22) /* RW */
Is there a white space change somewhere here? I don't see any changes
to MSDC_PAD_TUNE_DATRRDLY and MSDC_PAD_TUNE_CMDRDLY.
> +#define MSDC_PAD_TUNE_CLKTDLY (0x1f << 27) /* RW */
>
> -#define PAD_DS_TUNE_DLY1 (0x1f << 2) /* RW */
> -#define PAD_DS_TUNE_DLY2 (0x1f << 7) /* RW */
> -#define PAD_DS_TUNE_DLY3 (0x1f << 12) /* RW */
> +#define PAD_DS_TUNE_DLY1 (0x1f << 2) /* RW */
> +#define PAD_DS_TUNE_DLY2 (0x1f << 7) /* RW */
> +#define PAD_DS_TUNE_DLY3 (0x1f << 12) /* RW */
> +
Ditto.
> +#define PAD_CMD_TUNE_RX_DLY3 (0x1f << 1) /* RW */
>
> #define EMMC50_CFG_PADCMD_LATCHCK (0x1 << 0) /* RW */
> #define EMMC50_CFG_CRCSTS_EDGE (0x1 << 3) /* RW */
> @@ -236,7 +242,9 @@
> #define CMD_TIMEOUT (HZ/10 * 5) /* 100ms x5 */
> #define DAT_TIMEOUT (HZ * 5) /* 1000ms x5 */
>
> -#define PAD_DELAY_MAX 32 /* PAD delay cells */
> +#define PAD_DELAY_MAX 32 /* PAD delay cells */
Ditto.
> +#define ENOUGH_MARGIN_MIN 12 /* Enough Margin */
> +#define PREFER_START_POS_MAX 4 /* Prefer start position */
> /*--------------------------------------------------------------------------*/
> /* Descriptor Structure */
> /*--------------------------------------------------------------------------*/
> @@ -284,12 +292,14 @@ struct msdc_save_para {
> u32 patch_bit0;
> u32 patch_bit1;
> u32 pad_ds_tune;
> + u32 pad_cmd_tune;
> u32 emmc50_cfg0;
> };
>
> struct msdc_tune_para {
> u32 iocon;
> u32 pad_tune;
> + u32 pad_cmd_tune;
> };
>
> struct msdc_delay_phase {
> @@ -331,6 +341,9 @@ struct msdc_host {
> unsigned char timing;
> bool vqmmc_enabled;
> u32 hs400_ds_delay;
> + u32 hs200_cmd_int_delay; /* cmd internal delay for HS200/SDR104 */
> + u32 hs400_cmd_int_delay; /* cmd internal delay for HS400 */
> + u32 hs200_cmd_resp_sel; /* cmd response sample selection */
> bool hs400_mode; /* current eMMC will run at hs400 mode */
> struct msdc_save_para save_para; /* used when gate HCLK */
> struct msdc_tune_para def_tune_para; /* default tune setting */
> @@ -596,12 +609,21 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
> */
> if (host->sclk <= 52000000) {
> writel(host->def_tune_para.iocon, host->base + MSDC_IOCON);
> - writel(host->def_tune_para.pad_tune, host->base + MSDC_PAD_TUNE);
> + writel(host->def_tune_para.pad_tune,
> + host->base + MSDC_PAD_TUNE);
Please don't change code just because you feel like doing it. This is
a completely unessarry change and it makes it harder for me to review.
Can you go thorugh the complete patch and make sure to undo all
similar changes, there are more of them.
> } else {
> - writel(host->saved_tune_para.iocon, host->base + MSDC_IOCON);
> - writel(host->saved_tune_para.pad_tune, host->base + MSDC_PAD_TUNE);
> + writel(host->saved_tune_para.iocon,
> + host->base + MSDC_IOCON);
> + writel(host->saved_tune_para.pad_tune,
> + host->base + MSDC_PAD_TUNE);
> + writel(host->saved_tune_para.pad_cmd_tune,
> + host->base + PAD_CMD_TUNE);
> }
>
> + if (timing == MMC_TIMING_MMC_HS400)
> + sdr_set_field(host->base + PAD_CMD_TUNE,
> + MSDC_PAD_TUNE_CMDRRDLY,
> + host->hs400_cmd_int_delay);
> dev_dbg(host->dev, "sclk: %d, timing: %d\n", host->sclk, timing);
> }
>
> @@ -1302,7 +1324,8 @@ static struct msdc_delay_phase get_best_delay(struct msdc_host *host, u32 delay)
> len_final = len;
> }
> start += len ? len : 1;
> - if (len >= 8 && start_final < 4)
> + if (len >= ENOUGH_MARGIN_MIN &&
> + start_final < PREFER_START_POS_MAX)
> break;
> }
>
> @@ -1325,48 +1348,128 @@ static int msdc_tune_response(struct mmc_host *mmc, u32 opcode)
> struct msdc_host *host = mmc_priv(mmc);
> u32 rise_delay = 0, fall_delay = 0;
> struct msdc_delay_phase final_rise_delay, final_fall_delay = { 0,};
> + struct msdc_delay_phase internal_delay_phase;
> u8 final_delay, final_maxlen;
> + u32 internal_delay = 0;
> int cmd_err;
> - int i;
> + int i, j;
>
> + if (mmc->ios.timing == MMC_TIMING_MMC_HS200 ||
> + mmc->ios.timing == MMC_TIMING_UHS_SDR104)
> + sdr_set_field(host->base + MSDC_PAD_TUNE,
> + MSDC_PAD_TUNE_CMDRRDLY,
> + host->hs200_cmd_int_delay);
> sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL);
> for (i = 0 ; i < PAD_DELAY_MAX; i++) {
> sdr_set_field(host->base + MSDC_PAD_TUNE,
> MSDC_PAD_TUNE_CMDRDLY, i);
> - mmc_send_tuning(mmc, opcode, &cmd_err);
> - if (!cmd_err)
> - rise_delay |= (1 << i);
> + for (j = 0; j < 3; j++) {
Any reason to why looping three times makes sense? Maybe add a comment?
> + mmc_send_tuning(mmc, opcode, &cmd_err);
> + if (!cmd_err) {
> + rise_delay |= (1 << i);
> + } else {
> + rise_delay &= ~(1 << i);
> + break;
> + }
> + }
> }
> final_rise_delay = get_best_delay(host, rise_delay);
> /* if rising edge has enough margin, then do not scan falling edge */
> - if (final_rise_delay.maxlen >= 10 ||
> - (final_rise_delay.start == 0 && final_rise_delay.maxlen >= 4))
> + if (final_rise_delay.maxlen >= ENOUGH_MARGIN_MIN &&
> + final_rise_delay.start < PREFER_START_POS_MAX)
This looks like clean-ups, as you are converting from magic numbers to
defines. Please make this kind of changes separately.
> goto skip_fall;
>
> sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL);
> for (i = 0; i < PAD_DELAY_MAX; i++) {
> sdr_set_field(host->base + MSDC_PAD_TUNE,
> MSDC_PAD_TUNE_CMDRDLY, i);
> - mmc_send_tuning(mmc, opcode, &cmd_err);
> - if (!cmd_err)
> - fall_delay |= (1 << i);
> + for (j = 0; j < 3; j++) {
3?
> + mmc_send_tuning(mmc, opcode, &cmd_err);
> + if (!cmd_err) {
> + fall_delay |= (1 << i);
> + } else {
> + fall_delay &= ~(1 << i);
> + break;
> + };
> + }
> }
> final_fall_delay = get_best_delay(host, fall_delay);
>
> skip_fall:
> final_maxlen = max(final_rise_delay.maxlen, final_fall_delay.maxlen);
> + if (final_fall_delay.maxlen >= ENOUGH_MARGIN_MIN &&
> + final_fall_delay.start < PREFER_START_POS_MAX)
> + final_maxlen = final_fall_delay.maxlen;
> if (final_maxlen == final_rise_delay.maxlen) {
> sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL);
> - sdr_set_field(host->base + MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRDLY,
> + sdr_set_field(host->base + MSDC_PAD_TUNE,
> + MSDC_PAD_TUNE_CMDRDLY,
> final_rise_delay.final_phase);
> final_delay = final_rise_delay.final_phase;
> } else {
> sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL);
> - sdr_set_field(host->base + MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRDLY,
> + sdr_set_field(host->base + MSDC_PAD_TUNE,
> + MSDC_PAD_TUNE_CMDRDLY,
> final_fall_delay.final_phase);
> final_delay = final_fall_delay.final_phase;
> }
> + if (host->hs200_cmd_int_delay)
> + goto skip_internal;
>
> + for (i = 0; i < PAD_DELAY_MAX; i++) {
> + sdr_set_field(host->base + MSDC_PAD_TUNE,
> + MSDC_PAD_TUNE_CMDRRDLY, i);
> + mmc_send_tuning(mmc, opcode, &cmd_err);
> + if (!cmd_err)
> + internal_delay |= (1 << i);
> + }
> + dev_info(host->dev, "Final internal delay: 0x%x\n", internal_delay);
I don't think dev_info() is what you want, right? Perhaps dev_dbg(),
anything at all.
> + internal_delay_phase = get_best_delay(host, internal_delay);
> + sdr_set_field(host->base + MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY,
> + internal_delay_phase.final_phase);
> +skip_internal:
> + dev_info(host->dev, "Final cmd pad delay: %x\n", final_delay);
I don't think dev_info() is what you want, right? Perhaps dev_dbg(),
anything at all.
> + return final_delay == 0xff ? -EIO : 0;
> +}
> +
> +static int hs400_tune_response(struct mmc_host *mmc, u32 opcode)
> +{
> + struct msdc_host *host = mmc_priv(mmc);
> + u32 cmd_delay = 0;
> + struct msdc_delay_phase final_cmd_delay = { 0,};
> + u8 final_delay;
> + int cmd_err;
> + int i, j;
> +
> + /* select EMMC50 PAD CMD tune */
> + sdr_set_bits(host->base + PAD_CMD_TUNE, BIT(0));
> +
> + if (mmc->ios.timing == MMC_TIMING_MMC_HS200 ||
> + mmc->ios.timing == MMC_TIMING_UHS_SDR104)
> + sdr_set_field(host->base + MSDC_PAD_TUNE,
> + MSDC_PAD_TUNE_CMDRRDLY,
> + host->hs200_cmd_int_delay);
> + sdr_set_field(host->base + MSDC_IOCON, MSDC_IOCON_RSPL,
> + host->hs200_cmd_resp_sel);
> + for (i = 0 ; i < PAD_DELAY_MAX; i++) {
> + sdr_set_field(host->base + PAD_CMD_TUNE,
> + PAD_CMD_TUNE_RX_DLY3, i);
> + for (j = 0; j < 3; j++) {
3?
> + mmc_send_tuning(mmc, opcode, &cmd_err);
> + if (!cmd_err) {
> + cmd_delay |= (1 << i);
> + } else {
> + cmd_delay &= ~(1 << i);
> + break;
> + }
> + }
> + }
> + final_cmd_delay = get_best_delay(host, cmd_delay);
> + sdr_set_field(host->base + PAD_CMD_TUNE, PAD_CMD_TUNE_RX_DLY3,
> + final_cmd_delay.final_phase);
> + final_delay = final_cmd_delay.final_phase;
> +
> + dev_info(host->dev, "Final cmd pad delay: %x\n", final_delay);
dev_info() -> dev_dbg() or remove it.
> return final_delay == 0xff ? -EIO : 0;
> }
>
> @@ -1389,7 +1492,7 @@ static int msdc_tune_data(struct mmc_host *mmc, u32 opcode)
> }
> final_rise_delay = get_best_delay(host, rise_delay);
> /* if rising edge has enough margin, then do not scan falling edge */
> - if (final_rise_delay.maxlen >= 10 ||
> + if (final_rise_delay.maxlen >= ENOUGH_MARGIN_MIN ||
Clean up. Move to separate change.
> (final_rise_delay.start == 0 && final_rise_delay.maxlen >= 4))
> goto skip_fall;
>
> @@ -1422,6 +1525,7 @@ static int msdc_tune_data(struct mmc_host *mmc, u32 opcode)
> final_delay = final_fall_delay.final_phase;
> }
>
> + dev_info(host->dev, "Final data pad delay: %x\n", final_delay);
dev_info() -> dev_dbg() or remove it.
> return final_delay == 0xff ? -EIO : 0;
> }
>
> @@ -1430,10 +1534,13 @@ static int msdc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> struct msdc_host *host = mmc_priv(mmc);
> int ret;
>
> + if (host->hs400_mode)
> + ret = hs400_tune_response(mmc, opcode);
> + else
> ret = msdc_tune_response(mmc, opcode);
Because of the new else clause, seems like above needs an intendation.
> if (ret == -EIO) {
> dev_err(host->dev, "Tune response fail!\n");
> - return ret;
> + goto out;
Not needed, remove the label and this change.
> }
> if (host->hs400_mode == false) {
> ret = msdc_tune_data(mmc, opcode);
> @@ -1443,6 +1550,8 @@ static int msdc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>
> host->saved_tune_para.iocon = readl(host->base + MSDC_IOCON);
> host->saved_tune_para.pad_tune = readl(host->base + MSDC_PAD_TUNE);
> + host->saved_tune_para.pad_cmd_tune = readl(host->base + PAD_CMD_TUNE);
> +out:
> return ret;
> }
>
> @@ -1553,6 +1662,18 @@ static int msdc_drv_probe(struct platform_device *pdev)
> dev_dbg(&pdev->dev, "hs400-ds-delay: %x\n",
> host->hs400_ds_delay);
>
> + if (!of_property_read_u32(pdev->dev.of_node, "hs200-cmd-int-delay",
> + &host->hs200_cmd_int_delay))
> + dev_dbg(&pdev->dev, "host->hs200-cmd-int-delay: %x\n",
> + host->hs200_cmd_int_delay);
> + if (!of_property_read_u32(pdev->dev.of_node, "hs400-cmd-int-delay",
> + &host->hs400_cmd_int_delay))
> + dev_dbg(&pdev->dev, "host->hs400-cmd-int-delay: %x\n",
> + host->hs400_cmd_int_delay);
> + if (!of_property_read_u32(pdev->dev.of_node, "cmd-resp-sel",
> + &host->hs200_cmd_resp_sel))
> + dev_dbg(&pdev->dev, "host->hs200_cmd-resp-sel: %x\n",
> + host->hs200_cmd_resp_sel);
I suggest you take the oppotunity to move the MTK DTS parsing into its
own function, include the existing parsing of the "hs400-ds-delay".
This improve the readablitity of the code.
> host->dev = &pdev->dev;
> host->mmc = mmc;
> host->src_clk_freq = clk_get_rate(host->src_clk);
> @@ -1663,6 +1784,7 @@ static void msdc_save_reg(struct msdc_host *host)
> host->save_para.patch_bit0 = readl(host->base + MSDC_PATCH_BIT);
> host->save_para.patch_bit1 = readl(host->base + MSDC_PATCH_BIT1);
> host->save_para.pad_ds_tune = readl(host->base + PAD_DS_TUNE);
> + host->save_para.pad_cmd_tune = readl(host->base + PAD_CMD_TUNE);
> host->save_para.emmc50_cfg0 = readl(host->base + EMMC50_CFG0);
> }
>
> @@ -1675,6 +1797,7 @@ static void msdc_restore_reg(struct msdc_host *host)
> writel(host->save_para.patch_bit0, host->base + MSDC_PATCH_BIT);
> writel(host->save_para.patch_bit1, host->base + MSDC_PATCH_BIT1);
> writel(host->save_para.pad_ds_tune, host->base + PAD_DS_TUNE);
> + writel(host->save_para.pad_cmd_tune, host->base + PAD_CMD_TUNE);
> writel(host->save_para.emmc50_cfg0, host->base + EMMC50_CFG0);
> }
>
> --
> 1.7.9.5
>
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Greg KH @ 2017-01-12 10:30 UTC (permalink / raw)
To: 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, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
xow-hpIqsD4AKlfQT0dZR+AlfA, jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <1484216163.11416.8.camel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Thu, Jan 12, 2017 at 09:16:03PM +1100, Cyril Bur wrote:
> On Thu, 2017-01-12 at 08:47 +0100, Greg KH wrote:
> > On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > > +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + if (!access_ok(VERIFY_WRITE, buf, count))
> > > + return -EFAULT;
> > > +
> > > + return -EPERM;
> > > +}
> > > +
> > > +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + if (!access_ok(VERIFY_READ, buf, count))
> > > + return -EFAULT;
> > > +
> > > + return -EPERM;
> > > +}
> >
>
> Hello Greg,
>
> > Those functions don't actually do anything, so why even include them?
> >
>
> Apologies, I should be more careful with what I send.
Hm, that implies you never tested what you sent, nor intended for us to
merge it, an odd thing for you to do :)
> > 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.
> > > +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > > + unsigned long param)
> > > +{
> > > + long rc;
> > > + struct lpc_mapping map;
> > > + struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > > + void __user *p = (void __user *)param;
> > > +
> > > + switch (cmd) {
> > > + case LPC_CTRL_IOCTL_SIZE:
> > > + return copy_to_user(p, &lpc_ctrl->size,
> > > + sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > > + case LPC_CTRL_IOCTL_MAP:
> > > + if (copy_from_user(&map, p, sizeof(map)))
> > > + return -EFAULT;
> > > +
> > > +
> > > + /*
> > > + * The top half of HICR7 is the MSB of the BMC address of the
> > > + * mapping.
> > > + * The bottom half of HICR7 is the MSB of the HOST LPC
> > > + * firmware space address of the mapping.
> > > + *
> > > + * The 1 bits in the top of half of HICR8 represent the bits
> > > + * (in the requested address) that should be ignored and
> > > + * replaced with those from the top half of HICR7.
> > > + * The 1 bits in the bottom half of HICR8 represent the bits
> > > + * (in the requested address) that should be kept and pass
> > > + * into the BMC address space.
> > > + */
> > > +
> > > + rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > > + (lpc_ctrl->base | (map.hostaddr >> 16)));
> > > + if (rc)
> > > + return rc;
> > > +
> > > + rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > > + (~(map.size - 1)) | ((map.size >> 16) - 1));
> >
> > Look Ma, a kernel exploit!
> >
>
> So 'evil' input here could allow the host to control the bmc,
> personally I file that under 'stupid' input. Also, stupid but not
> accidental, I don't believe one could accidentally come up with such
> input, although you never know what silly things human beings sometimes
> do. For what its worth, I'm not even sure that can happen but I'll
> grant you the benifit of the doubt.
I think you didn't get the main point here, again:
> > {sigh}
> >
> > Your assignment is to go find a whiteboard/blackboard/whatever and write
> > on it 100 times:
> > All input is evil.
You can NEVER trust any input values sent to the kernel, you have to
ALWAYS verify they are within certain safe ranges.
> > I want to see the picture of that before you send any more kernel patches.
> >
> > > +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> > > +{
> > > + atomic_dec(&lpc_ctrl_open_count);
> >
> > Totally unneeded and unnecessary, why do you care?
> >
>
> My aim here was to only have one process playing with the LPC mapping
> registers at a time.
Why? Who cares? You don't have internal state being kept by the
driver, so it shouldn't matter.
And again, don't treat an atomic variable as a lock, use a real lock for
the task, it works better, and is the correct thing to do.
thanks,
greg k-h
--
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 37/37] ARM: dts: DRA7: Add pcie1 dt node for EP mode
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci, linux-doc, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-samsung-soc, linuxppc-dev,
linux-arm-kernel, linux-arm-msm, nsekhar, kishon
In-Reply-To: <1484216786-17292-1-git-send-email-kishon@ti.com>
Add pcie1 dt node in order for the controller to operate in
endpoint mode. However since none of the dra7 based boards have
slots configured to operate in endpoint mode, keep EP mode
disabled.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
arch/arm/boot/dts/am572x-idk.dts | 7 ++++++-
arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 7 ++++++-
arch/arm/boot/dts/dra7-evm.dts | 4 ++++
arch/arm/boot/dts/dra7.dtsi | 22 +++++++++++++++++++++-
arch/arm/boot/dts/dra72-evm-common.dtsi | 4 ++++
5 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/am572x-idk.dts b/arch/arm/boot/dts/am572x-idk.dts
index 1540f7a..2ca2839 100644
--- a/arch/arm/boot/dts/am572x-idk.dts
+++ b/arch/arm/boot/dts/am572x-idk.dts
@@ -88,6 +88,11 @@
load-gpios = <&gpio3 19 GPIO_ACTIVE_LOW>;
};
-&pcie1 {
+&pcie1_rc {
+ status = "okay";
+ gpios = <&gpio3 23 GPIO_ACTIVE_HIGH>;
+};
+
+&pcie1_ep {
gpios = <&gpio3 23 GPIO_ACTIVE_HIGH>;
};
diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
index 78bee26..079a7e1 100644
--- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
+++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
@@ -556,7 +556,12 @@
};
};
-&pcie1 {
+&pcie1_rc {
+ status = "ok";
+ gpios = <&gpio2 8 GPIO_ACTIVE_LOW>;
+};
+
+&pcie1_ep {
gpios = <&gpio2 8 GPIO_ACTIVE_LOW>;
};
diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts
index 132f2be..fd0aa3a 100644
--- a/arch/arm/boot/dts/dra7-evm.dts
+++ b/arch/arm/boot/dts/dra7-evm.dts
@@ -937,3 +937,7 @@
status = "okay";
};
};
+
+&pcie1_rc {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index addb753..bf9c668 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -272,7 +272,11 @@
#address-cells = <1>;
ranges = <0x51000000 0x51000000 0x3000
0x0 0x20000000 0x10000000>;
- pcie1: pcie@51000000 {
+ /**
+ * To enable PCI endpoint mode, disable the pcie1_rc
+ * node and enable pcie1_ep mode.
+ */
+ pcie1_rc: pcie@51000000 {
compatible = "ti,dra7-pcie";
reg = <0x51000000 0x2000>, <0x51002000 0x14c>, <0x1000 0x2000>;
reg-names = "rc_dbics", "ti_conf", "config";
@@ -293,12 +297,28 @@
<0 0 0 2 &pcie1_intc 2>,
<0 0 0 3 &pcie1_intc 3>,
<0 0 0 4 &pcie1_intc 4>;
+ status = "disabled";
pcie1_intc: interrupt-controller {
interrupt-controller;
#address-cells = <0>;
#interrupt-cells = <1>;
};
};
+
+ pcie1_ep: pcie_ep@51000000 {
+ compatible = "ti,dra7-pcie-ep";
+ reg = <0x51000000 0x28>, <0x51002000 0x14c>, <0x51001000 0x28>, <0x1000 0x10000000>;
+ reg-names = "ep_dbics", "ti_conf", "ep_dbics2", "addr_space";
+ interrupts = <0 232 0x4>;
+ num-lanes = <1>;
+ num-ib-windows = <4>;
+ num-ob-windows = <16>;
+ ti,hwmods = "pcie1";
+ phys = <&pcie1_phy>;
+ phy-names = "pcie-phy0";
+ syscon-legacy-mode = <&scm_conf1 0x14 2>;
+ status = "disabled";
+ };
};
axi@1 {
diff --git a/arch/arm/boot/dts/dra72-evm-common.dtsi b/arch/arm/boot/dts/dra72-evm-common.dtsi
index e50fbee..5d9762c 100644
--- a/arch/arm/boot/dts/dra72-evm-common.dtsi
+++ b/arch/arm/boot/dts/dra72-evm-common.dtsi
@@ -545,3 +545,7 @@
status = "okay";
};
};
+
+&pcie1_rc {
+ status = "okay";
+};
--
1.7.9.5
^ permalink raw reply related
* [PATCH 36/37] ARM: DRA7: clockdomain: Change the CLKTRCTRL of CM_PCIE_CLKSTCTRL to SW_WKUP
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci, linux-doc, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-samsung-soc, linuxppc-dev,
linux-arm-kernel, linux-arm-msm, nsekhar, kishon
In-Reply-To: <1484216786-17292-1-git-send-email-kishon@ti.com>
The PCIe programming sequence in TRM suggests CLKSTCTRL of PCIe should
be set to SW_WKUP. There are no issues when CLKSTCTRL is set to HW_AUTO
in RC mode. However in EP mode, the host system is not able to access the
MEMSPACE and setting the CLKSTCTRL to SW_WKUP fixes it.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
arch/arm/mach-omap2/clockdomains7xx_data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-omap2/clockdomains7xx_data.c b/arch/arm/mach-omap2/clockdomains7xx_data.c
index 6c67965..67ebff8 100644
--- a/arch/arm/mach-omap2/clockdomains7xx_data.c
+++ b/arch/arm/mach-omap2/clockdomains7xx_data.c
@@ -524,7 +524,7 @@
.dep_bit = DRA7XX_PCIE_STATDEP_SHIFT,
.wkdep_srcs = pcie_wkup_sleep_deps,
.sleepdep_srcs = pcie_wkup_sleep_deps,
- .flags = CLKDM_CAN_HWSUP_SWSUP,
+ .flags = CLKDM_CAN_SWSUP,
};
static struct clockdomain atl_7xx_clkdm = {
--
1.7.9.5
^ permalink raw reply related
* [PATCH 35/37] MAINTAINERS: add PCI EP maintainer
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci, linux-doc, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-samsung-soc, linuxppc-dev,
linux-arm-kernel, linux-arm-msm, nsekhar, kishon
In-Reply-To: <1484216786-17292-1-git-send-email-kishon@ti.com>
Add maintainer for the newly introduced PCI EP framework.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8672f18..021f676 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9407,6 +9407,15 @@ F: include/linux/pci*
F: arch/x86/pci/
F: arch/x86/kernel/quirks.c
+PCI EP SUBSYSTEM
+M: Kishon Vijay Abraham I <kishon@ti.com>
+L: linux-pci@vger.kernel.org
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
+S: Supported
+F: drivers/pci/endpoint/
+F: drivers/misc/pci_endpoint_test.c
+F: tools/pci/
+
PCI DRIVER FOR ALTERA PCIE IP
M: Ley Foon Tan <lftan@altera.com>
L: rfi@lists.rocketboards.org (moderated for non-subscribers)
--
1.7.9.5
^ permalink raw reply related
* [PATCH 34/37] tools: PCI: Add sample test script to invoke pcitest
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci, linux-doc, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-samsung-soc, linuxppc-dev,
linux-arm-kernel, linux-arm-msm, nsekhar, kishon
In-Reply-To: <1484216786-17292-1-git-send-email-kishon@ti.com>
Add a simple test script that invokes the pcitest userspace tool
to perform all the PCI endpoint tests (BAR tests, interrupt tests,
read tests, write tests and copy tests).
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
tools/pci/pcitest.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
create mode 100644 tools/pci/pcitest.sh
diff --git a/tools/pci/pcitest.sh b/tools/pci/pcitest.sh
new file mode 100644
index 0000000..5442bbe
--- /dev/null
+++ b/tools/pci/pcitest.sh
@@ -0,0 +1,56 @@
+#!/bin/sh
+
+echo "BAR tests"
+echo
+
+bar=0
+
+while [ $bar -lt 6 ]
+do
+ pcitest -b $bar
+ bar=`expr $bar + 1`
+done
+echo
+
+echo "Interrupt tests"
+echo
+
+pcitest -l
+msi=1
+
+while [ $msi -lt 33 ]
+do
+ pcitest -m $msi
+ msi=`expr $msi + 1`
+done
+echo
+
+echo "Read Tests"
+echo
+
+pcitest -r -s 1
+pcitest -r -s 1024
+pcitest -r -s 1025
+pcitest -r -s 1024000
+pcitest -r -s 1024001
+echo
+
+echo "Write Tests"
+echo
+
+pcitest -w -s 1
+pcitest -w -s 1024
+pcitest -w -s 1025
+pcitest -w -s 1024000
+pcitest -w -s 1024001
+echo
+
+echo "Copy Tests"
+echo
+
+pcitest -c -s 1
+pcitest -c -s 1024
+pcitest -c -s 1025
+pcitest -c -s 1024000
+pcitest -c -s 1024001
+echo
--
1.7.9.5
^ permalink raw reply related
* [PATCH 33/37] tools: PCI: Add a userspace tool to test PCI endpoint
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci, linux-doc, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-samsung-soc, linuxppc-dev,
linux-arm-kernel, linux-arm-msm, nsekhar, kishon
In-Reply-To: <1484216786-17292-1-git-send-email-kishon@ti.com>
Add a userspace tool to invoke the ioctls exposed by the
PCI endpoint test driver to perform various PCI tests.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
tools/pci/pcitest.c | 186 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 186 insertions(+)
create mode 100644 tools/pci/pcitest.c
diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c
new file mode 100644
index 0000000..39b5b0b
--- /dev/null
+++ b/tools/pci/pcitest.c
@@ -0,0 +1,186 @@
+/**
+ * Userspace PCI Endpoint Test Module
+ *
+ * Copyright (C) 2017 Texas Instruments
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 of
+ * the License as published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <time.h>
+#include <unistd.h>
+
+#include <linux/pcitest.h>
+
+#define BILLION 1E9
+
+static char *result[] = { "NOT OKAY", "OKAY" };
+
+struct pci_test {
+ char *device;
+ char barnum;
+ bool legacyirq;
+ unsigned int msinum;
+ bool read;
+ bool write;
+ bool copy;
+ unsigned long size;
+};
+
+static int run_test(struct pci_test *test)
+{
+ long ret;
+ int fd;
+ struct timespec start, end;
+ double time;
+
+ fd = open(test->device, O_RDWR);
+ if (fd < 0) {
+ perror("can't open PCI Endpoint Test device");
+ return fd;
+ }
+
+ if (test->barnum >= 0 && test->barnum <= 5) {
+ ret = ioctl(fd, PCITEST_BAR, test->barnum);
+ fprintf(stdout, "BAR%d:\t\t", test->barnum);
+ if (ret < 0)
+ fprintf(stdout, "TEST FAILED\n");
+ else
+ fprintf(stdout, "%s\n", result[ret]);
+ }
+
+ if (test->legacyirq) {
+ ret = ioctl(fd, PCITEST_LEGACY_IRQ, 0);
+ fprintf(stdout, "LEGACY IRQ:\t");
+ if (ret < 0)
+ fprintf(stdout, "TEST FAILED\n");
+ else
+ fprintf(stdout, "%s\n", result[ret]);
+ }
+
+ if (test->msinum > 0 && test->msinum <= 32) {
+ ret = ioctl(fd, PCITEST_MSI, test->msinum);
+ fprintf(stdout, "MSI%d:\t\t", test->msinum);
+ if (ret < 0)
+ fprintf(stdout, "TEST FAILED\n");
+ else
+ fprintf(stdout, "%s\n", result[ret]);
+ }
+
+ if (test->write) {
+ ret = ioctl(fd, PCITEST_WRITE, test->size);
+ fprintf(stdout, "WRITE (%7ld bytes):\t\t", test->size);
+ if (ret < 0)
+ fprintf(stdout, "TEST FAILED\n");
+ else
+ fprintf(stdout, "%s\n", result[ret]);
+ }
+
+ if (test->read) {
+ ret = ioctl(fd, PCITEST_READ, test->size);
+ fprintf(stdout, "READ (%7ld bytes):\t\t", test->size);
+ if (ret < 0)
+ fprintf(stdout, "TEST FAILED\n");
+ else
+ fprintf(stdout, "%s\n", result[ret]);
+ }
+
+ if (test->copy) {
+ ret = ioctl(fd, PCITEST_COPY, test->size);
+ fprintf(stdout, "COPY (%7ld bytes):\t\t", test->size);
+ if (ret < 0)
+ fprintf(stdout, "TEST FAILED\n");
+ else
+ fprintf(stdout, "%s\n", result[ret]);
+ }
+
+ fflush(stdout);
+}
+
+int main(int argc, char **argv)
+{
+ int c;
+ struct pci_test *test;
+
+ test = calloc(1, sizeof(*test));
+ if (!test) {
+ perror("Fail to allocate memory for pci_test\n");
+ return -ENOMEM;
+ }
+
+ /* since '0' is a valid BAR number, initialize it to -1 */
+ test->barnum = -1;
+
+ /* set default size as 100KB */
+ test->size = 0x19000;
+
+ /* set default endpoint device */
+ test->device = "/dev/pci-endpoint-test.0";
+
+ while ((c = getopt(argc, argv, "D:b:m:lrwcs:")) != EOF)
+ switch (c) {
+ case 'D':
+ test->device = optarg;
+ continue;
+ case 'b':
+ test->barnum = atoi(optarg);
+ if (test->barnum < 0 || test->barnum > 5)
+ goto usage;
+ continue;
+ case 'l':
+ test->legacyirq = true;
+ continue;
+ case 'm':
+ test->msinum = atoi(optarg);
+ if (test->msinum < 1 || test->msinum > 32)
+ goto usage;
+ continue;
+ case 'r':
+ test->read = true;
+ continue;
+ case 'w':
+ test->write = true;
+ continue;
+ case 'c':
+ test->copy = true;
+ continue;
+ case 's':
+ test->size = strtoul(optarg, NULL, 0);
+ continue;
+ case '?':
+ case 'h':
+ default:
+usage:
+ fprintf(stderr,
+ "usage: %s [options]\n"
+ "Options:\n"
+ "\t-D <dev> pci endpoint test device {default: /dev/pci-endpoint-test.0}\n"
+ "\t-b <bar num> BAR test (bar number between 0..5)\n"
+ "\t-m <msi num> MSI test (msi number between 1..32)\n"
+ "\t-r Read buffer test\n"
+ "\t-w Write buffer test\n"
+ "\t-c Copy buffer test\n"
+ "\t-s <size> Size of buffer {default: 100KB}\n",
+ argv[0]);
+ return -EINVAL;
+ }
+
+ run_test(test);
+ return 0;
+}
--
1.7.9.5
^ permalink raw reply related
* [PATCH 32/37] Documentation: misc-devices: Add Documentation for pci-endpoint-test driver
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci, linux-doc, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-samsung-soc, linuxppc-dev,
linux-arm-kernel, linux-arm-msm, nsekhar, kishon
In-Reply-To: <1484216786-17292-1-git-send-email-kishon@ti.com>
Add Documentation for pci-endpoint-test driver.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
Documentation/misc-devices/pci-endpoint-test.txt | 35 ++++++++++++++++++++++
1 file changed, 35 insertions(+)
create mode 100644 Documentation/misc-devices/pci-endpoint-test.txt
diff --git a/Documentation/misc-devices/pci-endpoint-test.txt b/Documentation/misc-devices/pci-endpoint-test.txt
new file mode 100644
index 0000000..4385718
--- /dev/null
+++ b/Documentation/misc-devices/pci-endpoint-test.txt
@@ -0,0 +1,35 @@
+Driver for PCI Endpoint Test Function
+
+This driver should be used as a host side driver if the root complex is
+connected to a configurable pci endpoint running *pci_epf_test* function
+driver configured according to [1].
+
+The "pci_endpoint_test" driver can be used to perform the following tests.
+
+The PCI driver for the test device performs the following tests
+ *) verifying addresses programmed in BAR
+ *) raise legacy IRQ
+ *) raise MSI IRQ
+ *) read data
+ *) write data
+ *) copy data
+
+This misc driver creates /dev/pci-endpoint-test.<num> for every
+*pci_epf_test* function connected to the root complex and "ioctls"
+should be used to perform the above tests.
+
+ioctl
+-----
+ PCITEST_BAR: Tests the BAR. The number of the BAR that has to be tested
+ should be passed as argument.
+ PCITEST_LEGACY_IRQ: Tests legacy IRQ
+ PCITEST_MSI: Tests message signalled interrupts. The MSI number that has
+ to be tested should be passed as argument.
+ PCITEST_WRITE: Perform write tests. The size of the buffer should be passed
+ as argument.
+ PCITEST_READ: Perform read tests. The size of the buffer should be passed
+ as argument.
+ PCITEST_COPY: Perform read tests. The size of the buffer should be passed
+ as argument.
+
+[1] -> Documentation/PCI/endpoint/function/binding/pci-test.txt
--
1.7.9.5
^ permalink raw reply related
* [PATCH 31/37] misc: Add host side pci driver for pci test function device
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-VrBV9hrLPhE,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, nsekhar-l0cyMroinI0,
kishon-l0cyMroinI0
In-Reply-To: <1484216786-17292-1-git-send-email-kishon-l0cyMroinI0@public.gmane.org>
Add PCI endpoint test driver that can verify base address
register, legacy interrupt/MSI interrupt and read/write/copy
buffers between host and device. The corresponding pci-epf-test
function driver should be used on the EP side.
Signed-off-by: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
---
drivers/misc/Kconfig | 7 +
drivers/misc/Makefile | 1 +
drivers/misc/pci_endpoint_test.c | 533 ++++++++++++++++++++++++++++++++++++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/pcitest.h | 19 ++
5 files changed, 561 insertions(+)
create mode 100644 drivers/misc/pci_endpoint_test.c
create mode 100644 include/uapi/linux/pcitest.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 64971ba..14a95a6 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -766,6 +766,13 @@ config PANEL_BOOT_MESSAGE
An empty message will only clear the display at driver init time. Any other
printf()-formatted message is valid with newline and escape codes.
+config PCI_ENDPOINT_TEST
+ depends on PCI || COMPILE_TEST
+ tristate "PCI Endpoint Test driver"
+ ---help---
+ Enable this configuration option to enable the host side test driver
+ for PCI Endpoint.
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 3198336..64a532ac2 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO) += echo/
obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
obj-$(CONFIG_CXL_BASE) += cxl/
obj-$(CONFIG_PANEL) += panel.o
+obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o
lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
new file mode 100644
index 0000000..920b14c
--- /dev/null
+++ b/drivers/misc/pci_endpoint_test.c
@@ -0,0 +1,533 @@
+/**
+ * Host side test driver to test endpoint functionality
+ *
+ * Copyright (C) 2017 Texas Instruments
+ * Author: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 of
+ * the License as published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/crc32.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/random.h>
+#include <linux/slab.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
+
+#include <linux/pci_regs.h>
+
+#include <uapi/linux/pcitest.h>
+
+#define DRV_MODULE_NAME "pci-endpoint-test"
+
+#define PCI_ENDPOINT_TEST_MAGIC 0x0
+
+#define PCI_ENDPOINT_TEST_COMMAND 0x4
+#define COMMAND_RAISE_LEGACY_IRQ BIT(0)
+#define COMMAND_RAISE_MSI_IRQ BIT(1)
+#define MSI_NUMBER_SHIFT 2
+/* 6 bits for MSI number */
+#define COMMAND_READ BIT(8)
+#define COMMAND_WRITE BIT(9)
+#define COMMAND_COPY BIT(10)
+
+#define PCI_ENDPOINT_TEST_STATUS 0x8
+#define STATUS_READ_SUCCESS BIT(0)
+#define STATUS_READ_FAIL BIT(1)
+#define STATUS_WRITE_SUCCESS BIT(2)
+#define STATUS_WRITE_FAIL BIT(3)
+#define STATUS_COPY_SUCCESS BIT(4)
+#define STATUS_COPY_FAIL BIT(5)
+#define STATUS_IRQ_RAISED BIT(6)
+#define STATUS_SRC_ADDR_INVALID BIT(7)
+#define STATUS_DST_ADDR_INVALID BIT(8)
+
+#define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0xc
+#define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10
+
+#define PCI_ENDPOINT_TEST_LOWER_DST_ADDR 0x14
+#define PCI_ENDPOINT_TEST_UPPER_DST_ADDR 0x18
+
+#define PCI_ENDPOINT_TEST_SIZE 0x1c
+#define PCI_ENDPOINT_TEST_CHECKSUM 0x20
+
+static DEFINE_IDA(pci_endpoint_test_ida);
+
+#define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
+ miscdev)
+enum pci_barno {
+ BAR_0,
+ BAR_1,
+ BAR_2,
+ BAR_3,
+ BAR_4,
+ BAR_5,
+};
+
+struct pci_endpoint_test {
+ struct pci_dev *pdev;
+ void __iomem *base;
+ void __iomem *bar[6];
+ struct completion irq_raised;
+ int last_irq;
+ /* mutex to protect the ioctls */
+ struct mutex mutex;
+ struct miscdevice miscdev;
+};
+
+static int bar_size[] = { 4, 512, 1024, 16384, 131072, 1048576 };
+
+static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
+ u32 offset)
+{
+ return readl(test->base + offset);
+}
+
+static inline void pci_endpoint_test_writel(struct pci_endpoint_test *test,
+ u32 offset, u32 value)
+{
+ writel(value, test->base + offset);
+}
+
+static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
+ int bar, int offset)
+{
+ return readl(test->bar[bar] + offset);
+}
+
+static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
+ int bar, u32 offset, u32 value)
+{
+ writel(value, test->bar[bar] + offset);
+}
+
+static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
+{
+ struct pci_endpoint_test *test = dev_id;
+ u32 reg;
+
+ reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
+ if (reg & STATUS_IRQ_RAISED) {
+ test->last_irq = irq;
+ complete(&test->irq_raised);
+ reg &= ~STATUS_IRQ_RAISED;
+ }
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS,
+ reg);
+
+ return IRQ_HANDLED;
+}
+
+static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
+ enum pci_barno barno)
+{
+ int j;
+ u32 val;
+ int size;
+
+ if (!test->bar[barno])
+ return false;
+
+ size = bar_size[barno];
+
+ for (j = 0; j < size; j += 4)
+ pci_endpoint_test_bar_writel(test, barno, j, 0xA0A0A0A0);
+
+ for (j = 0; j < size; j += 4) {
+ val = pci_endpoint_test_bar_readl(test, barno, j);
+ if (val != 0xA0A0A0A0)
+ return false;
+ }
+
+ return true;
+}
+
+static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
+{
+ u32 val;
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+ COMMAND_RAISE_LEGACY_IRQ);
+ val = wait_for_completion_timeout(&test->irq_raised,
+ msecs_to_jiffies(1000));
+ if (!val)
+ return false;
+
+ return true;
+}
+
+static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
+ u8 msi_num)
+{
+ u32 val;
+ struct pci_dev *pdev = test->pdev;
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+ msi_num << MSI_NUMBER_SHIFT |
+ COMMAND_RAISE_MSI_IRQ);
+ val = wait_for_completion_timeout(&test->irq_raised,
+ msecs_to_jiffies(1000));
+ if (!val)
+ return false;
+
+ if (test->last_irq - pdev->irq == msi_num - 1)
+ return true;
+
+ return false;
+}
+
+static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, size_t size)
+{
+ bool ret = false;
+ void *src_addr;
+ void *dst_addr;
+ dma_addr_t src_phys_addr;
+ dma_addr_t dst_phys_addr;
+ struct pci_dev *pdev = test->pdev;
+ struct device *dev = &pdev->dev;
+ u32 src_crc32;
+ u32 dst_crc32;
+
+ src_addr = dma_alloc_coherent(dev, size, &src_phys_addr, GFP_KERNEL);
+ if (!src_addr) {
+ dev_err(dev, "failed to allocate source buffer\n");
+ ret = false;
+ goto err;
+ }
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
+ lower_32_bits(src_phys_addr));
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
+ upper_32_bits(src_phys_addr));
+
+ get_random_bytes(src_addr, size);
+ src_crc32 = crc32_le(~0, src_addr, size);
+
+ dst_addr = dma_alloc_coherent(dev, size, &dst_phys_addr, GFP_KERNEL);
+ if (!dst_addr) {
+ dev_err(dev, "failed to allocate destination address\n");
+ ret = false;
+ goto err_src_addr;
+ }
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
+ lower_32_bits(dst_phys_addr));
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
+ upper_32_bits(dst_phys_addr));
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE,
+ size);
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+ 1 << MSI_NUMBER_SHIFT | COMMAND_COPY);
+
+ wait_for_completion(&test->irq_raised);
+
+ dst_crc32 = crc32_le(~0, dst_addr, size);
+ if (dst_crc32 == src_crc32)
+ ret = true;
+
+ dma_free_coherent(dev, size, dst_addr, dst_phys_addr);
+
+err_src_addr:
+ dma_free_coherent(dev, size, src_addr, src_phys_addr);
+
+err:
+ return ret;
+}
+
+static bool pci_endpoint_test_write(struct pci_endpoint_test *test, size_t size)
+{
+ bool ret = false;
+ u32 reg;
+ void *addr;
+ dma_addr_t phys_addr;
+ struct pci_dev *pdev = test->pdev;
+ struct device *dev = &pdev->dev;
+ u32 crc32;
+
+ addr = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
+ if (!addr) {
+ dev_err(dev, "failed to allocate address\n");
+ ret = false;
+ goto err;
+ }
+
+ get_random_bytes(addr, size);
+
+ crc32 = crc32_le(~0, addr, size);
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
+ crc32);
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
+ lower_32_bits(phys_addr));
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
+ upper_32_bits(phys_addr));
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+ 1 << MSI_NUMBER_SHIFT | COMMAND_READ);
+
+ wait_for_completion(&test->irq_raised);
+
+ reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
+ if (reg & STATUS_READ_SUCCESS)
+ ret = true;
+
+ dma_free_coherent(dev, size, addr, phys_addr);
+
+err:
+ return ret;
+}
+
+static bool pci_endpoint_test_read(struct pci_endpoint_test *test, size_t size)
+{
+ bool ret = false;
+ void *addr;
+ dma_addr_t phys_addr;
+ struct pci_dev *pdev = test->pdev;
+ struct device *dev = &pdev->dev;
+ u32 crc32;
+
+ addr = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
+ if (!addr) {
+ dev_err(dev, "failed to allocate destination address\n");
+ ret = false;
+ goto err;
+ }
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
+ lower_32_bits(phys_addr));
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
+ upper_32_bits(phys_addr));
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
+
+ pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+ 1 << MSI_NUMBER_SHIFT | COMMAND_WRITE);
+
+ wait_for_completion(&test->irq_raised);
+
+ crc32 = crc32_le(~0, addr, size);
+ if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
+ ret = true;
+
+ dma_free_coherent(dev, size, addr, phys_addr);
+err:
+ return ret;
+}
+
+static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ int ret = -EINVAL;
+ enum pci_barno bar;
+ struct pci_endpoint_test *test = to_endpoint_test(file->private_data);
+
+ mutex_lock(&test->mutex);
+ switch (cmd) {
+ case PCITEST_BAR:
+ bar = arg;
+ if (bar < 0 || bar > 5)
+ goto ret;
+ ret = pci_endpoint_test_bar(test, bar);
+ break;
+ case PCITEST_LEGACY_IRQ:
+ ret = pci_endpoint_test_legacy_irq(test);
+ break;
+ case PCITEST_MSI:
+ ret = pci_endpoint_test_msi_irq(test, arg);
+ break;
+ case PCITEST_WRITE:
+ ret = pci_endpoint_test_write(test, arg);
+ break;
+ case PCITEST_READ:
+ ret = pci_endpoint_test_read(test, arg);
+ break;
+ case PCITEST_COPY:
+ ret = pci_endpoint_test_copy(test, arg);
+ break;
+ }
+
+ret:
+ mutex_unlock(&test->mutex);
+ return ret;
+}
+
+static const struct file_operations pci_endpoint_test_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = pci_endpoint_test_ioctl,
+};
+
+static int pci_endpoint_test_probe(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ int i;
+ int err;
+ int irq;
+ int id;
+ char name[20];
+ enum pci_barno bar;
+ void __iomem *base;
+ struct device *dev = &pdev->dev;
+ struct pci_endpoint_test *test;
+ struct miscdevice *misc_device;
+
+ if (pci_is_bridge(pdev))
+ return -ENODEV;
+
+ test = devm_kzalloc(dev, sizeof(*test), GFP_KERNEL);
+ if (!test)
+ return -ENOMEM;
+
+ test->pdev = pdev;
+ init_completion(&test->irq_raised);
+ mutex_init(&test->mutex);
+
+ err = pci_enable_device(pdev);
+ if (err) {
+ dev_err(dev, "Cannot enable PCI device\n");
+ return err;
+ }
+
+ err = pci_request_regions(pdev, DRV_MODULE_NAME);
+ if (err) {
+ dev_err(dev, "Cannot obtain PCI resources\n");
+ goto err_disable_pdev;
+ }
+
+ pci_set_master(pdev);
+
+ irq = pci_enable_msi_range(pdev, 1, 32);
+ if (irq < 0)
+ dev_err(dev, "failed to get MSI interrupts\n");
+
+ err = devm_request_irq(dev, pdev->irq, pci_endpoint_test_irqhandler,
+ IRQF_SHARED, DRV_MODULE_NAME, test);
+ if (err) {
+ dev_err(dev, "failed to request irq\n");
+ goto err_disable_msi;
+ }
+
+ for (i = 1; i < irq; i++) {
+ err = devm_request_irq(dev, pdev->irq + i,
+ pci_endpoint_test_irqhandler,
+ IRQF_SHARED, DRV_MODULE_NAME, test);
+ if (err)
+ dev_err(dev, "failed to request irq for MSI %d\n",
+ i + 1);
+ }
+
+ for (bar = BAR_0; bar <= BAR_5; bar++) {
+ base = pci_ioremap_bar(pdev, bar);
+ if (!base) {
+ dev_err(dev, "failed to read BAR%d\n", bar);
+ WARN_ON(bar == BAR_0);
+ }
+ test->bar[bar] = base;
+ }
+
+ test->base = test->bar[0];
+ if (!test->base) {
+ dev_err(dev, "Cannot perform PCI test without BAR0\n");
+ goto err_iounmap;
+ }
+
+ pci_set_drvdata(pdev, test);
+
+ id = ida_simple_get(&pci_endpoint_test_ida, 0, 0, GFP_KERNEL);
+ if (id < 0) {
+ dev_err(dev, "unable to get id\n");
+ goto err_iounmap;
+ }
+
+ snprintf(name, sizeof(name), DRV_MODULE_NAME ".%d", id);
+ misc_device = &test->miscdev;
+ misc_device->minor = MISC_DYNAMIC_MINOR;
+ misc_device->name = name;
+ misc_device->fops = &pci_endpoint_test_fops,
+
+ err = misc_register(misc_device);
+ if (err) {
+ dev_err(dev, "failed to register device\n");
+ goto err_ida_remove;
+ }
+
+ return 0;
+
+err_ida_remove:
+ ida_simple_remove(&pci_endpoint_test_ida, id);
+
+err_iounmap:
+ for (bar = BAR_0; bar <= BAR_5; bar++) {
+ if (test->bar[bar])
+ pci_iounmap(pdev, test->bar[bar]);
+ }
+
+err_disable_msi:
+ pci_disable_msi(pdev);
+ pci_release_regions(pdev);
+
+err_disable_pdev:
+ pci_disable_device(pdev);
+
+ return err;
+}
+
+static void pci_endpoint_test_remove(struct pci_dev *pdev)
+{
+ int id;
+ enum pci_barno bar;
+ struct pci_endpoint_test *test = pci_get_drvdata(pdev);
+ struct miscdevice *misc_device = &test->miscdev;
+
+ if (sscanf(misc_device->name, DRV_MODULE_NAME ".%d", &id) != 1)
+ return;
+
+ misc_deregister(&test->miscdev);
+ ida_simple_remove(&pci_endpoint_test_ida, id);
+ for (bar = BAR_0; bar <= BAR_5; bar++) {
+ if (test->bar[bar])
+ pci_iounmap(pdev, test->bar[bar]);
+ }
+ pci_disable_msi(pdev);
+ pci_release_regions(pdev);
+ pci_disable_device(pdev);
+}
+
+static const struct pci_device_id pci_endpoint_test_tbl[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_ANY_ID) },
+ { }
+};
+MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);
+
+static struct pci_driver pci_endpoint_test_driver = {
+ .name = DRV_MODULE_NAME,
+ .id_table = pci_endpoint_test_tbl,
+ .probe = pci_endpoint_test_probe,
+ .remove = pci_endpoint_test_remove,
+};
+module_pci_driver(pci_endpoint_test_driver);
+
+MODULE_DESCRIPTION("PCI ENDPOINT TEST DRIVER");
+MODULE_AUTHOR("Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index a8b93e6..f3c2769 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -329,6 +329,7 @@ header-y += parport.h
header-y += patchkey.h
header-y += pci.h
header-y += pci_regs.h
+header-y += pcitest.h
header-y += perf_event.h
header-y += personality.h
header-y += pfkeyv2.h
diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
new file mode 100644
index 0000000..a6aa10c
--- /dev/null
+++ b/include/uapi/linux/pcitest.h
@@ -0,0 +1,19 @@
+/**
+ * pcitest.h - PCI test uapi defines
+ *
+ * Copyright (C) 2017 Texas Instruments
+ * Author: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
+ *
+ */
+
+#ifndef __UAPI_LINUX_PCITEST_H
+#define __UAPI_LINUX_PCITEST_H
+
+#define PCITEST_BAR _IO('P', 0x1)
+#define PCITEST_LEGACY_IRQ _IO('P', 0x2)
+#define PCITEST_MSI _IOW('P', 0x3, int)
+#define PCITEST_WRITE _IOW('P', 0x4, unsigned long)
+#define PCITEST_READ _IOW('P', 0x5, unsigned long)
+#define PCITEST_COPY _IOW('P', 0x6, unsigned long)
+
+#endif /* __UAPI_LINUX_PCITEST_H */
--
1.7.9.5
--
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
* [PATCH 30/37] dt-bindings: PCI: dra7xx: Add dt bindings to enable legacy mode
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci, linux-doc, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-samsung-soc, linuxppc-dev,
linux-arm-kernel, linux-arm-msm, nsekhar, kishon
In-Reply-To: <1484216786-17292-1-git-send-email-kishon@ti.com>
Update device tree binding documentation of TI's dra7xx PCI
controller to include property for enabling legacy mode.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
Documentation/devicetree/bindings/pci/ti-pci.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
index 62f5f59..ed85e8e 100644
--- a/Documentation/devicetree/bindings/pci/ti-pci.txt
+++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
@@ -39,6 +39,10 @@ DEVICE MODE
- interrupts : one interrupt entries must be specified for main interrupt.
- num-ib-windows : number of inbound address translation windows
- num-ob-windows : number of outbound address translation windows
+ - syscon-legacy-mode: phandle to the syscon dt node. The 1st argument should
+ contain the register offset within syscon and the 2nd
+ argument should contain the bit field for setting the
+ legacy mode
Optional Property:
- gpios : Should be added if a gpio line is required to drive PERST# line
--
1.7.9.5
^ permalink raw reply related
* [PATCH 29/37] PCI: dwc: dra7xx: Workaround for errata id i870
From: Kishon Vijay Abraham I @ 2017-01-12 10:26 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Joao Pinto, Arnd Bergmann
Cc: linux-pci, linux-doc, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-samsung-soc, linuxppc-dev,
linux-arm-kernel, linux-arm-msm, nsekhar, kishon
In-Reply-To: <1484216786-17292-1-git-send-email-kishon@ti.com>
According to errata i870, access to the PCIe slave port
that are not 32-bit aligned will result in incorrect mapping
to TLP Address and Byte enable fields.
Accessing non 32-bit aligned data causes incorrect data in the target
buffer if memcpy is used. Implement the workaround for this
errata here.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
drivers/pci/dwc/pci-dra7xx.c | 50 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 333aa56..7666e3e 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -26,6 +26,8 @@
#include <linux/pm_runtime.h>
#include <linux/resource.h>
#include <linux/types.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
#include "pcie-designware.h"
@@ -531,6 +533,48 @@ static int dra7xx_pcie_enable_phy(struct dra7xx_pcie *dra7xx)
{},
};
+/*
+ * dra7xx_pcie_ep_legacy_mode: workaround for AM572x/AM571x Errata i870
+ * @dra7xx: the dra7xx device where the workaround should be applied
+ *
+ * Access to the PCIe slave port that are not 32-bit aligned will result
+ * in incorrect mapping to TLP Address and Byte enable fields. Therefore,
+ * byte and half-word accesses are not possible to byte offset 0x1, 0x2, or
+ * 0x3.
+ *
+ * To avoid this issue set PCIE_SS1_AXI2OCP_LEGACY_MODE_ENABLE to 1.
+ */
+static int dra7xx_pcie_ep_legacy_mode(struct device *dev)
+{
+ int ret;
+ struct device_node *np = dev->of_node;
+ struct regmap *regmap;
+ unsigned int reg;
+ unsigned int field;
+
+ regmap = syscon_regmap_lookup_by_phandle(np, "syscon-legacy-mode");
+ if (IS_ERR(regmap)) {
+ dev_dbg(dev, "can't get syscon-legacy-mode\n");
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32_index(np, "syscon-legacy-mode", 1, ®)) {
+ dev_err(dev, "couldn't get legacy mode register offset\n");
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32_index(np, "syscon-legacy-mode", 2, &field)) {
+ dev_err(dev, "can't get bit field for setting legacy mode\n");
+ return -EINVAL;
+ }
+
+ ret = regmap_update_bits(regmap, reg, field, field);
+ if (ret)
+ dev_err(dev, "failed to set legacy mode\n");
+
+ return ret;
+}
+
static int __init dra7xx_pcie_probe(struct platform_device *pdev)
{
u32 reg;
@@ -643,6 +687,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
case DW_PCIE_RC_TYPE:
dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE,
DEVICE_TYPE_RC);
+
ret = dra7xx_add_pcie_port(dra7xx, pdev);
if (ret < 0)
goto err_gpio;
@@ -650,6 +695,11 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
case DW_PCIE_EP_TYPE:
dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE,
DEVICE_TYPE_EP);
+
+ ret = dra7xx_pcie_ep_legacy_mode(dev);
+ if (ret)
+ goto err_gpio;
+
ret = dra7xx_add_pcie_ep(dra7xx, pdev);
if (ret < 0)
goto err_gpio;
--
1.7.9.5
^ permalink raw reply related
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