DRAFT: ARM: Introduce Infineon PSoC 6 SoC

[npal-cy]

This introduce an Infineon PSoC 6 SoC. This is a first MR which include following part:

• Infineon PSoC 6 SOC Devicetree
• Infineon CAT1/PSoC 6 SOC integration
• Infineon CAT1 UART driver / binding for Infineon CAT1 UART driver
• cy8cproto_062_4343w board support

The next PR: will include GPIO driver, ADC driver, CYW42XXX BT HCI extension driver, CYW42XXX wifi driver, etc.

PSoC 6 Integration structure:

This is a draft PR. We are still working on integration, but we would appreciate to get feedback as soon as possible.

[carlescufi]

Thanks for this contribution!

I see that you have added the cy8cproto_062_4343w board and the SoC family SOC_FAMILY_CAT1A as well as SOC_DIE_PSOC6 in soc/arm/infineon_cat1/psoc6. But in the tree we already have cy8ckit_062_ble and cy8ckit_062_wifi_bt and also SOC_FAMILY_PSOC6 in soc/arm/cypress/, so this seems to be duplicating a SoC and a board that is already in-tree, and that was submitted by Cypress, see the commit.

We can't have two implementations of the same board and SoC on the tree, so you need to remove the previous one if you want to rework the whole SoC and board support.

[nandojve]

Hi @carlescufi,

We can't have two implementations of the same board and SoC on the tree, so you need to remove the previous one if you want to rework the whole SoC and board support.

I don 't think it is prudent remove current implementation. I would say it should be renamed to, instead. I remember that a very complex part was treat interrupt for both Cortex-M0+/M4 and that is already working. That was deeply discussed with @galak and @mbolivar-nordic and took months. I mean, I imagine that still have parts in this architecture that should be well discussed.

My view is simple, Infineon should propose a RFC and tell to everyone how this migration will be made. That should present a clear path about how things will be organized and if they want introduce WIFI/BLE I imagine they want to use blobs.

So, instead try reintroduce something ignoring completely what already is working seems not to be right path. I would say, maybe, better way is rework what already works based on a clear path step by step. This is how things are recommended to be made with Zephyr and will involve less people on each review.

[gmarull]

Thanks for this contribution. I see numerous issues in the approach taken here, please check my comments. I agree with others that existing Infineon PSoC support can't be ignored, so there should be an integration plan.

[gmarull]

please split 87a40b2 into smaller chunks, to ease review process. I can spot a few issues, but I can't hardly review thousands of line in one go.

[gmarull]

this needs to be removed, nothing in this header needs them.

[gmarull]

please default based on dt compat status (see some other drivers)

[gmarull]

this PR has dozens of formatting issues (mixing tabs/spaces one of them), can you please review/fix all of them?

[gmarull]

no // comments, please check styleguide.

[gmarull]

you'll need to implement a pinctrl driver, this is no longer allowed. please check #39740

[npal-cy]

i will rework to use pinctrl data instead tx_pin, rx_pin, cts_pin, rts_pin.

[gmarull]

note: pinctrl data is opaque, so drivers are not allowed to use such information. cyhal_uart_init will likely need to be reworked to not initialize pins.

[npal-cy]

do you mean, that i can not retrieve information about port and pin from pinctrl data ? or i can but it is not allowed...
I expected to create objects of cyhat_gpio_t from pinctrl data, which can be used for cyhal_uart_init().

[gmarull]

Opaque types such as pinctrl_soc_pin_t are meant to be opaque, so using their fields is not allowed. With the pinctrl model, drivers need to call the pinctrl_apply_state API to configure pins. There are now a few drivers doing this. You can read this guide for more details https://docs.zephyrproject.org/latest/guides/pinctrl/index.html

[gmarull]

why define a custom init function for each instance? please store DT config to driver's config and create a single one.

[gmarull]

what's going on with all these autogenerated files? I guess this, at best, should be part of a HAL. Some look suspicious, though, like custom clock/pin control

[nandojve]

I agreed! I'll add that everything that is autogenerated should stay at vendors hal and not ay Zephyr main tree.
This is why a plan is important!

[carlescufi]

Agreed as well, please move this to the HAL repo.

[gmarull]

why is "board support package" needed in Zephyr?

[carlescufi]

Right, in Zephyr we do not have "BSP"s, we just have "boards".

[ifyall]

@gmarull and @carlescufi,

The cybsp_* APIs are APIs from our SOC SDK. They are used to act on a board (represented by a BSP) to initialize or control peripherals. Rather than reimplement how we initialize our board in different environments, we just reuse the SDK.

Does this make sense and address your concerns?

[gmarull]

I think this PR is missing some fundamental Zephyr concepts. There's no "BSP" in Zephyr. Boards define instead the enabled SoC peripherals or other hardware devices in Devicetree. Then, drivers pick this information to setup the system accordingly. We still have some boards doing manual pinmux, something that could be seen as "BSP code", but still this will soon be deprecated because there's now a pinctrl API. So the approach taken here has to be reworked to follow this model. Note that you can still plug some init code into the SoC init function, if needed.

[npal-cy]

The board uses GPIO, UART from Devicetree configuration. Another peripherals support (ADC, SPI, I2C, etc) will be done in future PRs.

"bsp" was reused from existing Asset (https://github.com/Infineon/TARGET_CY8CPROTO-062-4343W) to do board configuration of system resources (PLL, FLL, WCO, CM4/CM0p clock configuration, SWD configuration, power, etc).

so we can do following:

1. move 'bsp' from board zephyr\boards\arm\cy8cproto_062_4343w\ to hal_infenion\bsp\TARGET_CY8CPROTO-062-4343W
2. call cybsp_init() from zephyr\soc\arm\infineon_cat1\psoc6\soc.c.

[gmarull]

No, you don't need any BSP code. You need drivers that configure your peripherals based on DT configuration. Sometimes, you'll need some init code part of soc.c, but not a BSP hidden within soc.c.

[gmarull]

please no autogenerated files of this kind in-tree

[carlescufi]

So, instead try reintroduce something ignoring completely what already is working seems not to be right path. I would say, maybe, better way is rework what already works based on a clear path step by step. This is how things are recommended to be made with Zephyr and will involve less people on each review.

@nandojve I agree, although ultimately it is the contributor's call how to present the change. As it exists today it certainly cannot go in, so I leave it to the contributors to pick a direction. I do agree that starting from scratch when there is an existing implementation in the tree seems counter-productive, but I do not know enough about the port or the SoC to argue against it. I would personally be much happier with smaller steps instead of a wholesale replace like this too.

[nandojve]

I agreed! I'll add that everything that is autogenerated should stay at vendors hal and not ay Zephyr main tree.
This is why a plan is important!

[nandojve]

So, instead try reintroduce something ignoring completely what already is working seems not to be right path. I would say, maybe, better way is rework what already works based on a clear path step by step. This is how things are recommended to be made with Zephyr and will involve less people on each review.

@nandojve I agree, although ultimately it is the contributor's call how to present the change. As it exists today it certainly cannot go in, so I leave it to the contributors to pick a direction. I do agree that starting from scratch when there is an existing implementation in the tree seems counter-productive, but I do not know enough about the port or the SoC to argue against it. I would personally be much happier with smaller steps instead of a wholesale replace like this too.

I know a little about PSoC-6 and it is a complex device. Sometimes it could be controversial in some aspects. Because of that I believe it will be better a plan about how to do it and make it easy to acceptance. I'm suggesting an RFC because experts can do suggestions and it keep records about decisions. The other thing which is good about RFC is that anyone can track all changes at one place, for instance #38657. This way anyone can have a big picture about what is happen and that makes life easy for everyone, mostly to reviewers and users.

[ifyall]

We have filed an RFC to discuss this planning. The RFC is here: #42883

[npal-cy]

per discussion, closing this PR.
will open new PRs (smaller portion) in scope of #42883 tasks.

NOTE: i will save/move all current comments, which requires code change to the new PRs.

Thanks you All for review!