ITE: drivers/spi: Add SPI Master driver module of ITE IT8XXX2 #53812

bjchentc · 2023-01-16T06:44:57Z

Added SPI driver supporting SPI master channels in command queue mode

Signed-off-by: BJ Chen bj.chen@ite.com.tw

galak · 2023-01-18T17:43:12Z

dts/bindings/spi/ite,it8xxx2-spi.yaml

+      0  SSPI_MODE_0  Bit6 and Bit 5 are 00b
+      1  SSPI_MODE_1  Bit6 and Bit 5 are 01b
+      2  SSPI_MODE_2  Bit6 and Bit 5 are 10b
+      3  SSPI_MODE_3  Bit6 and Bit 5 are 11b


what do these modes actually mean?

Hi galak,

These constants represent the different clock phases which is defined as different modes and can be selected in the SPICTRL1 register.

Bit 6 of the SPICTRL1 represents the Clock Polarity (CLPOL)
0: SSCK is low in the idle mode
1: SSCK is high in the idle mode

Bit 5 of the SPICTRL1 represents the Clock Phase (CLPHS)
0: Latch data on the first SSCK edge
1: Latch data on the second SSCK edge

Therefore, it behaves as follows:

Mode 0:
SSCK is low in the idle mode. Data is sampled on the rising edge, and shifted on the falling edge.

Mode 1:
SSCK is low in the idle mode. Data is sampled on the falling edge, and shifted on the rising edge.

Mode 2:
SSCK is high in the idle mode. Data is sampled on the falling edge, and shifted on the rising edge.

Mode 3:
SSCK is high in the idle mode. Data is sampled on the rising edge, and shifted on the falling edge.

Thanks for feedbacks & Best regards,
BJ

It would be better to document the SPI mode in terms of the CPOL (clock polarity) and CPHA (clock phase) parameters.
https://en.wikipedia.org/wiki/Serial_Peripheral_Interface#Clock_polarity_and_phase

Some other drivers provide spi-cpol and spi-cpha properties.

Dear Keith,

Thank you sir, I totally agree with that and I'll change the terms when I add the comments.
Secondly, are the spi-cpol and spi-cpha properties recommended methods of implementing this part?

Thanks & Warmest regards,
BJ

Yes, please use spi-cpol and spi-cpha as your properties. Your driver will process the properties to set the correct SSPI_MODE setting required by the ITE SPI controller.

Thanks for guidance, sir. I've modified the SPI mode to be spi-cpol and spi-cpha properties in this update.

keith-zephyr · 2023-01-20T22:53:06Z

dts/bindings/spi/ite,it8xxx2-spi.yaml

+      0  SSPI_MODE_0  Bit6 and Bit 5 are 00b
+      1  SSPI_MODE_1  Bit6 and Bit 5 are 01b
+      2  SSPI_MODE_2  Bit6 and Bit 5 are 10b
+      3  SSPI_MODE_3  Bit6 and Bit 5 are 11b


It would be better to document the SPI mode in terms of the CPOL (clock polarity) and CPHA (clock phase) parameters.
https://en.wikipedia.org/wiki/Serial_Peripheral_Interface#Clock_polarity_and_phase

Some other drivers provide spi-cpol and spi-cpha properties.

keith-zephyr · 2023-01-20T23:09:04Z

drivers/spi/spi_ite_it8xxx2.c

+		it8xxx2_spi_cmdq_init(dev);
+		it8xxx2_spi_configure_sckfreq(dev);
+		it8xxx2_spi_configure_spimode(dev);
+		it8xxx2_spi_int_init(dev);


I think you need to call these functions for every SPI instance used, correct?

Dear Keith,

Well, theoretically, yes. However, the SPI CE0 & CE1 shares these configurations because they share the same clock. Some configurations for both channels need to be added then. I'll update it in the next revision.

Thanks again for guidance, sir.

Best regards,
BJ

This has been modified to be invoked for each instance but so far the IT8XXX2 would not have the spi1.

keith-zephyr · 2023-01-20T23:13:19Z

drivers/spi/spi_ite_it8xxx2.c

+static void spi_it8xxx2_isr(const struct device *dev)
+{
+	const struct spi_it8xxx2_config *cfg = dev->config;
+	struct spi_it8xxx2_regs *const spi_regs =
+		(struct spi_it8xxx2_regs *)cfg->base_addr;
+
+	irq_disable(cfg->irq_no);
+
+	spi_regs->INTSTS |= SPICMDQEND;
+	ite_intc_isr_clear(cfg->irq_no);
+	spi_regs->SPICTRL5 &= ~(CH1SELCMDQ | CH0SELCMDQ);
+	k_sem_give(&it8xxx2_sem);
+	chip_permit_idle();
+}


This ISR doesn't appear to support configurations where both SPI controllers are enabled. The 2nd SPI controller initialized will never generate or process any interrupts.

You likely need shared interrupt support as proposed in #49427.

The alternative here would be to use the DT_INST_FOREACH_STATUS_OKAY() and DEVICE_DT_INST_GET() to lookup the the struct device for all SPI controllers, and check if there is pending interrupt on each controller. I think you would also need an semaphore for each SPI controller instance instead of shared global one.

Dear Keith,

Sorry for delayed response coz we were in a long vacation of Lunar New Year.
Yes, I think that you're absolutely right, the interrupt connection only works for one instance.

For the instance 0 and 1 share the same interrupt number, I'm wondering if it's still possible to tell if there is pending interrupt on each controller.

I think that the #49427 which you mentioned matches the requirement we'd need here. In another way, we can utilize some registers to tell if it's instance 0 or 1 but when it comes to the irq connection in Zephyr kernel, the *dev and related structure seem delivered among layers will still stick to the instance 0. I'll back to this after some internal discussions.

Truly appreciative of your guidance & helps.

Warmest regards,
BJ

See this example for how to handle a shared interrupt by two driver instances:

To adapt this to the ITE SPI driver, you would read the INTSTS register to determine if an interrupt is pending:

spi_it8xxx2_instance_isr(dev) { /* Clear interrupts, give semaphore, etc */ } #define HANDLE_SHARED_IRQ(n, active_irq) static const struct device *const dev_##n = DEVICE_DT_INST_GET(n); const struct spi_it8xxx2_regs *spi_regs_##n = dev_##n->config->base_addr;; if (spi_regs_##->INTSTS) { spi_it8xxx2_instance_isr(dev_##); } void spi_it8xxx2_shared_isr(const struct device *dev) { ARG_UNUSED(dev); DT_INST_FOREACH_STATUS_OKAY(HANDLE_SHARED_IRQ); }

Thank you, sir. We also found that our chip can only support spi0 according to the current pin numbers, only specific chip with more pins can support second instance but so far we don't have it. So the second instance has been removed in dtsi files. Though we keep the init style like multiple instance but it's just for the scalability in the future.

fkokosinski · 2023-01-24T09:15:51Z

drivers/spi/CMakeLists.txt

 zephyr_library_sources_ifdef(CONFIG_SPI_ASYNC spi_signal.c)
+zephyr_library_sources_ifdef(CONFIG_SPI_ITE_IT8XXX2	spi_ite_it8xxx2.c)


Nitpick: This is a wrong place to add this line. IMO it should be added after line 36, and the blank line should separate this group from the more generic files (async, userspace etc.)

Dear Filip,

Thanks for reminding, yes you're right. I'll correct this in the next upload.
Thanks again for helps.

Warmest regards,
BJ

github-actions · 2023-04-05T00:29:28Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

bjchentc · 2023-06-13T02:58:39Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Is it still possible to remove the label now? I'd really like to remove the Stale label and reopen this PR but I cannot see the reopen button below the comment field.

henrikbrixandersen · 2023-06-13T07:20:49Z

@itebj This PR cannot be reopened since the branch was force-pushed after the PR was closed. If you can push the last SHA as seen in this PR, I might be able to reopen it. Otherwise you will need to create a new PR and reference this one.

bjchentc · 2023-06-13T07:33:40Z

@henrikbrixandersen
Thank you so much for response. Then I think that I'd create a new one.
Truly grateful!

bjchentc · 2023-06-13T08:30:14Z

@henrikbrixandersen Thank you so much for reopening it, sir. I'll update it ASAP.

Added SPI driver supporting SPI master channels in command queue mode Signed-off-by: BJ Chen <bj.chen@ite.com.tw>

bjchentc · 2023-06-14T08:37:53Z

Dear sirs,

This update removes the spi1 and replace spi-mode property with spi-cpol and spi-cpha. In the meanwhile, we also remove the ch1 related stuffs for the command queue mode usage. The initialization style remains for the future scalability. but this update still need some modifications, I'll update it ASAP.

If there's any suggestion/advice, please kindly let me know about it.
Thanks for your revisiting and time sharing.

Best regards,
BJ

keith-zephyr · 2023-07-11T19:22:36Z

drivers/spi/spi_ite_it8xxx2.c

+	IRQ_CONNECT(DT_INST_IRQN(0), DT_INST_IRQ(0, priority),
+	spi_it8xxx2_isr, DEVICE_DT_INST_GET(0), 0);


Your code assumes SPI instance 0. You can use this instead.

Suggested change

IRQ_CONNECT(DT_INST_IRQN(0), DT_INST_IRQ(0, priority),

spi_it8xxx2_isr, DEVICE_DT_INST_GET(0), 0);

IRQ_CONNECT(cfg->irq_no, cfg->irq_priority, spi_it8xxx2_isr, dev, 0);

Got it, sir. Actually I think that we'd add the multiple-instance declaration back, but for supporting ASYNC option, the arguments spi_callback_t cb & void *userdata should be listed in the _transceive_async api, so I'm now trying to figure out how to replace original semaphore declaration and usage with helper functions in spi_context.h. So far this case is kinda pending temporarily. Thanks for great patience and guidnace, sir.

bjchentc force-pushed the z-spi branch 3 times, most recently from 137e0fb to 185ec9f Compare January 17, 2023 02:58

bjchentc marked this pull request as ready for review January 17, 2023 03:32

bjchentc requested review from nashif, carlescufi, galak, MaureenHelm, tbursztyka, kgugala and pgielda as code owners January 17, 2023 03:32

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: SPI SPI bus platform: ITE ITE area: RISCV RISCV Architecture (32-bit & 64-bit) labels Jan 17, 2023

zephyrbot requested review from brockus-zephyr, Dino-Li, fkokosinski, GTLin08, jackrosenthal, keith-zephyr, sjg20 and tgorochowik January 17, 2023 03:33

zephyrbot assigned Dino-Li Jan 17, 2023

bjchentc force-pushed the z-spi branch 2 times, most recently from ae22721 to 3918c0b Compare January 17, 2023 06:17

galak reviewed Jan 18, 2023

View reviewed changes

keith-zephyr requested changes Jan 20, 2023

View reviewed changes

fkokosinski reviewed Jan 24, 2023

View reviewed changes

github-actions bot added the Stale label Apr 5, 2023

github-actions bot closed this Apr 19, 2023

bjchentc deleted the z-spi branch June 13, 2023 07:39

bjchentc restored the z-spi branch June 13, 2023 07:39

henrikbrixandersen reopened this Jun 13, 2023

bjchentc force-pushed the z-spi branch 3 times, most recently from 055678b to e41f876 Compare June 13, 2023 10:30

github-actions bot removed the Stale label Jun 14, 2023

ITE: drivers/spi: Add SPI Master driver module of ITE IT8xxx2

72b3d12

Added SPI driver supporting SPI master channels in command queue mode Signed-off-by: BJ Chen <bj.chen@ite.com.tw>

bjchentc force-pushed the z-spi branch from e41f876 to 72b3d12 Compare June 14, 2023 07:22

keith-zephyr requested changes Jul 11, 2023

View reviewed changes

bjchentc closed this Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ITE: drivers/spi: Add SPI Master driver module of ITE IT8XXX2 #53812

ITE: drivers/spi: Add SPI Master driver module of ITE IT8XXX2 #53812

bjchentc commented Jan 16, 2023

galak Jan 18, 2023

bjchentc Jan 19, 2023 •

edited

keith-zephyr Jan 20, 2023

bjchentc Feb 2, 2023 •

edited

keith-zephyr Feb 2, 2023

bjchentc Jun 13, 2023

keith-zephyr Jan 20, 2023

keith-zephyr Jan 20, 2023

bjchentc Feb 2, 2023 •

edited

bjchentc Jun 14, 2023

keith-zephyr Jan 20, 2023

bjchentc Feb 2, 2023 •

edited

keith-zephyr Feb 2, 2023

bjchentc Jun 13, 2023 •

edited

fkokosinski Jan 24, 2023

bjchentc Feb 2, 2023

github-actions bot commented Apr 5, 2023

bjchentc commented Jun 13, 2023

henrikbrixandersen commented Jun 13, 2023

bjchentc commented Jun 13, 2023

bjchentc commented Jun 13, 2023

bjchentc commented Jun 14, 2023

keith-zephyr Jul 11, 2023

bjchentc Jul 24, 2023

		zephyr_library_sources_ifdef(CONFIG_SPI_ASYNC spi_signal.c)
		zephyr_library_sources_ifdef(CONFIG_SPI_ITE_IT8XXX2 spi_ite_it8xxx2.c)

		IRQ_CONNECT(DT_INST_IRQN(0), DT_INST_IRQ(0, priority),
		spi_it8xxx2_isr, DEVICE_DT_INST_GET(0), 0);

	IRQ_CONNECT(DT_INST_IRQN(0), DT_INST_IRQ(0, priority),
	spi_it8xxx2_isr, DEVICE_DT_INST_GET(0), 0);
	IRQ_CONNECT(cfg->irq_no, cfg->irq_priority, spi_it8xxx2_isr, dev, 0);

ITE: drivers/spi: Add SPI Master driver module of ITE IT8XXX2 #53812

ITE: drivers/spi: Add SPI Master driver module of ITE IT8XXX2 #53812

Conversation

bjchentc commented Jan 16, 2023

Choose a reason for hiding this comment

bjchentc Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjchentc Feb 2, 2023 • edited

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

bjchentc Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjchentc Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjchentc Jun 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2023

bjchentc commented Jun 13, 2023

henrikbrixandersen commented Jun 13, 2023

bjchentc commented Jun 13, 2023

bjchentc commented Jun 13, 2023

bjchentc commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjchentc Jan 19, 2023 •

edited

bjchentc Feb 2, 2023 •

edited

bjchentc Feb 2, 2023 •

edited

bjchentc Feb 2, 2023 •

edited

bjchentc Jun 13, 2023 •

edited