Add MSPI + flash driver for the TI K3 platform

[m-braunschweig]

This PR adds support for MSPI and the Infineon S25H flash on the TI K3 platform. It is tested with the onboard flash of the AM2434 launchpad. (PR for the driver flash driver: #88446). It's now part of this PR.

For running the code on the AM243x Launchpad this branch is used. Until the AM243x Launchpad is generally supported this shouldn't be merged. However I would still like some early review to speed up merging later. Especially on how to potentially handle the timeout parameter better and whether I interpreted the unit being microseconds right.

The driver itself is pretty basic (e.g. no async and XIP support) but enough for running MCUboot on the board (by loading the image into SRAM).

Edit: Mention that this PR also includes the S25H flash driver now

[gramsay0]

k_uptime_get is the current uptime in milliseconds, calling k_cyc_to_us_floor32 is probably not what you want.
The timeout parameter is not well documented, but it appears to be in milliseconds (from looking at other drivers)

[swift-tk]

It is defined in kconfig

zephyr/drivers/mspi/Kconfig

Line 35 in fe6366b

int "Completion timeout tolerance (ms)"

[gramsay0]

Ahh my bad, I'm not familiar with mspi driver API and only looked at the struct definition. It would be helpful to mention it there too

[m-braunschweig]

Ok I will update that to use milliseconds everywhere in the MSPI driver. I will also switch over to k_uptime_get so I don't need the conversions anymore

[m-braunschweig]

Also: How are the Kconfig option and timeout field correlated? For me it seems quite unclear since it's used both in flash and MSPI drivers, additionally to the already existing timeout field

[swift-tk]

Also: How are the Kconfig option and timeout field correlated? For me it seems quite unclear since it's used both in flash and MSPI drivers, additionally to the already existing timeout field

The Kconfig option is offered as a setting to the controllers for its maximum timeout allowence. The timeout field is variable in the transfer struct but should not exceed this Kconfig option.

[gramsay0]

Seems like the chip select delay should come from struct mspi_ce_control

[swift-tk]

No, not really. struct mspi_ce_control is for software controlled CE. However, I do think that this CE timing maybe peripheral device specific but not transfer specific. NXP have a similar setting as well. We can try add that to struct mspi_dev_cfg
Another option is to slide it in through struct mspi_timing_cfg along with your custom timing parameters.

[m-braunschweig]

For now I'm going to add it to the mspi_dev_cfg to be read from the devicetree
I'm going to add a custom timing config

[gramsay0]

There are a number of mentions of "flash" in this file that should be removed.
This driver may be used for things other than flash memory

[m-braunschweig]

Will update that. They are there because it initially started as a standalone flash driver

[gramsay0]

Consider using GENMASK/FIELD_PREP/FIELD_GET, the macro concatenations (TI_K3_OSPI_##reg##_##field##_FLD_OFFSET) make it difficult to navigate to the register definitions

#define TI_K3_OSPI_CONFIG_REG_IDLE_MASK GENMASK(31, 31)

uint32_t config_reg = sys_read32(base_addr + TI_K3_OSPI_CONFIG_REG_OFFSET);
uint32_t idle = FIELD_GET(TI_K3_OSPI_CONFIG_REG_IDLE_MASK, config_reg);

[m-braunschweig]

I would prefer to keep the macro since it keeps the code in the mspi_ti_k3.c quite short. I think I will add comments that explains the macros and register definitions

[gramsay0]

Add DEVICE_MMIO_RAM; as the first field

[swift-tk]

Are you not going to add any soc DTS for the controller?
To ensure compile across PRs, it is best to add the controller to one of the example or testcase.

[swift-tk]

It is used as part of device_id.

[m-braunschweig]

Can you explain what you exactly mean with that?

Currently this driver uses hardware implemented chip select in the OSPI_CONFIG_REG via the PERIPH_CS_LINES_FLD field. There are 4 dedicated pins for that and they are only referenced by the numbers 0..3, which I directly use in the mspi_ti_k3_dev_config function. When I would use ce-gpios with phandle to the GPIO pins I think it would be necassary to do software CS control.

[swift-tk]

Can you explain what you exactly mean with that?

Currently this driver uses hardware implemented chip select in the OSPI_CONFIG_REG via the PERIPH_CS_LINES_FLD field. There are 4 dedicated pins for that and they are only referenced by the numbers 0..3, which I directly use in the mspi_ti_k3_dev_config function. When I would use ce-gpios with phandle to the GPIO pins I think it would be necassary to do software CS control.

ce-gpios can be used to for software CE control, but it is also used as part of the device id, see struct mspi_dev_id which is filled by MSPI_DEVICE_ID_DT. So you would have to have at least a ce-gpio defined in controller dts. The dev_id should be used to validate the peripheral(child) device within the controller, although some choose not to as no plan to support software-multiperipheral function. i.e. switching between child devices by software under the same controller.
If your controller is capable of switching by hardware CE number, then you can pass that 0~3 number in struct mspi_dev_cfg.ce_num .
I do recognize there might be a compatiblity problem if your CEs are not within one of your GPIO alternate function but dedicated pin. But we will fix it when we get there.

[m-braunschweig]

For now I removed the ce-gpios part from the bindings. However it's still ignored in the driver and added the requirement that GPIO has to be selected in the Kconfig

[swift-tk]

No, not really. struct mspi_ce_control is for software controlled CE. However, I do think that this CE timing maybe peripheral device specific but not transfer specific. NXP have a similar setting as well. We can try add that to struct mspi_dev_cfg
Another option is to slide it in through struct mspi_timing_cfg along with your custom timing parameters.

[swift-tk]

It is defined in kconfig

zephyr/drivers/mspi/Kconfig

Line 35 in fe6366b

int "Completion timeout tolerance (ms)"

[m-braunschweig]

Are you not going to add any soc DTS for the controller?
To ensure compile across PRs, it is best to add the controller to one of the example or testcase.

I will add that to this PR when the AM2434 SoC and AM2434 Launchpad are officially supported. The AM2434 SoC support is currently being done in #87321. After that is merged I will add the AM243x LaunchPad board and after that I can also add the MSPI flash test to this PR. Before that I will only add it to the https://github.com/siemens/zephyr/commits/mika/ti/ti-am2434-native-boot/ branch

[swift-tk]

@m-braunschweig You should squash the 13 commits into 2, one for the controller driver, one for the flash driver. That is zephyr PR policy.

[m-braunschweig]

@swift-tk done.
My original thought for adding extra commits instead of amending them was that this PR is still somewhat WIP / a draft and that it would make reviewing new changes a bit easier. The "final" squash could then have been done when leaving the draft stage.

[m-braunschweig]

Also I have some more questions to the MSPI driver API:

• The TX and RX delay for MSPI devices can be configured both in the mspi_dev_cfg that's used in mspi_dev_config() and it's additionally part of the mspi_xfer struct that's given on every transfer request. In which cases should which value be honored?
• The Infineon S25H flash has different delays for different commands. How should that be handled in the devicetree since there is only one rx-dummy/tx-dummy field? One option would be to add all of them in the devicetree binding, however the original rx-dummy/tx-dummy comes from the generic MSPI device binding
• How should read-command and write-command be handled in this case? Should they use the opcode for single or quad mode transfers?

For the devicetree things I was unsure about I currently check during build time that they are unset. However I don't really like that solution and would be happy to hear about ideas to improve it

[swift-tk]

• The TX and RX delay for MSPI devices can be configured both in the mspi_dev_cfg that's used in mspi_dev_config() and it's additionally part of the mspi_xfer struct that's given on every transfer request. In which cases should which value be honored?

The mspi_dev_cfg contains device specific controller settings. There are cases where a device is initialized with mspi_dev_config() but does not uses mspi_xfer at all(e.g psram or XIP mode) or it needs to use different cmds with different settings when using mspi_xfer. The overlapping members are only the transfer related settings and should not involve changing device state.

The settings given to mspi_xfer takes precedence over mspi_dev_cfg in the context of the peripheral device driver, so tracking the state change with a global mspi_dev_cfg is desirable as the device would have to repeatedly obtain controller access and tries to override controller settings in each device APIs.

• The Infineon S25H flash has different delays for different commands. How should that be handled in the devicetree since there is only one rx-dummy/tx-dummy field? One option would be to add all of them in the devicetree binding, however the original rx-dummy/tx-dummy comes from the generic MSPI device binding

The device tree properties represent the target configuration that the device will run on most of the time. In the case of flash memory, it represents settings when conducting memory write and read. The default settings for device initialization should be stored as const and possibly used for all register access to maintain maximum compatibility as they should typically be low speed.

• How should read-command and write-command be handled in this case? Should they use the opcode for single or quad mode transfers?

If your target operating mode is single mode, then swap the cmds in device tree with cmds used in single mode and the same way with quad mode. Mode switching should be done by mspi_dev_config() and device state change should be tracked by the device driver itself if some commands are not available in one mode or the other. IMO, one would only need one target operating condition for a specific device throughout the software program cycle. So intermediate states should be managed by the device driver.

For the devicetree things I was unsure about I currently check during build time that they are unset. However I don't really like that solution and would be happy to hear about ideas to improve it

You have to check it each time when a config is passed from device driver. To reduce unnecessary config from repeated mspi_dev_config() calls and to maintain maximum compatibility, you should use the acquire and release function in the existing flash mspi drivers.

[natto1784]

Hello, excellent work on the drivers. However, the MSPI controller would be redundant with the cdns,qspi-nor driver (flash_cadence_qspi_nor.c), which is surprisingly in the flash subsystem. I believe we should only keep one of the drivers around and get rid of the other.

If preferred I can move the old cadence driver to the MSPI subsystem and rewrite the flash driver for micron_n25q support which is currently coupled with the aforementioned driver. Please give your inputs on which direction we want to move in, to avoid further duplicated efforts.

[natto1784]

can reuse opcodes from the generic spi_nor.h definition

[natto1784]

ignore this, current approach is better

[swift-tk]

We should reuse code from spi_nor.h when possible and leave only the special ones in this file.

[natto1784]

did you mean to do a ret = flash_mspi_infineon_s25h_prepare_mspi_bus(dev); here?

[m-braunschweig]

Yes. Good catch

[m-braunschweig]

Hello, excellent work on the drivers. However, the MSPI controller would be redundant with the cdns,qspi-nor driver (flash_cadence_qspi_nor.c), which is surprisingly in the flash subsystem. I believe we should only keep one of the drivers around and get rid of the other.

If preferred I can move the old cadence driver to the MSPI subsystem and rewrite the flash driver for micron_n25q support which is currently coupled with the aforementioned driver. Please give your inputs on which direction we want to move in, to avoid further duplicated efforts.

Ok I sadly didn't know that TI used a Cadence IP core as octal SPI peripheral. The existing flash driver seems a bit odd with the N25Q support since I searched with grep for the devicetree compatible string and only found usage inside a Intel SoC FPGA:

zephyr/dts/arm64/intel/intel_socfpga_agilex.dtsi

Lines 83 to 95 in 1207880

	qspi: qspi@ff8d2000 {
	#address-cells = <0x1>;
	#size-cells = <0x0>;
	compatible = "cdns,qspi-nor";
	reg = <0xff8d2000 0x100>,
	<0xff900000 0x100000>;
	reg-names = "qspi_reg", "qspi_data";
	interrupt-parent = <&gic>;
	interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL
	IRQ_DEFAULT_PRIORITY>;
	clock-frequency = <50000000>;
	status = "disabled";
	};

zephyr/boards/intel/socfpga/agilex_socdk/intel_socfpga_agilex_socdk.dts

Lines 31 to 39 in 1207880

	&qspi {
	status = "okay";
	mt25qu02g@0 {
	compatible = "micron,mt25qu02g";
	reg = <0>;
	size = <DT_SIZE_M(256*8)>; /* in bits */
	status = "okay";
	};
	};

The N25Q option in the driver is basically always enabled since it's enabled by default and there is also no occurence where it gets disabled (confirmed via grep). It also was added in the same commit c3dd728. The name difference between N25Q (Kconfig) and mt25qu02g (devicetree) is a bit odd but they might just share the same important registers for the code.

For the code itself I would also prefer no redundant drivers but it's probably necessary until we can confirm we have a driver that works with the already existing Agilex SoC FPGA (especially the micron part is interesting here; however if it doesn't have any odd features it might be enough to just use the generic MSPI flash driver with the right devicetree options) so we don't break anything.

Going further I think the benefits of continuing this driver (I already test it locally with the AM2434 and it's a separate MSPI and flash driver) outweights the disadvantages (no XIP and timing configuration support) so my plan would be to rename the names so it reflects that it's for a cadence IP core and also incorporate the things from #88487 (comment). Maybe XIP and timing configuration can be taken from the existing driver later. I will probably continue with this PR once #87321 is merged.

[natto1784]

I also tested your MSPI driver with a draft S28H driver and it works perfectly, so I would also prefer to keep that around. In the process I have changed the S25H driver into a generic SEMPER driver that also supports octal configuration (for s28h). This is done by reading the SFDP. Do you mind if I open a PR in your fork to allow more discussion on this?

[m-braunschweig]

It would probably be enough for me, if you push it somewhere and send me a link so I can git cherry-pick it. However we should decide whether we want to add it to this PR as replacement for the S25H driver. I would do that after looking at the driver.

I will probably also push new changes to my branch soon when I have my west flash minimal proof-of-concept finished. The MSPI/S25H driver didn't get notable changes except the missing flash_mspi_infineon_s25h_prepare_mspi_bus you pointed out and a workaround k_sleep because I stumbled over some odd behaviour while writing my west flash support (which also includes a minimal XMODEM1k sample receiver that flashes incoming data)

[natto1784]

Owner for the controller is @m-braunschweig but we have decided that I will cherry-pick their changes onto my other PR, since it is a dependency. Maybe we should draft this one in the meantime.

[m-braunschweig]

@natto1784 I updated the PR to address most change requests related to the MSPI driver. The old version is now on the branch mika/ti/ti-am2434-native-boot-pre-refactor. Due to the definition changes requested that I should have addressed way sooner it will sadly require some work in case you want to use it as base. My flash driver wasn't changed since your S28 one should be merged first. I tested the changes on the AM243x Launchpad by putting it into UART boot, sending a firmware that uses the flash driver by flashing received UART data to the flash and then flashing a blinky sample.

The changes I made include:

• Rebase on current main branch
• Use FIELD_GET / FIELD_PREP / GENMASK / BIT instead of MSPI_CADENCE_REG_WRITE (link). I'm open for change suggestions here since the line-breaks and forced newline after variable definitions make it IMO still quite unpleasant to read
• Move MPU configuration out of driver commit and into dts: ti: k3: add mspi0 bus to am64x soc (link), stop using DT_NODELABEL for referencing (link) and change region configuration to 128 MiB as strongly-ordered (link)
• Add access control via Mutex and mspi_dev_config / mspi_get_channel_status (link)
• Update binding to address name (link) and specifying the unit (though the datasheet only mentions "SRAM locations") (link)

Changes I didn't address:

• Use nanoseconds instead of the amount of cycles for specifying the delays (link). The reason for this is that currently no clock nodes exist in the devicetree that can be used to get the reference clock and therefor it would be necessary to provide it in a hardcoded way in the devicetree which would somewhat defeat the purpose.
• The comment related to read_delay (link). Since this if for DDR and not SDR it wasn't part of my code.
• Move the opcode configuration into an own function (link). I think it would be better to do it when DAC is actually added

Please tell me, if I should also look into converting your changes to the FIELD_GET etc. approach too

[natto1784]

Hi Mika, thank you for your work, it is well appreciated.

@natto1784 I updated the PR to address most change requests related to the MSPI driver. The old version is now on the branch mika/ti/ti-am2434-native-boot-pre-refactor. Due to the definition changes requested that I should have addressed way sooner it will sadly require some work in case you want to use it as base.

That is quite alright, no problem.

My flash driver wasn't changed since your S28 one should be merged first. I tested the changes on the AM243x Launchpad by putting it into UART boot, sending a firmware that uses the flash driver by flashing received UART data to the flash and then flashing a blinky sample.

Okay that is good to know

The changes I made include:

* Rebase on current `main` branch

* Use `FIELD_GET` / `FIELD_PREP` / `GENMASK` / `BIT` instead of `MSPI_CADENCE_REG_WRITE` ([link](https://github.com/zephyrproject-rtos/zephyr/pull/88487#discussion_r2041315707)). I'm open for change suggestions here since the line-breaks and forced newline after variable definitions make it IMO still quite unpleasant to read

* Move MPU configuration out of driver commit and into [dts: ti: k3: add mspi0 bus to am64x soc](https://github.com/siemens/zephyr/commit/90368cb7dce34a24962626186cf16c3a5e1a543a) ([link](https://github.com/zephyrproject-rtos/zephyr/pull/88487#discussion_r2731426299)), stop using `DT_NODELABEL` for referencing ([link](https://github.com/zephyrproject-rtos/zephyr/pull/88487#discussion_r2732118862)) and change region configuration to 128 MiB as strongly-ordered ([link](https://github.com/zephyrproject-rtos/zephyr/pull/102513#discussion_r2715933049))

* Add access control via Mutex and `mspi_dev_config` / `mspi_get_channel_status` ([link](https://github.com/zephyrproject-rtos/zephyr/pull/102513#issuecomment-3771939691))

Good addition, thanks.

* Update binding to address name ([link](https://github.com/zephyrproject-rtos/zephyr/pull/88487#discussion_r2716540356)) and specifying the unit (though the datasheet only mentions "SRAM locations") ([link](https://github.com/zephyrproject-rtos/zephyr/pull/88487#discussion_r2716541860))

Changes I didn't address:

* Use nanoseconds instead of the amount of cycles for specifying the delays ([link](https://github.com/zephyrproject-rtos/zephyr/pull/102513#discussion_r2720513614)). 

The reason for this is that currently no clock nodes exist in the devicetree that can be used to get the reference clock and therefor it would be necessary to provide it in a hardcoded way in the devicetree which would somewhat defeat the purpose.

We have TISCI now that most of the K3 devices use for clock and power management. However, we do not have any driver using it explicitly for the purpose. I will look into adding some conditional macro to get the clock rate but that is something that can be done after the drivers are merged.

* The comment related to read_delay ([link](https://github.com/zephyrproject-rtos/zephyr/pull/102513#discussion_r2720710411)). Since this if for DDR and not SDR it wasn't part of my code.

* Move the opcode configuration into an own function ([link](https://github.com/zephyrproject-rtos/zephyr/pull/102513#discussion_r2721046502)). I think it would be better to do it when DAC is actually added

Please tell me, if I should also look into converting your changes to the FIELD_GET etc. approach too

That is alright, I will cherry-pick the new patch and drop the old one

[natto1784]

Can you add static keyword in front of these functions

  grep "^int " mspi_cadence.c
int mspi_cadence_wait_for_idle(const struct device *controller)
int mspi_cadence_dev_config(const struct device *controller, const struct mspi_dev_id *dev_id,
int mspi_cadence_get_channel_status(const struct device *controller, uint8_t channel)
int mspi_cadence_timing(const struct device *controller, const struct mspi_dev_id *dev_id,

[m-braunschweig]

Changes I didn't address:

* Use nanoseconds instead of the amount of cycles for specifying the delays ([link](https://github.com/zephyrproject-rtos/zephyr/pull/102513#discussion_r2720513614)). 

The reason for this is that currently no clock nodes exist in the devicetree that can be used to get the reference clock and therefor it would be necessary to provide it in a hardcoded way in the devicetree which would somewhat defeat the purpose.

We have TISCI now that most of the K3 devices use for clock and power management. However, we do not have any driver using it explicitly for the purpose. I will look into adding some conditional macro to get the clock rate but that is something that can be done after the drivers are merged.

Ok. I also had a quick look and the main problems were the missing clock devicetree nodes and that we can't get the clock frequency in the init function due to the driver using a semaphore and the driver initialization happening before the kernel. But it seems these things are already fixed in your draft PR #101280

[m-braunschweig]

Can you add static keyword in front of these functions

  grep "^int " mspi_cadence.c
int mspi_cadence_wait_for_idle(const struct device *controller)
int mspi_cadence_dev_config(const struct device *controller, const struct mspi_dev_id *dev_id,
int mspi_cadence_get_channel_status(const struct device *controller, uint8_t channel)
int mspi_cadence_timing(const struct device *controller, const struct mspi_dev_id *dev_id,

Done

[natto1784]

hi can you also get rid of this line as suggested here: #102513 (comment)

[m-braunschweig]

Done. However you don't need to do another rebasing on my changes since you added your version for the generic MSPI flash driver in 767dbb2

[natto1784]

oh right my bad, i forgot that it's a different driver

[natto1784]

im not sure if returning -ENOSYS here is a good choice since it causes problems with flash drivers (such as flash_mspi_nor to fail prematurely. Just an error/warning should suffice to inform that there are parameters in the request which we are going to ignore.

[m-braunschweig]

I think returning no error value would be not acceptable since it would mean potentially silent failures (especially if no logging is available/enabled). Since the MSPI NOR driver sets nearly all flags for non-XIP usage (see [1]) it seems like it will be necessary to check more fields for undesired values (mem_boundary and time_to_break).

However the other problem is that it tries to set the frequency. I think the best solution for now would be to provide the reference clock frequency via the devicetree and then set MSTR_BAUD_DIV_FLD in the OSPI_CONFIG_REG register to achieve the frequency (or the highest possible lower than the desired frequency).

Additionally after looking at one of the Ambiq MSPI drivers (see [2]) I will change the "completly ignored parameters" to be based off a negation of what is implemented too (so if a new value is introduced it will fail due to not implementing that one). This will however lead to problems once the MSPI API is expanded but IMO it's favourable over completely silent failures.

I will implement and test these changes in the next few days.

[1] mspi_dev_config mask in MSPI NOR driver

zephyr/drivers/flash/flash_mspi_nor.c

Lines 19 to 26 in b246d3f

	#define XIP_DEV_CFG_MASK (MSPI_DEVICE_CONFIG_CMD_LEN | \
	MSPI_DEVICE_CONFIG_ADDR_LEN | \
	MSPI_DEVICE_CONFIG_READ_CMD | \
	MSPI_DEVICE_CONFIG_WRITE_CMD | \
	MSPI_DEVICE_CONFIG_RX_DUMMY | \
	MSPI_DEVICE_CONFIG_TX_DUMMY)
	#define NON_XIP_DEV_CFG_MASK (MSPI_DEVICE_CONFIG_ALL & ~XIP_DEV_CFG_MASK)

[2] bitmask that checks for unknown/unimplemented values in Ambiq driver

zephyr/drivers/mspi/mspi_ambiq_ap3.c

Lines 665 to 675 in b246d3f

	} else if (param_mask != MSPI_DEVICE_CONFIG_ALL) {
	if ((param_mask & (~(MSPI_DEVICE_CONFIG_FREQUENCY |
	MSPI_DEVICE_CONFIG_IO_MODE |
	MSPI_DEVICE_CONFIG_CE_NUM |
	MSPI_DEVICE_CONFIG_DATA_RATE |
	MSPI_DEVICE_CONFIG_CMD_LEN |
	MSPI_DEVICE_CONFIG_ADDR_LEN)))) {
	LOG_INST_ERR(cfg->log, "%u, config type not supported.", __LINE__);
	ret = -EINVAL;
	goto e_return;
	}

[natto1784]

However the other problem is that it tries to set the frequency. I think the best solution for now would be to provide the reference clock frequency via the devicetree and then set MSTR_BAUD_DIV_FLD in the OSPI_CONFIG_REG register to achieve the frequency (or the highest possible lower than the desired frequency).

I can do this if you are not already working.

[m-braunschweig]

I implemented the changes already (and noticed that we check for mem_boundary and time_to_break already). Before pushing I wanted to measure the frequency with a logic analyzer to be sure it works properly but I will only be able to do it on Monday (I however already tested the calculation manually and flashing with 50 MHz + 10 MHz). If you want I can already push these changes.

Also according to the AM64x TRM the OSPI clock is configured by the ROM bootloader to be either 200 MHz or 160 MHz. In UART and QSPI bootmode it is configured to be 200 MHz on the AM243x Launchpad (checked via tisci_cmd_clk_get_freq(dmsc, 75, 6, &frequency);). Do you happen to know under which circumstance it can be 160 MHz?

[natto1784]

Yes, OSPI only has two reference clock selections MAIN_PLL0_HSDIV5 (1000MHz / 5 = 200MHz) and PER0_PLL0_HSDIV4 (960MHz / 6 = 160MHz). The clock selection is done via the relevant CLKSEL register in the MMR region - this is done by the DM via TISCI calls. Personally during the testing I have done I manually set the frequency via TISCI calls to 200MHz which selects the former parent.

Now clock controlling is a separate and bigger topic, currently there are some bugfixes [1] pending post which I am going to add a small polling API for secproxy [2]. However I am going to remove the initialization part (power domains and clock frequencies) from the [2] and move it to the individual drivers instead i.e, the drivers will set the frequency (better yet, parent source) during init(). Now the problem with that is that is vendor specific clock configuration for TI - which is solved by a new proposed way of clock management [3]. Hence, it is hard to make decisions on which direction to move in.

P.S. I have personally never seen anyone using the 160MHz source.

[1] #100642
[2] #101280
[3] #72102

[m-braunschweig]

Just pushed it. I will verify that the frequency is actually what it should be on Monday.

The clock mainly concerns me since there might be cases where the ROM bootloader could choose the 160 MHz as reference clock instead of the 200 MHz meaning the calculation will be wrong (though a lower frequency shouldn't be a problem in most cases). Due to the current state of clocks that you mentioned the specification as constant in the devicetree is also mainly a workaround and should ideally be improved later.

[m-braunschweig]

I verified the clock frequencies with the following values + an oscilloscope and they all were the one expected

Request	Equal or highest possible lower frequency
100 MHz	100 MHz
50.1 MHz	50 MHz
50.0 MHz	50 MHz
49.9 MHz	33.3333 MHz
7 MHz	6.6667 MHz
6.25 MHz	6.25 MHz
6 MHz	not possible

[natto1784]

I see, that is great, thanks a lot for the effort.

[natto1784]

also can you take a look at this patch if you get the time

d02076b

if it's okay you can drop the changes that im deleting in the above patch

I introduce the timing_config API back in the next patch

770153f

[m-braunschweig]

also can you take a look at this patch if you get the time

d02076b

if it's okay you can drop the changes that im deleting in the above patch

I introduce the timing_config API back in the next patch

770153f

I will remove the timing API and DT bindings but keep resetting the timing configuration during initialization to a default value

[sonarqubecloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

outdated