[PATCH] ARM: dts: aspeed: Add FSI pins to Zaius device tree

[PATCH] ARM: dts: aspeed: Add FSI pins to Zaius device tree

[Joel Stanley]

Pin assignments for the EVT-2 board as described in pdbg. The status is disabled
so this does not enable the in-kernel FSI driver yet.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---

Tested on zaius5 with the driver enabled and it worked.

 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
index e2195a641414..e1c9b3f4fe44 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
@@ -74,6 +74,18 @@
 			gpios = <&gpio ASPEED_GPIO(AA, 2) GPIO_ACTIVE_LOW>;
 		};
 	};
+
+	fsi-master {
+		compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
+
+		status = "disabled";
+
+		clock-gpios = <&gpio ASPEED_GPIO(C, 3) GPIO_ACTIVE_HIGH>;
+		data-gpios = <&gpio ASPEED_GPIO(C, 2) GPIO_ACTIVE_HIGH>;
+		trans-gpios = <&gpio ASPEED_GPIO(O, 6) GPIO_ACTIVE_HIGH>;
+		enable-gpios = <&gpio ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
+		mux-gpios = <&gpio ASPEED_GPIO(P, 6) GPIO_ACTIVE_HIGH>;
+	};
 };
 
 &fmc {
-- 
2.11.0

Re: [PATCH] ARM: dts: aspeed: Add FSI pins to Zaius device tree

[Xo Wang]

Hi Joel,

Thanks for keeping us up to date.

On Mon, Feb 6, 2017 at 6:37 PM, Joel Stanley <joel@jms.id.au> wrote:
> Pin assignments for the EVT-2 board as described in pdbg. The status is disabled
> so this does not enable the in-kernel FSI driver yet.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>
> Tested on zaius5 with the driver enabled and it worked.
>
>  arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> index e2195a641414..e1c9b3f4fe44 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> @@ -74,6 +74,18 @@
>                         gpios = <&gpio ASPEED_GPIO(AA, 2) GPIO_ACTIVE_LOW>;
>                 };
>         };
> +
> +       fsi-master {
> +               compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";

Andrew's comment for Witherspoon applies here as well
("ibm,fsi-master-gpio" makes sense as the "definitive" name).

> +
> +               status = "disabled";
> +
> +               clock-gpios = <&gpio ASPEED_GPIO(C, 3) GPIO_ACTIVE_HIGH>;
> +               data-gpios = <&gpio ASPEED_GPIO(C, 2) GPIO_ACTIVE_HIGH>;

FYI, we'll return to the Norm-recommended assignments for these at the
next board spin to keep FSI pins on their own GPIO port. Not sure how
helpful that is in the context of the FSI kernel driver since it's not
poking the registers directly.

> +               trans-gpios = <&gpio ASPEED_GPIO(O, 6) GPIO_ACTIVE_HIGH>;
> +               enable-gpios = <&gpio ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
> +               mux-gpios = <&gpio ASPEED_GPIO(P, 6) GPIO_ACTIVE_HIGH>;
> +       };
>  };
>
>  &fmc {
> --
> 2.11.0
>

cheers
xo

Re: [PATCH] ARM: dts: aspeed: Add FSI pins to Zaius device tree

[Joel Stanley]

On Wed, Feb 8, 2017 at 6:29 AM, Xo Wang <xow@google.com> wrote:
> Hi Joel,
>
> Thanks for keeping us up to date.
>
> On Mon, Feb 6, 2017 at 6:37 PM, Joel Stanley <joel@jms.id.au> wrote:
>> Pin assignments for the EVT-2 board as described in pdbg. The status is disabled
>> so this does not enable the in-kernel FSI driver yet.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>
>> Tested on zaius5 with the driver enabled and it worked.
>>
>>  arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>> index e2195a641414..e1c9b3f4fe44 100644
>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>> @@ -74,6 +74,18 @@
>>                         gpios = <&gpio ASPEED_GPIO(AA, 2) GPIO_ACTIVE_LOW>;
>>                 };
>>         };
>> +
>> +       fsi-master {
>> +               compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>
> Andrew's comment for Witherspoon applies here as well
> ("ibm,fsi-master-gpio" makes sense as the "definitive" name).

I agree. I had an offline chat with Andrew about it, suggesting that
he review the bindings that Chris Bostic has sent upstream. There's
already a few issues identified with them that we will have to clean
up.

I suggest leave this matching the other systems for now, and when we
settle on the new binding I will migrated all machines to use that.

>
>> +
>> +               status = "disabled";
>> +
>> +               clock-gpios = <&gpio ASPEED_GPIO(C, 3) GPIO_ACTIVE_HIGH>;
>> +               data-gpios = <&gpio ASPEED_GPIO(C, 2) GPIO_ACTIVE_HIGH>;
>
> FYI, we'll return to the Norm-recommended assignments for these at the
> next board spin to keep FSI pins on their own GPIO port. Not sure how
> helpful that is in the context of the FSI kernel driver since it's not
> poking the registers directly.

I assume there are electrical reasons for doing that.

How do you suggest we handle the old and new device tree?

We could detect the board revision and then fix up the device tree in u-boot.

>
>> +               trans-gpios = <&gpio ASPEED_GPIO(O, 6) GPIO_ACTIVE_HIGH>;
>> +               enable-gpios = <&gpio ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
>> +               mux-gpios = <&gpio ASPEED_GPIO(P, 6) GPIO_ACTIVE_HIGH>;
>> +       };
>>  };

Re: [PATCH] ARM: dts: aspeed: Add FSI pins to Zaius device tree

[Xo Wang]

On Wed, Feb 8, 2017 at 6:18 PM, Joel Stanley <joel@jms.id.au> wrote:
> On Wed, Feb 8, 2017 at 6:29 AM, Xo Wang <xow@google.com> wrote:
>> Hi Joel,
>>
>> Thanks for keeping us up to date.
>>
>> On Mon, Feb 6, 2017 at 6:37 PM, Joel Stanley <joel@jms.id.au> wrote:
>>> Pin assignments for the EVT-2 board as described in pdbg. The status is disabled
>>> so this does not enable the in-kernel FSI driver yet.
>>>
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>> ---
>>>
>>> Tested on zaius5 with the driver enabled and it worked.
>>>
>>>  arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>>> index e2195a641414..e1c9b3f4fe44 100644
>>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>>> @@ -74,6 +74,18 @@
>>>                         gpios = <&gpio ASPEED_GPIO(AA, 2) GPIO_ACTIVE_LOW>;
>>>                 };
>>>         };
>>> +
>>> +       fsi-master {
>>> +               compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>>
>> Andrew's comment for Witherspoon applies here as well
>> ("ibm,fsi-master-gpio" makes sense as the "definitive" name).
>
> I agree. I had an offline chat with Andrew about it, suggesting that
> he review the bindings that Chris Bostic has sent upstream. There's
> already a few issues identified with them that we will have to clean
> up.
>
> I suggest leave this matching the other systems for now, and when we
> settle on the new binding I will migrated all machines to use that.
>

Reviewed-by: Xo Wang <xow@google.com>

>>
>>> +
>>> +               status = "disabled";
>>> +
>>> +               clock-gpios = <&gpio ASPEED_GPIO(C, 3) GPIO_ACTIVE_HIGH>;
>>> +               data-gpios = <&gpio ASPEED_GPIO(C, 2) GPIO_ACTIVE_HIGH>;
>>
>> FYI, we'll return to the Norm-recommended assignments for these at the
>> next board spin to keep FSI pins on their own GPIO port. Not sure how
>> helpful that is in the context of the FSI kernel driver since it's not
>> poking the registers directly.
>
> I assume there are electrical reasons for doing that.
>

Not that I know of. Somebody was thinking that moving all the FSI pins
to their own GPIO port(s) would allow a single byte or word write to
the output data register to set pin states without read-modify-write.

I don't think that's an optimization we can apply through gpiolib for
the kernel FSI driver. Hence my skepticism about helpfulness.

> How do you suggest we handle the old and new device tree?
>
> We could detect the board revision and then fix up the device tree in u-boot.
>

Yeah, sounds good. Having board revision detection and device tree
fixups in U-boot seems useful in general.

>>
>>> +               trans-gpios = <&gpio ASPEED_GPIO(O, 6) GPIO_ACTIVE_HIGH>;
>>> +               enable-gpios = <&gpio ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
>>> +               mux-gpios = <&gpio ASPEED_GPIO(P, 6) GPIO_ACTIVE_HIGH>;
>>> +       };
>>>  };

cheers
xo

Re: [PATCH] ARM: dts: aspeed: Add FSI pins to Zaius device tree

[Joel Stanley]

On Fri, Feb 10, 2017 at 6:12 AM, Xo Wang <xow@google.com> wrote:
> On Wed, Feb 8, 2017 at 6:18 PM, Joel Stanley <joel@jms.id.au> wrote:
>> On Wed, Feb 8, 2017 at 6:29 AM, Xo Wang <xow@google.com> wrote:
>>> Hi Joel,
>>>
>>> Thanks for keeping us up to date.
>>>
>>> On Mon, Feb 6, 2017 at 6:37 PM, Joel Stanley <joel@jms.id.au> wrote:
>>>> Pin assignments for the EVT-2 board as described in pdbg. The status is disabled
>>>> so this does not enable the in-kernel FSI driver yet.
>>>>
>>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>>> ---
>>>>
>>>> Tested on zaius5 with the driver enabled and it worked.
>>>>
>>>>  arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>>>> index e2195a641414..e1c9b3f4fe44 100644
>>>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>>>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
>>>> @@ -74,6 +74,18 @@
>>>>                         gpios = <&gpio ASPEED_GPIO(AA, 2) GPIO_ACTIVE_LOW>;
>>>>                 };
>>>>         };
>>>> +
>>>> +       fsi-master {
>>>> +               compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>>>
>>> Andrew's comment for Witherspoon applies here as well
>>> ("ibm,fsi-master-gpio" makes sense as the "definitive" name).
>>
>> I agree. I had an offline chat with Andrew about it, suggesting that
>> he review the bindings that Chris Bostic has sent upstream. There's
>> already a few issues identified with them that we will have to clean
>> up.
>>
>> I suggest leave this matching the other systems for now, and when we
>> settle on the new binding I will migrated all machines to use that.
>>
>
> Reviewed-by: Xo Wang <xow@google.com>

Applied to dev-4.7.

>>>
>>> FYI, we'll return to the Norm-recommended assignments for these at the
>>> next board spin to keep FSI pins on their own GPIO port. Not sure how
>>> helpful that is in the context of the FSI kernel driver since it's not
>>> poking the registers directly.
>>
>> I assume there are electrical reasons for doing that.
>>
>
> Not that I know of. Somebody was thinking that moving all the FSI pins
> to their own GPIO port(s) would allow a single byte or word write to
> the output data register to set pin states without read-modify-write.
>
> I don't think that's an optimization we can apply through gpiolib for
> the kernel FSI driver. Hence my skepticism about helpfulness.

Doing that in the kernel would be quite tricky.

>> How do you suggest we handle the old and new device tree?
>>
>> We could detect the board revision and then fix up the device tree in u-boot.
>>
>
> Yeah, sounds good. Having board revision detection and device tree
> fixups in U-boot seems useful in general.

Great. It would be good to move some of the rev-specific code you have
in arch/arm/mach-aspeed/aspeed.c into the kernel. We are inching
closer to being able to boot a host without any of those workarounds.

Cheers,

Joel