Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably

[Jae Hyun Yoo]

Hi Jarkko,

Thanks for the review. Please see my answer below.

On 6/27/2018 12:48 AM, Jarkko Nikula wrote:
> 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.

Yes, that makes sense and I agree with you. I'll split out this patch
into multiple smaller patches as you suggested.

Thanks,

Jae