Re: Openblocks AX3-4 i2c bus lockup

Re: Openblocks AX3-4 i2c bus lockup

[Gregory CLEMENT]

Hi Andrew,

On 22/12/2013 00:06, Gregory CLEMENT wrote:
> Hi Andrew,
> 
> I am away this weekend (Christmas with the first part of the family).
> 
> But I remember having few issue on i2c with the first AX3-4 I received. With the second one (using a B0 steping CPU), I didn't remember having any issues. As I thought the first AX3-4 I
> received was an early release I didn't paid more attention. The last i2c patches I sent were tested on the AX3-4 CPU rev B.
> 
> Could you tell me what kind of command did you do to get your issues ? Then I will try to reproduce it on Monday on both revision
> 
> Thanks,
> 
> Gregory


Sorry to not have answered earlier but when I investigated it I found
an unexpected issue.

First I wanted to be sure that there the issue was not introduce by a
commit so reverted one by one the commits on the file
drivers/i2c/busses/i2c-mv64xxx.c. I tested it on both version of the
OpenBlock AX-4 (with CPU A0 and B0). After each commit the kernel
continue to work on the B0 version as expected, but it was when I
reverted the commit "i2c: mv64xxx: Add I2C Transaction Generator
support" that it worked also on the A0 version.

Then I had a look on the errata datasheet and I found issues that I
missed when I worked on it. This issues were fixed in B0 version.

The fix should be pretty simple: disabling the offload_enabled flag when
an A0 version of the CPU is used. For this there are 2 solutions:
introducing a new compatible string or trying to detect the CPU
stepping at runtime. I would prefer the second solution and I am looking
for a way to get this information.

Thanks,

Gregory


> 
> Le 21 déc. 2013 17:43, "Andrew Lunn" <andrew-g2DYL2Zd6BY@public.gmane.org <mailto:andrew@lunn.ch>> a écrit :
> 
>     Hi Nobuhiro
> 
>     You added I2C into the Openblocks AX3-4 device tree and the subnode
>     for the RTC. Did you have any problems with I2C? I'm having lots of
>     problems with my AX3-4 and i2c. I get i2c errors and the bus being
>     locked and then the whole machine locks solid.
> 
>     Gregory, did you test your I2C patches for 370/XP on an Openblocks
>     AX3-4?
> 
>     Thanks
>             Andrew
> 
>     _______________________________________________
>     linux-arm-kernel mailing list
>     linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org <mailto:linux-arm-kernel@lists.infradead.org>
>     http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: Openblocks AX3-4 i2c bus lockup

[Gregory CLEMENT]

Hi Andrew,

[added Jason, Sebastian and Ezequiel in case they have better ideas]
[I also changed the order of the chunk of this email to make it easier
for the new comer to understand the subject]

On 31/12/2013 12:08, Gregory CLEMENT wrote:> Hi Andrew,
>
>>>
>>> Le 21 déc. 2013 17:43, "Andrew Lunn" <andrew-g2DYL2Zd6BY@public.gmane.org <mailto:andrew-g2DYL2Zd6BY@public.gmane.org>> a écrit :
>>>
>>>     Hi Nobuhiro
>>>
>>>     You added I2C into the Openblocks AX3-4 device tree and the subnode
>>>     for the RTC. Did you have any problems with I2C? I'm having lots of
>>>     problems with my AX3-4 and i2c. I get i2c errors and the bus being
>>>     locked and then the whole machine locks solid.
>>>
>>>     Gregory, did you test your I2C patches for 370/XP on an Openblocks
>>>     AX3-4?
>>>
>>>     Thanks
>>>             Andrew

> On 22/12/2013 00:06, Gregory CLEMENT wrote:
>> Hi Andrew,
>>
>> I am away this weekend (Christmas with the first part of the family).
>>
>> But I remember having few issue on i2c with the first AX3-4 I received. With the second one (using a B0 steping CPU), I didn't remember having any issues. As I thought the first AX3-4 I
>> received was an early release I didn't paid more attention. The last i2c patches I sent were tested on the AX3-4 CPU rev B.
>>
>> Could you tell me what kind of command did you do to get your issues ? Then I will try to reproduce it on Monday on both revision
>>
>> Thanks,
>>
>> Gregory
>
>
> Sorry to not have answered earlier but when I investigated it I found
> an unexpected issue.
>
> First I wanted to be sure that there the issue was not introduce by a
> commit so reverted one by one the commits on the file
> drivers/i2c/busses/i2c-mv64xxx.c. I tested it on both version of the
> OpenBlock AX-4 (with CPU A0 and B0). After each commit the kernel
> continue to work on the B0 version as expected, but it was when I
> reverted the commit "i2c: mv64xxx: Add I2C Transaction Generator
> support" that it worked also on the A0 version.
>
> Then I had a look on the errata datasheet and I found issues that I
> missed when I worked on it. This issues were fixed in B0 version.
>
> The fix should be pretty simple: disabling the offload_enabled flag when
> an A0 version of the CPU is used. For this there are 2 solutions:
> introducing a new compatible string or trying to detect the CPU
> stepping at runtime. I would prefer the second solution and I am looking
> for a way to get this information.


We can have this information in the same way that it is currently done
by the other mvebu SoC: accessing the PCIE_DEV_REV_OFF register. In
arch/arm/plat-orion/pcie.c there were functions named
orion_pcie_dev_id() and orion_pcie_dev_id() to retrieve information
about the CPU variant and its version.

We could the same in drivers/pci/host/pci-mvebu.c, however it would
add a dependency between PCIe and I2C for the mvebu SoCs. I can think
of several options:

1. Using only a new compatible strings: mv78230-A0-i2c. The benefits
of it are that it is very easy to implement and it don't touch anything
else than the driver itself. The drawback is that we need to add an
new dts file for the A0 variant of the AX3-4.

2. Using the CPU revision information from the pcie driver. The
benefits of it are that we don't need modify any device tree and a fix
in the kernel will be enough. The drawback is that we introduce a
dependency between PCIe and I2C.

3. Using the CPU revision information as primary solution and the
compatible string as fallback. The benefits of it are there won't be a
mandatory dependency between PCI and I2c and the change of the dts
won't be mandatory too. The drawback is that we could have a non
working kernel if on a Rev A0 CPU, PCIe is not enable _and_
mv78230-A0-i2c compatible string is not selected.

I am open to other ideas as I am not found of none of them.


Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: Openblocks AX3-4 i2c bus lockup

[Sebastian Hesselbarth]

On 12/31/2013 01:28 PM, Gregory CLEMENT wrote:
>> First I wanted to be sure that there the issue was not introduce by a
>> commit so reverted one by one the commits on the file
>> drivers/i2c/busses/i2c-mv64xxx.c. I tested it on both version of the
>> OpenBlock AX-4 (with CPU A0 and B0). After each commit the kernel
>> continue to work on the B0 version as expected, but it was when I
>> reverted the commit "i2c: mv64xxx: Add I2C Transaction Generator
>> support" that it worked also on the A0 version.
>>
>> Then I had a look on the errata datasheet and I found issues that I
>> missed when I worked on it. This issues were fixed in B0 version.
>>
>> The fix should be pretty simple: disabling the offload_enabled flag when
>> an A0 version of the CPU is used. For this there are 2 solutions:
>> introducing a new compatible string or trying to detect the CPU
>> stepping at runtime. I would prefer the second solution and I am looking
>> for a way to get this information.
>
>
> We can have this information in the same way that it is currently done
> by the other mvebu SoC: accessing the PCIE_DEV_REV_OFF register. In
> arch/arm/plat-orion/pcie.c there were functions named
> orion_pcie_dev_id() and orion_pcie_dev_id() to retrieve information
> about the CPU variant and its version.

Depending on running pcie when calling is tricky, as it can be clock
gated. Maybe we should have some mach code to get the SoC revision
early for all SoCs. That should look for a pcie controller node,
enable the clock, store the revision once, and disable the clock
again. The callback can then return the stored value.

> We could the same in drivers/pci/host/pci-mvebu.c, however it would
> add a dependency between PCIe and I2C for the mvebu SoCs. I can think
> of several options:
>
> 1. Using only a new compatible strings: mv78230-A0-i2c. The benefits
> of it are that it is very easy to implement and it don't touch anything
> else than the driver itself. The drawback is that we need to add an
> new dts file for the A0 variant of the AX3-4.

I know that DT should describe HW, but at this point I tend to not
fork off another dts. If it is probable, we should probe it. SoC
revisions are really hard to see even from looking at the pcb, there
is no way for users to determine the correct dts.

> 2. Using the CPU revision information from the pcie driver. The
> benefits of it are that we don't need modify any device tree and a fix
> in the kernel will be enough. The drawback is that we introduce a
> dependency between PCIe and I2C.
>
> 3. Using the CPU revision information as primary solution and the
> compatible string as fallback. The benefits of it are there won't be a
> mandatory dependency between PCI and I2c and the change of the dts
> won't be mandatory too. The drawback is that we could have a non
> working kernel if on a Rev A0 CPU, PCIe is not enable _and_
> mv78230-A0-i2c compatible string is not selected.
>
> I am open to other ideas as I am not found of none of them.

Re: Openblocks AX3-4 i2c bus lockup

[Jason Cooper]

On Tue, Dec 31, 2013 at 01:50:34PM +0100, Sebastian Hesselbarth wrote:
> On 12/31/2013 01:28 PM, Gregory CLEMENT wrote:
> >>First I wanted to be sure that there the issue was not introduce by a
> >>commit so reverted one by one the commits on the file
> >>drivers/i2c/busses/i2c-mv64xxx.c. I tested it on both version of the
> >>OpenBlock AX-4 (with CPU A0 and B0). After each commit the kernel
> >>continue to work on the B0 version as expected, but it was when I
> >>reverted the commit "i2c: mv64xxx: Add I2C Transaction Generator
> >>support" that it worked also on the A0 version.
> >>
> >>Then I had a look on the errata datasheet and I found issues that I
> >>missed when I worked on it. This issues were fixed in B0 version.
> >>
> >>The fix should be pretty simple: disabling the offload_enabled flag when
> >>an A0 version of the CPU is used. For this there are 2 solutions:
> >>introducing a new compatible string or trying to detect the CPU
> >>stepping at runtime. I would prefer the second solution and I am looking
> >>for a way to get this information.
> >
> >
> >We can have this information in the same way that it is currently done
> >by the other mvebu SoC: accessing the PCIE_DEV_REV_OFF register. In
> >arch/arm/plat-orion/pcie.c there were functions named
> >orion_pcie_dev_id() and orion_pcie_dev_id() to retrieve information
> >about the CPU variant and its version.
> 
> Depending on running pcie when calling is tricky, as it can be clock
> gated. Maybe we should have some mach code to get the SoC revision
> early for all SoCs. That should look for a pcie controller node,
> enable the clock, store the revision once, and disable the clock
> again. The callback can then return the stored value.

Agreed.

> >We could the same in drivers/pci/host/pci-mvebu.c, however it would
> >add a dependency between PCIe and I2C for the mvebu SoCs. I can think
> >of several options:
> >
> >1. Using only a new compatible strings: mv78230-A0-i2c. The benefits
> >of it are that it is very easy to implement and it don't touch anything
> >else than the driver itself. The drawback is that we need to add an
> >new dts file for the A0 variant of the AX3-4.

If we decide to do this, it should be mv78230-i2c runs assuming it is
on the A0 variant.  mv78230-B0-i2c would permit offloading.

> I know that DT should describe HW, but at this point I tend to not
> fork off another dts. If it is probable, we should probe it. SoC
> revisions are really hard to see even from looking at the pcb, there
> is no way for users to determine the correct dts.

In theory, this is something that could be tweaked at runtime by the
bootloader.  But the bootloaders aren't there yet, and requiring a
bootloader upgrade isn't an option.  However, this is something that
should definitely be expressed in the DT.

I have no problem with doing both.  eg check for -B0-i2c, if that's
missing, retrieve the CPU variant and then enable/disable offloading.

thx,

Jason.

Re: Openblocks AX3-4 i2c bus lockup

[Gregory CLEMENT]

Hi Jason, Sebastian,

On 31/12/2013 14:33, Jason Cooper wrote:
> On Tue, Dec 31, 2013 at 01:50:34PM +0100, Sebastian Hesselbarth wrote:
>> On 12/31/2013 01:28 PM, Gregory CLEMENT wrote:
>>>> First I wanted to be sure that there the issue was not introduce by a
>>>> commit so reverted one by one the commits on the file
>>>> drivers/i2c/busses/i2c-mv64xxx.c. I tested it on both version of the
>>>> OpenBlock AX-4 (with CPU A0 and B0). After each commit the kernel
>>>> continue to work on the B0 version as expected, but it was when I
>>>> reverted the commit "i2c: mv64xxx: Add I2C Transaction Generator
>>>> support" that it worked also on the A0 version.
>>>>
>>>> Then I had a look on the errata datasheet and I found issues that I
>>>> missed when I worked on it. This issues were fixed in B0 version.
>>>>
>>>> The fix should be pretty simple: disabling the offload_enabled flag when
>>>> an A0 version of the CPU is used. For this there are 2 solutions:
>>>> introducing a new compatible string or trying to detect the CPU
>>>> stepping at runtime. I would prefer the second solution and I am looking
>>>> for a way to get this information.
>>>
>>>
>>> We can have this information in the same way that it is currently done
>>> by the other mvebu SoC: accessing the PCIE_DEV_REV_OFF register. In
>>> arch/arm/plat-orion/pcie.c there were functions named
>>> orion_pcie_dev_id() and orion_pcie_dev_id() to retrieve information
>>> about the CPU variant and its version.
>>
>> Depending on running pcie when calling is tricky, as it can be clock
>> gated. Maybe we should have some mach code to get the SoC revision
>> early for all SoCs. That should look for a pcie controller node,
>> enable the clock, store the revision once, and disable the clock
>> again. The callback can then return the stored value.
> 
> Agreed.

I will try to submit something as knowing the variant and the revision
of the SoCs will be very useful.

> 
>>> We could the same in drivers/pci/host/pci-mvebu.c, however it would
>>> add a dependency between PCIe and I2C for the mvebu SoCs. I can think
>>> of several options:
>>>
>>> 1. Using only a new compatible strings: mv78230-A0-i2c. The benefits
>>> of it are that it is very easy to implement and it don't touch anything
>>> else than the driver itself. The drawback is that we need to add an
>>> new dts file for the A0 variant of the AX3-4.
> 
> If we decide to do this, it should be mv78230-i2c runs assuming it is
> on the A0 variant.  mv78230-B0-i2c would permit offloading.

That means changing the meaning of the actual compatible string
mv78230-i2c.

> 
>> I know that DT should describe HW, but at this point I tend to not
>> fork off another dts. If it is probable, we should probe it. SoC
>> revisions are really hard to see even from looking at the pcb, there
>> is no way for users to determine the correct dts.
> 
> In theory, this is something that could be tweaked at runtime by the
> bootloader.  But the bootloaders aren't there yet, and requiring a
> bootloader upgrade isn't an option.  However, this is something that
> should definitely be expressed in the DT.
> 
> I have no problem with doing both.  eg check for -B0-i2c, if that's
> missing, retrieve the CPU variant and then enable/disable offloading.
> 

I would do this in the opposite order: probed value would prevail over
the static one.

My plan is:

First introducing a new compatible string and handle it in the driver.
Theses patches will be simple and small enough to be applied on the
current rc kernel and the stable kernels. I will also add a new dts
file: armada-xp-a0-openblocks-ax3-4.dts (and maybe introduce adding
also a armada-xp-common-openblocks-ax3-4.dtsi file). A0 SoCs are no
more shipped, that's why I try to provide a solution which uses the B0
SoC by default and which makes the A0 the exception.

Then adding a way to get the variant and the revision of the SoCs.

Finally using this information in the i2c-mv64xxx driver to decide to
enable or not the offloading, and using the compatible string as a
fallback.

Thanks,

Gregory



-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: Openblocks AX3-4 i2c bus lockup

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Tue, 31 Dec 2013 15:23:40 +0100, Gregory CLEMENT wrote:

> My plan is:
> 
> First introducing a new compatible string and handle it in the driver.
> Theses patches will be simple and small enough to be applied on the
> current rc kernel and the stable kernels. I will also add a new dts
> file: armada-xp-a0-openblocks-ax3-4.dts (and maybe introduce adding
> also a armada-xp-common-openblocks-ax3-4.dtsi file). A0 SoCs are no
> more shipped, that's why I try to provide a solution which uses the B0
> SoC by default and which makes the A0 the exception.

I personally don't think it's worth spending time on doing a separate
DT for this, and that we should go directly to the solution that
runtime detects the SoC revision instead.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Openblocks AX3-4 i2c bus lockup

[Andrew Lunn]

> >We can have this information in the same way that it is currently done
> >by the other mvebu SoC: accessing the PCIE_DEV_REV_OFF register. In
> >arch/arm/plat-orion/pcie.c there were functions named
> >orion_pcie_dev_id() and orion_pcie_dev_id() to retrieve information
> >about the CPU variant and its version.
> 
> Depending on running pcie when calling is tricky, as it can be clock
> gated. Maybe we should have some mach code to get the SoC revision
> early for all SoCs. That should look for a pcie controller node,
> enable the clock, store the revision once, and disable the clock
> again. The callback can then return the stored value.

I think this is something we need, not just for this i2c problem, but
for other issues, in particular, helping userspace get the right DT
file.

We already have kirkwood-ts219-6281.dts and kirkwood-ts219-6282.dts,
different QNAP 21X variants which differ by SoC. I will soon be adding
kirkwood-ts419-6281.dts and kirkwood-ts419-6282.dts, once testing is
complete.

It would be nice if the Debian installer can figure out which variant
is needed. I don't think we currently reliably export this information
to user space. We do print it at boot time, but there is no guarantee
it will still be there later. lspci does not seem to show it, and that
assumes PCIe devices are actually enabled in the DT, which for
ts219-6181 they are not.

At the moment, PCI_MVEBU is a bool in Kconfig. It is either built in,
or not available. That helps a little, but not too much. It could be
the i2c driver gets probed before PCI. Also, for Dove, PCI is
optional, since things like CuBox does not have any use of PCI.  

So maybe some mach code would be best, which directly peeks the PCIe
registers, and twiddles the clocks as needed. We can hard code the
addresses, and use dt_compat to determine which set of addresses to
use, and do it from init_machine, so we don't need to worry about what
the PCIe and clock drivers are doing.

    Andrew

Re: Openblocks AX3-4 i2c bus lockup

[Gregory CLEMENT]

Hi Andrew,

On 31/12/2013 23:21, Andrew Lunn wrote:
>>> We can have this information in the same way that it is currently done
>>> by the other mvebu SoC: accessing the PCIE_DEV_REV_OFF register. In
>>> arch/arm/plat-orion/pcie.c there were functions named
>>> orion_pcie_dev_id() and orion_pcie_dev_id() to retrieve information
>>> about the CPU variant and its version.
>>
>> Depending on running pcie when calling is tricky, as it can be clock
>> gated. Maybe we should have some mach code to get the SoC revision
>> early for all SoCs. That should look for a pcie controller node,
>> enable the clock, store the revision once, and disable the clock
>> again. The callback can then return the stored value.
> 
> I think this is something we need, not just for this i2c problem, but
> for other issues, in particular, helping userspace get the right DT
> file.
> 
> We already have kirkwood-ts219-6281.dts and kirkwood-ts219-6282.dts,
> different QNAP 21X variants which differ by SoC. I will soon be adding
> kirkwood-ts419-6281.dts and kirkwood-ts419-6282.dts, once testing is
> complete.
> 
> It would be nice if the Debian installer can figure out which variant
> is needed. I don't think we currently reliably export this information
> to user space. We do print it at boot time, but there is no guarantee
> it will still be there later. lspci does not seem to show it, and that

I also think it could be useful to have a way to know the SoC ID and its
revision. But I don't know where to export it, in /proc/cpuinfo there is a
filed named Revision but I think it refers the board itself. There is also
the /sys/bus/platform/devices/soc.0/ directory which would be a good candidate
to export the SoC ID and revision.

> assumes PCIe devices are actually enabled in the DT, which for
> ts219-6181 they are not.
> 
> At the moment, PCI_MVEBU is a bool in Kconfig. It is either built in,
> or not available. That helps a little, but not too much. It could be
> the i2c driver gets probed before PCI. Also, for Dove, PCI is
> optional, since things like CuBox does not have any use of PCI.  
> 
> So maybe some mach code would be best, which directly peeks the PCIe
> registers, and twiddles the clocks as needed. We can hard code the
> addresses, and use dt_compat to determine which set of addresses to
> use, and do it from init_machine, so we don't need to worry about what
> the PCIe and clock drivers are doing.

I have just written something like that:
http://www.spinics.net/lists/arm-kernel/msg297642.html


Gregory