arm: dts: Update coraz7s to match changes on the hdl. #2657

gastmaier · 2024-11-26T14:15:33Z

PR Description

To allow automatic EVB detection with the CoraZ7S, an IIC Controller is added to pin ARD SCL SDA at of the CoraZ7S.
The projects shall remove the controller if it is unused, but they cannot reuse the pins for Active In High signal (out from the evb),
to avoid a short during the runtime with the common bitstream, prior to switching to the project bitstream.

Since some projects already had IIC Controller, and to normalize across projects the addresses and irq pins, the following changes are applied:

    IIC Changes:
    Project       Old          New          IIC Subordinate     EEPROM ADDR**
    ad4170_asdz   ps-12, 44a4  ps-11, 4160  24AA32A             0x52
    ad57xx_ardz   PS7 IIC      ps-11, 4160  24AA32A             0x54
    pulsar_adc    none         ps-11, 4160  - 
    ad719x_asdz   none         ps-11, 4160  -
    cn0579        ps-12, 44a4  ps-11, 4160  AD5696
    cn0540        ps-12, 44a4  ps-11, 4160  LTC2606
    cn0561        PS7 IIC      ps-11, 4160  M24C02

    IRQ index changes:
    ad4170_asdz
    * $hier_spi_engine 11 -> 12

    ad57xx_ardz
    * ad57xx_dma 12 -> 13
    * $hier_spi_engine 11 -> 12

    cn0579
    * cn0579_dma 13 -> 12
    * axi_iic_ard 12 -> 11

    cn0540
    * $hier_spi_engine 11 -> 12
    * axi_iic_cn0540   12 -> 11

** Stopped checking after the first different value.

And to sync the changes to committed devicetrees, the following files were analyzed and modified:

devicetree                      HDL project     Change
zynq-coraz7s-ad7687-pmdz.dts    pulsar_adc      None, ignore IIC
zynq-coraz7s-ad7689-ardz.dts    pulsar_adc      None, ignore IIC
zynq-coraz7s-ad7946.dts         pulsar_adc      None, ignore IIC
zynq-coraz7s-ad7984.dts         pulsar_adc      None, ignore IIC
zynq-coraz7s-adaq4003.dts       pulsar_adc      None, ignore IIC
zynq-coraz7s-cn0501.dts         -               Removed
zynq-coraz7s-cn0540.dts         cn0540          IIC IRQ, ADDR ; SPI IRQ
zynq-coraz7s-cn0579_i2c.dts     cn0579          IIC IRQ, ADDR ; DMA IRQ
zynq-coraz7s.dts                common          Added IIC with EEPROM at 0x50
zynq-coraz7s.dtsi               -               None

Changes:

Remove zynq-coraz7s-cn0501.dts, since the hdl project was removed, see analogdevicesinc/hdl@f5184b4
Add IIC Controller to zynq-coraz7s.dts with EEPROM (address 0x50 is placeholder, evb change)
Update cn0540 and cn0579_i2c

Notes:

dts for hdl project ad4170_asdz and ad57xx_ardz not on main, ad4170_asdz may be staging.
No dts is upstream

Faq:

Why not add the IIC Controller to zynq-coraz7s.dtsi?
The projects shall not instantiate the IIC Controller, or replace to something else.

What is the expected behavior for the common project with and EVB without EEPROM?

The IIC controller (i2c-xiic.c) will probe and the i2c eeprom driver (at24.c) will fail to probe due to NACK transfer at test_byte.
The software layer then acts accordingly (no project detection).

If the common project has the controller, how can the project not have the controller?
The devicetrees described are used as a base for a more complex setup where they are converted to overlays,
and the pipeline is:

┌──► load common bitstream ─► load common overlay ─► (detect evb | user) ─► unload overlay ─┐
│                                                                                           │
│ ┌─────────────────────────────────────────────────────────────────────────────────────────┘
│ │                                                                                          
│ └► load (auto | manual) project bitstream ─► load overlay ─►(evaluation)─► unload overlay ┐
│                                                                                           │
└───────────────────────────────────────────────────────────────────────────────────────────┘

This PR aims to sync the HDL changes with the merged to main devicetrees,
fixing the addresses and irq indexes.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly (if there is the case)

nunojsa · 2024-11-28T08:12:23Z

arch/arm/boot/dts/zynq-coraz7s.dts

+		};
+
+	};
+};


I wonder if we should then add it to the common DT? To me is always odd to add a device that's not present just to have it failing probe... Following this approach could be dangerous (example with MMIO devices). For it to be in the common DT, I assume it's used in the vast majority of the projects? For the ones where's not used we could just delete the node from the top level DT...

Hi, if /delete-node/ is an option I can add it to the necessary devicetrees and move the controller&eeprom to the dtsi.
But we need to clarify to which device: The IIC Controller or the EEPROM?
The IIC Controller needs to removed if not on the bitstream because r/w to the axi addr always causes a kernel panic.
Drivers on the I2C bus fail to probe gracefully.

At the HDL side, I kept the iic controller for the pulsar project even though no pulsar-evb has an eeprom; for this one in particular*, we can remove either the device or controller from the devicetree.
What do you prefer?
*zynq-coraz7s-ad7687-pmdz.dts,zynq-coraz7s-ad7689-ardz.dts,zynq-coraz7s-ad7946.dts,zynq-coraz7s-ad7984.dts,zynq-coraz7s-adaq4003.dts

I prefer the 4th option better or the current pr solution, but here are other ideas:

2th option (move to dtsi, keep pulsar unused):

devicetree HDL project Change zynq-coraz7s-ad7687-pmdz.dts pulsar_adc None (already on dtsi) zynq-coraz7s-ad7689-ardz.dts pulsar_adc None (already on dtsi) zynq-coraz7s-ad7946.dts pulsar_adc None (already on dtsi) zynq-coraz7s-ad7984.dts pulsar_adc None (already on dtsi) zynq-coraz7s-adaq4003.dts pulsar_adc None (already on dtsi) zynq-coraz7s-cn0501.dts - Removed zynq-coraz7s-cn0540.dts cn0540 Remove IIC (already on dtsi); SPI IRQ zynq-coraz7s-cn0579_i2c.dts cn0579 Remove IIC (already on dtsi); DMA IRQ zynq-coraz7s.dts common None zynq-coraz7s.dtsi - Added IIC with EEPROM at 0x50

3th option (move to dtsi, remove from pulsar:

devicetree HDL project Change zynq-coraz7s-ad7687-pmdz.dts pulsar_adc Delete ICC (unused) zynq-coraz7s-ad7689-ardz.dts pulsar_adc Delete ICC (unused) zynq-coraz7s-ad7946.dts pulsar_adc Delete ICC (unused) zynq-coraz7s-ad7984.dts pulsar_adc Delete ICC (unused) zynq-coraz7s-adaq4003.dts pulsar_adc Delete ICC (unused) zynq-coraz7s-cn0501.dts - Removed zynq-coraz7s-cn0540.dts cn0540 Remove IIC (already on dtsi); SPI IRQ zynq-coraz7s-cn0579_i2c.dts cn0579 Remove IIC (already on dtsi); DMA IRQ zynq-coraz7s.dts common None zynq-coraz7s.dtsi - Added IIC with EEPROM at 0x50

Also, the EEPROM addresses are not consistent; and is a problem we already have with the zed projects.

And I'm using zynq-coraz7s.dts as zynq-zed-adv7511.dtsi, a fourth option is to create
zynq-coraz7s-iic.dtsi and import that to other projects:

zynq-coraz7s-ad7687-pmdz.dts pulsar_adc None zynq-coraz7s-ad7689-ardz.dts pulsar_adc None zynq-coraz7s-ad7946.dts pulsar_adc None zynq-coraz7s-ad7984.dts pulsar_adc None zynq-coraz7s-adaq4003.dts pulsar_adc None zynq-coraz7s-cn0501.dts - Removed zynq-coraz7s-cn0540.dts cn0540 Remove IIC; SPI IRQ, import zynq-coraz7s-iic.dtsi zynq-coraz7s-cn0579_i2c.dts cn0579 Remove IIC; DMA IRQ, import zynq-coraz7s-iic.dtsi zynq-coraz7s.dts common None zynq-coraz7s.dtsi - None zynq-coraz7s-iic.dtsi - Created with IIC and EEPROM at 0x50

But we need to clarify to which device: The IIC Controller or the EEPROM?
The IIC Controller needs to removed if not on the bitstream because r/w to the axi addr always causes a kernel panic.
Drivers on the I2C bus fail to probe gracefully.

Hmm, I would just remove the device that's not present...

a fourth option is to create
zynq-coraz7s-iic.dtsi and import that to other projects:

Hmm, this indeed seems the cleanest to me. You mean, having a dedicated dtsi file for the IIC and import it only on the projects used? We would still need to delete the EEPROM node on projects not having them right?

Another question would be, are these nodes (IIC and EEPROM) consistent over zynq/zed projects? I'm wondering if we could even have a dtsi file more generic (and not only for coraz). I do realize this is a bigger project and a bit out of scope of this PR but something to keep in mind. It looks like we could use some refactoring in our DTs.

OK, let's stick with the 4th option.

We would still need to delete the EEPROM node on projects not having them right?
We don't need to delete the EEPROM from projects that don't have it because part drivers will fail to probe at the first I2C NACK.
But I can remove to avoid the warning during boot if preferred.
I will check each schematic individually to map that.

I added the IIC at address 0x4160 for the cora, and zed instantiates at 0x4162 since 0x4160 is used for another IIC controller.
I wouldn't try to make a generic dtsi for multiple carriers, they have different address offsets. Zynq UltraScale+ MP AXI IPs start on 0x8000_0000 instead 0x4000_0000, so it isn't much use long term place the IIC on 0x4162.

So, can I update the PR with the 4th option, and remove the EEPROM (but not the controller) from the projects that don't have one?

Also, it is worth noting that the EEPROM address changes for each EVB, the dts entry is more of a template as is.

I added the IIC at address 0x4160 for the cora, and zed instantiates at 0x4162 since 0x4160 is used for another IIC controller.
I wouldn't try to make a generic dtsi for multiple carriers, they have different address offsets. Zynq UltraScale+ MP AXI IPs start on 0x8000_0000 instead 0x4000_0000, so it isn't much use long term place the IIC on 0x4162.

I see, then the generic node does not make sense as we would have to anyways change it for every carrier.

So, can I update the PR with the 4th option, and remove the EEPROM (but not the controller) from the projects that don't have one?

Sounds good...

gastmaier · 2024-12-02T16:48:42Z

V0->V1
implement the following:

devicetree                      HDL project     Change
zynq-coraz7s-ad7687-pmdz.dts    pulsar_adc      None
zynq-coraz7s-ad7689-ardz.dts    pulsar_adc      None
zynq-coraz7s-ad7946.dts         pulsar_adc      None
zynq-coraz7s-ad7984.dts         pulsar_adc      None
zynq-coraz7s-adaq4003.dts       pulsar_adc      None
zynq-coraz7s-cn0501.dts         -               Removed
zynq-coraz7s-cn0540.dts         cn0540          import zynq-coraz7s-iic.dtsi, remove old IIC, update SPI IRQ
zynq-coraz7s-cn0579_i2c.dts     cn0579          import zynq-coraz7s-iic.dtsi, remove old IIC, update DMA IRQ
zynq-coraz7s.dts                common          import zynq-coraz7s-iic.dtsi, add EEPROM at 0x50
zynq-coraz7s.dtsi               -               None
zynq-coraz7s-iic.dtsi           -               Created with IIC

Pulsar projects do not instantiate the IIC Controller, even if in the bitstream, since those are PMOD boards.
CN**: Use the common IIC Controller, but do not include the EEPROM since they are not present.
Common: Use the common IIC Controller and add the EEPROM, as a template, since the I2C address changes between evb,

Add common IIC Controller instantiation for Coraz7s projects, to be imported by projects that use it, including the common project. Signed-off-by: Jorge Marques <[email protected]>

Import IIC Controller from the -iic,dtsi and add the EEPROM to common dts, used to detect the evb if the EEPROM is available. If not, the at24.c driver fails to probe at the first test_byte due to NACKed transfer. Signed-off-by: Jorge Marques <[email protected]>

Include IIC Controller from the coraz7s-iic.dtsi. Update SPI Engine IRQ index. To sync with the changes on the HDL project. Signed-off-by: Jorge Marques <[email protected]>

Include IIC Controller from the coraz7s-iic.dtsi. Update RX DMA IRQ index. To sync with the changes on the HDL project. Signed-off-by: Jorge Marques <[email protected]>

The board development was canceled. See hdl/f5184b4e14bc61cd18dad7b5a648ea76e1f1ec5e Signed-off-by: Jorge Marques <[email protected]>

gastmaier · 2024-12-03T17:13:32Z

V1->V2
Force pushed to resolve main rebase.
Used rebase checkout --theirs and git diff public/main..@

gastmaier marked this pull request as ready for review November 27, 2024 13:07

gastmaier requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners November 27, 2024 13:07

nunojsa reviewed Nov 28, 2024

View reviewed changes

gastmaier force-pushed the coraz7s branch from c0cbc5f to 3188bdf Compare December 2, 2024 16:45

gastmaier requested a review from nunojsa December 3, 2024 12:35

gastmaier added 5 commits December 3, 2024 14:07

arch: arm: boot: dtsi: Add coraz7s-iic

886a644

Add common IIC Controller instantiation for Coraz7s projects, to be imported by projects that use it, including the common project. Signed-off-by: Jorge Marques <[email protected]>

arch: arm: boot: dts: Update coraz7s-cn0540

fbdda4c

Include IIC Controller from the coraz7s-iic.dtsi. Update SPI Engine IRQ index. To sync with the changes on the HDL project. Signed-off-by: Jorge Marques <[email protected]>

arch: arm: boot: dts: Update coraz7s-cn0579

6f37504

Include IIC Controller from the coraz7s-iic.dtsi. Update RX DMA IRQ index. To sync with the changes on the HDL project. Signed-off-by: Jorge Marques <[email protected]>

arch: arm: boot: dts: Remove cn0501

07c4aa2

The board development was canceled. See hdl/f5184b4e14bc61cd18dad7b5a648ea76e1f1ec5e Signed-off-by: Jorge Marques <[email protected]>

gastmaier force-pushed the coraz7s branch from 3188bdf to 07c4aa2 Compare December 3, 2024 17:11

nunojsa approved these changes Dec 6, 2024

View reviewed changes

gastmaier merged commit 7339f50 into main Dec 6, 2024
11 of 13 checks passed

gastmaier deleted the coraz7s branch December 6, 2024 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arm: dts: Update coraz7s to match changes on the hdl. #2657

arm: dts: Update coraz7s to match changes on the hdl. #2657

gastmaier commented Nov 26, 2024 •

edited

Loading

nunojsa Nov 28, 2024

gastmaier Nov 28, 2024 •

edited

Loading

nunojsa Nov 29, 2024

gastmaier Nov 29, 2024

nunojsa Dec 2, 2024

gastmaier commented Dec 2, 2024

gastmaier commented Dec 3, 2024

+              		};
+              	};
+              };

arm: dts: Update coraz7s to match changes on the hdl. #2657

arm: dts: Update coraz7s to match changes on the hdl. #2657

Conversation

gastmaier commented Nov 26, 2024 • edited Loading

PR Description

PR Type

PR Checklist

nunojsa Nov 28, 2024

Choose a reason for hiding this comment

gastmaier Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

nunojsa Nov 29, 2024

Choose a reason for hiding this comment

gastmaier Nov 29, 2024

Choose a reason for hiding this comment

nunojsa Dec 2, 2024

Choose a reason for hiding this comment

gastmaier commented Dec 2, 2024

gastmaier commented Dec 3, 2024

gastmaier commented Nov 26, 2024 •

edited

Loading

gastmaier Nov 28, 2024 •

edited

Loading