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

[Brendan Higgins <brendanhiggins@google.com>]

On Tue, Jul 17, 2018 at 9:18 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
>
> On 7/15/2018 8:05 PM, Gary Hsu wrote:
> > Hi Jae,
> >
> > In originally, we reserved these register bits for debug purpose. But for some error handling case, we found it is also useful to help to clarify some error conditions. So driver also can use these fields information to check something.
> > As for how driver use these information in their code, I have no comment. I don’t understand the driver. But these information is the real controller state, it had no problem to use information.
> >
>
> Thanks Gary!
>
> Hi Brendan,
> Is it acceptable now if I add this as a comment like below?

Yep, that's fine. I didn't mean to hold you up. I was just curious.

>
> Thanks,
> Jae
>
> > Best Regards,
> >
> > 許馥疇 Gary Hsu
> >
> > 信驊科技股份有限公司
> > ASPEED Technology Inc.
> >
> > 2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City 30077, Taiwan
> > 新竹科學園區工業東四路 15 號 2F
> >
> > Tel : 886-3-5789568 ext:807
> > Fax : 886-3-5789586
> > Web : http://www.aspeedtech.com
> >
> > ************* Email Confidentiality Notice ********************
> > 免責聲明:
> > 因應個人資料保護法施行,本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!
> >
> > DISCLAIMER:
> > This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
> >
> > -----Original Message-----
> > From: Jae Hyun Yoo [mailto:jae.hyun.yoo@linux.intel.com]
> > Sent: Saturday, July 14, 2018 2:54 AM
> > To: Brendan Higgins <brendanhiggins@google.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; linux-i2c@vger.kernel.org; OpenBMC Maillist <openbmc@lists.ozlabs.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>; linux-aspeed@lists.ozlabs.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; james.feist@linux.intel.com; vernon.mauery@linux.intel.com; Benjamin Fair <benjaminfair@google.com>; Patrick Venture <venture@google.com>; Gary Hsu <gary_hsu@aspeedtech.com>; Ryan Chen <ryan_chen@aspeedtech.com>
> > Subject: Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
> >
> > On 7/13/2018 11:12 AM, Brendan Higgins wrote:
> >> On Fri, Jul 13, 2018 at 10:22 AM Jae Hyun Yoo
> >> <jae.hyun.yoo@linux.intel.com> wrote:
> >>>
> >>> On 7/12/2018 11:21 AM, Jae Hyun Yoo wrote:
> >>>> On 7/12/2018 2:33 AM, Brendan Higgins wrote:
> >>>>> On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
> >>>>> <jae.hyun.yoo@linux.intel.com> wrote:
> >> <snip>
> >>>>> <snip>
> >>>>>>>> +       for (;;) {
> >>>>>>>> +               if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> >>>>>>>> +                     (ASPEED_I2CD_BUS_BUSY_STS |
> >>>>>>>> +                      ASPEED_I2CD_XFER_MODE_STS_MASK)))
> >>>>>>>
> >>>>>>> Is using the Transfer Mode State Machine bits necessary? The
> >>>>>>> documentation marks it as "for debugging purpose only," so
> >>>>>>> relying on it makes me nervous.
> >>>>>>>
> >>>>>>
> >>>>>> As you said, the documentation marks it as "for debugging purpose only."
> >>>>>> but ASPEED also uses this way in their SDK code because it's the
> >>>>>> best way for checking bus busy status which can cover both single
> >>>>>> and multi-master use cases.
> >>>>>>
> >>>>>
> >>>>> Well, it would also be really nice to have access to this bit if
> >>>>> someone wants to implement MCTP. Could we maybe check with Aspeed
> >>>>> what them meant by "for debugging purposes only" and document it
> >>>>> here? It makes me nervous to rely on debugging functionality for
> >>>>> normal usage.
> >>>>>
> >>>>
> >>>> Okay, I'll check it with Aspeed. Will let you know their response.
> >>>>
> >>>
> >>> I've checked it with Gary Hsu <gary_hsu@aspeedtech.com> and he
> >>> confirmed that the bits reflect real information and good to be used
> >>> in practical code.
> >>
> >> Huh. For my own edification, could you ask them why they said "for
> >> debugging purpose only" in the documentation? I am just really curious
> >> what they meant by that. I would be satisfied if you just CC'ed me on
> >> your email thread with Gary, and I can ask him myself.
> >>
> >
> > I've already CC'ed Gary and Ryan in this thread.
> >
> > Hi Gary,
> >
> > Can you explain why the documentation says that the bit field is 'for debugging purpose only'? Any plan to change the description?
> >
> > Thanks,
> >
> > Jae
> >
> >>>
> >>> I'll add a comment like below:
> >>>
> >>> /*
> >>>     * This is marked as 'for debugging purpose only' in datasheet but
> >>>     * ASPEED confirmed that this reflects real information and good
> >>>     * to be used in practical code.
> >>>     */
> >>>
> >>> Is it acceptable then?
> >>
> >> Yeah, that's fine.
> >>
> >> <snip>
> >>
> >> Cheers
> >>