DRAFT: ARM: Introduce Infineon PSoC 6 SoC #42725

npal-cy · 2022-02-11T08:10:20Z

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

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.

@ifyall

Add Ian Fyall (@ifyall) as codeowner for Infineon/Cypress sources Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

Add initial version of Devicetree for Infineon PSoC 6 SOC . Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

Add initial version of Infineon CAT1/PSoC 6 SOC integration. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

Add initial version of Infineon CAT1 UART driver. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

Add initial version of binding file for Infineon CAT1 UART driver. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

Add initial version of cy8cproto_062_4343w board. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

Enable 'submodules' feature in hal_infineon module. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

carlescufi · 2022-02-11T12:01:01Z

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 · 2022-02-11T13:20:58Z

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 · 2022-02-11T14:41:45Z

dts/arm/infineon/psoc6/common/gpio_pins_def.h

@@ -0,0 +1,160 @@
+/*


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 · 2022-02-11T14:44:03Z

soc/arm/infineon_cat1/psoc6/soc.h

+/* Add include for DTS generated information */
+#include <devicetree.h>
+#include <cy_device_headers.h>


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

gmarull · 2022-02-11T14:44:32Z

drivers/serial/Kconfig.infineon_cat1

+	bool "Infineon CAT1 UART driver"
+		default y


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

gmarull · 2022-02-11T14:45:17Z

drivers/serial/uart_infineon_cat1.c

+    #if CONFIG_UART_INTERRUPT_DRIVEN
+	uart_irq_callback_user_data_t irq_cb;   /* Interrupt Callback */
+	void *irq_cb_data;                      /* Interrupt Callback Arg */
+    #endif /* CONFIG_UART_INTERRUPT_DRIVEN */
+
+    #if CONFIG_UART_ASYNC_API


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

gmarull · 2022-02-11T14:45:37Z

drivers/serial/uart_infineon_cat1.c

+///////////////////////////////////////////////////////////////////////////////
+//                INTERNAL API                                               //
+///////////////////////////////////////////////////////////////////////////////


no // comments, please check styleguide.

gmarull · 2022-02-11T14:47:06Z

drivers/serial/uart_infineon_cat1.c

+		result = cyhal_uart_init(&DEV_DATA(dev)->obj,			  \
+					 (cyhal_gpio_t) DT_INST_PROP(n, tx_pin),  \
+					 (cyhal_gpio_t) DT_INST_PROP(n, rx_pin),  \
+					 (cyhal_gpio_t) DT_INST_PROP(n, cts_pin), \
+					 (cyhal_gpio_t) DT_INST_PROP(n, rts_pin), \
+					 NULL, NULL);				  \


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

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

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.

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().

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 · 2022-02-11T14:47:46Z

drivers/serial/uart_infineon_cat1.c

+#define INFINEON_CAT1_UART_INIT(n)						  \
+	static struct uart_cat1_data INFINEON_CAT1_uart##n##_data = { 0 };	  \
+										  \
+	static int INFINEON_CAT1_uart##n##_init(const struct device *dev)	  \


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

gmarull · 2022-02-11T14:49:17Z

boards/arm/cy8cproto_062_4343w/CMakeLists.txt

+zephyr_library_sources(bsp/cybsp.c)
+zephyr_library_sources(bsp/system_psoc6_cm4.c)
+
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_clocks.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_peripherals.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_pins.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_qspi_memslot.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_routing.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_system.c)


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

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!

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

gmarull · 2022-02-11T14:49:55Z

boards/arm/cy8cproto_062_4343w/board.c

+	/* Initializes the board support package */
+	result = cybsp_init();


why is "board support package" needed in Zephyr?

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

@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?

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.

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:

move 'bsp' from board zephyr\boards\arm\cy8cproto_062_4343w\ to hal_infenion\bsp\TARGET_CY8CPROTO-062-4343W

call cybsp_init() from zephyr\soc\arm\infineon_cat1\psoc6\soc.c.

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 · 2022-02-11T14:50:42Z

boards/arm/cy8cproto_062_4343w/bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg.c

+ * Description:
+ * Wrapper function to initialize all generated code.
+ * This file was automatically generated and should not be modified.
+ * Tools Package 2.4.0.5721


please no autogenerated files of this kind in-tree

carlescufi · 2022-02-11T15:06:43Z

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 · 2022-02-11T15:20:43Z

boards/arm/cy8cproto_062_4343w/CMakeLists.txt

+zephyr_library_sources(bsp/cybsp.c)
+zephyr_library_sources(bsp/system_psoc6_cm4.c)
+
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_clocks.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_peripherals.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_pins.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_qspi_memslot.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_routing.c)
+zephyr_library_sources(bsp/COMPONENT_BSP_DESIGN_MODUS/GeneratedSource/cycfg_system.c)


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 · 2022-02-11T16:02:42Z

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 · 2022-02-17T17:11:43Z

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

npal-cy · 2022-02-18T12:51:07Z

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!

npal-cy added 7 commits February 1, 2022 18:11

CODEOWNERS: Add ifyall for Infineon/Cypress sources

de6800d

Add Ian Fyall (@ifyall) as codeowner for Infineon/Cypress sources Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

dts: arm: Introduce Infineon PSoC 6 SOC Devicetree

87a40b2

Add initial version of Devicetree for Infineon PSoC 6 SOC . Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

soc: arm: Introduce Infineon CAT1/PSoC 6 SOC integration

38b7f55

Add initial version of Infineon CAT1/PSoC 6 SOC integration. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

drivers: serial: Add Infineon CAT1 UART driver

df21cfa

Add initial version of Infineon CAT1 UART driver. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

dts: bindings: serial: Add binding for Infineon CAT1 UART driver

dde040d

Add initial version of binding file for Infineon CAT1 UART driver. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

boards: arm: Introduce cy8cproto_062_4343w board

5c27075

Add initial version of cy8cproto_062_4343w board. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

west.yml: update hal_infineon

051d048

Enable 'submodules' feature in hal_infineon module. Signed-off-by: Nazar Palamar <nazar.palamar@infineon.com>

npal-cy requested review from nashif, galak, nandojve, MaureenHelm and ioannisg as code owners February 11, 2022 08:10

github-actions bot added the area: Boards label Feb 11, 2022

npal-cy changed the title ~~ARM: Introduce Infineon PSoC 6 SoC~~ DRAFT: ARM: Introduce Infineon PSoC 6 SoC Feb 11, 2022

npal-cy marked this pull request as draft February 11, 2022 08:15

carlescufi requested review from gmarull, anangl and mbolivar-nordic February 11, 2022 12:03

gmarull requested changes Feb 11, 2022

View reviewed changes

nandojve requested changes Feb 11, 2022

View reviewed changes

carlocaione added the dev-review To be discussed in dev-review meeting label Feb 17, 2022

carlocaione self-requested a review February 17, 2022 08:12

npal-cy closed this Feb 18, 2022

npal-cy mentioned this pull request Feb 25, 2022

hal: infineon: Add Infineon assets for CAT1/PSoC 6 SoC zephyrproject-rtos/hal_infineon#2

Merged

npal-cy mentioned this pull request Mar 25, 2022

ARM: Introduce Infineon PSoC 6 SoC #44211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRAFT: ARM: Introduce Infineon PSoC 6 SoC #42725

DRAFT: ARM: Introduce Infineon PSoC 6 SoC #42725

npal-cy commented Feb 11, 2022

carlescufi commented Feb 11, 2022

nandojve commented Feb 11, 2022

gmarull left a comment

gmarull Feb 11, 2022

gmarull Feb 11, 2022

gmarull Feb 11, 2022

gmarull Feb 11, 2022

gmarull Feb 11, 2022

gmarull Feb 11, 2022

npal-cy Feb 14, 2022

gmarull Feb 14, 2022

npal-cy Feb 14, 2022

gmarull Feb 14, 2022 •

edited

Loading

gmarull Feb 11, 2022

gmarull Feb 11, 2022

nandojve Feb 11, 2022

carlescufi Feb 11, 2022

gmarull Feb 11, 2022

carlescufi Feb 11, 2022

ifyall Feb 11, 2022

gmarull Feb 13, 2022 •

edited

Loading

npal-cy Feb 14, 2022

gmarull Feb 14, 2022

gmarull Feb 11, 2022

carlescufi commented Feb 11, 2022

nandojve Feb 11, 2022

nandojve commented Feb 11, 2022

ifyall commented Feb 17, 2022

npal-cy commented Feb 18, 2022

		/* Initializes the board support package */
		result = cybsp_init();

DRAFT: ARM: Introduce Infineon PSoC 6 SoC #42725

DRAFT: ARM: Introduce Infineon PSoC 6 SoC #42725

Conversation

npal-cy commented Feb 11, 2022

carlescufi commented Feb 11, 2022

nandojve commented Feb 11, 2022

gmarull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarull Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarull Feb 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Feb 11, 2022

Choose a reason for hiding this comment

nandojve commented Feb 11, 2022

ifyall commented Feb 17, 2022

npal-cy commented Feb 18, 2022

gmarull Feb 14, 2022 •

edited

Loading

gmarull Feb 13, 2022 •

edited

Loading