From: Jarkko Nikula <jarkko.nikula@linux.intel.com>
To: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>,
Brendan Higgins <brendanhiggins@google.com>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Joel Stanley <joel@jms.id.au>, Andrew Jeffery <andrew@aj.id.au>,
linux-i2c@vger.kernel.org, openbmc@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org,
linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org
Cc: James Feist <james.feist@linux.intel.com>,
Vernon Mauery <vernon.mauery@linux.intel.com>
Subject: Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
Date: Wed, 27 Jun 2018 10:48:19 +0300 [thread overview]
Message-ID: <60d0dad1-b735-0650-47b4-40c57b1a5209@linux.intel.com> (raw)
In-Reply-To: <20180626165812.4141-1-jae.hyun.yoo@linux.intel.com>
Hi
On 06/26/2018 07:58 PM, Jae Hyun Yoo wrote:
> BMC firmware should support some multi-master use cases such as multi-node,
> IPMB, BMC-ME link and so on but the current ASPEED I2C driver is a bit
> unstable for the multi-master use case. So this patch improves ASPEED I2C
> driver to support the multi-master use case stably.
>
> Changes:
> * Added XFER_MODE status register checking logic into
> aspeed_i2c_master_xfer to improve the current bus busy checking logic.
> * Changed the order of enum aspeed_i2c_master_state and
> enum aspeed_i2c_slave_state defines to make their initial values set to
> ASPEED_I2C_MASTER_INACTIVE and ASPEED_I2C_SLAVE_STOP respectively.
> In case of multi-master use with previous code, if a slave data comes
> ahead of the first master xfer, master_state starts from an invalid
> state. This change fixed the issue.
> * Adjusted spin_lock scope to make it wrap the whole irq handler using
> a single lock and unlock pair covers both master and slave handlers.
> * Added irq_status variable as a member of the struct aspeed_i2c_bus to
> collect handled interrupt bits throughout the master and the slave irq
> handlers.
> * Added control logic to put an order on calling the master and the slave
> irq handlers based on their current states.
>
This does many unrelated looking changes in one patch making it more
vulnerable for potential multiple regressions. For instance busy
checking goes from single read to loop with 250 ms timeout in this patch
while changing also spin lock logic and interrupt handling.
Now if there is some regression it might be difficult to find what
change in this patch is causing it and more over things goes more
complicated if some other kind of regressions are found pointing into
the same commit.
I suggest splitting this into multiple smaller patches. For instance
having first simple conversions patches that are unlikely to cause a
regression like one patch adding '\n' to error print, another moving
irq_status variable into struct aspeed_i2c_bus and so on followed by
patches that change logic like busy checking, spin lock change and then
patch or more for multi-master support.
--
Jarkko
next prev parent reply other threads:[~2018-06-27 7:47 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-26 16:58 [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably Jae Hyun Yoo
2018-06-27 7:36 ` Brendan Higgins
2018-06-27 17:55 ` Jae Hyun Yoo
2018-07-12 9:33 ` Brendan Higgins
2018-07-12 18:21 ` Jae Hyun Yoo
2018-07-13 17:21 ` Jae Hyun Yoo
2018-07-13 18:12 ` Brendan Higgins
2018-07-13 18:54 ` Jae Hyun Yoo
2018-07-16 3:05 ` Gary Hsu
2018-07-17 16:18 ` Jae Hyun Yoo
2018-07-19 16:57 ` Brendan Higgins
2018-07-19 16:58 ` Brendan Higgins
2018-07-13 18:02 ` Brendan Higgins
2018-07-13 18:50 ` Jae Hyun Yoo
2018-06-27 7:48 ` Jarkko Nikula [this message]
2018-06-27 18:01 ` Jae Hyun Yoo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=60d0dad1-b735-0650-47b4-40c57b1a5209@linux.intel.com \
--to=jarkko.nikula@linux.intel.com \
--cc=andrew@aj.id.au \
--cc=benh@kernel.crashing.org \
--cc=brendanhiggins@google.com \
--cc=jae.hyun.yoo@linux.intel.com \
--cc=james.feist@linux.intel.com \
--cc=joel@jms.id.au \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=openbmc@lists.ozlabs.org \
--cc=vernon.mauery@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).