* [PATCH 7/7] arm: dts: aspeed: Enable vhub on port A of AST2500 EVB
From: Andrew Jeffery @ 2018-07-16 5:39 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180629035106.27181-8-benh@kernel.crashing.org>
On Fri, 29 Jun 2018, at 13:21, Benjamin Herrenschmidt wrote:
> This is an eval board, it makes sense to enable many
> functions by default. This changes the device-tree to
> set port A to be a USB device and leave port B as a
> host, along with a little comment explaining how to
> change it.
>
> (the vhub device can only exist on port A on this SoC)
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> arch/arm/boot/dts/aspeed-ast2500-evb.dts | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-ast2500-evb.dts b/arch/arm/boot/
> dts/aspeed-ast2500-evb.dts
> index 2bff1b253842..2375449c02d0 100644
> --- a/arch/arm/boot/dts/aspeed-ast2500-evb.dts
> +++ b/arch/arm/boot/dts/aspeed-ast2500-evb.dts
> @@ -80,7 +80,13 @@
> };
> };
>
> -&ehci0 {
> +/*
> + * Enable port A as device (via the virtual hub) and port B as
> + * host by default on the eval board. This can be easily changed
> + * by replacing the override below with &ehci0 { ... } to enable
> + * host on both ports.
> + */
> +&vhub {
> status = "okay";
> };
>
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH 6/7] arm: configs: Add USB gadget to Aspeed G5 defconfig
From: Andrew Jeffery @ 2018-07-16 5:38 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180629035106.27181-7-benh@kernel.crashing.org>
On Fri, 29 Jun 2018, at 13:21, Benjamin Herrenschmidt wrote:
> Now that the vhub driver is upstream and the device-trees
> updated, let's enable this by default.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> arch/arm/configs/aspeed_g5_defconfig | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/
> aspeed_g5_defconfig
> index 8c7ea033cdc2..38e9b2d43df3 100644
> --- a/arch/arm/configs/aspeed_g5_defconfig
> +++ b/arch/arm/configs/aspeed_g5_defconfig
> @@ -140,6 +140,23 @@ CONFIG_USB_DYNAMIC_MINORS=y
> CONFIG_USB_EHCI_HCD=y
> CONFIG_USB_EHCI_ROOT_HUB_TT=y
> CONFIG_USB_EHCI_HCD_PLATFORM=y
> +CONFIG_USB_GADGET=y
> +CONFIG_U_SERIAL_CONSOLE=y
> +CONFIG_USB_ASPEED_VHUB=y
> +CONFIG_USB_CONFIGFS=y
> +CONFIG_USB_CONFIGFS_SERIAL=y
> +CONFIG_USB_CONFIGFS_ACM=y
> +CONFIG_USB_CONFIGFS_OBEX=y
> +CONFIG_USB_CONFIGFS_NCM=y
> +CONFIG_USB_CONFIGFS_ECM=y
> +CONFIG_USB_CONFIGFS_ECM_SUBSET=y
> +CONFIG_USB_CONFIGFS_RNDIS=y
> +CONFIG_USB_CONFIGFS_EEM=y
> +CONFIG_USB_CONFIGFS_MASS_STORAGE=y
> +CONFIG_USB_CONFIGFS_F_LB_SS=y
> +CONFIG_USB_CONFIGFS_F_FS=y
> +CONFIG_USB_CONFIGFS_F_HID=y
> +CONFIG_USB_CONFIGFS_F_PRINTER=y
> CONFIG_NEW_LEDS=y
> CONFIG_LEDS_CLASS=y
> CONFIG_LEDS_CLASS_FLASH=y
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH 5/7] arm: configs: Add USB gadget to Aspeed G4 defconfig
From: Andrew Jeffery @ 2018-07-16 5:37 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180629035106.27181-6-benh@kernel.crashing.org>
On Fri, 29 Jun 2018, at 13:21, Benjamin Herrenschmidt wrote:
> Now that the vhub driver is upstream and the device-trees
> updated, let's enable this by default.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> arch/arm/configs/aspeed_g4_defconfig | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/
> aspeed_g4_defconfig
> index 95946dee9c77..be714ea088ed 100644
> --- a/arch/arm/configs/aspeed_g4_defconfig
> +++ b/arch/arm/configs/aspeed_g4_defconfig
> @@ -138,6 +138,23 @@ CONFIG_USB_DYNAMIC_MINORS=y
> CONFIG_USB_EHCI_HCD=y
> CONFIG_USB_EHCI_ROOT_HUB_TT=y
> CONFIG_USB_EHCI_HCD_PLATFORM=y
> +CONFIG_USB_GADGET=y
> +CONFIG_U_SERIAL_CONSOLE=y
> +CONFIG_USB_ASPEED_VHUB=y
> +CONFIG_USB_CONFIGFS=y
> +CONFIG_USB_CONFIGFS_SERIAL=y
> +CONFIG_USB_CONFIGFS_ACM=y
> +CONFIG_USB_CONFIGFS_OBEX=y
> +CONFIG_USB_CONFIGFS_NCM=y
> +CONFIG_USB_CONFIGFS_ECM=y
> +CONFIG_USB_CONFIGFS_ECM_SUBSET=y
> +CONFIG_USB_CONFIGFS_RNDIS=y
> +CONFIG_USB_CONFIGFS_EEM=y
> +CONFIG_USB_CONFIGFS_MASS_STORAGE=y
> +CONFIG_USB_CONFIGFS_F_LB_SS=y
> +CONFIG_USB_CONFIGFS_F_FS=y
> +CONFIG_USB_CONFIGFS_F_HID=y
> +CONFIG_USB_CONFIGFS_F_PRINTER=y
> CONFIG_NEW_LEDS=y
> CONFIG_LEDS_CLASS=y
> CONFIG_LEDS_CLASS_FLASH=y
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH 4/7] arm: dts: aspeed: Add Aspeed 54 USB Virtual Hub
From: Andrew Jeffery @ 2018-07-16 5:36 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180629035106.27181-5-benh@kernel.crashing.org>
Typo: '54' in the subject should be 'G5'
On Fri, 29 Jun 2018, at 13:21, Benjamin Herrenschmidt wrote:
> This adds the (disabled by default) device node for the
> Aspeed virtual hub,a long with clocks and pinmux.
>
> This also adds the missing pinmux definition for it
> (the kernel driver already knows about it).
Whoops! Thanks for catching that.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/arm/boot/dts/aspeed-g5.dtsi | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> index dfdc239b86f6..6274d3eaf374 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -176,6 +176,16 @@
> */
> };
>
> + vhub: usb-vhub at 1e6a0000 {
> + compatible = "aspeed,ast2500-usb-vhub";
> + reg = <0x1e6a0000 0x300>;
> + interrupts = <5>;
> + clocks = <&syscon ASPEED_CLK_GATE_USBPORT1CLK>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_usb2ad_default>;
> + status = "disabled";
> + };
> +
Same query about the compatible before.
> apb {
> compatible = "simple-bus";
> #address-cells = <1>;
> @@ -1425,6 +1435,11 @@
> groups = "USB2AH";
> };
>
> + pinctrl_usb2ad_default: usb2ad_default {
> + function = "USB2AD";
> + groups = "USB2AD";
> + };
> +
> pinctrl_usb11bhid_default: usb11bhid_default {
> function = "USB11BHID";
> groups = "USB11BHID";
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH 3/7] arm: dts: aspeed: Add Aspeed G4 USB Virtual Hub
From: Andrew Jeffery @ 2018-07-16 5:34 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180629035106.27181-4-benh@kernel.crashing.org>
On Fri, 29 Jun 2018, at 13:21, Benjamin Herrenschmidt wrote:
> This adds the (disabled by default) device node for the
> Aspeed virtual hub,a long with clocks and pinmux.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/arm/boot/dts/aspeed-g4.dtsi | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
> index 1d7ffa9fdb11..54524564037c 100644
> --- a/arch/arm/boot/dts/aspeed-g4.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi
> @@ -131,6 +131,16 @@
> */
> };
>
> + vhub: usb-vhub at 1e6a0000 {
> + compatible = "aspeed,ast2400-usb-vhub";
> + reg = <0x1e6a0000 0x300>;
> + interrupts = <5>;
> + clocks = <&syscon ASPEED_CLK_GATE_USBPORT1CLK>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_usb2d_default>;
> + status = "disabled";
> + };
> +
These are all generic properties, so it's pretty clear what's going on, but it seems there's no bindings document capturing the compatible string? Not wanting to be a pain, but shouldn't we have documented it?
> apb {
> compatible = "simple-bus";
> #address-cells = <1>;
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH 2/7] arm: dts: aspeed: Add aspeed G5 USB host pinmux
From: Andrew Jeffery @ 2018-07-16 5:23 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180629035106.27181-3-benh@kernel.crashing.org>
On Fri, 29 Jun 2018, at 13:21, Benjamin Herrenschmidt wrote:
> Set the default pinmux for EHCIs so boards don't have to do
> it an document why it is not set for UHCI.
>
> Remove the properties from the AST2500 EVB board which are
> now redundant
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> arch/arm/boot/dts/aspeed-ast2500-evb.dts | 6 ------
> arch/arm/boot/dts/aspeed-g5.dtsi | 8 ++++++++
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-ast2500-evb.dts b/arch/arm/boot/
> dts/aspeed-ast2500-evb.dts
> index ede11c597673..2bff1b253842 100644
> --- a/arch/arm/boot/dts/aspeed-ast2500-evb.dts
> +++ b/arch/arm/boot/dts/aspeed-ast2500-evb.dts
> @@ -82,18 +82,12 @@
>
> &ehci0 {
> status = "okay";
> - pinctrl-names = "default";
> - pinctrl-0 = <&pinctrl_usb2ah_default>;
> };
>
> &ehci1 {
> status = "okay";
> - pinctrl-names = "default";
> - pinctrl-0 = <&pinctrl_usb2bh_default>;
> };
>
> &uhci {
> status = "okay";
> -
> - /* No pinctrl, this follows the above EHCI settings */
> };
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> index 17f2714d18a7..dfdc239b86f6 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -148,6 +148,8 @@
> reg = <0x1e6a1000 0x100>;
> interrupts = <5>;
> clocks = <&syscon ASPEED_CLK_GATE_USBPORT1CLK>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_usb2ah_default>;
> status = "disabled";
> };
>
> @@ -156,6 +158,8 @@
> reg = <0x1e6a3000 0x100>;
> interrupts = <13>;
> clocks = <&syscon ASPEED_CLK_GATE_USBPORT2CLK>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_usb2bh_default>;
> status = "disabled";
> };
>
> @@ -166,6 +170,10 @@
> #ports = <2>;
> clocks = <&syscon ASPEED_CLK_GATE_USBUHCICLK>;
> status = "disabled";
> + /*
> + * No default pinmux, it will follow EHCI, use an explicit pinmux
> + * override if you don't enable EHCI
> + */
> };
>
> apb {
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH 1/7] arm: dts: aspeed: Add aspeed G4 USB pinmux
From: Andrew Jeffery @ 2018-07-16 5:17 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180629035106.27181-2-benh@kernel.crashing.org>
On Fri, 29 Jun 2018, at 13:21, Benjamin Herrenschmidt wrote:
> Set the default pinmux for EHCI so boards don't have to do
> it an document why it is not set for UHCI.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/arm/boot/dts/aspeed-g4.dtsi | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
> index 75df1573380e..1d7ffa9fdb11 100644
> --- a/arch/arm/boot/dts/aspeed-g4.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi
> @@ -113,6 +113,8 @@
> reg = <0x1e6a1000 0x100>;
> interrupts = <5>;
> clocks = <&syscon ASPEED_CLK_GATE_USBPORT1CLK>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_usb2h_default>;
> status = "disabled";
> };
>
> @@ -123,6 +125,10 @@
> #ports = <3>;
> clocks = <&syscon ASPEED_CLK_GATE_USBUHCICLK>;
> status = "disabled";
> + /*
> + * No default pinmux, it will follow EHCI, use an explicit pinmux
> + * override if you don't enable EHCI
> + */
This is only part of the story on the AST2400 which has 3 USB ports:
* Port 1 is fixed-function UHCI
* Port 2 can be muxed to the UHCI (host) or HID (device) controllers
* Port 3 can be muxed to the EHCI or vHub controllers
I assume if we plug a USB 1.1 device into port 3 it gets rate matched and dealt with (therefore UHCI follows EHCI as suggested in your comment). But given port 2, which *may* route to UHCI *specifically* (not EHCI), UHCI *doesn't* always follow EHCI.
However, port 2 has its other function, HID, and HID is only available on port 2. So, if we had a HID devicetree node, we would put the pinmux for port 2 in there and leave it to the board to configure the HID controller as enabled or not. However, given this conflict, the UHCI node still shouldn't have any pinmux properties as your comment suggests, as it's up to the board whether HID or UHCI mode should be selected for port 2, but we'll need to enable the UHCI controller for port 1 (or 3) and we don't want to force the mux state of port 2 (or 3).
So whilst partially correct, the comment is a bit misleading in that it suggests muxing EHCI is enough in general, which isn't really true. Maybe say instead that due to flexible configuration, UHCI pinmux should be overridden in the platform dts explicitly?
Cheers,
Andrew
> };
>
> apb {
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
From: Gary Hsu @ 2018-07-16 3:05 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <b8232faa-58f5-f3a5-d12a-3f326cf1d03d@linux.intel.com>
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.
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 at 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 at vger.kernel.org; OpenBMC Maillist <openbmc@lists.ozlabs.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>; linux-aspeed at lists.ozlabs.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; james.feist at linux.intel.com; vernon.mauery at 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
>
^ permalink raw reply
* [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
From: Jae Hyun Yoo @ 2018-07-13 20:37 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180713203327.47yq4gucidbzn2hd@katana>
On 7/13/2018 1:33 PM, Wolfram Sang wrote:
>
>> BTW, to whom should I ask applying this patch? Should I send v2 after
>> adding your reviewed-by tag?
>
> Not needed, thanks. I use 'patchwork' and this tool collects all the
> tags for me. If I see a patch reviewed by a driver maintainer, I'll pick
> it up in the next queue.
>
Oh, I see. Thanks a lot Wolfram!
^ permalink raw reply
* [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
From: Wolfram Sang @ 2018-07-13 20:33 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <59538d26-f025-28da-63df-f675e80fd68b@linux.intel.com>
> BTW, to whom should I ask applying this patch? Should I send v2 after
> adding your reviewed-by tag?
Not needed, thanks. I use 'patchwork' and this tool collects all the
tags for me. If I see a patch reviewed by a driver maintainer, I'll pick
it up in the next queue.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20180713/d761c31d/attachment.sig>
^ permalink raw reply
* [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
From: Jae Hyun Yoo @ 2018-07-13 19:21 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g47nBCbF0mcYqEnQkDpz0Dhh0F0S1-wEL_yBHgs1b3yE5w@mail.gmail.com>
On 7/12/2018 1:41 AM, Brendan Higgins wrote:
> On Mon, Jul 2, 2018 at 2:40 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> This patch adjusts spinlock scope to make it wrap the whole irq
>> handler using a single lock/unlock which covers both master and
>> slave handlers.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 60e4d0e939a3..9f02aa959a3e 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -234,7 +234,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> bool irq_handled = true;
>> u8 value;
>>
>> - spin_lock(&bus->lock);
>> if (!slave) {
>> irq_handled = false;
>> goto out;
>> @@ -325,7 +324,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>>
>> out:
>> - spin_unlock(&bus->lock);
>> return irq_handled;
>> }
>> #endif /* CONFIG_I2C_SLAVE */
>> @@ -389,7 +387,6 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> u8 recv_byte;
>> int ret;
>>
>> - spin_lock(&bus->lock);
>> irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> /* Ack all interrupt bits. */
>> writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> @@ -547,22 +544,29 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> dev_err(bus->dev,
>> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> irq_status, status_ack);
>> - spin_unlock(&bus->lock);
>> return !!irq_status;
>> }
>>
>> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> {
>> struct aspeed_i2c_bus *bus = dev_id;
>> + bool ret;
>> +
>> + spin_lock(&bus->lock);
>>
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> if (aspeed_i2c_slave_irq(bus)) {
>> dev_dbg(bus->dev, "irq handled by slave.\n");
>> - return IRQ_HANDLED;
>> + ret = true;
>> + goto out;
>> }
>> #endif /* CONFIG_I2C_SLAVE */
>>
>> - return aspeed_i2c_master_irq(bus) ? IRQ_HANDLED : IRQ_NONE;
>> + ret = aspeed_i2c_master_irq(bus);
>> +
>> +out:
>> + spin_unlock(&bus->lock);
>> + return ret ? IRQ_HANDLED : IRQ_NONE;
>> }
>>
>> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>> --
>> 2.17.1
>>
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>
> Thanks!
>
Thanks Brendan!
BTW, to whom should I ask applying this patch? Should I send v2 after
adding your reviewed-by tag?
Thanks,
Jae
^ permalink raw reply
* [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
From: Jae Hyun Yoo @ 2018-07-13 18:54 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g47zdPdqqjVJtNHtFk+jKgciKbMcEH9M-W9rL4DgLM5hkg@mail.gmail.com>
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
>
^ permalink raw reply
* [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
From: Jae Hyun Yoo @ 2018-07-13 18:50 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g47gOs-eVmnwoPuSiZaMmkgKKOdZd_6EFCVBHbmVmqADQA@mail.gmail.com>
On 7/13/2018 11:02 AM, Brendan Higgins wrote:
> On Thu, Jul 12, 2018 at 11:21 AM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> 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>
>>>>>> +/* Timeout for bus busy checking */
>>>>>> +#define BUS_BUSY_CHECK_TIMEOUT 250000 /* 250ms */
>>>>>> +#define BUS_BUSY_CHECK_INTERVAL 10000 /* 10ms */
>>>>>
>>>>> Could you add a comment on where you got these values from?
>>>>>
>>>>
>>>> These are coming from ASPEED SDK code. Actually, they use 100ms for
>>>> timeout and 10ms for interval but I increased the timeout value to
>>>> 250ms so that it covers a various range of bus speed. I think, it
>>>> should be computed at run time based on the current bus speed, or
>>>> we could add these as device tree settings. How do you think about it?
>>>>
>>>
>>> This should definitely be a device tree setting. If one of the busses is being
>>> used as a regular I2C bus, it could hold the bus for an unlimited amount of
>>> time before sending a STOP. As for a default, 100ms is probably fine given
>>> that, a) the limit will only apply to multi-master mode, and b) multi-master
>>> mode will probably almost always be used with IPMB, or MCTP (MCTP actually
>>> recommends a 100ms timeout for this purpose, see
>>> https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
>>> symbol PT2a). That being said, if you actually want to implement IPMB, or MCTP
>>> arbitration logic, it is much more complicated.
>>>
>>
>> Okay then, I think, we can fix the timeout value to 100ms and enable the
>> bus busy checking logic only when 'multi-master' is set in device tree.
>> My thought is, no additional arbitration logic is needed because
>> arbitration is performed in H/W level and H/W reports
>> ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
>> ARBIT_LOSS event is already being handled well by this driver code you
>> implemented.
>
> I still think it would be best to provide an option to specify the timeout value
> it in the device tree regardless of master mode or not. Also, I am talking about
> fairness arbitration not the physical level arbitration provided by the I2C
> spec. The physical arbitration that Aspeed provides just allows multiple masters
> to operate on the same bus according to the specification; this bus arbitration
> does not guarantee forward progress or even guarentee that the actual message
> will be sent, which is what you are trying to do here.
>
> Since you are planning on implementing IPMB, you will probably need to implement
> fairness arbitration. I am not familiar with your BMC-ME channel. It sounds like
> it pre-dates MCTP, so it must implement its own fairness arbitration on top of
> the IPMB layer (more like you bake in some assumptions about what the possible
> state is at anytime that guarentee fairness).
>
> Are you using the Aspeed BMC on both sides of the connection? If so, you might
> be further ahead to implement MCTP fairness arbitration which can be used in
> conjunction with IPMB. This will require a bit of work to do, but everyone will
> be much happier in the long term (assuming MCTP does eventually become a thing).
>
Okay. I'll add an additional option into device tree to set the timeout
value.
My thought is, the fairness arbitration you are saying should be
considered in IPMB or MCTP layer not in this driver code.
The intention of the code I added is, preventing state corruption in
this driver code. In multi-master environment, this driver side master
cannot know exactly when peer master send data to this side master so a
case can be happened that this master tries to send data through the
master_xfer function but slave data from peer master is still being
processed by this driver. Previous code treats this as an error and
tries recovering a bus which is definitely wrong. So the patch code
checks whether there is any ongoing slave operation or not and wait up
to the timeout duration before start master xfer.
>>
>>>> >
>>> <snip>
>>>>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>> - if (aspeed_i2c_slave_irq(bus)) {
>>>>>> - dev_dbg(bus->dev, "irq handled by slave.\n");
>>>>>> - return IRQ_HANDLED;
>>>>>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>>>>>> + if (!aspeed_i2c_master_irq(bus))
>>>>>
>>>>> Why do you check the slave if master fails (or vice versa)? I
>>>>> understand that there are some status bits that have not been handled,
>>>>> but it doesn't seem reasonable to assume that there is state that the
>>>>> other should do something with; the only way this would happen is if
>>>>> the state that you think you are in does not match the status bits you
>>>>> have been given, but if this is the case, you are already hosed; I
>>>>> don't think trying the other handler is likely to make things better,
>>>>> unless there is something that I am missing.
>>>>>
>>>>
>>>> In most of cases, interrupt bits are set one by one but there are also a
>>>> lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
>>>> with combining master and slave events using a single interrupt call. It
>>>> happens much in multi-master environment than single-master. For
>>>> example, when master is waiting for a NORMAL_STOP interrupt in its
>>>> MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along
>>>> with the NORMAL_STOP in case of an another master immediately sends data
>>>> just after acquiring the bus - it happens a lot in BMC-ME connection
>>>> practically. In this case, the NORMAL_STOP interrupt should be handled
>>>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>>>> handled by slave_irq so it's the reason why this code is added.
>>>
>>> That sucks. Well, it sounds like there are only a handful of cases in which
>>> this can happen. Maybe enumerate these cases and error out or at least warn if
>>> it is not one of them?
>>>
>>
>> Yes, that sucks but that is Aspeed's I2C IP behavior and that's the
>> reason why they implemented some combination bits handling code in
>> their SDK. Actually, the cases are happening somewhat frequently
>> but that would not be a problem if we handle the cases properly instead
>> of making error out or warn.
>>
>>>>
>>> <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.
>
> Sounds good.
>
>>
>>>>>> + return 0;
>>>>>> + if (ktime_compare(ktime_get(), timeout) > 0)
>>>>>> + break;
>>>>>> + usleep_range((BUS_BUSY_CHECK_INTERVAL >> 2) + 1,
>>>>>
>>>>> Where did you get this minimum value?
>>>>>
>>>>
>>>> No source for the minimum value. ASPEED uses mdelay(10) in their SDK
>>>> but I changed that code using usleep_range and the range value was set
>>>> with considering time stretching of usleep_range.
>>>> regmap_read_poll_timeout was a reference for this code.
>>>
>>> What protocol are you trying to implement on top of this? You mentioned BMC-ME
>>> above; that's IPMB, right? For most use cases, this should work, but if you
>>> need arbitration, you will need to do quite a bit more work.
>>>
>>
>> Yes, I'm implementing IPMB for a BMC-ME channel. As I said above,
>> arbitration will be performed in H/W level and it's already been handled
>> well by your code. This bus busy checking logic is for checking whether
>> any slave operation is currently ongoing or not at the timing of
>> master_xfer is called. It's not for arbitration but for preventing state
>> conflicts between master and slave operations.
>
> Discussed above.
>
>>
>> FYI, I broke down this patch into smaller patches you reviewed
>> Today. Thanks for sharing your time for reviewing the patches.
>> I'll send remaining patches after completing review on those
>> patches because the remaining patches have dependency on them.
>
> Sounds good.
>
>>
>> Thanks!
>>
>
> Cheers
^ permalink raw reply
* [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
From: Brendan Higgins @ 2018-07-13 18:12 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <d3f10fc1-635b-3a5d-a967-2816eac005fc@linux.intel.com>
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'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
^ permalink raw reply
* [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
From: Brendan Higgins @ 2018-07-13 18:02 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <921b1ab7-9c9f-0aeb-da89-5a1a27d009f0@linux.intel.com>
On Thu, Jul 12, 2018 at 11:21 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> 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>
> >>>> +/* Timeout for bus busy checking */
> >>>> +#define BUS_BUSY_CHECK_TIMEOUT 250000 /* 250ms */
> >>>> +#define BUS_BUSY_CHECK_INTERVAL 10000 /* 10ms */
> >>>
> >>> Could you add a comment on where you got these values from?
> >>>
> >>
> >> These are coming from ASPEED SDK code. Actually, they use 100ms for
> >> timeout and 10ms for interval but I increased the timeout value to
> >> 250ms so that it covers a various range of bus speed. I think, it
> >> should be computed at run time based on the current bus speed, or
> >> we could add these as device tree settings. How do you think about it?
> >>
> >
> > This should definitely be a device tree setting. If one of the busses is being
> > used as a regular I2C bus, it could hold the bus for an unlimited amount of
> > time before sending a STOP. As for a default, 100ms is probably fine given
> > that, a) the limit will only apply to multi-master mode, and b) multi-master
> > mode will probably almost always be used with IPMB, or MCTP (MCTP actually
> > recommends a 100ms timeout for this purpose, see
> > https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
> > symbol PT2a). That being said, if you actually want to implement IPMB, or MCTP
> > arbitration logic, it is much more complicated.
> >
>
> Okay then, I think, we can fix the timeout value to 100ms and enable the
> bus busy checking logic only when 'multi-master' is set in device tree.
> My thought is, no additional arbitration logic is needed because
> arbitration is performed in H/W level and H/W reports
> ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
> ARBIT_LOSS event is already being handled well by this driver code you
> implemented.
I still think it would be best to provide an option to specify the timeout value
it in the device tree regardless of master mode or not. Also, I am talking about
fairness arbitration not the physical level arbitration provided by the I2C
spec. The physical arbitration that Aspeed provides just allows multiple masters
to operate on the same bus according to the specification; this bus arbitration
does not guarantee forward progress or even guarentee that the actual message
will be sent, which is what you are trying to do here.
Since you are planning on implementing IPMB, you will probably need to implement
fairness arbitration. I am not familiar with your BMC-ME channel. It sounds like
it pre-dates MCTP, so it must implement its own fairness arbitration on top of
the IPMB layer (more like you bake in some assumptions about what the possible
state is at anytime that guarentee fairness).
Are you using the Aspeed BMC on both sides of the connection? If so, you might
be further ahead to implement MCTP fairness arbitration which can be used in
conjunction with IPMB. This will require a bit of work to do, but everyone will
be much happier in the long term (assuming MCTP does eventually become a thing).
>
> >> >
> > <snip>
> >>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >>>> - if (aspeed_i2c_slave_irq(bus)) {
> >>>> - dev_dbg(bus->dev, "irq handled by slave.\n");
> >>>> - return IRQ_HANDLED;
> >>>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> >>>> + if (!aspeed_i2c_master_irq(bus))
> >>>
> >>> Why do you check the slave if master fails (or vice versa)? I
> >>> understand that there are some status bits that have not been handled,
> >>> but it doesn't seem reasonable to assume that there is state that the
> >>> other should do something with; the only way this would happen is if
> >>> the state that you think you are in does not match the status bits you
> >>> have been given, but if this is the case, you are already hosed; I
> >>> don't think trying the other handler is likely to make things better,
> >>> unless there is something that I am missing.
> >>>
> >>
> >> In most of cases, interrupt bits are set one by one but there are also a
> >> lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
> >> with combining master and slave events using a single interrupt call. It
> >> happens much in multi-master environment than single-master. For
> >> example, when master is waiting for a NORMAL_STOP interrupt in its
> >> MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along
> >> with the NORMAL_STOP in case of an another master immediately sends data
> >> just after acquiring the bus - it happens a lot in BMC-ME connection
> >> practically. In this case, the NORMAL_STOP interrupt should be handled
> >> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> >> handled by slave_irq so it's the reason why this code is added.
> >
> > That sucks. Well, it sounds like there are only a handful of cases in which
> > this can happen. Maybe enumerate these cases and error out or at least warn if
> > it is not one of them?
> >
>
> Yes, that sucks but that is Aspeed's I2C IP behavior and that's the
> reason why they implemented some combination bits handling code in
> their SDK. Actually, the cases are happening somewhat frequently
> but that would not be a problem if we handle the cases properly instead
> of making error out or warn.
>
> >>
> > <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.
Sounds good.
>
> >>>> + return 0;
> >>>> + if (ktime_compare(ktime_get(), timeout) > 0)
> >>>> + break;
> >>>> + usleep_range((BUS_BUSY_CHECK_INTERVAL >> 2) + 1,
> >>>
> >>> Where did you get this minimum value?
> >>>
> >>
> >> No source for the minimum value. ASPEED uses mdelay(10) in their SDK
> >> but I changed that code using usleep_range and the range value was set
> >> with considering time stretching of usleep_range.
> >> regmap_read_poll_timeout was a reference for this code.
> >
> > What protocol are you trying to implement on top of this? You mentioned BMC-ME
> > above; that's IPMB, right? For most use cases, this should work, but if you
> > need arbitration, you will need to do quite a bit more work.
> >
>
> Yes, I'm implementing IPMB for a BMC-ME channel. As I said above,
> arbitration will be performed in H/W level and it's already been handled
> well by your code. This bus busy checking logic is for checking whether
> any slave operation is currently ongoing or not at the timing of
> master_xfer is called. It's not for arbitration but for preventing state
> conflicts between master and slave operations.
Discussed above.
>
> FYI, I broke down this patch into smaller patches you reviewed
> Today. Thanks for sharing your time for reviewing the patches.
> I'll send remaining patches after completing review on those
> patches because the remaining patches have dependency on them.
Sounds good.
>
> Thanks!
>
Cheers
On Thu, Jul 12, 2018 at 11:21 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> 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>
> >>>> +/* Timeout for bus busy checking */
> >>>> +#define BUS_BUSY_CHECK_TIMEOUT 250000 /* 250ms */
> >>>> +#define BUS_BUSY_CHECK_INTERVAL 10000 /* 10ms */
> >>>
> >>> Could you add a comment on where you got these values from?
> >>>
> >>
> >> These are coming from ASPEED SDK code. Actually, they use 100ms for
> >> timeout and 10ms for interval but I increased the timeout value to
> >> 250ms so that it covers a various range of bus speed. I think, it
> >> should be computed at run time based on the current bus speed, or
> >> we could add these as device tree settings. How do you think about it?
> >>
> >
> > This should definitely be a device tree setting. If one of the busses is being
> > used as a regular I2C bus, it could hold the bus for an unlimited amount of
> > time before sending a STOP. As for a default, 100ms is probably fine given
> > that, a) the limit will only apply to multi-master mode, and b) multi-master
> > mode will probably almost always be used with IPMB, or MCTP (MCTP actually
> > recommends a 100ms timeout for this purpose, see
> > https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
> > symbol PT2a). That being said, if you actually want to implement IPMB, or MCTP
> > arbitration logic, it is much more complicated.
> >
>
> Okay then, I think, we can fix the timeout value to 100ms and enable the
> bus busy checking logic only when 'multi-master' is set in device tree.
> My thought is, no additional arbitration logic is needed because
> arbitration is performed in H/W level and H/W reports
> ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
> ARBIT_LOSS event is already being handled well by this driver code you
> implemented.
I still think it would be best to provide an option to specify the timeout value
it in the device tree regardless of master mode or not. Also, I am talking about
fairness arbitration not the physical level arbitration provided by the I2C
spec. The physical arbitration that Aspeed provides just allows multiple masters
to operate on the same bus according to the specification; this bus arbitration
does not guarantee forward progress or even guarentee that the actual message
will be sent, which is what you are trying to do here.
Since you are planning on implementing IPMB, you will probably need to implement
fairness arbitration. I am not familiar with your BMC-ME channel. It sounds like
it pre-dates MCTP, so it must implement its own fairness arbitration on top of
the IPMB layer (more like you bake in some assumptions about what the possible
state is at anytime that guarentee fairness).
Are you using the Aspeed BMC on both sides of the connection? If so, you might
be further ahead to implement MCTP fairness arbitration which can be used in
conjunction with IPMB. This will require a bit of work to do, but everyone will
be much happier in the long term (assuming MCTP does eventually become a thing).
>
> >> >
> > <snip>
> >>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >>>> - if (aspeed_i2c_slave_irq(bus)) {
> >>>> - dev_dbg(bus->dev, "irq handled by slave.\n");
> >>>> - return IRQ_HANDLED;
> >>>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> >>>> + if (!aspeed_i2c_master_irq(bus))
> >>>
> >>> Why do you check the slave if master fails (or vice versa)? I
> >>> understand that there are some status bits that have not been handled,
> >>> but it doesn't seem reasonable to assume that there is state that the
> >>> other should do something with; the only way this would happen is if
> >>> the state that you think you are in does not match the status bits you
> >>> have been given, but if this is the case, you are already hosed; I
> >>> don't think trying the other handler is likely to make things better,
> >>> unless there is something that I am missing.
> >>>
> >>
> >> In most of cases, interrupt bits are set one by one but there are also a
> >> lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
> >> with combining master and slave events using a single interrupt call. It
> >> happens much in multi-master environment than single-master. For
> >> example, when master is waiting for a NORMAL_STOP interrupt in its
> >> MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along
> >> with the NORMAL_STOP in case of an another master immediately sends data
> >> just after acquiring the bus - it happens a lot in BMC-ME connection
> >> practically. In this case, the NORMAL_STOP interrupt should be handled
> >> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> >> handled by slave_irq so it's the reason why this code is added.
> >
> > That sucks. Well, it sounds like there are only a handful of cases in which
> > this can happen. Maybe enumerate these cases and error out or at least warn if
> > it is not one of them?
> >
>
> Yes, that sucks but that is Aspeed's I2C IP behavior and that's the
> reason why they implemented some combination bits handling code in
> their SDK. Actually, the cases are happening somewhat frequently
> but that would not be a problem if we handle the cases properly instead
> of making error out or warn.
>
> >>
> > <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.
Sounds good.
>
> >>>> + return 0;
> >>>> + if (ktime_compare(ktime_get(), timeout) > 0)
> >>>> + break;
> >>>> + usleep_range((BUS_BUSY_CHECK_INTERVAL >> 2) + 1,
> >>>
> >>> Where did you get this minimum value?
> >>>
> >>
> >> No source for the minimum value. ASPEED uses mdelay(10) in their SDK
> >> but I changed that code using usleep_range and the range value was set
> >> with considering time stretching of usleep_range.
> >> regmap_read_poll_timeout was a reference for this code.
> >
> > What protocol are you trying to implement on top of this? You mentioned BMC-ME
> > above; that's IPMB, right? For most use cases, this should work, but if you
> > need arbitration, you will need to do quite a bit more work.
> >
>
> Yes, I'm implementing IPMB for a BMC-ME channel. As I said above,
> arbitration will be performed in H/W level and it's already been handled
> well by your code. This bus busy checking logic is for checking whether
> any slave operation is currently ongoing or not at the timing of
> master_xfer is called. It's not for arbitration but for preventing state
> conflicts between master and slave operations.
Discussed above.
>
> FYI, I broke down this patch into smaller patches you reviewed
> Today. Thanks for sharing your time for reviewing the patches.
> I'll send remaining patches after completing review on those
> patches because the remaining patches have dependency on them.
Sounds good.
>
> Thanks!
>
Cheers
^ permalink raw reply
* [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
From: Jae Hyun Yoo @ 2018-07-13 17:21 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <921b1ab7-9c9f-0aeb-da89-5a1a27d009f0@linux.intel.com>
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>
>>>>> +/* Timeout for bus busy checking */
>>>>> +#define BUS_BUSY_CHECK_TIMEOUT???????????????????????? 250000 /*
>>>>> 250ms */
>>>>> +#define BUS_BUSY_CHECK_INTERVAL
>>>>> 10000? /* 10ms */
>>>>
>>>> Could you add a comment on where you got these values from?
>>>>
>>>
>>> These are coming from ASPEED SDK code. Actually, they use 100ms for
>>> timeout and 10ms for interval but I increased the timeout value to
>>> 250ms so that it covers a various range of bus speed. I think, it
>>> should be computed at run time based on the current bus speed, or
>>> we could add these as device tree settings. How do you think about it?
>>>
>>
>> This should definitely be a device tree setting. If one of the busses
>> is being
>> used as a regular I2C bus, it could hold the bus for an unlimited
>> amount of
>> time before sending a STOP. As for a default, 100ms is probably fine
>> given
>> that, a) the limit will only apply to multi-master mode, and b)
>> multi-master
>> mode will probably almost always be used with IPMB, or MCTP (MCTP
>> actually
>> recommends a 100ms timeout for this purpose, see
>> https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
>>
>> symbol PT2a). That being said, if you actually want to implement IPMB,
>> or MCTP
>> arbitration logic, it is much more complicated.
>>
>
> Okay then, I think, we can fix the timeout value to 100ms and enable the
> bus busy checking logic only when 'multi-master' is set in device tree.
> My thought is, no additional arbitration logic is needed because
> arbitration is performed in H/W level and H/W reports
> ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
> ARBIT_LOSS event is already being handled well by this driver code you
> implemented.
>
>>> ? >
>> <snip>
>>>>> ?? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>> -?????? if (aspeed_i2c_slave_irq(bus)) {
>>>>> -?????????????? dev_dbg(bus->dev, "irq handled by slave.\n");
>>>>> -?????????????? return IRQ_HANDLED;
>>>>> +?????? if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>>>>> +?????????????? if (!aspeed_i2c_master_irq(bus))
>>>>
>>>> Why do you check the slave if master fails (or vice versa)? I
>>>> understand that there are some status bits that have not been handled,
>>>> but it doesn't seem reasonable to assume that there is state that the
>>>> other should do something with; the only way this would happen is if
>>>> the state that you think you are in does not match the status bits you
>>>> have been given, but if this is the case, you are already hosed; I
>>>> don't think trying the other handler is likely to make things better,
>>>> unless there is something that I am missing.
>>>>
>>>
>>> In most of cases, interrupt bits are set one by one but there are also a
>>> lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
>>> with combining master and slave events using a single interrupt call. It
>>> happens much in multi-master environment than single-master. For
>>> example, when master is waiting for a NORMAL_STOP interrupt in its
>>> MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along
>>> with the NORMAL_STOP in case of an another master immediately sends data
>>> just after acquiring the bus - it happens a lot in BMC-ME connection
>>> practically. In this case, the NORMAL_STOP interrupt should be handled
>>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>>> handled by slave_irq so it's the reason why this code is added.
>>
>> That sucks. Well, it sounds like there are only a handful of cases in
>> which
>> this can happen. Maybe enumerate these cases and error out or at least
>> warn if
>> it is not one of them?
>>
>
> Yes, that sucks but that is Aspeed's I2C IP behavior and that's the
> reason why they implemented some combination bits handling code in
> their SDK. Actually, the cases are happening somewhat frequently
> but that would not be a problem if we handle the cases properly instead
> of making error out or warn.
>
>>>
>> <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.
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?
>>>>> +?????????????????????? return 0;
>>>>> +?????????????? if (ktime_compare(ktime_get(), timeout) > 0)
>>>>> +?????????????????????? break;
>>>>> +?????????????? usleep_range((BUS_BUSY_CHECK_INTERVAL >> 2) + 1,
>>>>
>>>> Where did you get this minimum value?
>>>>
>>>
>>> No source for the minimum value. ASPEED uses mdelay(10) in their SDK
>>> but I changed that code using usleep_range and the range value was set
>>> with considering time stretching of usleep_range.
>>> regmap_read_poll_timeout was a reference for this code.
>>
>> What protocol are you trying to implement on top of this? You
>> mentioned BMC-ME
>> above; that's IPMB, right? For most use cases, this should work, but
>> if you
>> need arbitration, you will need to do quite a bit more work.
>>
>
> Yes, I'm implementing IPMB for a BMC-ME channel. As I said above,
> arbitration will be performed in H/W level and it's already been handled
> well by your code. This bus busy checking logic is for checking whether
> any slave operation is currently ongoing or not at the timing of
> master_xfer is called. It's not for arbitration but for preventing state
> conflicts between master and slave operations.
>
> FYI, I broke down this patch into smaller patches you reviewed
> Today. Thanks for sharing your time for reviewing the patches.
> I'll send remaining patches after completing review on those
> patches because the remaining patches have dependency on them.
>
> Thanks!
>
>>>
>>> Thanks,
>>>
>>> Jae
>> <snip>
>>
>> Cheers
>>
>
^ permalink raw reply
* [PATCH] gpio: aspeed: fix compile testing warning
From: Linus Walleij @ 2018-07-13 7:06 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <34126dddef64a7669946c354d4dd9e2adfb3344f.camel@kernel.crashing.org>
On Tue, Jul 10, 2018 at 1:53 AM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2018-07-09 at 16:56 +0200, Arnd Bergmann wrote:
> > Gcc cannot always see that BUG_ON(1) is guaranteed to not
> > return, so we get a warning message in some configurations:
> >
> > drivers/gpio/gpio-aspeed.c: In function 'bank_reg':
> > drivers/gpio/gpio-aspeed.c:244:1: error: control reaches end of non-void function [-Werror=return-type]
> >
> > Using a plain BUG() is easier here and avoids the problem.
> >
> > Fixes: 44ddf559d579 ("gpio: aspeed: Rework register type accessors")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> Linus, can you plonk that on top of the patches in that topic branch
> you created ?
I put it on top of my devel branch where I merged in the topic
branch.
As it's a fringe thing anyways I don't think we need to recreate the
topic branch for this.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] gpio: aspeed: fix compile testing warning
From: Linus Walleij @ 2018-07-13 7:05 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180709145612.4166409-1-arnd@arndb.de>
On Mon, Jul 9, 2018 at 4:56 PM Arnd Bergmann <arnd@arndb.de> wrote:
> Gcc cannot always see that BUG_ON(1) is guaranteed to not
> return, so we get a warning message in some configurations:
>
> drivers/gpio/gpio-aspeed.c: In function 'bank_reg':
> drivers/gpio/gpio-aspeed.c:244:1: error: control reaches end of non-void function [-Werror=return-type]
>
> Using a plain BUG() is easier here and avoids the problem.
>
> Fixes: 44ddf559d579 ("gpio: aspeed: Rework register type accessors")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Patch applied with Benjamin's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
From: Jae Hyun Yoo @ 2018-07-12 18:21 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g47j_h4ZrT8Eg1FZ8sAj5ekCUH20jUVub75ozdkg8_YW7Q@mail.gmail.com>
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>
>>>> +/* Timeout for bus busy checking */
>>>> +#define BUS_BUSY_CHECK_TIMEOUT 250000 /* 250ms */
>>>> +#define BUS_BUSY_CHECK_INTERVAL 10000 /* 10ms */
>>>
>>> Could you add a comment on where you got these values from?
>>>
>>
>> These are coming from ASPEED SDK code. Actually, they use 100ms for
>> timeout and 10ms for interval but I increased the timeout value to
>> 250ms so that it covers a various range of bus speed. I think, it
>> should be computed at run time based on the current bus speed, or
>> we could add these as device tree settings. How do you think about it?
>>
>
> This should definitely be a device tree setting. If one of the busses is being
> used as a regular I2C bus, it could hold the bus for an unlimited amount of
> time before sending a STOP. As for a default, 100ms is probably fine given
> that, a) the limit will only apply to multi-master mode, and b) multi-master
> mode will probably almost always be used with IPMB, or MCTP (MCTP actually
> recommends a 100ms timeout for this purpose, see
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
> symbol PT2a). That being said, if you actually want to implement IPMB, or MCTP
> arbitration logic, it is much more complicated.
>
Okay then, I think, we can fix the timeout value to 100ms and enable the
bus busy checking logic only when 'multi-master' is set in device tree.
My thought is, no additional arbitration logic is needed because
arbitration is performed in H/W level and H/W reports
ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
ARBIT_LOSS event is already being handled well by this driver code you
implemented.
>> >
> <snip>
>>>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>> - if (aspeed_i2c_slave_irq(bus)) {
>>>> - dev_dbg(bus->dev, "irq handled by slave.\n");
>>>> - return IRQ_HANDLED;
>>>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>>>> + if (!aspeed_i2c_master_irq(bus))
>>>
>>> Why do you check the slave if master fails (or vice versa)? I
>>> understand that there are some status bits that have not been handled,
>>> but it doesn't seem reasonable to assume that there is state that the
>>> other should do something with; the only way this would happen is if
>>> the state that you think you are in does not match the status bits you
>>> have been given, but if this is the case, you are already hosed; I
>>> don't think trying the other handler is likely to make things better,
>>> unless there is something that I am missing.
>>>
>>
>> In most of cases, interrupt bits are set one by one but there are also a
>> lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
>> with combining master and slave events using a single interrupt call. It
>> happens much in multi-master environment than single-master. For
>> example, when master is waiting for a NORMAL_STOP interrupt in its
>> MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along
>> with the NORMAL_STOP in case of an another master immediately sends data
>> just after acquiring the bus - it happens a lot in BMC-ME connection
>> practically. In this case, the NORMAL_STOP interrupt should be handled
>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>> handled by slave_irq so it's the reason why this code is added.
>
> That sucks. Well, it sounds like there are only a handful of cases in which
> this can happen. Maybe enumerate these cases and error out or at least warn if
> it is not one of them?
>
Yes, that sucks but that is Aspeed's I2C IP behavior and that's the
reason why they implemented some combination bits handling code in
their SDK. Actually, the cases are happening somewhat frequently
but that would not be a problem if we handle the cases properly instead
of making error out or warn.
>>
> <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.
>>>> + return 0;
>>>> + if (ktime_compare(ktime_get(), timeout) > 0)
>>>> + break;
>>>> + usleep_range((BUS_BUSY_CHECK_INTERVAL >> 2) + 1,
>>>
>>> Where did you get this minimum value?
>>>
>>
>> No source for the minimum value. ASPEED uses mdelay(10) in their SDK
>> but I changed that code using usleep_range and the range value was set
>> with considering time stretching of usleep_range.
>> regmap_read_poll_timeout was a reference for this code.
>
> What protocol are you trying to implement on top of this? You mentioned BMC-ME
> above; that's IPMB, right? For most use cases, this should work, but if you
> need arbitration, you will need to do quite a bit more work.
>
Yes, I'm implementing IPMB for a BMC-ME channel. As I said above,
arbitration will be performed in H/W level and it's already been handled
well by your code. This bus busy checking logic is for checking whether
any slave operation is currently ongoing or not at the timing of
master_xfer is called. It's not for arbitration but for preventing state
conflicts between master and slave operations.
FYI, I broke down this patch into smaller patches you reviewed
Today. Thanks for sharing your time for reviewing the patches.
I'll send remaining patches after completing review on those
patches because the remaining patches have dependency on them.
Thanks!
>>
>> Thanks,
>>
>> Jae
> <snip>
>
> Cheers
>
^ permalink raw reply
* [PATCH] i2c: aspeed: Add newline characters into message printings.
From: Jae Hyun Yoo @ 2018-07-12 17:10 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g47-Bq_wUkL4vP93si+urDs-A0j26ukZ+Riy24HWi5AuuA@mail.gmail.com>
On 7/12/2018 1:38 AM, Brendan Higgins wrote:
> On Wed, Jul 11, 2018 at 10:10 AM Joe Perches <joe@perches.com> wrote:
>>
>> On Wed, 2018-07-11 at 09:53 -0700, Jae Hyun Yoo wrote:
>>> On 7/10/2018 10:42 PM, Brendan Higgins wrote:
>>>> On Mon, Jul 2, 2018 at 2:14 PM Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
>>>>> There are some log printing without a newline character. This
>>>>> patch adds the missing newline characters.
>> []
>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> []
>>>>> @@ -431,7 +431,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>>>>> */
>>>>> if (bus->master_state == ASPEED_I2C_MASTER_START) {
>>>>> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>>>>> - pr_devel("no slave present at %02x", msg->addr);
>>>>> + pr_devel("no slave present at %02x\n", msg->addr);
>>>>
>>>> Unless something changed in the last couple versions of the kernel, this is the
>>>> only line that actually changes anything. dev_* inserts a newline for every
>>>> call.
>>
>> Not true.
>>
>> Any printk without KERN_CONT inserts a newline
>> if the last character
>> emitted is not a newline.
>>
>> dev_<level> uses can also be followed by pr_cont.
>>
>> So this patch does reduce the possibility of
>> interleaved messages from multiple processes.
>
> My mistake. Thanks for pointing that out.
>
> Jae, forget what I said earlier. This looks good to me.
>
Okay. I'll keep this patch as is.
Thanks Joe for your clarification!
Thanks Brendan for your 'Reviewed-by' tag!
Jae
^ permalink raw reply
* [PATCH v3 3/5] fsi: master-ast-cf: Add new FSI master using Aspeed ColdFire
From: Benjamin Herrenschmidt @ 2018-07-12 11:45 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACPK8XdZKbh2NLQaiZOx66FAo+3csS_D9-BXjJKeppKm8aEM6w@mail.gmail.com>
On Thu, 2018-07-12 at 17:53 +1000, Joel Stanley wrote:
> On 12 July 2018 at 13:48, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > The Aspeed AST2x00 can contain a ColdFire v1 coprocessor which
> > is currently unused on OpenPower systems.
> >
> > This adds an alternative to the fsi-master-gpio driver that
> > uses that coprocessor instead of bit banging from the ARM
> > core itself. The end result is about 4 times faster.
> >
> > The firmware for the coprocessor and its source code can be
> > found at https://github.com/ozbenh/cf-fsi and is system specific.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > drivers/fsi/Kconfig | 9 +
> > drivers/fsi/Makefile | 1 +
> > drivers/fsi/cf-fsi-fw.h | 157 +++
> > drivers/fsi/fsi-master-ast-cf.c | 1438 ++++++++++++++++++++++
> > include/trace/events/fsi_master_ast_cf.h | 150 +++
> > 5 files changed, 1755 insertions(+)
> > create mode 100644 drivers/fsi/cf-fsi-fw.h
> > create mode 100644 drivers/fsi/fsi-master-ast-cf.c
> > create mode 100644 include/trace/events/fsi_master_ast_cf.h
> >
> > diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
> > index 322cec393cf2..e0220d1e1357 100644
> > --- a/drivers/fsi/Kconfig
> > +++ b/drivers/fsi/Kconfig
> > @@ -27,6 +27,15 @@ config FSI_MASTER_HUB
> > allow chaining of FSI links to an arbitrary depth. This allows for
> > a high target device fanout.
> >
> > +config FSI_MASTER_AST_CF
> > + tristate "FSI master based on Aspeed ColdFire coprocessor"
> > + depends on GPIOLIB
> > + depends on GPIO_ASPEED
> > + ---help---
> > + This option enables a FSI master using the AST2400 and AST2500 GPIO
> > + lines driven by the internal ColdFire coprocessor. This requires
> > + the corresponding machine specific ColdFire firmware to be available.
>
> The "machine specific" part isn't true anymore, is it?
Right, I'll fixup the changelog before pushing if that's the last spin.
> I gave this a spin on a palmetto and it appeared to work fine for me.
>
> Tested-by: Joel Stanley <joel@jms.id.au>
Thanks !
Cheers,
Ben
.
^ permalink raw reply
* [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
From: Brendan Higgins @ 2018-07-12 9:33 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <e51471e6-0f8a-bffc-34c8-1e74239fdbcf@linux.intel.com>
On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
<snip>
> >> +/* Timeout for bus busy checking */
> >> +#define BUS_BUSY_CHECK_TIMEOUT 250000 /* 250ms */
> >> +#define BUS_BUSY_CHECK_INTERVAL 10000 /* 10ms */
> >
> > Could you add a comment on where you got these values from?
> >
>
> These are coming from ASPEED SDK code. Actually, they use 100ms for
> timeout and 10ms for interval but I increased the timeout value to
> 250ms so that it covers a various range of bus speed. I think, it
> should be computed at run time based on the current bus speed, or
> we could add these as device tree settings. How do you think about it?
>
This should definitely be a device tree setting. If one of the busses is being
used as a regular I2C bus, it could hold the bus for an unlimited amount of
time before sending a STOP. As for a default, 100ms is probably fine given
that, a) the limit will only apply to multi-master mode, and b) multi-master
mode will probably almost always be used with IPMB, or MCTP (MCTP actually
recommends a 100ms timeout for this purpose, see
https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
symbol PT2a). That being said, if you actually want to implement IPMB, or MCTP
arbitration logic, it is much more complicated.
> >
<snip>
> >> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >> - if (aspeed_i2c_slave_irq(bus)) {
> >> - dev_dbg(bus->dev, "irq handled by slave.\n");
> >> - return IRQ_HANDLED;
> >> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> >> + if (!aspeed_i2c_master_irq(bus))
> >
> > Why do you check the slave if master fails (or vice versa)? I
> > understand that there are some status bits that have not been handled,
> > but it doesn't seem reasonable to assume that there is state that the
> > other should do something with; the only way this would happen is if
> > the state that you think you are in does not match the status bits you
> > have been given, but if this is the case, you are already hosed; I
> > don't think trying the other handler is likely to make things better,
> > unless there is something that I am missing.
> >
>
> In most of cases, interrupt bits are set one by one but there are also a
> lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
> with combining master and slave events using a single interrupt call. It
> happens much in multi-master environment than single-master. For
> example, when master is waiting for a NORMAL_STOP interrupt in its
> MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along
> with the NORMAL_STOP in case of an another master immediately sends data
> just after acquiring the bus - it happens a lot in BMC-ME connection
> practically. In this case, the NORMAL_STOP interrupt should be handled
> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> handled by slave_irq so it's the reason why this code is added.
That sucks. Well, it sounds like there are only a handful of cases in which
this can happen. Maybe enumerate these cases and error out or at least warn if
it is not one of them?
>
<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.
> >> + return 0;
> >> + if (ktime_compare(ktime_get(), timeout) > 0)
> >> + break;
> >> + usleep_range((BUS_BUSY_CHECK_INTERVAL >> 2) + 1,
> >
> > Where did you get this minimum value?
> >
>
> No source for the minimum value. ASPEED uses mdelay(10) in their SDK
> but I changed that code using usleep_range and the range value was set
> with considering time stretching of usleep_range.
> regmap_read_poll_timeout was a reference for this code.
What protocol are you trying to implement on top of this? You mentioned BMC-ME
above; that's IPMB, right? For most use cases, this should work, but if you
need arbitration, you will need to do quite a bit more work.
>
> Thanks,
>
> Jae
<snip>
Cheers
^ permalink raw reply
* [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
From: Brendan Higgins @ 2018-07-12 8:41 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180702214011.16071-1-jae.hyun.yoo@linux.intel.com>
On Mon, Jul 2, 2018 at 2:40 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> This patch adjusts spinlock scope to make it wrap the whole irq
> handler using a single lock/unlock which covers both master and
> slave handlers.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 60e4d0e939a3..9f02aa959a3e 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -234,7 +234,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> bool irq_handled = true;
> u8 value;
>
> - spin_lock(&bus->lock);
> if (!slave) {
> irq_handled = false;
> goto out;
> @@ -325,7 +324,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>
> out:
> - spin_unlock(&bus->lock);
> return irq_handled;
> }
> #endif /* CONFIG_I2C_SLAVE */
> @@ -389,7 +387,6 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> u8 recv_byte;
> int ret;
>
> - spin_lock(&bus->lock);
> irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> /* Ack all interrupt bits. */
> writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> @@ -547,22 +544,29 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> dev_err(bus->dev,
> "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> irq_status, status_ack);
> - spin_unlock(&bus->lock);
> return !!irq_status;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> + bool ret;
> +
> + spin_lock(&bus->lock);
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> if (aspeed_i2c_slave_irq(bus)) {
> dev_dbg(bus->dev, "irq handled by slave.\n");
> - return IRQ_HANDLED;
> + ret = true;
> + goto out;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> - return aspeed_i2c_master_irq(bus) ? IRQ_HANDLED : IRQ_NONE;
> + ret = aspeed_i2c_master_irq(bus);
> +
> +out:
> + spin_unlock(&bus->lock);
> + return ret ? IRQ_HANDLED : IRQ_NONE;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> --
> 2.17.1
>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Thanks!
^ permalink raw reply
* [PATCH] i2c: aspeed: Add newline characters into message printings.
From: Brendan Higgins @ 2018-07-12 8:38 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <c13fc12e859f7bdb300f08ad805cdf6d7ec09f52.camel@perches.com>
On Wed, Jul 11, 2018 at 10:10 AM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2018-07-11 at 09:53 -0700, Jae Hyun Yoo wrote:
> > On 7/10/2018 10:42 PM, Brendan Higgins wrote:
> > > On Mon, Jul 2, 2018 at 2:14 PM Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote:
> > > > There are some log printing without a newline character. This
> > > > patch adds the missing newline characters.
> []
> > > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> []
> > > > @@ -431,7 +431,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> > > > */
> > > > if (bus->master_state == ASPEED_I2C_MASTER_START) {
> > > > if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> > > > - pr_devel("no slave present at %02x", msg->addr);
> > > > + pr_devel("no slave present at %02x\n", msg->addr);
> > >
> > > Unless something changed in the last couple versions of the kernel, this is the
> > > only line that actually changes anything. dev_* inserts a newline for every
> > > call.
>
> Not true.
>
> Any printk without KERN_CONT inserts a newline
> if the last character
> emitted is not a newline.
>
> dev_<level> uses can also be followed by pr_cont.
>
> So this patch does reduce the possibility of
> interleaved messages from multiple processes.
My mistake. Thanks for pointing that out.
Jae, forget what I said earlier. This looks good to me.
^ permalink raw reply
* [PATCH v3 3/5] fsi: master-ast-cf: Add new FSI master using Aspeed ColdFire
From: Joel Stanley @ 2018-07-12 7:53 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180712034847.12878-4-benh@kernel.crashing.org>
On 12 July 2018 at 13:48, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> The Aspeed AST2x00 can contain a ColdFire v1 coprocessor which
> is currently unused on OpenPower systems.
>
> This adds an alternative to the fsi-master-gpio driver that
> uses that coprocessor instead of bit banging from the ARM
> core itself. The end result is about 4 times faster.
>
> The firmware for the coprocessor and its source code can be
> found at https://github.com/ozbenh/cf-fsi and is system specific.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> drivers/fsi/Kconfig | 9 +
> drivers/fsi/Makefile | 1 +
> drivers/fsi/cf-fsi-fw.h | 157 +++
> drivers/fsi/fsi-master-ast-cf.c | 1438 ++++++++++++++++++++++
> include/trace/events/fsi_master_ast_cf.h | 150 +++
> 5 files changed, 1755 insertions(+)
> create mode 100644 drivers/fsi/cf-fsi-fw.h
> create mode 100644 drivers/fsi/fsi-master-ast-cf.c
> create mode 100644 include/trace/events/fsi_master_ast_cf.h
>
> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
> index 322cec393cf2..e0220d1e1357 100644
> --- a/drivers/fsi/Kconfig
> +++ b/drivers/fsi/Kconfig
> @@ -27,6 +27,15 @@ config FSI_MASTER_HUB
> allow chaining of FSI links to an arbitrary depth. This allows for
> a high target device fanout.
>
> +config FSI_MASTER_AST_CF
> + tristate "FSI master based on Aspeed ColdFire coprocessor"
> + depends on GPIOLIB
> + depends on GPIO_ASPEED
> + ---help---
> + This option enables a FSI master using the AST2400 and AST2500 GPIO
> + lines driven by the internal ColdFire coprocessor. This requires
> + the corresponding machine specific ColdFire firmware to be available.
The "machine specific" part isn't true anymore, is it?
I gave this a spin on a palmetto and it appeared to work fine for me.
Tested-by: Joel Stanley <joel@jms.id.au>
^ 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