* [PATCH 0/4] media: platform: Add Aspeed Video Engine driver
From: Eddie James @ 2018-09-13 19:11 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <364c2565-fdb0-dd31-5852-6358066810a5@xs4all.nl>
On 09/03/2018 06:57 AM, Hans Verkuil wrote:
> Hi Eddie,
>
> Thank you for your work on this. Interesting to see support for this SoC :-)
>
> On 08/29/2018 11:09 PM, Eddie James wrote:
>> The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
>> can capture and compress video data from digital or analog sources. With
>> the Aspeed chip acting as a service processor, the Video Engine can
>> capture the host processor graphics output.
>>
>> This series adds a V4L2 driver for the VE, providing a read() interface
>> only. The driver triggers the hardware to capture the host graphics output
>> and compress it to JPEG format.
>>
>> Testing on an AST2500 determined that the videobuf/streaming/mmap interface
>> was significantly slower than the simple read() interface, so I have not
>> included the streaming part.
> Do you know why? It should be equal or faster, not slower.
Yes, it seems to be an issue with the timing of the video engine
interrupts compared with how a normal v4l2 application queues buffers.
With the simple read() application, the driver can swap between DMA
buffers freely and get a frame ahead. With the streaming buffers, I
found the driver ran through the queue quite quickly, but then, once
userspace queues again, we had to wait for the next frame, as I couldn't
get a frame ahead since no buffers were available during that time
period. This could possibly be solved with more buffers but this gets to
require a lot of memory, since each buffer is allocated for the full
frame size even though we only fill a fraction of it with JPEG data...
>
> I reviewed about half of the driver, but then I stopped since there were too
> many things missing.
>
> First of all, you need to test your driver with v4l2-compliance (available here:
> https://git.linuxtv.org/v4l-utils.git/). Always compile from the git repo since
> the versions from distros tend to be too old.
>
> Just run 'v4l2-compliance -d /dev/videoX' and fix all issues. Then run
> 'v4l2-compliance -s -d /dev/videoX' to test streaming.
>
> This utility checks if the driver follows the V4L2 API correctly, implements
> all ioctls that it should and fills in all the fields that it should.
>
> Please add the output of 'v4l2-compliance -s' to future versions of this patch
> series: I don't accept V4L2 drivers without a clean report of this utility.
Sure thing. Thanks for the guidance.
>
> If you have any questions, then mail me or (usually quicker) ask on the #v4l
> freenode irc channel (I'm in the CET timezone).
>
> One thing that needs more explanation: from what I could tell from the driver
> the VIDIOC_G_FMT ioctl returns the detected format instead of the current
> format. This is wrong. Instead you should implement the VIDIOC_*_DV_TIMINGS
> ioctls and the V4L2_EVENT_SOURCE_CHANGE event.
>
> The normal sequence is that userspace queries the current timings with
> VIDIOC_QUERY_DV_TIMINGS, if it finds valid timings, then it sets these
> timings with _S_DV_TIMINGS. Now it can call G/S_FMT. If the timings
> change, then the driver should detect that and send a V4L2_EVENT_SOURCE_CHANGE
> event.
OK I see. I ended up simplifying this part anyway since it's not
possible to change the video size from the driver. I don't think there
is a need for VIDIOC_QUERY_DV_TIMINGS now, but feel free to review.
Thanks again,
Eddie
>
> When the application receives this event it can take action, such as
> increasing the size of the buffer for the jpeg data that it reads into.
>
> The reason for this sequence of events is that you can't just change the
> format/resolution mid-stream without giving userspace the chance to
> reconfigure.
>
> Regards,
>
> Hans
>
>> It's also possible to use an automatic mode for the VE such that
>> re-triggering the HW every frame isn't necessary. However this wasn't
>> reliable on the AST2400, and probably used more CPU anyway due to excessive
>> interrupts. It was approximately 15% faster.
>>
>> The series also adds the necessary parent clock definitions to the Aspeed
>> clock driver, with both a mux and clock divider.
>>
>> Eddie James (4):
>> clock: aspeed: Add VIDEO reset index definition
>> clock: aspeed: Setup video engine clocking
>> dt-bindings: media: Add Aspeed Video Engine binding documentation
>> media: platform: Add Aspeed Video Engine driver
>>
>> .../devicetree/bindings/media/aspeed-video.txt | 23 +
>> drivers/clk/clk-aspeed.c | 41 +-
>> drivers/media/platform/Kconfig | 8 +
>> drivers/media/platform/Makefile | 1 +
>> drivers/media/platform/aspeed-video.c | 1307 ++++++++++++++++++++
>> include/dt-bindings/clock/aspeed-clock.h | 1 +
>> 6 files changed, 1379 insertions(+), 2 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/media/aspeed-video.txt
>> create mode 100644 drivers/media/platform/aspeed-video.c
>>
^ permalink raw reply
* [PATCH 4/4] media: platform: Add Aspeed Video Engine driver
From: Eddie James @ 2018-09-13 19:00 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <77243417-e832-dcfa-0eeb-d45f356dd9e8@xs4all.nl>
On 09/03/2018 06:40 AM, Hans Verkuil wrote:
> On 08/29/2018 11:09 PM, Eddie James wrote:
>> The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
>> can capture and compress video data from digital or analog sources. With
>> the Aspeed chip acting a service processor, the Video Engine can capture
>> the host processor graphics output.
>>
>> Add a V4L2 driver to capture video data and compress it to JPEG images,
>> making the data available through a standard read interface.
>>
>> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
>> ---
>> drivers/media/platform/Kconfig | 8 +
>> drivers/media/platform/Makefile | 1 +
>> drivers/media/platform/aspeed-video.c | 1307 +++++++++++++++++++++++++++++++++
> Missing MAINTAINERS file update.
>
>> 3 files changed, 1316 insertions(+)
>> create mode 100644 drivers/media/platform/aspeed-video.c
>> +
>> +static void aspeed_video_init_jpeg_table(u32 *table, bool yuv420)
>> +{
>> + int i;
>> + unsigned int base;
>> +
>> + for (i = 0; i < ASPEED_VIDEO_JPEG_NUM_QUALITIES; i++) {
>> + int j;
>> +
>> + base = 256 * i; /* AST HW requires this header spacing */
>> +
>> + for (j = 0; j < ASPEED_VIDEO_JPEG_HEADER_SIZE; j++)
>> + table[base + j] =
>> + le32_to_cpu(aspeed_video_jpeg_header[j]);
> This doesn't look right. These tables are in cpu format to begin with,
> so le32_to_cpu doesn't make sense.
>
> BTW, what is the endianness of an aspeed SoC?
Hi,
Yes, Aspeed SoCs are LE, it's a standard ARM core. I'll do a simple copy
instead of converting endianness, since this code can really only run on
an Aspeed chip anyway.
>
>> +
>> + base += ASPEED_VIDEO_JPEG_HEADER_SIZE;
>> + for (j = 0; j < ASPEED_VIDEO_JPEG_DCT_SIZE; j++)
>> + table[base + j] =
>> + le32_to_cpu(aspeed_video_jpeg_dct[i][j]);
>> +
>> + base += ASPEED_VIDEO_JPEG_DCT_SIZE;
>> + for (j = 0; j < ASPEED_VIDEO_JPEG_QUANT_SIZE; j++)
>> + table[base + j] =
>> + le32_to_cpu(aspeed_video_jpeg_quant[j]);
>> +
>> + if (yuv420)
>> + table[base + 2] = le32_to_cpu(0x00220103);
>> + }
>> +}
>> +
>> +
>> +static int aspeed_video_querycap(struct file *file, void *fh,
>> + struct v4l2_capability *cap)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + strncpy(cap->driver, DEVICE_NAME, sizeof(cap->driver));
> Use strlcpy.
>
> Also fill in bus_info ("platform:<foo>").
>
>> + cap->capabilities = video->vdev.device_caps | V4L2_CAP_DEVICE_CAPS;
> You can drop this, it's filled in for you.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_get_format(struct file *file, void *fh,
>> + struct v4l2_format *f)
>> +{
>> + int rc;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (test_bit(VIDEO_RES_CHANGE, &video->flags)) {
>> + if (file->f_flags & O_NONBLOCK)
>> + return -EAGAIN;
>> +
>> + rc = wait_event_interruptible(video->wait,
>> + !test_bit(VIDEO_RES_CHANGE,
>> + &video->flags));
>> + if (rc)
>> + return -EINTR;
> No, get_format should always just return the currently set format,
> not the format that is detected.
>
> Use VIDIOC_QUERY_DV_TIMINGS for that.
>
>> + }
>> +
>> + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> Drop this, not needed.
>
>> + f->fmt.pix = video->fmt;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_set_format(struct file *file, void *fh,
>> + struct v4l2_format *f)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (f->fmt.pix.pixelformat == video->fmt.pixelformat)
>> + return 0;
>> +
>> + if (f->fmt.pix.pixelformat == V4L2_PIX_FMT_YUV444) {
>> + video->fmt.pixelformat = V4L2_PIX_FMT_YUV444;
>> + aspeed_video_init_jpeg_table(video->jpeg.virt, false);
>> + aspeed_video_update(video, VE_SEQ_CTRL, ~VE_SEQ_CTRL_YUV420,
>> + 0);
>> + } else if (f->fmt.pix.pixelformat == V4L2_PIX_FMT_YUV420) {
>> + video->fmt.pixelformat = V4L2_PIX_FMT_YUV420;
>> + aspeed_video_init_jpeg_table(video->jpeg.virt, true);
>> + aspeed_video_update(video, VE_SEQ_CTRL, 0xFFFFFFFF,
>> + VE_SEQ_CTRL_YUV420);
> This isn't filling in any of the other fields.
>
>> + } else {
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_get_jpegcomp(struct file *file, void *fh,
>> + struct v4l2_jpegcompression *a)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + a->quality = video->jpeg_quality;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_set_jpegcomp(struct file *file, void *fh,
>> + const struct v4l2_jpegcompression *a)
>> +{
>> + u32 comp_ctrl;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (a->quality < 0 || a->quality > 11)
>> + return -EINVAL;
>> +
>> + video->jpeg_quality = a->quality;
>> + comp_ctrl = FIELD_PREP(VE_COMP_CTRL_DCT_LUM, video->jpeg_quality) |
>> + FIELD_PREP(VE_COMP_CTRL_DCT_CHR, video->jpeg_quality | 0x10);
>> +
>> + aspeed_video_update(video, VE_COMP_CTRL,
>> + ~(VE_COMP_CTRL_DCT_LUM | VE_COMP_CTRL_DCT_CHR),
>> + comp_ctrl);
>> +
>> + return 0;
>> +}
> As the spec says, the jpegcomp ioctls are deprecated and you should use
> JPEG controls instead.
>
> See: https://hverkuil.home.xs4all.nl/spec/uapi/v4l/vidioc-g-jpegcomp.html
>
> I stop reviewing here, since the first thing you need to do is to run
> v4l2-compliance for your device driver.
>
> See my reply to patch 0/4.
>
> Regards,
>
> Hans
Thanks for all the comments! I have addressed these items and will push
up another patch set.
Thanks,
Eddie
>
>> +
>> +static int aspeed_video_get_parm(struct file *file, void *fh,
>> + struct v4l2_streamparm *a)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>>
^ permalink raw reply
* [PATCH] ARM: dts: aspeed: Fix I2C bus warnings
From: Rob Herring @ 2018-09-13 18:12 UTC (permalink / raw)
To: linux-aspeed
dtc has new checks for I2C buses. The ASpeed dts files have a node named
'i2c' which causes a false positive warning. As the node is a 'simple-bus',
correct the node name to be 'bus' to fix the warnings.
arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-ast2500-evb.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-bmc-arm-centriq2400-rep.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-bmc-intel-s2600wf.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-bmc-opp-zaius.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-bmc-portwell-neptune.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dtb: Warning (i2c_bus_bridge): /ahb/apb/i2c at 1e78a000: incorrect #size-cells for I2C bus
Cc: Joel Stanley <joel@jms.id.au>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: linux-aspeed at lists.ozlabs.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
Please apply to the sub-arch tree. The dtc changes haven't landed, but
will for 4.20.
arch/arm/boot/dts/aspeed-g4.dtsi | 2 +-
arch/arm/boot/dts/aspeed-g5.dtsi | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index b23a983f95a5..69f6b9d2e7e7 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -350,7 +350,7 @@
status = "disabled";
};
- i2c: i2c at 1e78a000 {
+ i2c: bus at 1e78a000 {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 87fdc146ff52..d107459fc0f8 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -410,7 +410,7 @@
status = "disabled";
};
- i2c: i2c at 1e78a000 {
+ i2c: bus at 1e78a000 {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
--
2.17.1
^ permalink raw reply related
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-13 17:01 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <798fd5fe-475f-f633-c830-62a3e4f1692d@kaod.org>
On 9/13/2018 9:51 AM, C?dric Le Goater wrote:
> Hello !
>
> On 09/13/2018 06:31 PM, Jae Hyun Yoo wrote:
>> Hi C?dric,
>>
>> On 9/12/2018 10:47 PM, C?dric Le Goater wrote:
>>> On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>>>> interrupt handler is always suspicious and tends to result in race
>>>>>>> conditions (because additional interrupts may have arrived while handling
>>>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>>>> problem for me.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>> ?????? spin_lock(&bus->lock);
>>>>>>> ?????? irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> +??? /* Ack all interrupt bits. */
>>>>>>> +??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> ?????? irq_remaining = irq_received;
>>>>>>> ?? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>> ?????????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>>> ?????????????? irq_received, irq_handled);
>>>>>>> -??? /* Ack all interrupt bits. */
>>>>>>> -??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> ?????? spin_unlock(&bus->lock);
>>>>>>> ?????? return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>>> ?? }
>>>>>>>
>>>>>>
>>>>>> My intention of putting the code at the end of interrupt handler was,
>>>>>> to reduce possibility of combined irq calls which is explained in this
>>>>>> patch. But YES, I agree with you. It could make a potential race
>>>>>
>>>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>>>> interrupts". If additional interrupts arrive while the interrupt handler
>>>>> is active, those will have to be acknowledged separately.
>>>>>
>>>>> Sure, there is a risk that an interrupt arrives while the handler is
>>>>> running, and that it is handled but not acknowledged. That can happen
>>>>> with pretty much all interrupt handlers, and there are mitigations to
>>>>> limit the impact (for example, read the interrupt status register in
>>>>> a loop until no more interrupts are pending). But acknowledging
>>>>> an interrupt that was possibly not handled is always bad idea.
>>>>
>>>> Well, that's generally right but not always. Sometimes that depends on
>>>> hardware and Aspeed I2C is the case.
>>>>
>>>> This is a description from Aspeed AST2500 datasheet:
>>>> ?? I2CD10 Interrupt Status Register
>>>> ?? bit 2 Receive Done Interrupt status
>>>> ???????? S/W needs to clear this status bit to allow next data receiving.
>>>>
>>>> It means, driver should hold this bit to prevent transition of hardware
>>>> state machine until the driver handles received data, so the bit should
>>>> be cleared at the end of interrupt handler.
>>>>
>>>> Let me share my test result. Your code change works on 100KHz bus speed
>>>> but doesn't work well on 1MHz bus speed. Investigated that interrupt
>>>> handling is fast enough in 100KHz test but in 1MHz, most of data is
>>>> corrupted because the bit is cleared at the beginning of interrupt
>>>> handler so it allows receiving of the next data but the interrupt
>>>> handler isn't fast enough to read the data buffer on time. I checked
>>>> this problem on BMC-ME channel which ME sends lots of IPMB packets to
>>>> BMC at 1MHz speed. You could simply check the data corruption problem on
>>>> the BMC-ME channel.
>>>
>>> OK.
>>>
>>>> My thought is, the current code is right for real Aspeed I2C hardware.
>>>> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
>>>> actual Aspeed I2C hardware correctly.
>>>
>>> That might be very well possible yes. it also misses support for the slave
>>> mode and the DMA registers.
>>>
>>
>> Yes, it would be good if qemu's Aspeed I2C model supports slave mode.
>> Since the current linux Aspeed I2C driver supports byte transfer mode
>> only, so DMA transfer mode support in qemu could be considered later.
>
> The Aspeed SDK already does, so yes, we will need to consider it.
>
Yes, Aspeed SDK does but the driver in upstream doesn't at this time.
So we will need to consider it because byte mode makes too many
interrupt calls which is not good for performance.
>> Implementing pool mode and DMA mode for linux Aspeed I2C are in my
>> backlog at this moment.
>
> Is there a QEMU model for an I2C/IPMB device ?
>
Not sure because I'm not familiar with QEMU. Need to check.
Thanks,
Jae
> There is already a large IPMI framework in QEMU, that would be interesting.
> to extend.
>
> Thanks,
>
> C.
>
>
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Cédric Le Goater @ 2018-09-13 16:51 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <7a1706b1-7787-eeff-2295-f6180fe84c6e@linux.intel.com>
Hello !
On 09/13/2018 06:31 PM, Jae Hyun Yoo wrote:
> Hi C?dric,
>
> On 9/12/2018 10:47 PM, C?dric Le Goater wrote:
>> On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote:
>>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>>> interrupt handler is always suspicious and tends to result in race
>>>>>> conditions (because additional interrupts may have arrived while handling
>>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>>> problem for me.
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>> ?????? spin_lock(&bus->lock);
>>>>>> ?????? irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>> +??? /* Ack all interrupt bits. */
>>>>>> +??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>> ?????? irq_remaining = irq_received;
>>>>>> ?? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>> ?????????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>> ?????????????? irq_received, irq_handled);
>>>>>> -??? /* Ack all interrupt bits. */
>>>>>> -??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>> ?????? spin_unlock(&bus->lock);
>>>>>> ?????? return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>> ?? }
>>>>>>
>>>>>
>>>>> My intention of putting the code at the end of interrupt handler was,
>>>>> to reduce possibility of combined irq calls which is explained in this
>>>>> patch. But YES, I agree with you. It could make a potential race
>>>>
>>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>>> interrupts". If additional interrupts arrive while the interrupt handler
>>>> is active, those will have to be acknowledged separately.
>>>>
>>>> Sure, there is a risk that an interrupt arrives while the handler is
>>>> running, and that it is handled but not acknowledged. That can happen
>>>> with pretty much all interrupt handlers, and there are mitigations to
>>>> limit the impact (for example, read the interrupt status register in
>>>> a loop until no more interrupts are pending). But acknowledging
>>>> an interrupt that was possibly not handled is always bad idea.
>>>
>>> Well, that's generally right but not always. Sometimes that depends on
>>> hardware and Aspeed I2C is the case.
>>>
>>> This is a description from Aspeed AST2500 datasheet:
>>> ?? I2CD10 Interrupt Status Register
>>> ?? bit 2 Receive Done Interrupt status
>>> ???????? S/W needs to clear this status bit to allow next data receiving.
>>>
>>> It means, driver should hold this bit to prevent transition of hardware
>>> state machine until the driver handles received data, so the bit should
>>> be cleared at the end of interrupt handler.
>>>
>>> Let me share my test result. Your code change works on 100KHz bus speed
>>> but doesn't work well on 1MHz bus speed. Investigated that interrupt
>>> handling is fast enough in 100KHz test but in 1MHz, most of data is
>>> corrupted because the bit is cleared at the beginning of interrupt
>>> handler so it allows receiving of the next data but the interrupt
>>> handler isn't fast enough to read the data buffer on time. I checked
>>> this problem on BMC-ME channel which ME sends lots of IPMB packets to
>>> BMC at 1MHz speed. You could simply check the data corruption problem on
>>> the BMC-ME channel.
>>
>> OK.
>> ?
>>> My thought is, the current code is right for real Aspeed I2C hardware.
>>> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
>>> actual Aspeed I2C hardware correctly.
>>
>> That might be very well possible yes. it also misses support for the slave
>> mode and the DMA registers.
>>
>
> Yes, it would be good if qemu's Aspeed I2C model supports slave mode.
> Since the current linux Aspeed I2C driver supports byte transfer mode
> only, so DMA transfer mode support in qemu could be considered later.
The Aspeed SDK already does, so yes, we will need to consider it.
> Implementing pool mode and DMA mode for linux Aspeed I2C are in my
> backlog at this moment.
Is there a QEMU model for an I2C/IPMB device ?
There is already a large IPMI framework in QEMU, that would be interesting.
to extend.
Thanks,
C.
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Cédric Le Goater @ 2018-09-13 16:35 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180913155703.GA22605@roeck-us.net>
On 09/13/2018 05:57 PM, Guenter Roeck wrote:
> On Thu, Sep 13, 2018 at 05:48:59PM +0200, C?dric Le Goater wrote:
>> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> [ ... ]
>>>>> ? /*
>>>>> ?? * The state machine needs some refinement. It is only used to track
>>>>> ?? * invalid STOP commands for the moment.
>>>>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>>> ? {
>>>>> ????? bus->cmd &= ~0xFFFF;
>>>>> ????? bus->cmd |= value & 0xFFFF;
>>>>> -??? bus->intr_status = 0;> +??? bus->intr_status &= I2CD_INTR_RX_DONE;
>>>>
>>>> it deserves a comment to understand which scenario we are trying to handle.
>>>> ??
>>>
>>> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
>>> but I neither have the hardware nor a datasheet, so I don't know if any bits
>>> are auto-cleared.
>>
>> I just pushed a patch on my branch with some more explanation :
>>
>> https://github.com/legoater/qemu/commits/aspeed-3.1
>>
> That seems to suggest that none of the status bits auto-clears, and that
> the above code clearing intr_status should be removed entirely.
> Am I missing something ?
You are right. I just pushed another version of the previous patch with this
new hunk :
@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
{
bus->cmd &= ~0xFFFF;
bus->cmd |= value & 0xFFFF;
- bus->intr_status = 0;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
it a try ?
Thanks,
C.
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-13 16:31 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <285ea914-5407-7fde-036d-95978f95a430@kaod.org>
Hi C?dric,
On 9/12/2018 10:47 PM, C?dric Le Goater wrote:
> On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote:
>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>> interrupt handler is always suspicious and tends to result in race
>>>>> conditions (because additional interrupts may have arrived while handling
>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>> problem for me.
>>>>>
>>>>> Guenter
>>>>>
>>>>> ---
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>> ????? spin_lock(&bus->lock);
>>>>> ????? irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>> +??? /* Ack all interrupt bits. */
>>>>> +??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>> ????? irq_remaining = irq_received;
>>>>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>> ????????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>> ????????????? irq_received, irq_handled);
>>>>> -??? /* Ack all interrupt bits. */
>>>>> -??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>> ????? spin_unlock(&bus->lock);
>>>>> ????? return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>> ? }
>>>>>
>>>>
>>>> My intention of putting the code at the end of interrupt handler was,
>>>> to reduce possibility of combined irq calls which is explained in this
>>>> patch. But YES, I agree with you. It could make a potential race
>>>
>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>> interrupts". If additional interrupts arrive while the interrupt handler
>>> is active, those will have to be acknowledged separately.
>>>
>>> Sure, there is a risk that an interrupt arrives while the handler is
>>> running, and that it is handled but not acknowledged. That can happen
>>> with pretty much all interrupt handlers, and there are mitigations to
>>> limit the impact (for example, read the interrupt status register in
>>> a loop until no more interrupts are pending). But acknowledging
>>> an interrupt that was possibly not handled is always bad idea.
>>
>> Well, that's generally right but not always. Sometimes that depends on
>> hardware and Aspeed I2C is the case.
>>
>> This is a description from Aspeed AST2500 datasheet:
>> ? I2CD10 Interrupt Status Register
>> ? bit 2 Receive Done Interrupt status
>> ??????? S/W needs to clear this status bit to allow next data receiving.
>>
>> It means, driver should hold this bit to prevent transition of hardware
>> state machine until the driver handles received data, so the bit should
>> be cleared at the end of interrupt handler.
>>
>> Let me share my test result. Your code change works on 100KHz bus speed
>> but doesn't work well on 1MHz bus speed. Investigated that interrupt
>> handling is fast enough in 100KHz test but in 1MHz, most of data is
>> corrupted because the bit is cleared at the beginning of interrupt
>> handler so it allows receiving of the next data but the interrupt
>> handler isn't fast enough to read the data buffer on time. I checked
>> this problem on BMC-ME channel which ME sends lots of IPMB packets to
>> BMC at 1MHz speed. You could simply check the data corruption problem on
>> the BMC-ME channel.
>
> OK.
>
>> My thought is, the current code is right for real Aspeed I2C hardware.
>> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
>> actual Aspeed I2C hardware correctly.
>
> That might be very well possible yes. it also misses support for the slave
> mode and the DMA registers.
>
Yes, it would be good if qemu's Aspeed I2C model supports slave mode.
Since the current linux Aspeed I2C driver supports byte transfer mode
only, so DMA transfer mode support in qemu could be considered later.
Implementing pool mode and DMA mode for linux Aspeed I2C are in my
backlog at this moment.
Thanks,
Jae
> Thanks for the info,
>
> C.
>
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-13 15:57 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <f47b3050-3abd-caa4-cc71-4b13ef60e2c0@kaod.org>
On Thu, Sep 13, 2018 at 05:48:59PM +0200, C?dric Le Goater wrote:
> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
[ ... ]
> >>> ? /*
> >>> ?? * The state machine needs some refinement. It is only used to track
> >>> ?? * invalid STOP commands for the moment.
> >>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> >>> ? {
> >>> ????? bus->cmd &= ~0xFFFF;
> >>> ????? bus->cmd |= value & 0xFFFF;
> >>> -??? bus->intr_status = 0;> +??? bus->intr_status &= I2CD_INTR_RX_DONE;
> >>
> >> it deserves a comment to understand which scenario we are trying to handle.
> >> ??
> >
> > Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> > but I neither have the hardware nor a datasheet, so I don't know if any bits
> > are auto-cleared.
>
> I just pushed a patch on my branch with some more explanation :
>
> https://github.com/legoater/qemu/commits/aspeed-3.1
>
That seems to suggest that none of the status bits auto-clears, and that
the above code clearing intr_status should be removed entirely.
Am I missing something ?
Thanks,
Guenter
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Cédric Le Goater @ 2018-09-13 15:48 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <3f86e75f-1502-eae8-0633-d087937111c8@roeck-us.net>
On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> On 09/12/2018 10:45 PM, C?dric Le Goater wrote
>
> [ ... ]
>
>>> ---
>>> qemu:
>>>
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index c762c73..0d4aa08 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>>> ????? return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>>> ? }
>>> ? +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
>>> +{
>>> +??? int ret;
>>> +
>>> +??? if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
>>> +??????? return;
>>> +??? }
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>>
>>> +??? if (bus->intr_status & I2CD_INTR_RX_DONE) {
>>> +??????? return;
>>> +??? }
>>
>> should be handled in aspeed_i2c_bus_handle_cmd() I think
>>
>
> I moved those two checks into the calling code.
ok
>>> +??? aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> +??? ret = i2c_recv(bus->bus);
>>> +??? if (ret < 0) {
>>> +??????? qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> +??????? ret = 0xff;
>>> +??? } else {
>>> +??????? bus->intr_status |= I2CD_INTR_RX_DONE;
>>> +??? }
>>> +??? bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> +??? if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> +??????? i2c_nack(bus->bus);
>>> +??? }
>>> +??? bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> +??? aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> +}
>>> +
>>> ? /*
>>> ?? * The state machine needs some refinement. It is only used to track
>>> ?? * invalid STOP commands for the moment.
>>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>> ? {
>>> ????? bus->cmd &= ~0xFFFF;
>>> ????? bus->cmd |= value & 0xFFFF;
>>> -??? bus->intr_status = 0;> +??? bus->intr_status &= I2CD_INTR_RX_DONE;
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>> ??
>
> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> but I neither have the hardware nor a datasheet, so I don't know if any bits
> are auto-cleared.
I just pushed a patch on my branch with some more explanation :
https://github.com/legoater/qemu/commits/aspeed-3.1
>
>>> ????? if (bus->cmd & I2CD_M_START_CMD) {
>>> ????????? uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>> ????? }
>>> ? ????? if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
>>> -??????? int ret;
>>> -
>>> -??????? aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> -??????? ret = i2c_recv(bus->bus);
>>> -??????? if (ret < 0) {
>>> -??????????? qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> -??????????? ret = 0xff;
>>> -??????? } else {
>>> -??????????? bus->intr_status |= I2CD_INTR_RX_DONE;
>>> -??????? }
>>> -??????? bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> -??????? if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> -??????????? i2c_nack(bus->bus);
>>> -??????? }
>>> -??????? bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> -??????? aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> +??????? aspeed_i2c_handle_rx_cmd(bus);
>>> ????? }
>>> ? ????? if (bus->cmd & I2CD_M_STOP_CMD) {
>>> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>> ?????????????????????????????????? uint64_t value, unsigned size)
>>> ? {
>>> ????? AspeedI2CBus *bus = opaque;
>>> +??? int status;
>>> ? ????? switch (offset) {
>>> ????? case I2CD_FUN_CTRL_REG:
>>> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>> ????????? bus->intr_ctrl = value & 0x7FFF;
>>> ????????? break;
>>> ????? case I2CD_INTR_STS_REG:
>>> +??????? status = bus->intr_status;
>>> ????????? bus->intr_status &= ~(value & 0x7FFF);
>>> -??????? bus->controller->intr_status &= ~(1 << bus->id);
>>> -??????? qemu_irq_lower(bus->controller->irq);
>>> +??????? if (!bus->intr_status) {
>>> +??????????? bus->controller->intr_status &= ~(1 << bus->id);
>>> +??????????? qemu_irq_lower(bus->controller->irq);
>>> +??????? }
>>
>> That part below is indeed something to fix. I had a similar patch.
>>
>
> Should I split it out as separate patch ? Alternatively, if you submitted
> your patch already, I'll be happy to use it as base for mine.
See below.
Thanks a lot,
C.
From dea796e8d35ba7a70e4b14fbc30ff970a347b195 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Thu, 13 Sep 2018 17:44:32 +0200
Subject: [PATCH] aspeed/i2c: lower bus interrupt when all interrupts have been
cleared
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Also include some documentation on the interrupt status bits and how
they should be cleared. Also, the model does not implement correctly
the RX_DONE bit behavior which should be cleared to allow more data to
be received. Yet to be fixed.
Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
hw/i2c/aspeed_i2c.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c762c7366ad9..275377c2ab38 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -52,6 +52,13 @@
#define I2CD_AC_TIMING_REG2 0x08 /* Clock and AC Timing Control #1 */
#define I2CD_INTR_CTRL_REG 0x0c /* I2CD Interrupt Control */
#define I2CD_INTR_STS_REG 0x10 /* I2CD Interrupt Status */
+
+#define I2CD_INTR_SLAVE_ADDR_MATCH (0x1 << 31) /* 0: addr1 1: addr2 */
+#define I2CD_INTR_SLAVE_ADDR_RX_PENDING (0x1 << 30)
+/* bits[19-16] Reserved */
+
+/* All bits below are cleared by writing 1 */
+#define I2CD_INTR_SLAVE_INACTIVE_TIMEOUT (0x1 << 15)
#define I2CD_INTR_SDA_DL_TIMEOUT (0x1 << 14)
#define I2CD_INTR_BUS_RECOVER_DONE (0x1 << 13)
#define I2CD_INTR_SMBUS_ALERT (0x1 << 12) /* Bus [0-3] only */
@@ -59,11 +66,16 @@
#define I2CD_INTR_SMBUS_DEV_ALERT_ADDR (0x1 << 10) /* Removed */
#define I2CD_INTR_SMBUS_DEF_ADDR (0x1 << 9) /* Removed */
#define I2CD_INTR_GCALL_ADDR (0x1 << 8) /* Removed */
-#define I2CD_INTR_SLAVE_MATCH (0x1 << 7) /* use RX_DONE */
+#define I2CD_INTR_SLAVE_ADDR_RX_MATCH (0x1 << 7) /* use RX_DONE */
#define I2CD_INTR_SCL_TIMEOUT (0x1 << 6)
#define I2CD_INTR_ABNORMAL (0x1 << 5)
#define I2CD_INTR_NORMAL_STOP (0x1 << 4)
#define I2CD_INTR_ARBIT_LOSS (0x1 << 3)
+
+/*
+ * TODO: handle correctly I2CD_INTR_RX_DONE which needs to be cleared
+ * to allow next data to be received.
+ */
#define I2CD_INTR_RX_DONE (0x1 << 2)
#define I2CD_INTR_TX_NAK (0x1 << 1)
#define I2CD_INTR_TX_ACK (0x1 << 0)
@@ -284,8 +296,10 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
break;
case I2CD_INTR_STS_REG:
bus->intr_status &= ~(value & 0x7FFF);
- bus->controller->intr_status &= ~(1 << bus->id);
- qemu_irq_lower(bus->controller->irq);
+ if (!bus->intr_status) {
+ bus->controller->intr_status &= ~(1 << bus->id);
+ qemu_irq_lower(bus->controller->irq);
+ }
break;
case I2CD_DEV_ADDR_REG:
qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
--
2.17.1
^ permalink raw reply related
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-13 13:33 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <fb185839-3ae0-01dc-09e8-f22db55ecb04@kaod.org>
On 09/12/2018 10:45 PM, C?dric Le Goater wrote
[ ... ]
>> ---
>> qemu:
>>
>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>> index c762c73..0d4aa08 100644
>> --- a/hw/i2c/aspeed_i2c.c
>> +++ b/hw/i2c/aspeed_i2c.c
>> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>> return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>> }
>>
>> +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
>> +{
>> + int ret;
>> +
>> + if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
>> + return;
>> + }
>
> it deserves a comment to understand which scenario we are trying to handle.
>
>> + if (bus->intr_status & I2CD_INTR_RX_DONE) {
>> + return;
>> + }
>
> should be handled in aspeed_i2c_bus_handle_cmd() I think
>
I moved those two checks into the calling code.
>> + aspeed_i2c_set_state(bus, I2CD_MRXD);
>> + ret = i2c_recv(bus->bus);
>> + if (ret < 0) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>> + ret = 0xff;
>> + } else {
>> + bus->intr_status |= I2CD_INTR_RX_DONE;
>> + }
>> + bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>> + if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>> + i2c_nack(bus->bus);
>> + }
>> + bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>> + aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>> +}
>> +
>> /*
>> * The state machine needs some refinement. It is only used to track
>> * invalid STOP commands for the moment.
>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>> {
>> bus->cmd &= ~0xFFFF;
>> bus->cmd |= value & 0xFFFF;
>> - bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
>
> it deserves a comment to understand which scenario we are trying to handle.
>
Ok. FWIW, I wonder if intr_status should be touched here in the first place,
but I neither have the hardware nor a datasheet, so I don't know if any bits
are auto-cleared.
>> if (bus->cmd & I2CD_M_START_CMD) {
>> uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>> }
>>
>> if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
>> - int ret;
>> -
>> - aspeed_i2c_set_state(bus, I2CD_MRXD);
>> - ret = i2c_recv(bus->bus);
>> - if (ret < 0) {
>> - qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>> - ret = 0xff;
>> - } else {
>> - bus->intr_status |= I2CD_INTR_RX_DONE;
>> - }
>> - bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>> - if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>> - i2c_nack(bus->bus);
>> - }
>> - bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>> - aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>> + aspeed_i2c_handle_rx_cmd(bus);
>> }
>>
>> if (bus->cmd & I2CD_M_STOP_CMD) {
>> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>> uint64_t value, unsigned size)
>> {
>> AspeedI2CBus *bus = opaque;
>> + int status;
>>
>> switch (offset) {
>> case I2CD_FUN_CTRL_REG:
>> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>> bus->intr_ctrl = value & 0x7FFF;
>> break;
>> case I2CD_INTR_STS_REG:
>> + status = bus->intr_status;
>> bus->intr_status &= ~(value & 0x7FFF);
>> - bus->controller->intr_status &= ~(1 << bus->id);
>> - qemu_irq_lower(bus->controller->irq);
>> + if (!bus->intr_status) {
>> + bus->controller->intr_status &= ~(1 << bus->id);
>> + qemu_irq_lower(bus->controller->irq);
>> + }
>
> That part below is indeed something to fix. I had a similar patch.
>
Should I split it out as separate patch ? Alternatively, if you submitted
your patch already, I'll be happy to use it as base for mine.
Thanks,
Guenter
>
>> + if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
>> + aspeed_i2c_handle_rx_cmd(bus);
>> + aspeed_i2c_bus_raise_interrupt(bus);
>> + }
>
> ok.
>
> Thanks for looking into this.
>
> C.
>
>> break;
>> case I2CD_DEV_ADDR_REG:
>> qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>>
>
>
^ permalink raw reply
* [PATCH] ARM: dts: aspeed: Add HXT StarDragon 4800 REP2 BMC
From: Yao Yuan @ 2018-09-13 6:06 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACPK8Xc6g9R9SyaJT3XzgKrxzZiV0ccTwAddf5Ot2tzQ0bmeag@mail.gmail.com>
On 13 September 2018 at 11:01, Joel Stanley <joel@jms.id.au> wrote:
> On Thu, 13 Sep 2018 at 11:55, Yuan Yao <yao.yuan@linaro.org> wrote:
>>
>> The HXT StarDragon 4800 REP2 (Reference Evaluation Platform) is
>> an aarch64 ARMv8 server platform with an ast2520 BMC.
>>
>> Signed-off-by: Yuan Yao <yao.yuan@linaro.org>
>
> Looks good. I will apply it to the Aspeed tree in a few days, after
> giving others a chance to review.
>
> Out of interest, are you planning on running OpenBMC on this platform?
Hi Joel,
Thanks for your work.
Yes, I will running OpenBMC on this platform.
And In fact, the OpenBMC basic startup is already running on REP2.
I'm also trying to push the OpenBMC patch for this platform.
> Cheers,
>
> Joel
>
>> ---
>> .../devicetree/bindings/vendor-prefixes.txt | 1 +
>> arch/arm/boot/dts/Makefile | 1 +
>> .../dts/aspeed-bmc-arm-stardragon4800-rep2.dts | 207 +++++++++++++++++++++
>> 3 files changed, 209 insertions(+)
>> create mode 100644 arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
>>
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> index 2c3fc51..c65c0c1 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> @@ -165,6 +165,7 @@ honeywell Honeywell
>> hp Hewlett Packard
>> holtek Holtek Semiconductor, Inc.
>> hwacom HwaCom Systems Inc.
>> +hxt Huaxintong Semiconductor Technology Co., Ltd.
>> i2se I2SE GmbH
>> ibm International Business Machines (IBM)
>> idt Integrated Device Technologies, Inc.
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index b5bd3de..a1b8816 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -1199,6 +1199,7 @@ dtb-$(CONFIG_ARCH_ZX) += zx296702-ad1.dtb
>> dtb-$(CONFIG_ARCH_ASPEED) += \
>> aspeed-ast2500-evb.dtb \
>> aspeed-bmc-arm-centriq2400-rep.dtb \
>> + aspeed-bmc-arm-stardragon4800-rep2.dtb \
>> aspeed-bmc-intel-s2600wf.dtb \
>> aspeed-bmc-opp-lanyang.dtb \
>> aspeed-bmc-opp-palmetto.dtb \
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
>> new file mode 100644
>> index 0000000..83335c0
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
>> @@ -0,0 +1,207 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +#include "aspeed-g5.dtsi"
>> +#include <dt-bindings/gpio/aspeed-gpio.h>
>> +
>> +/ {
>> + model = "HXT StarDragon 4800 REP2 AST2520";
>> + compatible = "hxt,stardragon4800-rep2-bmc", "aspeed,ast2500";
>> +
>> + chosen {
>> + stdout-path = &uart5;
>> + bootargs = "console=ttyS4,115200 earlyprintk";
>> + };
>> +
>> + memory {
>> + reg = <0x80000000 0x40000000>;
>> + };
>> +
>> + iio-hwmon {
>> + compatible = "iio-hwmon";
>> + io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
>> + <&adc 4>, <&adc 5>, <&adc 6>, <&adc 8>;
>> + };
>> +
>> + iio-hwmon-battery {
>> + compatible = "iio-hwmon";
>> + io-channels = <&adc 7>;
>> + };
>> +
>> + leds {
>> + compatible = "gpio-leds";
>> +
>> + system_fault1 {
>> + label = "System_fault1";
>> + gpios = <&gpio ASPEED_GPIO(I, 3) GPIO_ACTIVE_LOW>;
>> + };
>> +
>> + system_fault2 {
>> + label = "System_fault2";
>> + gpios = <&gpio ASPEED_GPIO(I, 2) GPIO_ACTIVE_LOW>;
>> + };
>> + };
>> +};
>> +
>> +&fmc {
>> + status = "okay";
>> + flash at 0 {
>> + status = "okay";
>> + m25p,fast-read;
>> + label = "bmc";
>> +#include "openbmc-flash-layout.dtsi"
>> + };
>> +};
>> +
>> +&spi1 {
>> + status = "okay";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_spi1_default>;
>> + flash at 0 {
>> + status = "okay";
>> + };
>> +};
>> +
>> +&spi2 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_spi2ck_default
>> + &pinctrl_spi2miso_default
>> + &pinctrl_spi2mosi_default
>> + &pinctrl_spi2cs0_default>;
>> +};
>> +
>> +&uart3 {
>> + status = "okay";
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_txd3_default &pinctrl_rxd3_default>;
>> + current-speed = <115200>;
>> +};
>> +
>> +&uart5 {
>> + status = "okay";
>> +};
>> +
>> +&mac0 {
>> + status = "okay";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
>> +};
>> +
>> +&mac1 {
>> + status = "okay";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_rmii2_default>;
>> + use-ncsi;
>> +};
>> +
>> +&i2c0 {
>> + status = "okay";
>> +};
>> +
>> +&i2c1 {
>> + status = "okay";
>> +
>> + tmp421 at 1e {
>> + compatible = "ti,tmp421";
>> + reg = <0x1e>;
>> + };
>> + tmp421 at 2a {
>> + compatible = "ti,tmp421";
>> + reg = <0x2a>;
>> + };
>> + tmp421 at 1c {
>> + compatible = "ti,tmp421";
>> + reg = <0x1c>;
>> + };
>> +};
>> +
>> +&i2c2 {
>> + status = "okay";
>> +};
>> +
>> +&i2c3 {
>> + status = "okay";
>> +};
>> +
>> +&i2c4 {
>> + status = "okay";
>> +};
>> +
>> +&i2c5 {
>> + status = "okay";
>> +};
>> +
>> +&i2c6 {
>> + status = "okay";
>> +
>> + tmp421 at 1f {
>> + compatible = "ti,tmp421";
>> + reg = <0x1f>;
>> + };
>> + nvt210 at 4c {
>> + compatible = "nvt210";
>> + reg = <0x4c>;
>> + };
>> + eeprom at 50 {
>> + compatible = "atmel,24c128";
>> + reg = <0x50>;
>> + pagesize = <128>;
>> + };
>> +};
>> +
>> +&i2c7 {
>> + status = "okay";
>> +};
>> +
>> +&i2c8 {
>> + status = "okay";
>> +
>> + pca9641 at 70 {
>> + compatible = "nxp,pca9641";
>> + reg = <0x70>;
>> + i2c-arb {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + eeprom at 50 {
>> + compatible = "atmel,24c02";
>> + reg = <0x50>;
>> + };
>> + dps650ab at 58 {
>> + compatible = "dps650ab";
>> + reg = <0x58>;
>> + };
>> + };
>> + };
>> +};
>> +
>> +&i2c9 {
>> + status = "okay";
>> +};
>> +
>> +&vuart {
>> + status = "okay";
>> +};
>> +
>> +&gfx {
>> + status = "okay";
>> +};
>> +
>> +&pinctrl {
>> + aspeed,external-nodes = <&gfx &lhc>;
>> +};
>> +
>> +&gpio {
>> + pin_gpio_c7 {
>> + gpio-hog;
>> + gpios = <ASPEED_GPIO(C, 7) GPIO_ACTIVE_HIGH>;
>> + output-low;
>> + line-name = "BIOS_SPI_MUX_S";
>> + };
>> + pin_gpio_d1 {
>> + gpio-hog;
>> + gpios = <ASPEED_GPIO(D, 1) GPIO_ACTIVE_HIGH>;
>> + output-high;
>> + line-name = "PHY2_RESET_N";
>> + };
>> +};
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* [PATCH] ARM: dts: aspeed: Add HXT StarDragon 4800 REP2 BMC
From: Yao Yuan @ 2018-09-13 6:02 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACPK8Xc6g9R9SyaJT3XzgKrxzZiV0ccTwAddf5Ot2tzQ0bmeag@mail.gmail.com>
On 13 September 2018 at 11:01, Joel Stanley <joel@jms.id.au> wrote:
> On Thu, 13 Sep 2018 at 11:55, Yuan Yao <yao.yuan@linaro.org> wrote:
> >
> > The HXT StarDragon 4800 REP2 (Reference Evaluation Platform) is
> > an aarch64 ARMv8 server platform with an ast2520 BMC.
> >
> > Signed-off-by: Yuan Yao <yao.yuan@linaro.org>
>
> Looks good. I will apply it to the Aspeed tree in a few days, after
> giving others a chance to review.
>
> Out of interest, are you planning on running OpenBMC on this platform?
>
Hi Joel,
Thanks for your work.
Yes, I will running OpenBMC on this platform.
And In fact, the OpenBMC basic startup is already running on REP2.
I'm also trying to push the OpenBMC patch for this platform.
>
> Cheers,
> Joel
>
> > ---
> > .../devicetree/bindings/vendor-prefixes.txt | 1 +
> > arch/arm/boot/dts/Makefile | 1 +
> > .../dts/aspeed-bmc-arm-stardragon4800-rep2.dts | 207
> +++++++++++++++++++++
> > 3 files changed, 209 insertions(+)
> > create mode 100644 arch/arm/boot/dts/aspeed-bmc-
> arm-stardragon4800-rep2.dts
> >
> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > index 2c3fc51..c65c0c1 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > @@ -165,6 +165,7 @@ honeywell Honeywell
> > hp Hewlett Packard
> > holtek Holtek Semiconductor, Inc.
> > hwacom HwaCom Systems Inc.
> > +hxt Huaxintong Semiconductor Technology Co., Ltd.
> > i2se I2SE GmbH
> > ibm International Business Machines (IBM)
> > idt Integrated Device Technologies, Inc.
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index b5bd3de..a1b8816 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -1199,6 +1199,7 @@ dtb-$(CONFIG_ARCH_ZX) += zx296702-ad1.dtb
> > dtb-$(CONFIG_ARCH_ASPEED) += \
> > aspeed-ast2500-evb.dtb \
> > aspeed-bmc-arm-centriq2400-rep.dtb \
> > + aspeed-bmc-arm-stardragon4800-rep2.dtb \
> > aspeed-bmc-intel-s2600wf.dtb \
> > aspeed-bmc-opp-lanyang.dtb \
> > aspeed-bmc-opp-palmetto.dtb \
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
> b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
> > new file mode 100644
> > index 0000000..83335c0
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/dts-v1/;
> > +
> > +#include "aspeed-g5.dtsi"
> > +#include <dt-bindings/gpio/aspeed-gpio.h>
> > +
> > +/ {
> > + model = "HXT StarDragon 4800 REP2 AST2520";
> > + compatible = "hxt,stardragon4800-rep2-bmc", "aspeed,ast2500";
> > +
> > + chosen {
> > + stdout-path = &uart5;
> > + bootargs = "console=ttyS4,115200 earlyprintk";
> > + };
> > +
> > + memory {
> > + reg = <0x80000000 0x40000000>;
> > + };
> > +
> > + iio-hwmon {
> > + compatible = "iio-hwmon";
> > + io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> > + <&adc 4>, <&adc 5>,
> <&adc 6>, <&adc 8>;
> > + };
> > +
> > + iio-hwmon-battery {
> > + compatible = "iio-hwmon";
> > + io-channels = <&adc 7>;
> > + };
> > +
> > + leds {
> > + compatible = "gpio-leds";
> > +
> > + system_fault1 {
> > + label = "System_fault1";
> > + gpios = <&gpio ASPEED_GPIO(I, 3)
> GPIO_ACTIVE_LOW>;
> > + };
> > +
> > + system_fault2 {
> > + label = "System_fault2";
> > + gpios = <&gpio ASPEED_GPIO(I, 2)
> GPIO_ACTIVE_LOW>;
> > + };
> > + };
> > +};
> > +
> > +&fmc {
> > + status = "okay";
> > + flash at 0 {
> > + status = "okay";
> > + m25p,fast-read;
> > + label = "bmc";
> > +#include "openbmc-flash-layout.dtsi"
> > + };
> > +};
> > +
> > +&spi1 {
> > + status = "okay";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_spi1_default>;
> > + flash at 0 {
> > + status = "okay";
> > + };
> > +};
> > +
> > +&spi2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_spi2ck_default
> > + &pinctrl_spi2miso_default
> > + &pinctrl_spi2mosi_default
> > + &pinctrl_spi2cs0_default>;
> > +};
> > +
> > +&uart3 {
> > + status = "okay";
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_txd3_default &pinctrl_rxd3_default>;
> > + current-speed = <115200>;
> > +};
> > +
> > +&uart5 {
> > + status = "okay";
> > +};
> > +
> > +&mac0 {
> > + status = "okay";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
> > +};
> > +
> > +&mac1 {
> > + status = "okay";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_rmii2_default>;
> > + use-ncsi;
> > +};
> > +
> > +&i2c0 {
> > + status = "okay";
> > +};
> > +
> > +&i2c1 {
> > + status = "okay";
> > +
> > + tmp421 at 1e {
> > + compatible = "ti,tmp421";
> > + reg = <0x1e>;
> > + };
> > + tmp421 at 2a {
> > + compatible = "ti,tmp421";
> > + reg = <0x2a>;
> > + };
> > + tmp421 at 1c {
> > + compatible = "ti,tmp421";
> > + reg = <0x1c>;
> > + };
> > +};
> > +
> > +&i2c2 {
> > + status = "okay";
> > +};
> > +
> > +&i2c3 {
> > + status = "okay";
> > +};
> > +
> > +&i2c4 {
> > + status = "okay";
> > +};
> > +
> > +&i2c5 {
> > + status = "okay";
> > +};
> > +
> > +&i2c6 {
> > + status = "okay";
> > +
> > + tmp421 at 1f {
> > + compatible = "ti,tmp421";
> > + reg = <0x1f>;
> > + };
> > + nvt210 at 4c {
> > + compatible = "nvt210";
> > + reg = <0x4c>;
> > + };
> > + eeprom at 50 {
> > + compatible = "atmel,24c128";
> > + reg = <0x50>;
> > + pagesize = <128>;
> > + };
> > +};
> > +
> > +&i2c7 {
> > + status = "okay";
> > +};
> > +
> > +&i2c8 {
> > + status = "okay";
> > +
> > + pca9641 at 70 {
> > + compatible = "nxp,pca9641";
> > + reg = <0x70>;
> > + i2c-arb {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + eeprom at 50 {
> > + compatible = "atmel,24c02";
> > + reg = <0x50>;
> > + };
> > + dps650ab at 58 {
> > + compatible = "dps650ab";
> > + reg = <0x58>;
> > + };
> > + };
> > + };
> > +};
> > +
> > +&i2c9 {
> > + status = "okay";
> > +};
> > +
> > +&vuart {
> > + status = "okay";
> > +};
> > +
> > +&gfx {
> > + status = "okay";
> > +};
> > +
> > +&pinctrl {
> > + aspeed,external-nodes = <&gfx &lhc>;
> > +};
> > +
> > +&gpio {
> > + pin_gpio_c7 {
> > + gpio-hog;
> > + gpios = <ASPEED_GPIO(C, 7) GPIO_ACTIVE_HIGH>;
> > + output-low;
> > + line-name = "BIOS_SPI_MUX_S";
> > + };
> > + pin_gpio_d1 {
> > + gpio-hog;
> > + gpios = <ASPEED_GPIO(D, 1) GPIO_ACTIVE_HIGH>;
> > + output-high;
> > + line-name = "PHY2_RESET_N";
> > + };
> > +};
> > --
> > 1.8.3.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20180913/5f8edb5c/attachment-0001.html>
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Cédric Le Goater @ 2018-09-13 5:47 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <7fd98646-fb5a-be4d-ce37-84b74e0fa8b3@linux.intel.com>
On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote:
> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>> interrupt handler is always suspicious and tends to result in race
>>>> conditions (because additional interrupts may have arrived while handling
>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>> another interrupt). With that in mind, the following patch fixes the
>>>> problem for me.
>>>>
>>>> Guenter
>>>>
>>>> ---
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>> ????? spin_lock(&bus->lock);
>>>> ????? irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>> +??? /* Ack all interrupt bits. */
>>>> +??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>> ????? irq_remaining = irq_received;
>>>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>> ????????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>> ????????????? irq_received, irq_handled);
>>>> -??? /* Ack all interrupt bits. */
>>>> -??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>> ????? spin_unlock(&bus->lock);
>>>> ????? return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>> ? }
>>>>
>>>
>>> My intention of putting the code at the end of interrupt handler was,
>>> to reduce possibility of combined irq calls which is explained in this
>>> patch. But YES, I agree with you. It could make a potential race
>>
>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>> the interrupt late. The interrupt ack only means "I am going to handle these
>> interrupts". If additional interrupts arrive while the interrupt handler
>> is active, those will have to be acknowledged separately.
>>
>> Sure, there is a risk that an interrupt arrives while the handler is
>> running, and that it is handled but not acknowledged. That can happen
>> with pretty much all interrupt handlers, and there are mitigations to
>> limit the impact (for example, read the interrupt status register in
>> a loop until no more interrupts are pending). But acknowledging
>> an interrupt that was possibly not handled is always bad idea.
>
> Well, that's generally right but not always. Sometimes that depends on
> hardware and Aspeed I2C is the case.
>
> This is a description from Aspeed AST2500 datasheet:
> ? I2CD10 Interrupt Status Register
> ? bit 2 Receive Done Interrupt status
> ??????? S/W needs to clear this status bit to allow next data receiving.
>
> It means, driver should hold this bit to prevent transition of hardware
> state machine until the driver handles received data, so the bit should
> be cleared at the end of interrupt handler.
>
> Let me share my test result. Your code change works on 100KHz bus speed
> but doesn't work well on 1MHz bus speed. Investigated that interrupt
> handling is fast enough in 100KHz test but in 1MHz, most of data is
> corrupted because the bit is cleared at the beginning of interrupt
> handler so it allows receiving of the next data but the interrupt
> handler isn't fast enough to read the data buffer on time. I checked
> this problem on BMC-ME channel which ME sends lots of IPMB packets to
> BMC at 1MHz speed. You could simply check the data corruption problem on
> the BMC-ME channel.
OK.
> My thought is, the current code is right for real Aspeed I2C hardware.
> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
> actual Aspeed I2C hardware correctly.
That might be very well possible yes. it also misses support for the slave
mode and the DMA registers.
Thanks for the info,
C.
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Cédric Le Goater @ 2018-09-13 5:45 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180912203059.GA18201@roeck-us.net>
On 09/12/2018 10:30 PM, Guenter Roeck wrote:
> On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote:
>> On 9/12/2018 12:58 PM, Guenter Roeck wrote:
>>> On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>>>> interrupt handler is always suspicious and tends to result in race
>>>>>>> conditions (because additional interrupts may have arrived while handling
>>>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>>>> problem for me.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>> spin_lock(&bus->lock);
>>>>>>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> + /* Ack all interrupt bits. */
>>>>>>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> irq_remaining = irq_received;
>>>>>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>>> irq_received, irq_handled);
>>>>>>> - /* Ack all interrupt bits. */
>>>>>>> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> spin_unlock(&bus->lock);
>>>>>>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>> My intention of putting the code at the end of interrupt handler was,
>>>>>> to reduce possibility of combined irq calls which is explained in this
>>>>>> patch. But YES, I agree with you. It could make a potential race
>>>>>
>>>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>>>> interrupts". If additional interrupts arrive while the interrupt handler
>>>>> is active, those will have to be acknowledged separately.
>>>>>
>>>>> Sure, there is a risk that an interrupt arrives while the handler is
>>>>> running, and that it is handled but not acknowledged. That can happen
>>>>> with pretty much all interrupt handlers, and there are mitigations to
>>>>> limit the impact (for example, read the interrupt status register in
>>>>> a loop until no more interrupts are pending). But acknowledging
>>>>> an interrupt that was possibly not handled is always bad idea.
>>>>
>>>> Well, that's generally right but not always. Sometimes that depends on
>>>> hardware and Aspeed I2C is the case.
>>>>
>>>> This is a description from Aspeed AST2500 datasheet:
>>>> I2CD10 Interrupt Status Register
>>>> bit 2 Receive Done Interrupt status
>>>> S/W needs to clear this status bit to allow next data receiving.
>>>>
>>>> It means, driver should hold this bit to prevent transition of hardware
>>>> state machine until the driver handles received data, so the bit should
>>>> be cleared at the end of interrupt handler.
>>>>
>>> That makes sense. Does that apply to the other status bits as well ?
>>> Reason for asking is that the current code actually gets stuck
>>> in transmit, not receive.
>>>
>> Only bit 2 has that description in datasheet. Is slave config enabled
>> for QEMU build? Does that get stuck in master sending or slave
>> receiving?
>>
> qemu does not support slave mode. Linux gets stuck in master tx.
>
> I played with the code on both sides. I had to make changes in both
> the linux kernel and in qemu to get the code to work again.
> See attached.
>
> Guenter
>
> ---
> Linux:
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..3d518e09369f 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>
> spin_lock(&bus->lock);
> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + /* Ack all interrupts except for Rx done */
> + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> + bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> irq_received, irq_handled);
>
> - /* Ack all interrupt bits. */
> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> + /* Ack Rx done */
> + if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> + writel(ASPEED_I2CD_INTR_RX_DONE,
> + bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
>
> ---
> qemu:
>
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index c762c73..0d4aa08 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
> return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
> }
>
> +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
> +{
> + int ret;
> +
> + if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> + return;
> + }
it deserves a comment to understand which scenario we are trying to handle.
> + if (bus->intr_status & I2CD_INTR_RX_DONE) {
> + return;
> + }
should be handled in aspeed_i2c_bus_handle_cmd() I think
> + aspeed_i2c_set_state(bus, I2CD_MRXD);
> + ret = i2c_recv(bus->bus);
> + if (ret < 0) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> + ret = 0xff;
> + } else {
> + bus->intr_status |= I2CD_INTR_RX_DONE;
> + }
> + bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> + if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> + i2c_nack(bus->bus);
> + }
> + bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> + aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> +}
> +
> /*
> * The state machine needs some refinement. It is only used to track
> * invalid STOP commands for the moment.
> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> {
> bus->cmd &= ~0xFFFF;
> bus->cmd |= value & 0xFFFF;
> - bus->intr_status = 0;> + bus->intr_status &= I2CD_INTR_RX_DONE;
it deserves a comment to understand which scenario we are trying to handle.
> if (bus->cmd & I2CD_M_START_CMD) {
> uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> }
>
> if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> - int ret;
> -
> - aspeed_i2c_set_state(bus, I2CD_MRXD);
> - ret = i2c_recv(bus->bus);
> - if (ret < 0) {
> - qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> - ret = 0xff;
> - } else {
> - bus->intr_status |= I2CD_INTR_RX_DONE;
> - }
> - bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> - if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> - i2c_nack(bus->bus);
> - }
> - bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> - aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> + aspeed_i2c_handle_rx_cmd(bus);
> }
>
> if (bus->cmd & I2CD_M_STOP_CMD) {
> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> uint64_t value, unsigned size)
> {
> AspeedI2CBus *bus = opaque;
> + int status;
>
> switch (offset) {
> case I2CD_FUN_CTRL_REG:
> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> bus->intr_ctrl = value & 0x7FFF;
> break;
> case I2CD_INTR_STS_REG:
> + status = bus->intr_status;
> bus->intr_status &= ~(value & 0x7FFF);
> - bus->controller->intr_status &= ~(1 << bus->id);
> - qemu_irq_lower(bus->controller->irq);
> + if (!bus->intr_status) {
> + bus->controller->intr_status &= ~(1 << bus->id);
> + qemu_irq_lower(bus->controller->irq);
> + }
That part below is indeed something to fix. I had a similar patch.
> + if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
> + aspeed_i2c_handle_rx_cmd(bus);
> + aspeed_i2c_bus_raise_interrupt(bus);
> + }
ok.
Thanks for looking into this.
C.
> break;
> case I2CD_DEV_ADDR_REG:
> qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>
^ permalink raw reply
* [PATCH] ARM: dts: aspeed: Add HXT StarDragon 4800 REP2 BMC
From: Joel Stanley @ 2018-09-13 3:01 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1536805516-21484-1-git-send-email-yao.yuan@linaro.org>
On Thu, 13 Sep 2018 at 11:55, Yuan Yao <yao.yuan@linaro.org> wrote:
>
> The HXT StarDragon 4800 REP2 (Reference Evaluation Platform) is
> an aarch64 ARMv8 server platform with an ast2520 BMC.
>
> Signed-off-by: Yuan Yao <yao.yuan@linaro.org>
Looks good. I will apply it to the Aspeed tree in a few days, after
giving others a chance to review.
Out of interest, are you planning on running OpenBMC on this platform?
Cheers,
Joel
> ---
> .../devicetree/bindings/vendor-prefixes.txt | 1 +
> arch/arm/boot/dts/Makefile | 1 +
> .../dts/aspeed-bmc-arm-stardragon4800-rep2.dts | 207 +++++++++++++++++++++
> 3 files changed, 209 insertions(+)
> create mode 100644 arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 2c3fc51..c65c0c1 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -165,6 +165,7 @@ honeywell Honeywell
> hp Hewlett Packard
> holtek Holtek Semiconductor, Inc.
> hwacom HwaCom Systems Inc.
> +hxt Huaxintong Semiconductor Technology Co., Ltd.
> i2se I2SE GmbH
> ibm International Business Machines (IBM)
> idt Integrated Device Technologies, Inc.
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index b5bd3de..a1b8816 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1199,6 +1199,7 @@ dtb-$(CONFIG_ARCH_ZX) += zx296702-ad1.dtb
> dtb-$(CONFIG_ARCH_ASPEED) += \
> aspeed-ast2500-evb.dtb \
> aspeed-bmc-arm-centriq2400-rep.dtb \
> + aspeed-bmc-arm-stardragon4800-rep2.dtb \
> aspeed-bmc-intel-s2600wf.dtb \
> aspeed-bmc-opp-lanyang.dtb \
> aspeed-bmc-opp-palmetto.dtb \
> diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
> new file mode 100644
> index 0000000..83335c0
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +#include "aspeed-g5.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> + model = "HXT StarDragon 4800 REP2 AST2520";
> + compatible = "hxt,stardragon4800-rep2-bmc", "aspeed,ast2500";
> +
> + chosen {
> + stdout-path = &uart5;
> + bootargs = "console=ttyS4,115200 earlyprintk";
> + };
> +
> + memory {
> + reg = <0x80000000 0x40000000>;
> + };
> +
> + iio-hwmon {
> + compatible = "iio-hwmon";
> + io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> + <&adc 4>, <&adc 5>, <&adc 6>, <&adc 8>;
> + };
> +
> + iio-hwmon-battery {
> + compatible = "iio-hwmon";
> + io-channels = <&adc 7>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> +
> + system_fault1 {
> + label = "System_fault1";
> + gpios = <&gpio ASPEED_GPIO(I, 3) GPIO_ACTIVE_LOW>;
> + };
> +
> + system_fault2 {
> + label = "System_fault2";
> + gpios = <&gpio ASPEED_GPIO(I, 2) GPIO_ACTIVE_LOW>;
> + };
> + };
> +};
> +
> +&fmc {
> + status = "okay";
> + flash at 0 {
> + status = "okay";
> + m25p,fast-read;
> + label = "bmc";
> +#include "openbmc-flash-layout.dtsi"
> + };
> +};
> +
> +&spi1 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_spi1_default>;
> + flash at 0 {
> + status = "okay";
> + };
> +};
> +
> +&spi2 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_spi2ck_default
> + &pinctrl_spi2miso_default
> + &pinctrl_spi2mosi_default
> + &pinctrl_spi2cs0_default>;
> +};
> +
> +&uart3 {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_txd3_default &pinctrl_rxd3_default>;
> + current-speed = <115200>;
> +};
> +
> +&uart5 {
> + status = "okay";
> +};
> +
> +&mac0 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
> +};
> +
> +&mac1 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_rmii2_default>;
> + use-ncsi;
> +};
> +
> +&i2c0 {
> + status = "okay";
> +};
> +
> +&i2c1 {
> + status = "okay";
> +
> + tmp421 at 1e {
> + compatible = "ti,tmp421";
> + reg = <0x1e>;
> + };
> + tmp421 at 2a {
> + compatible = "ti,tmp421";
> + reg = <0x2a>;
> + };
> + tmp421 at 1c {
> + compatible = "ti,tmp421";
> + reg = <0x1c>;
> + };
> +};
> +
> +&i2c2 {
> + status = "okay";
> +};
> +
> +&i2c3 {
> + status = "okay";
> +};
> +
> +&i2c4 {
> + status = "okay";
> +};
> +
> +&i2c5 {
> + status = "okay";
> +};
> +
> +&i2c6 {
> + status = "okay";
> +
> + tmp421 at 1f {
> + compatible = "ti,tmp421";
> + reg = <0x1f>;
> + };
> + nvt210 at 4c {
> + compatible = "nvt210";
> + reg = <0x4c>;
> + };
> + eeprom at 50 {
> + compatible = "atmel,24c128";
> + reg = <0x50>;
> + pagesize = <128>;
> + };
> +};
> +
> +&i2c7 {
> + status = "okay";
> +};
> +
> +&i2c8 {
> + status = "okay";
> +
> + pca9641 at 70 {
> + compatible = "nxp,pca9641";
> + reg = <0x70>;
> + i2c-arb {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + eeprom at 50 {
> + compatible = "atmel,24c02";
> + reg = <0x50>;
> + };
> + dps650ab at 58 {
> + compatible = "dps650ab";
> + reg = <0x58>;
> + };
> + };
> + };
> +};
> +
> +&i2c9 {
> + status = "okay";
> +};
> +
> +&vuart {
> + status = "okay";
> +};
> +
> +&gfx {
> + status = "okay";
> +};
> +
> +&pinctrl {
> + aspeed,external-nodes = <&gfx &lhc>;
> +};
> +
> +&gpio {
> + pin_gpio_c7 {
> + gpio-hog;
> + gpios = <ASPEED_GPIO(C, 7) GPIO_ACTIVE_HIGH>;
> + output-low;
> + line-name = "BIOS_SPI_MUX_S";
> + };
> + pin_gpio_d1 {
> + gpio-hog;
> + gpios = <ASPEED_GPIO(D, 1) GPIO_ACTIVE_HIGH>;
> + output-high;
> + line-name = "PHY2_RESET_N";
> + };
> +};
> --
> 1.8.3.1
>
^ permalink raw reply
* [PATCH] ARM: dts: aspeed: Add HXT StarDragon 4800 REP2 BMC
From: Yuan Yao @ 2018-09-13 2:25 UTC (permalink / raw)
To: linux-aspeed
The HXT StarDragon 4800 REP2 (Reference Evaluation Platform) is
an aarch64 ARMv8 server platform with an ast2520 BMC.
Signed-off-by: Yuan Yao <yao.yuan@linaro.org>
---
.../devicetree/bindings/vendor-prefixes.txt | 1 +
arch/arm/boot/dts/Makefile | 1 +
.../dts/aspeed-bmc-arm-stardragon4800-rep2.dts | 207 +++++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 2c3fc51..c65c0c1 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -165,6 +165,7 @@ honeywell Honeywell
hp Hewlett Packard
holtek Holtek Semiconductor, Inc.
hwacom HwaCom Systems Inc.
+hxt Huaxintong Semiconductor Technology Co., Ltd.
i2se I2SE GmbH
ibm International Business Machines (IBM)
idt Integrated Device Technologies, Inc.
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index b5bd3de..a1b8816 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1199,6 +1199,7 @@ dtb-$(CONFIG_ARCH_ZX) += zx296702-ad1.dtb
dtb-$(CONFIG_ARCH_ASPEED) += \
aspeed-ast2500-evb.dtb \
aspeed-bmc-arm-centriq2400-rep.dtb \
+ aspeed-bmc-arm-stardragon4800-rep2.dtb \
aspeed-bmc-intel-s2600wf.dtb \
aspeed-bmc-opp-lanyang.dtb \
aspeed-bmc-opp-palmetto.dtb \
diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
new file mode 100644
index 0000000..83335c0
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+#include "aspeed-g5.dtsi"
+#include <dt-bindings/gpio/aspeed-gpio.h>
+
+/ {
+ model = "HXT StarDragon 4800 REP2 AST2520";
+ compatible = "hxt,stardragon4800-rep2-bmc", "aspeed,ast2500";
+
+ chosen {
+ stdout-path = &uart5;
+ bootargs = "console=ttyS4,115200 earlyprintk";
+ };
+
+ memory {
+ reg = <0x80000000 0x40000000>;
+ };
+
+ iio-hwmon {
+ compatible = "iio-hwmon";
+ io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
+ <&adc 4>, <&adc 5>, <&adc 6>, <&adc 8>;
+ };
+
+ iio-hwmon-battery {
+ compatible = "iio-hwmon";
+ io-channels = <&adc 7>;
+ };
+
+ leds {
+ compatible = "gpio-leds";
+
+ system_fault1 {
+ label = "System_fault1";
+ gpios = <&gpio ASPEED_GPIO(I, 3) GPIO_ACTIVE_LOW>;
+ };
+
+ system_fault2 {
+ label = "System_fault2";
+ gpios = <&gpio ASPEED_GPIO(I, 2) GPIO_ACTIVE_LOW>;
+ };
+ };
+};
+
+&fmc {
+ status = "okay";
+ flash at 0 {
+ status = "okay";
+ m25p,fast-read;
+ label = "bmc";
+#include "openbmc-flash-layout.dtsi"
+ };
+};
+
+&spi1 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_spi1_default>;
+ flash at 0 {
+ status = "okay";
+ };
+};
+
+&spi2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_spi2ck_default
+ &pinctrl_spi2miso_default
+ &pinctrl_spi2mosi_default
+ &pinctrl_spi2cs0_default>;
+};
+
+&uart3 {
+ status = "okay";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_txd3_default &pinctrl_rxd3_default>;
+ current-speed = <115200>;
+};
+
+&uart5 {
+ status = "okay";
+};
+
+&mac0 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
+};
+
+&mac1 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_rmii2_default>;
+ use-ncsi;
+};
+
+&i2c0 {
+ status = "okay";
+};
+
+&i2c1 {
+ status = "okay";
+
+ tmp421 at 1e {
+ compatible = "ti,tmp421";
+ reg = <0x1e>;
+ };
+ tmp421 at 2a {
+ compatible = "ti,tmp421";
+ reg = <0x2a>;
+ };
+ tmp421 at 1c {
+ compatible = "ti,tmp421";
+ reg = <0x1c>;
+ };
+};
+
+&i2c2 {
+ status = "okay";
+};
+
+&i2c3 {
+ status = "okay";
+};
+
+&i2c4 {
+ status = "okay";
+};
+
+&i2c5 {
+ status = "okay";
+};
+
+&i2c6 {
+ status = "okay";
+
+ tmp421 at 1f {
+ compatible = "ti,tmp421";
+ reg = <0x1f>;
+ };
+ nvt210 at 4c {
+ compatible = "nvt210";
+ reg = <0x4c>;
+ };
+ eeprom at 50 {
+ compatible = "atmel,24c128";
+ reg = <0x50>;
+ pagesize = <128>;
+ };
+};
+
+&i2c7 {
+ status = "okay";
+};
+
+&i2c8 {
+ status = "okay";
+
+ pca9641 at 70 {
+ compatible = "nxp,pca9641";
+ reg = <0x70>;
+ i2c-arb {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ eeprom at 50 {
+ compatible = "atmel,24c02";
+ reg = <0x50>;
+ };
+ dps650ab at 58 {
+ compatible = "dps650ab";
+ reg = <0x58>;
+ };
+ };
+ };
+};
+
+&i2c9 {
+ status = "okay";
+};
+
+&vuart {
+ status = "okay";
+};
+
+&gfx {
+ status = "okay";
+};
+
+&pinctrl {
+ aspeed,external-nodes = <&gfx &lhc>;
+};
+
+&gpio {
+ pin_gpio_c7 {
+ gpio-hog;
+ gpios = <ASPEED_GPIO(C, 7) GPIO_ACTIVE_HIGH>;
+ output-low;
+ line-name = "BIOS_SPI_MUX_S";
+ };
+ pin_gpio_d1 {
+ gpio-hog;
+ gpios = <ASPEED_GPIO(D, 1) GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "PHY2_RESET_N";
+ };
+};
--
1.8.3.1
^ permalink raw reply related
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-12 23:30 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <6a99dc09-b06d-88ee-2843-3bd88f883dc2@linux.intel.com>
On Wed, Sep 12, 2018 at 03:31:06PM -0700, Jae Hyun Yoo wrote:
> >
> >I played with the code on both sides. I had to make changes in both
> >the linux kernel and in qemu to get the code to work again.
> >See attached.
> >
> >Guenter
> >
> >---
> >Linux:
> >
> >diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >index c258c4d9a4c0..3d518e09369f 100644
> >--- a/drivers/i2c/busses/i2c-aspeed.c
> >+++ b/drivers/i2c/busses/i2c-aspeed.c
> >@@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> > spin_lock(&bus->lock);
> > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >+ /* Ack all interrupts except for Rx done */
> >+ writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> >+ bus->base + ASPEED_I2C_INTR_STS_REG);
> > irq_remaining = irq_received;
> > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >@@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> > "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> > irq_received, irq_handled);
> >- /* Ack all interrupt bits. */
> >- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >+ /* Ack Rx done */
> >+ if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> >+ writel(ASPEED_I2CD_INTR_RX_DONE,
> >+ bus->base + ASPEED_I2C_INTR_STS_REG);
> > spin_unlock(&bus->lock);
> > return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> > }
> >
> >---
> >qemu:
> >
> >diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> >index c762c73..0d4aa08 100644
> >--- a/hw/i2c/aspeed_i2c.c
> >+++ b/hw/i2c/aspeed_i2c.c
> >@@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
> > return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
> > }
> >+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
> >+{
> >+ int ret;
> >+
> >+ if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> >+ return;
> >+ }
> >+ if (bus->intr_status & I2CD_INTR_RX_DONE) {
> >+ return;
> >+ }
> >+
> >+ aspeed_i2c_set_state(bus, I2CD_MRXD);
> >+ ret = i2c_recv(bus->bus);
> >+ if (ret < 0) {
> >+ qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> >+ ret = 0xff;
> >+ } else {
> >+ bus->intr_status |= I2CD_INTR_RX_DONE;
> >+ }
> >+ bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> >+ if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> >+ i2c_nack(bus->bus);
> >+ }
> >+ bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> >+ aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> >+}
> >+
> > /*
> > * The state machine needs some refinement. It is only used to track
> > * invalid STOP commands for the moment.
> >@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > {
> > bus->cmd &= ~0xFFFF;
> > bus->cmd |= value & 0xFFFF;
> >- bus->intr_status = 0;
> >+ bus->intr_status &= I2CD_INTR_RX_DONE;
> > if (bus->cmd & I2CD_M_START_CMD) {
> > uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> >@@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> > }
> > if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> >- int ret;
> >-
> >- aspeed_i2c_set_state(bus, I2CD_MRXD);
> >- ret = i2c_recv(bus->bus);
> >- if (ret < 0) {
> >- qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> >- ret = 0xff;
> >- } else {
> >- bus->intr_status |= I2CD_INTR_RX_DONE;
> >- }
> >- bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> >- if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> >- i2c_nack(bus->bus);
> >- }
> >- bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> >- aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> >+ aspeed_i2c_handle_rx_cmd(bus);
> > }
> > if (bus->cmd & I2CD_M_STOP_CMD) {
> >@@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> > uint64_t value, unsigned size)
> > {
> > AspeedI2CBus *bus = opaque;
> >+ int status;
> > switch (offset) {
> > case I2CD_FUN_CTRL_REG:
> >@@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> > bus->intr_ctrl = value & 0x7FFF;
> > break;
> > case I2CD_INTR_STS_REG:
> >+ status = bus->intr_status;
> > bus->intr_status &= ~(value & 0x7FFF);
> >- bus->controller->intr_status &= ~(1 << bus->id);
> >- qemu_irq_lower(bus->controller->irq);
> >+ if (!bus->intr_status) {
> >+ bus->controller->intr_status &= ~(1 << bus->id);
> >+ qemu_irq_lower(bus->controller->irq);
> >+ }
> >+ if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
> >+ aspeed_i2c_handle_rx_cmd(bus);
> >+ aspeed_i2c_bus_raise_interrupt(bus);
> >+ }
> > break;
> > case I2CD_DEV_ADDR_REG:
> > qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> >
>
> Nice fix! LGTM. I've tested the new patch and checked that it works well
> on both low and high bus speed environments. Thanks a lot!
>
> Can you please submit this patch?
>
Assuming you mean both patches, sure, can do. I'll need to clean up
the qemu patch a bit, though; it looks too messy for my liking.
Guenter
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-12 22:31 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180912203059.GA18201@roeck-us.net>
On 9/12/2018 1:30 PM, Guenter Roeck wrote:
> On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote:
>> On 9/12/2018 12:58 PM, Guenter Roeck wrote:
>>> On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>>>> interrupt handler is always suspicious and tends to result in race
>>>>>>> conditions (because additional interrupts may have arrived while handling
>>>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>>>> problem for me.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>> spin_lock(&bus->lock);
>>>>>>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> + /* Ack all interrupt bits. */
>>>>>>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> irq_remaining = irq_received;
>>>>>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>>> irq_received, irq_handled);
>>>>>>> - /* Ack all interrupt bits. */
>>>>>>> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> spin_unlock(&bus->lock);
>>>>>>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>> My intention of putting the code at the end of interrupt handler was,
>>>>>> to reduce possibility of combined irq calls which is explained in this
>>>>>> patch. But YES, I agree with you. It could make a potential race
>>>>>
>>>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>>>> interrupts". If additional interrupts arrive while the interrupt handler
>>>>> is active, those will have to be acknowledged separately.
>>>>>
>>>>> Sure, there is a risk that an interrupt arrives while the handler is
>>>>> running, and that it is handled but not acknowledged. That can happen
>>>>> with pretty much all interrupt handlers, and there are mitigations to
>>>>> limit the impact (for example, read the interrupt status register in
>>>>> a loop until no more interrupts are pending). But acknowledging
>>>>> an interrupt that was possibly not handled is always bad idea.
>>>>
>>>> Well, that's generally right but not always. Sometimes that depends on
>>>> hardware and Aspeed I2C is the case.
>>>>
>>>> This is a description from Aspeed AST2500 datasheet:
>>>> I2CD10 Interrupt Status Register
>>>> bit 2 Receive Done Interrupt status
>>>> S/W needs to clear this status bit to allow next data receiving.
>>>>
>>>> It means, driver should hold this bit to prevent transition of hardware
>>>> state machine until the driver handles received data, so the bit should
>>>> be cleared at the end of interrupt handler.
>>>>
>>> That makes sense. Does that apply to the other status bits as well ?
>>> Reason for asking is that the current code actually gets stuck
>>> in transmit, not receive.
>>>
>> Only bit 2 has that description in datasheet. Is slave config enabled
>> for QEMU build? Does that get stuck in master sending or slave
>> receiving?
>>
> qemu does not support slave mode. Linux gets stuck in master tx.
>
> I played with the code on both sides. I had to make changes in both
> the linux kernel and in qemu to get the code to work again.
> See attached.
>
> Guenter
>
> ---
> Linux:
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..3d518e09369f 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>
> spin_lock(&bus->lock);
> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + /* Ack all interrupts except for Rx done */
> + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> + bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> irq_received, irq_handled);
>
> - /* Ack all interrupt bits. */
> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> + /* Ack Rx done */
> + if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> + writel(ASPEED_I2CD_INTR_RX_DONE,
> + bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
>
> ---
> qemu:
>
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index c762c73..0d4aa08 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
> return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
> }
>
> +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
> +{
> + int ret;
> +
> + if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> + return;
> + }
> + if (bus->intr_status & I2CD_INTR_RX_DONE) {
> + return;
> + }
> +
> + aspeed_i2c_set_state(bus, I2CD_MRXD);
> + ret = i2c_recv(bus->bus);
> + if (ret < 0) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> + ret = 0xff;
> + } else {
> + bus->intr_status |= I2CD_INTR_RX_DONE;
> + }
> + bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> + if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> + i2c_nack(bus->bus);
> + }
> + bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> + aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> +}
> +
> /*
> * The state machine needs some refinement. It is only used to track
> * invalid STOP commands for the moment.
> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> {
> bus->cmd &= ~0xFFFF;
> bus->cmd |= value & 0xFFFF;
> - bus->intr_status = 0;
> + bus->intr_status &= I2CD_INTR_RX_DONE;
>
> if (bus->cmd & I2CD_M_START_CMD) {
> uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> }
>
> if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> - int ret;
> -
> - aspeed_i2c_set_state(bus, I2CD_MRXD);
> - ret = i2c_recv(bus->bus);
> - if (ret < 0) {
> - qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> - ret = 0xff;
> - } else {
> - bus->intr_status |= I2CD_INTR_RX_DONE;
> - }
> - bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> - if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> - i2c_nack(bus->bus);
> - }
> - bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> - aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> + aspeed_i2c_handle_rx_cmd(bus);
> }
>
> if (bus->cmd & I2CD_M_STOP_CMD) {
> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> uint64_t value, unsigned size)
> {
> AspeedI2CBus *bus = opaque;
> + int status;
>
> switch (offset) {
> case I2CD_FUN_CTRL_REG:
> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> bus->intr_ctrl = value & 0x7FFF;
> break;
> case I2CD_INTR_STS_REG:
> + status = bus->intr_status;
> bus->intr_status &= ~(value & 0x7FFF);
> - bus->controller->intr_status &= ~(1 << bus->id);
> - qemu_irq_lower(bus->controller->irq);
> + if (!bus->intr_status) {
> + bus->controller->intr_status &= ~(1 << bus->id);
> + qemu_irq_lower(bus->controller->irq);
> + }
> + if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
> + aspeed_i2c_handle_rx_cmd(bus);
> + aspeed_i2c_bus_raise_interrupt(bus);
> + }
> break;
> case I2CD_DEV_ADDR_REG:
> qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>
Nice fix! LGTM. I've tested the new patch and checked that it works well
on both low and high bus speed environments. Thanks a lot!
Can you please submit this patch?
Thanks,
Jae
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-12 20:30 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <da7f4691-7428-3114-32bb-e0171f6c7228@linux.intel.com>
On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote:
> On 9/12/2018 12:58 PM, Guenter Roeck wrote:
> >On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
> >>On 9/11/2018 6:34 PM, Guenter Roeck wrote:
> >>>On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
> >>>>On 9/11/2018 4:33 PM, Guenter Roeck wrote:
> >>>>>Looking into the patch, clearing the interrupt status at the end of an
> >>>>>interrupt handler is always suspicious and tends to result in race
> >>>>>conditions (because additional interrupts may have arrived while handling
> >>>>>the existing interrupts, or because interrupt handling itself may trigger
> >>>>>another interrupt). With that in mind, the following patch fixes the
> >>>>>problem for me.
> >>>>>
> >>>>>Guenter
> >>>>>
> >>>>>---
> >>>>>
> >>>>>diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >>>>>index c258c4d9a4c0..c488e6950b7c 100644
> >>>>>--- a/drivers/i2c/busses/i2c-aspeed.c
> >>>>>+++ b/drivers/i2c/busses/i2c-aspeed.c
> >>>>>@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >>>>> spin_lock(&bus->lock);
> >>>>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>>>+ /* Ack all interrupt bits. */
> >>>>>+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>>> irq_remaining = irq_received;
> >>>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >>>>>@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >>>>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> >>>>> irq_received, irq_handled);
> >>>>>- /* Ack all interrupt bits. */
> >>>>>- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>>> spin_unlock(&bus->lock);
> >>>>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> >>>>> }
> >>>>>
> >>>>
> >>>>My intention of putting the code at the end of interrupt handler was,
> >>>>to reduce possibility of combined irq calls which is explained in this
> >>>>patch. But YES, I agree with you. It could make a potential race
> >>>
> >>>Hmm, yes, but that doesn't explain why it would make sense to acknowledge
> >>>the interrupt late. The interrupt ack only means "I am going to handle these
> >>>interrupts". If additional interrupts arrive while the interrupt handler
> >>>is active, those will have to be acknowledged separately.
> >>>
> >>>Sure, there is a risk that an interrupt arrives while the handler is
> >>>running, and that it is handled but not acknowledged. That can happen
> >>>with pretty much all interrupt handlers, and there are mitigations to
> >>>limit the impact (for example, read the interrupt status register in
> >>>a loop until no more interrupts are pending). But acknowledging
> >>>an interrupt that was possibly not handled is always bad idea.
> >>
> >>Well, that's generally right but not always. Sometimes that depends on
> >>hardware and Aspeed I2C is the case.
> >>
> >>This is a description from Aspeed AST2500 datasheet:
> >> I2CD10 Interrupt Status Register
> >> bit 2 Receive Done Interrupt status
> >> S/W needs to clear this status bit to allow next data receiving.
> >>
> >>It means, driver should hold this bit to prevent transition of hardware
> >>state machine until the driver handles received data, so the bit should
> >>be cleared at the end of interrupt handler.
> >>
> >That makes sense. Does that apply to the other status bits as well ?
> >Reason for asking is that the current code actually gets stuck
> >in transmit, not receive.
> >
> Only bit 2 has that description in datasheet. Is slave config enabled
> for QEMU build? Does that get stuck in master sending or slave
> receiving?
>
qemu does not support slave mode. Linux gets stuck in master tx.
I played with the code on both sides. I had to make changes in both
the linux kernel and in qemu to get the code to work again.
See attached.
Guenter
---
Linux:
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index c258c4d9a4c0..3d518e09369f 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
spin_lock(&bus->lock);
irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ /* Ack all interrupts except for Rx done */
+ writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
+ bus->base + ASPEED_I2C_INTR_STS_REG);
irq_remaining = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
@@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
irq_received, irq_handled);
- /* Ack all interrupt bits. */
- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
+ /* Ack Rx done */
+ if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
+ writel(ASPEED_I2CD_INTR_RX_DONE,
+ bus->base + ASPEED_I2C_INTR_STS_REG);
spin_unlock(&bus->lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
---
qemu:
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c762c73..0d4aa08 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
}
+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+{
+ int ret;
+
+ if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
+ return;
+ }
+ if (bus->intr_status & I2CD_INTR_RX_DONE) {
+ return;
+ }
+
+ aspeed_i2c_set_state(bus, I2CD_MRXD);
+ ret = i2c_recv(bus->bus);
+ if (ret < 0) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
+ ret = 0xff;
+ } else {
+ bus->intr_status |= I2CD_INTR_RX_DONE;
+ }
+ bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+ if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
+ i2c_nack(bus->bus);
+ }
+ bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
+ aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+}
+
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
{
bus->cmd &= ~0xFFFF;
bus->cmd |= value & 0xFFFF;
- bus->intr_status = 0;
+ bus->intr_status &= I2CD_INTR_RX_DONE;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
@@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
}
if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
- int ret;
-
- aspeed_i2c_set_state(bus, I2CD_MRXD);
- ret = i2c_recv(bus->bus);
- if (ret < 0) {
- qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
- ret = 0xff;
- } else {
- bus->intr_status |= I2CD_INTR_RX_DONE;
- }
- bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
- if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
- i2c_nack(bus->bus);
- }
- bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
- aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+ aspeed_i2c_handle_rx_cmd(bus);
}
if (bus->cmd & I2CD_M_STOP_CMD) {
@@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
uint64_t value, unsigned size)
{
AspeedI2CBus *bus = opaque;
+ int status;
switch (offset) {
case I2CD_FUN_CTRL_REG:
@@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
bus->intr_ctrl = value & 0x7FFF;
break;
case I2CD_INTR_STS_REG:
+ status = bus->intr_status;
bus->intr_status &= ~(value & 0x7FFF);
- bus->controller->intr_status &= ~(1 << bus->id);
- qemu_irq_lower(bus->controller->irq);
+ if (!bus->intr_status) {
+ bus->controller->intr_status &= ~(1 << bus->id);
+ qemu_irq_lower(bus->controller->irq);
+ }
+ if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
+ aspeed_i2c_handle_rx_cmd(bus);
+ aspeed_i2c_bus_raise_interrupt(bus);
+ }
break;
case I2CD_DEV_ADDR_REG:
qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
^ permalink raw reply related
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-12 20:10 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180912195844.GA6893@roeck-us.net>
On 9/12/2018 12:58 PM, Guenter Roeck wrote:
> On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>> interrupt handler is always suspicious and tends to result in race
>>>>> conditions (because additional interrupts may have arrived while handling
>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>> problem for me.
>>>>>
>>>>> Guenter
>>>>>
>>>>> ---
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>> spin_lock(&bus->lock);
>>>>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>> + /* Ack all interrupt bits. */
>>>>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>> irq_remaining = irq_received;
>>>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>> irq_received, irq_handled);
>>>>> - /* Ack all interrupt bits. */
>>>>> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>> spin_unlock(&bus->lock);
>>>>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>> }
>>>>>
>>>>
>>>> My intention of putting the code at the end of interrupt handler was,
>>>> to reduce possibility of combined irq calls which is explained in this
>>>> patch. But YES, I agree with you. It could make a potential race
>>>
>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>> interrupts". If additional interrupts arrive while the interrupt handler
>>> is active, those will have to be acknowledged separately.
>>>
>>> Sure, there is a risk that an interrupt arrives while the handler is
>>> running, and that it is handled but not acknowledged. That can happen
>>> with pretty much all interrupt handlers, and there are mitigations to
>>> limit the impact (for example, read the interrupt status register in
>>> a loop until no more interrupts are pending). But acknowledging
>>> an interrupt that was possibly not handled is always bad idea.
>>
>> Well, that's generally right but not always. Sometimes that depends on
>> hardware and Aspeed I2C is the case.
>>
>> This is a description from Aspeed AST2500 datasheet:
>> I2CD10 Interrupt Status Register
>> bit 2 Receive Done Interrupt status
>> S/W needs to clear this status bit to allow next data receiving.
>>
>> It means, driver should hold this bit to prevent transition of hardware
>> state machine until the driver handles received data, so the bit should
>> be cleared at the end of interrupt handler.
>>
> That makes sense. Does that apply to the other status bits as well ?
> Reason for asking is that the current code actually gets stuck
> in transmit, not receive.
>
Only bit 2 has that description in datasheet. Is slave config enabled
for QEMU build? Does that get stuck in master sending or slave
receiving?
Thanks,
Jae
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-12 19:58 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <7fd98646-fb5a-be4d-ce37-84b74e0fa8b3@linux.intel.com>
On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
> >On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
> >>On 9/11/2018 4:33 PM, Guenter Roeck wrote:
> >>>Looking into the patch, clearing the interrupt status at the end of an
> >>>interrupt handler is always suspicious and tends to result in race
> >>>conditions (because additional interrupts may have arrived while handling
> >>>the existing interrupts, or because interrupt handling itself may trigger
> >>>another interrupt). With that in mind, the following patch fixes the
> >>>problem for me.
> >>>
> >>>Guenter
> >>>
> >>>---
> >>>
> >>>diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >>>index c258c4d9a4c0..c488e6950b7c 100644
> >>>--- a/drivers/i2c/busses/i2c-aspeed.c
> >>>+++ b/drivers/i2c/busses/i2c-aspeed.c
> >>>@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >>> spin_lock(&bus->lock);
> >>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>+ /* Ack all interrupt bits. */
> >>>+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >>> irq_remaining = irq_received;
> >>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >>>@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> >>> irq_received, irq_handled);
> >>>- /* Ack all interrupt bits. */
> >>>- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >>> spin_unlock(&bus->lock);
> >>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> >>> }
> >>>
> >>
> >>My intention of putting the code at the end of interrupt handler was,
> >>to reduce possibility of combined irq calls which is explained in this
> >>patch. But YES, I agree with you. It could make a potential race
> >
> >Hmm, yes, but that doesn't explain why it would make sense to acknowledge
> >the interrupt late. The interrupt ack only means "I am going to handle these
> >interrupts". If additional interrupts arrive while the interrupt handler
> >is active, those will have to be acknowledged separately.
> >
> >Sure, there is a risk that an interrupt arrives while the handler is
> >running, and that it is handled but not acknowledged. That can happen
> >with pretty much all interrupt handlers, and there are mitigations to
> >limit the impact (for example, read the interrupt status register in
> >a loop until no more interrupts are pending). But acknowledging
> >an interrupt that was possibly not handled is always bad idea.
>
> Well, that's generally right but not always. Sometimes that depends on
> hardware and Aspeed I2C is the case.
>
> This is a description from Aspeed AST2500 datasheet:
> I2CD10 Interrupt Status Register
> bit 2 Receive Done Interrupt status
> S/W needs to clear this status bit to allow next data receiving.
>
> It means, driver should hold this bit to prevent transition of hardware
> state machine until the driver handles received data, so the bit should
> be cleared at the end of interrupt handler.
>
That makes sense. Does that apply to the other status bits as well ?
Reason for asking is that the current code actually gets stuck
in transmit, not receive.
Thanks,
Guenter
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-12 16:54 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180912013449.GA12612@roeck-us.net>
On 9/11/2018 6:34 PM, Guenter Roeck wrote:
> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>> Looking into the patch, clearing the interrupt status at the end of an
>>> interrupt handler is always suspicious and tends to result in race
>>> conditions (because additional interrupts may have arrived while handling
>>> the existing interrupts, or because interrupt handling itself may trigger
>>> another interrupt). With that in mind, the following patch fixes the
>>> problem for me.
>>>
>>> Guenter
>>>
>>> ---
>>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>> index c258c4d9a4c0..c488e6950b7c 100644
>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>> spin_lock(&bus->lock);
>>> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>> + /* Ack all interrupt bits. */
>>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>> irq_remaining = irq_received;
>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>> irq_received, irq_handled);
>>> - /* Ack all interrupt bits. */
>>> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>> spin_unlock(&bus->lock);
>>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>> }
>>>
>>
>> My intention of putting the code at the end of interrupt handler was,
>> to reduce possibility of combined irq calls which is explained in this
>> patch. But YES, I agree with you. It could make a potential race
>
> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
> the interrupt late. The interrupt ack only means "I am going to handle these
> interrupts". If additional interrupts arrive while the interrupt handler
> is active, those will have to be acknowledged separately.
>
> Sure, there is a risk that an interrupt arrives while the handler is
> running, and that it is handled but not acknowledged. That can happen
> with pretty much all interrupt handlers, and there are mitigations to
> limit the impact (for example, read the interrupt status register in
> a loop until no more interrupts are pending). But acknowledging
> an interrupt that was possibly not handled is always bad idea.
Well, that's generally right but not always. Sometimes that depends on
hardware and Aspeed I2C is the case.
This is a description from Aspeed AST2500 datasheet:
I2CD10 Interrupt Status Register
bit 2 Receive Done Interrupt status
S/W needs to clear this status bit to allow next data receiving.
It means, driver should hold this bit to prevent transition of hardware
state machine until the driver handles received data, so the bit should
be cleared at the end of interrupt handler.
Let me share my test result. Your code change works on 100KHz bus speed
but doesn't work well on 1MHz bus speed. Investigated that interrupt
handling is fast enough in 100KHz test but in 1MHz, most of data is
corrupted because the bit is cleared at the beginning of interrupt
handler so it allows receiving of the next data but the interrupt
handler isn't fast enough to read the data buffer on time. I checked
this problem on BMC-ME channel which ME sends lots of IPMB packets to
BMC at 1MHz speed. You could simply check the data corruption problem on
the BMC-ME channel.
My thought is, the current code is right for real Aspeed I2C hardware.
It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
actual Aspeed I2C hardware correctly.
Thanks,
Jae
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Cédric Le Goater @ 2018-09-12 5:57 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180911233302.GA18799@roeck-us.net>
On 09/12/2018 01:33 AM, Guenter Roeck wrote:
> On Wed, Sep 12, 2018 at 08:23:29AM +0930, Joel Stanley wrote:
>> On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>>>
>>> On 9/11/2018 1:41 PM, Guenter Roeck wrote:
>>>> On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote:
>>
>>>>> I checked this patch again but it doesn't have any change that could
>>>>> affect to the probing flow. I'll debug the issue on qemu 3.0 environment
>>>>> and will share if I find something.
>>>>>
>>>> The problem may be that qemu and the new code disagree how interrupts
>>>> should be generated and handled, and the new code does not handle the
>>>> interrupts it receives from the simulated hardware. This will result
>>>> in i2c device probe failure, which in turn can cause all kinds of
>>>> problems.
>>>>
>>>
>>> Yes, that makes sense. Looks like it should be reverted until the issue
>>> is fixed. Will submit a patch to revert it.
>>
>> Let's not rush. The qemu model was written in order to allow us to
>> test the kernel code, and was validated by the kernel driver we have.
>> We've had situations in the past (with the i2c driver in fact) where a
>> change in the driver required an update of the model to be more
>> accurate.
>>
>> I suggest we wait until Cedric has a chance to look at the issue
>> before reverting the patch.
>>
>
> Looking into the patch, clearing the interrupt status at the end of an
> interrupt handler is always suspicious and tends to result in race
yes. That happened in the past with the I2C aspeed driver. I can not find
the thread anymore but we had to move up the ack of the interrupts.
QEMU tends to be much faster to fire interrupts than real HW.
> conditions (because additional interrupts may have arrived while handling
> the existing interrupts, or because interrupt handling itself may trigger
> another interrupt). With that in mind, the following patch fixes the
> problem for me.
Acked-by: C?dric Le Goater <clg@kaod.org>
Thanks,
C.
> Guenter
>
> ---
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..c488e6950b7c 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>
> spin_lock(&bus->lock);
> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> irq_received, irq_handled);
>
> - /* Ack all interrupt bits. */
> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
>
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Guenter Roeck @ 2018-09-12 1:34 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <5698ca34-14c9-8d05-c4e6-5acf85ff9d14@linux.intel.com>
On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
> >Looking into the patch, clearing the interrupt status at the end of an
> >interrupt handler is always suspicious and tends to result in race
> >conditions (because additional interrupts may have arrived while handling
> >the existing interrupts, or because interrupt handling itself may trigger
> >another interrupt). With that in mind, the following patch fixes the
> >problem for me.
> >
> >Guenter
> >
> >---
> >
> >diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >index c258c4d9a4c0..c488e6950b7c 100644
> >--- a/drivers/i2c/busses/i2c-aspeed.c
> >+++ b/drivers/i2c/busses/i2c-aspeed.c
> >@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> > spin_lock(&bus->lock);
> > irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >+ /* Ack all interrupt bits. */
> >+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> > irq_remaining = irq_received;
> > #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> > "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> > irq_received, irq_handled);
> >- /* Ack all interrupt bits. */
> >- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> > spin_unlock(&bus->lock);
> > return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> > }
> >
>
> My intention of putting the code at the end of interrupt handler was,
> to reduce possibility of combined irq calls which is explained in this
> patch. But YES, I agree with you. It could make a potential race
Hmm, yes, but that doesn't explain why it would make sense to acknowledge
the interrupt late. The interrupt ack only means "I am going to handle these
interrupts". If additional interrupts arrive while the interrupt handler
is active, those will have to be acknowledged separately.
Sure, there is a risk that an interrupt arrives while the handler is
running, and that it is handled but not acknowledged. That can happen
with pretty much all interrupt handlers, and there are mitigations to
limit the impact (for example, read the interrupt status register in
a loop until no more interrupts are pending). But acknowledging
an interrupt that was possibly not handled is always bad idea.
Thanks,
Guenter
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-09-11 23:58 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180911233302.GA18799@roeck-us.net>
On 9/11/2018 4:33 PM, Guenter Roeck wrote:
> Looking into the patch, clearing the interrupt status at the end of an
> interrupt handler is always suspicious and tends to result in race
> conditions (because additional interrupts may have arrived while handling
> the existing interrupts, or because interrupt handling itself may trigger
> another interrupt). With that in mind, the following patch fixes the
> problem for me.
>
> Guenter
>
> ---
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..c488e6950b7c 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>
> spin_lock(&bus->lock);
> irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> irq_received, irq_handled);
>
> - /* Ack all interrupt bits. */
> - writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
>
My intention of putting the code at the end of interrupt handler was,
to reduce possibility of combined irq calls which is explained in this
patch. But YES, I agree with you. It could make a potential race
condition as you pointed out. I tested your code change and checked that
it works well. Let me take more sufficient test on real H/W. Will share
the test result.
Thanks a lot!
Jae
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox