This patch series include the accumulative updates and fixes for the
driver from Broadcom. It also added a new driver for the updated SPI
controller found in the new BCMBCA SoC. The device tree document is
converted to yaml format and updated accordingly.
William Zhang (16):
dt-bindings: spi: Convert bcm63xx-hsspi bindings to json-schema
dt-bindings: spi: Add bcmbca-hsspi controller support
dt-bindings: spi: Add spi peripheral specific property
ARM: dts: broadcom: bcmbca: Add spi controller node
arm64: dts: broadcom: bcmbca: Add spi controller node
spi: bcm63xx-hsspi: Endianness fix for ARM based SoC
spi: bcm63xx-hsspi: Add polling mode support
spi: bcm63xx-hsspi: Handle cs_change correctly
spi: bcm63xx-hsspi: Fix multi-bit mode setting
spi: bcm63xx-hsspi: Make dummy cs workaround as an option
spi: bcm63xx-hsspi: Add prepend feature support
spi: bcm63xx-hsspi: Add clock gate disable option support
spi: spi-mem: Allow controller supporting mem_ops without exec_op
spi: bcm63xx-hsspi: prepend: Disable spi mem dual io read op support
spi: bcmbca-hsspi: Add driver for newer HSSPI controller
MAINTAINERS: Add entry for Broadcom Broadband SoC HS SPI drivers
.../brcm,bcm63xx-hsspi-peripheral-props.yaml | 27 +
.../bindings/spi/brcm,bcm63xx-hsspi.yaml | 124 ++++
.../bindings/spi/spi-bcm63xx-hsspi.txt | 33 -
.../bindings/spi/spi-peripheral-props.yaml | 1 +
MAINTAINERS | 12 +
arch/arm/boot/dts/bcm47622.dtsi | 17 +
arch/arm/boot/dts/bcm63138.dtsi | 17 +
arch/arm/boot/dts/bcm63148.dtsi | 17 +
arch/arm/boot/dts/bcm63178.dtsi | 18 +
arch/arm/boot/dts/bcm6756.dtsi | 18 +
arch/arm/boot/dts/bcm6846.dtsi | 17 +
arch/arm/boot/dts/bcm6855.dtsi | 18 +
arch/arm/boot/dts/bcm6878.dtsi | 18 +
arch/arm/boot/dts/bcm947622.dts | 4 +
arch/arm/boot/dts/bcm963138.dts | 4 +
arch/arm/boot/dts/bcm963138dvt.dts | 4 +
arch/arm/boot/dts/bcm963148.dts | 4 +
arch/arm/boot/dts/bcm963178.dts | 4 +
arch/arm/boot/dts/bcm96756.dts | 4 +
arch/arm/boot/dts/bcm96846.dts | 4 +
arch/arm/boot/dts/bcm96855.dts | 4 +
arch/arm/boot/dts/bcm96878.dts | 4 +
.../boot/dts/broadcom/bcmbca/bcm4908.dtsi | 17 +
.../boot/dts/broadcom/bcmbca/bcm4912.dtsi | 19 +
.../boot/dts/broadcom/bcmbca/bcm63146.dtsi | 18 +
.../boot/dts/broadcom/bcmbca/bcm63158.dtsi | 18 +
.../boot/dts/broadcom/bcmbca/bcm6813.dtsi | 19 +
.../boot/dts/broadcom/bcmbca/bcm6856.dtsi | 17 +
.../boot/dts/broadcom/bcmbca/bcm6858.dtsi | 17 +
.../boot/dts/broadcom/bcmbca/bcm94908.dts | 4 +
.../boot/dts/broadcom/bcmbca/bcm94912.dts | 4 +
.../boot/dts/broadcom/bcmbca/bcm963146.dts | 4 +
.../boot/dts/broadcom/bcmbca/bcm963158.dts | 4 +
.../boot/dts/broadcom/bcmbca/bcm96813.dts | 4 +
.../boot/dts/broadcom/bcmbca/bcm96856.dts | 4 +
.../boot/dts/broadcom/bcmbca/bcm96858.dts | 4 +
drivers/spi/Kconfig | 9 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-bcm63xx-hsspi.c | 391 ++++++++++-
drivers/spi/spi-bcmbca-hsspi.c | 629 ++++++++++++++++++
drivers/spi/spi-mem.c | 2 +-
drivers/spi/spi.c | 13 +-
42 files changed, 1498 insertions(+), 73 deletions(-)
create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml
create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt
create mode 100644 drivers/spi/spi-bcmbca-hsspi.c
--
2.37.3
Polling mode provides better throughput in general by avoiding the
interrupt overhead as the maximum data size one interrupt can handle is
only 512 bytes.
When interrupt is not defined in the HSSPI dts node, driver switches to
polling mode for controller SPI message processing. Also add driver
banner message when the driver is loaded successfully.
When test on a Broadcom BCM47622(ARM A7 dual core) reference board with
WINBOND W25N01GV SPI NAND chip at 100MHz SPI clock using the MTD speed
test suite, it shows about 15% improvement on the write and 30% on
the read:
** Interrupt mode **
mtd_speedtest: MTD device: 0 count: 16
mtd_speedtest: MTD device size 134217728, eraseblock size 131072, page
size 2048, count of eraseblocks 1024, pages per eraseblock 64, OOB size
64
mtd_test: scanning for bad eraseblocks
mtd_test: scanned 16 eraseblocks, 0 are bad
mtd_speedtest: testing eraseblock write speed
mtd_speedtest: eraseblock write speed is 3072 KiB/s
mtd_speedtest: testing eraseblock read speed
mtd_speedtest: eraseblock read speed is 6690 KiB/s
mtd_speedtest: testing page write speed
mtd_speedtest: page write speed is 3066 KiB/s
mtd_speedtest: testing page read speed
mtd_speedtest: page read speed is 6762 KiB/s
mtd_speedtest: testing 2 page write speed
mtd_speedtest: 2 page write speed is 3071 KiB/s
mtd_speedtest: testing 2 page read speed
mtd_speedtest: 2 page read speed is 6772 KiB/s
** Polling mode **
mtd_speedtest: MTD device: 0 count: 16
mtd_speedtest: MTD device size 134217728, eraseblock size 131072, page
size 2048, count of eraseblocks 1024, pages per eraseblock 64, OOB size
64
mtd_test: scanning for bad eraseblocks
mtd_test: scanned 16 eraseblocks, 0 are bad
mtd_speedtest: testing eraseblock write speed
mtd_speedtest: eraseblock write speed is 3542 KiB/s
mtd_speedtest: testing eraseblock read speed
mtd_speedtest: eraseblock read speed is 8825 KiB/s
mtd_speedtest: testing page write speed
mtd_speedtest: page write speed is 3563 KiB/s
mtd_speedtest: testing page read speed
mtd_speedtest: page read speed is 8787 KiB/s
mtd_speedtest: testing 2 page write speed
mtd_speedtest: 2 page write speed is 3572 KiB/s
mtd_speedtest: testing 2 page read speed
mtd_speedtest: 2 page read speed is 8806 KiB/s
Signed-off-by: William Zhang <[email protected]>
---
drivers/spi/spi-bcm63xx-hsspi.c | 49 +++++++++++++++++++++++++--------
1 file changed, 38 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
index 63682c8ff955..2b4cdf7e7002 100644
--- a/drivers/spi/spi-bcm63xx-hsspi.c
+++ b/drivers/spi/spi-bcm63xx-hsspi.c
@@ -57,6 +57,7 @@
#define PINGPONG_CMD_SS_SHIFT 12
#define HSSPI_PINGPONG_STATUS_REG(x) (0x84 + (x) * 0x40)
+#define HSSPI_PINGPONG_STATUS_SRC_BUSY BIT(1)
#define HSSPI_PROFILE_CLK_CTRL_REG(x) (0x100 + (x) * 0x20)
#define CLK_CTRL_FREQ_CTRL_MASK 0x0000ffff
@@ -96,6 +97,7 @@
#define HSSPI_SPI_MAX_CS 8
#define HSSPI_BUS_NUM 1 /* 0 is legacy SPI */
+#define HSSPI_POLL_STATUS_TIMEOUT_MS 100
struct bcm63xx_hsspi {
struct completion done;
@@ -109,6 +111,7 @@ struct bcm63xx_hsspi {
u32 speed_hz;
u8 cs_polarity;
+ int irq;
};
static void bcm63xx_hsspi_set_cs(struct bcm63xx_hsspi *bs, unsigned int cs,
@@ -163,6 +166,8 @@ static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
int step_size = HSSPI_BUFFER_LEN;
const u8 *tx = t->tx_buf;
u8 *rx = t->rx_buf;
+ u32 val;
+ unsigned long limit;
bcm63xx_hsspi_set_clk(bs, spi, t->speed_hz);
bcm63xx_hsspi_set_cs(bs, spi->chip_select, true);
@@ -197,8 +202,9 @@ static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
__raw_writew(cpu_to_be16(opcode | curr_step), bs->fifo);
/* enable interrupt */
- __raw_writel(HSSPI_PINGx_CMD_DONE(0),
- bs->regs + HSSPI_INT_MASK_REG);
+ if (bs->irq > 0)
+ __raw_writel(HSSPI_PINGx_CMD_DONE(0),
+ bs->regs + HSSPI_INT_MASK_REG);
/* start the transfer */
__raw_writel(!chip_select << PINGPONG_CMD_SS_SHIFT |
@@ -206,9 +212,21 @@ static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
PINGPONG_COMMAND_START_NOW,
bs->regs + HSSPI_PINGPONG_COMMAND_REG(0));
- if (wait_for_completion_timeout(&bs->done, HZ) == 0) {
- dev_err(&bs->pdev->dev, "transfer timed out!\n");
- return -ETIMEDOUT;
+ if (bs->irq > 0) {
+ if (wait_for_completion_timeout(&bs->done, HZ) == 0)
+ goto err_timeout;
+ } else {
+ /* polling mode checks for status busy bit */
+ limit = jiffies + msecs_to_jiffies(HSSPI_POLL_STATUS_TIMEOUT_MS);
+ while (!time_after(jiffies, limit)) {
+ val = __raw_readl(bs->regs + HSSPI_PINGPONG_STATUS_REG(0));
+ if (val & HSSPI_PINGPONG_STATUS_SRC_BUSY)
+ cpu_relax();
+ else
+ break;
+ }
+ if (val & HSSPI_PINGPONG_STATUS_SRC_BUSY)
+ goto err_timeout;
}
if (rx) {
@@ -220,6 +238,10 @@ static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
}
return 0;
+
+err_timeout:
+ dev_err(&bs->pdev->dev, "transfer timed out!\n");
+ return -ETIMEDOUT;
}
static int bcm63xx_hsspi_setup(struct spi_device *spi)
@@ -338,8 +360,8 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
u32 reg, rate, num_cs = HSSPI_SPI_MAX_CS;
struct reset_control *reset;
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
+ irq = platform_get_irq_optional(pdev, 0);
+ if (irq < 0 && irq != -ENXIO)
return irq;
regs = devm_platform_ioremap_resource(pdev, 0);
@@ -398,6 +420,7 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
bs->regs = regs;
bs->speed_hz = rate;
bs->fifo = (u8 __iomem *)(bs->regs + HSSPI_FIFO_REG(0));
+ bs->irq = irq;
mutex_init(&bs->bus_mutex);
init_completion(&bs->done);
@@ -434,11 +457,13 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
__raw_writel(reg | GLOBAL_CTRL_CLK_GATE_SSOFF,
bs->regs + HSSPI_GLOBAL_CTRL_REG);
- ret = devm_request_irq(dev, irq, bcm63xx_hsspi_interrupt, IRQF_SHARED,
- pdev->name, bs);
+ if (bs->irq > 0) {
+ ret = devm_request_irq(dev, irq, bcm63xx_hsspi_interrupt, IRQF_SHARED,
+ pdev->name, bs);
- if (ret)
- goto out_put_master;
+ if (ret)
+ goto out_put_master;
+ }
pm_runtime_enable(&pdev->dev);
@@ -447,6 +472,8 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
if (ret)
goto out_pm_disable;
+ dev_info(dev, "Broadcom 63XX High Speed SPI Controller driver");
+
return 0;
out_pm_disable:
--
2.37.3
HSSPI controller uses big endian for the opcode in the message to the
controller ping pong buffer. Use cpu_to_be16 to properly handle the
endianness for both big and little endian host.
Fixes: 142168eba9dc ("spi: bcm63xx-hsspi: add bcm63xx HSSPI driver")
Signed-off-by: Kursad Oney <[email protected]>
Signed-off-by: William Zhang <[email protected]>
---
drivers/spi/spi-bcm63xx-hsspi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
index b871fd810d80..63682c8ff955 100644
--- a/drivers/spi/spi-bcm63xx-hsspi.c
+++ b/drivers/spi/spi-bcm63xx-hsspi.c
@@ -194,7 +194,7 @@ static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
tx += curr_step;
}
- __raw_writew(opcode | curr_step, bs->fifo);
+ __raw_writew(cpu_to_be16(opcode | curr_step), bs->fifo);
/* enable interrupt */
__raw_writel(HSSPI_PINGx_CMD_DONE(0),
--
2.37.3
Multiple transfers within a SPI message may be combined into one
transfer to the controller using its prepend feature. A SPI message is
prependable only if the following are all true:
* One or more half duplex write transfer
* Optional full duplex read/write at the end
* No delay and cs_change between transfers
Most of the SPI device meets this requirements such as SPI NOR,
SPI NAND flash, Broadcom SPI voice card and etc. So this patch
makes use of the prepend feature as the default mode as it has no board
design requirement as the dummy cs workaround needs.
For any SPI device that does not meet the above requirement, dummy cs
mode can be used as long as the board design meets its cs pin usage
requirement or runs below 30MHz clock speed.
Signed-off-by: William Zhang <[email protected]>
---
drivers/spi/spi-bcm63xx-hsspi.c | 246 ++++++++++++++++++++++++++++----
1 file changed, 221 insertions(+), 25 deletions(-)
diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
index b5043251edec..58f2b495c13c 100644
--- a/drivers/spi/spi-bcm63xx-hsspi.c
+++ b/drivers/spi/spi-bcm63xx-hsspi.c
@@ -113,8 +113,208 @@ struct bcm63xx_hsspi {
u8 cs_polarity;
bool use_cs_workaround;
int irq;
+ u32 prepend_cnt;
+ u8 *prepend_buf;
};
+static void bcm63xx_hsspi_set_clk(struct bcm63xx_hsspi *bs,
+ struct spi_device *spi, int hz);
+
+static size_t bcm63xx_hsspi_max_message_size(struct spi_device *spi)
+{
+ return HSSPI_BUFFER_LEN - HSSPI_OPCODE_LEN;
+}
+
+static int bcm63xx_hsspi_wait_cmd(struct bcm63xx_hsspi *bs)
+{
+ unsigned long limit;
+ u32 reg = 0;
+ int rc = 0;
+
+ if (bs->irq > 0) {
+ if (wait_for_completion_timeout(&bs->done, HZ) == 0)
+ rc = 1;
+ } else {
+ /* polling mode checks for status busy bit */
+ limit = jiffies + msecs_to_jiffies(HSSPI_POLL_STATUS_TIMEOUT_MS);
+
+ while (!time_after(jiffies, limit)) {
+ reg = __raw_readl(bs->regs + HSSPI_PINGPONG_STATUS_REG(0));
+ if (reg & HSSPI_PINGPONG_STATUS_SRC_BUSY)
+ cpu_relax();
+ else
+ break;
+ }
+ if (reg & HSSPI_PINGPONG_STATUS_SRC_BUSY)
+ rc = 1;
+ }
+
+ if (rc)
+ dev_err(&bs->pdev->dev, "transfer timed out!\n");
+
+ return rc;
+}
+
+static bool bcm63xx_check_msg_prependable(struct spi_master *master,
+ struct spi_message *msg,
+ struct spi_transfer *t_prepend)
+{
+
+ struct bcm63xx_hsspi *bs = spi_master_get_devdata(master);
+ bool prepend = false, tx_only = false;
+ struct spi_transfer *t;
+
+ /* If cs dummy workaround used, no need to prepend message */
+ if (bs->use_cs_workaround)
+ goto check_done;
+
+ /*
+ * Multiple transfers within a message may be combined into one transfer
+ * to the controller using its prepend feature. A SPI message is prependable
+ * only if the following are all true:
+ * 1. One or more half duplex write transfer
+ * 2. Optional full duplex read/write at the end
+ * 3. No delay and cs_change between transfers
+ */
+ bs->prepend_cnt = 0;
+ list_for_each_entry(t, &msg->transfers, transfer_list) {
+ if ((spi_delay_to_ns(&t->delay, t) > 0) || t->cs_change) {
+ dev_warn(&bs->pdev->dev,
+ "Delay or cs change not supported in prepend mode!\n");
+ break;
+ }
+
+ tx_only = false;
+ if (t->tx_buf && !t->rx_buf) {
+ tx_only = true;
+ if (bs->prepend_cnt + t->len >
+ (HSSPI_BUFFER_LEN - HSSPI_OPCODE_LEN)) {
+ dev_warn(&bs->pdev->dev,
+ "exceed max buf len, abort prepending transfers!\n");
+ break;
+ }
+ memcpy(bs->prepend_buf + bs->prepend_cnt, t->tx_buf,
+ t->len);
+ bs->prepend_cnt += t->len;
+ } else {
+ if (!list_is_last(&t->transfer_list, &msg->transfers)) {
+ dev_warn(&bs->pdev->dev,
+ "rx/tx_rx transfer not supported when it is not last one!\n");
+ break;
+ }
+ }
+
+ if (list_is_last(&t->transfer_list, &msg->transfers)) {
+ memcpy(t_prepend, t, sizeof(struct spi_transfer));
+
+ if (tx_only) {
+ /*
+ * if the last one is also a tx only transfer, merge all
+ * them into one single tx transfer
+ */
+ t_prepend->len = bs->prepend_cnt;
+ t_prepend->tx_buf = bs->prepend_buf;
+ bs->prepend_cnt = 0;
+ } else {
+ /*
+ * if the last one is not a tx only transfer, all the previous
+ * transfers are sent through prepend bytes and make sure it does
+ * not exceed the max prepend len
+ */
+ if (bs->prepend_cnt > HSSPI_MAX_PREPEND_LEN) {
+ dev_warn(&bs->pdev->dev,
+ "exceed max prepend len, abort prepending transfers!\n");
+ break;
+ }
+ }
+ prepend = true;
+ }
+ }
+
+check_done:
+ if (!bs->use_cs_workaround && !prepend)
+ dev_warn(&bs->pdev->dev,
+ "Msg not prependable and cs war not used. Transfer may fail!\n");
+
+ return prepend;
+}
+
+static int bcm63xx_hsspi_do_prepend_txrx(struct spi_device *spi,
+ struct spi_transfer *t)
+{
+ struct bcm63xx_hsspi *bs = spi_master_get_devdata(spi->master);
+ unsigned int chip_select = spi->chip_select;
+ u16 opcode = 0;
+ const u8 *tx = t->tx_buf;
+ u8 *rx = t->rx_buf;
+ u32 reg = 0;
+
+ /*
+ * shouldn't happen as we set the max_message_size in the probe.
+ * but check it again in case some driver does not honor the max size
+ */
+ if (t->len + bs->prepend_cnt > (HSSPI_BUFFER_LEN - HSSPI_OPCODE_LEN)) {
+ dev_warn(&bs->pdev->dev,
+ "Prepend message large than fifo size len %d prepend %d\n",
+ t->len, bs->prepend_cnt);
+ return -EINVAL;
+ }
+
+ bcm63xx_hsspi_set_clk(bs, spi, t->speed_hz);
+
+ if (tx && rx)
+ opcode = HSSPI_OP_READ_WRITE;
+ else if (tx)
+ opcode = HSSPI_OP_WRITE;
+ else if (rx)
+ opcode = HSSPI_OP_READ;
+
+ if ((opcode == HSSPI_OP_READ && t->rx_nbits == SPI_NBITS_DUAL) ||
+ (opcode == HSSPI_OP_WRITE && t->tx_nbits == SPI_NBITS_DUAL)) {
+ opcode |= HSSPI_OP_MULTIBIT;
+
+ if (t->rx_nbits == SPI_NBITS_DUAL) {
+ reg |= 1 << MODE_CTRL_MULTIDATA_RD_SIZE_SHIFT;
+ reg |= bs->prepend_cnt << MODE_CTRL_MULTIDATA_RD_STRT_SHIFT;
+ }
+ if (t->tx_nbits == SPI_NBITS_DUAL) {
+ reg |= 1 << MODE_CTRL_MULTIDATA_WR_SIZE_SHIFT;
+ reg |= bs->prepend_cnt << MODE_CTRL_MULTIDATA_WR_STRT_SHIFT;
+ }
+ }
+
+ reg |= bs->prepend_cnt << MODE_CTRL_PREPENDBYTE_CNT_SHIFT;
+ __raw_writel(reg | 0xff,
+ bs->regs + HSSPI_PROFILE_MODE_CTRL_REG(chip_select));
+
+ reinit_completion(&bs->done);
+ if (bs->prepend_cnt)
+ memcpy_toio(bs->fifo + HSSPI_OPCODE_LEN, bs->prepend_buf,
+ bs->prepend_cnt);
+ if (tx)
+ memcpy_toio(bs->fifo + HSSPI_OPCODE_LEN + bs->prepend_cnt, tx,
+ t->len);
+
+ __raw_writew(cpu_to_be16(opcode | t->len), bs->fifo);
+ /* enable interrupt */
+ if (bs->irq > 0)
+ __raw_writel(HSSPI_PINGx_CMD_DONE(0), bs->regs + HSSPI_INT_MASK_REG);
+
+ /* start the transfer */
+ reg = chip_select << PINGPONG_CMD_SS_SHIFT |
+ chip_select << PINGPONG_CMD_PROFILE_SHIFT |
+ PINGPONG_COMMAND_START_NOW;
+ __raw_writel(reg, bs->regs + HSSPI_PINGPONG_COMMAND_REG(0));
+
+ if (bcm63xx_hsspi_wait_cmd(bs))
+ return -ETIMEDOUT;
+
+ if (rx)
+ memcpy_fromio(rx, bs->fifo, t->len);
+
+ return 0;
+}
+
static void bcm63xx_hsspi_set_cs(struct bcm63xx_hsspi *bs, unsigned int cs,
bool active)
{
@@ -168,7 +368,6 @@ static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
int step_size = HSSPI_BUFFER_LEN;
const u8 *tx = t->tx_buf;
u8 *rx = t->rx_buf;
- unsigned long limit;
u32 reg = 0;
bcm63xx_hsspi_set_clk(bs, spi, t->speed_hz);
@@ -220,23 +419,8 @@ static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
PINGPONG_COMMAND_START_NOW;
__raw_writel(reg, bs->regs + HSSPI_PINGPONG_COMMAND_REG(0));
- if (bs->irq > 0) {
- if (wait_for_completion_timeout(&bs->done, HZ) == 0)
- goto err_timeout;
- } else {
- /* polling mode checks for status busy bit */
- limit = jiffies + msecs_to_jiffies(HSSPI_POLL_STATUS_TIMEOUT_MS);
-
- while (!time_after(jiffies, limit)) {
- reg = __raw_readl(bs->regs + HSSPI_PINGPONG_STATUS_REG(0));
- if (reg & HSSPI_PINGPONG_STATUS_SRC_BUSY)
- cpu_relax();
- else
- break;
- }
- if (reg & HSSPI_PINGPONG_STATUS_SRC_BUSY)
- goto err_timeout;
- }
+ if (bcm63xx_hsspi_wait_cmd(bs))
+ return -ETIMEDOUT;
if (rx) {
memcpy_fromio(rx, bs->fifo, curr_step);
@@ -247,10 +431,6 @@ static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
}
return 0;
-
-err_timeout:
- dev_err(&bs->pdev->dev, "transfer timed out!\n");
- return -ETIMEDOUT;
}
static int bcm63xx_hsspi_setup(struct spi_device *spi)
@@ -300,8 +480,16 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
int dummy_cs;
u32 reg;
bool restore_polarity = true;
+ struct spi_transfer t_prepend;
+
+ if (bcm63xx_check_msg_prependable(master, msg, &t_prepend)) {
+ status = bcm63xx_hsspi_do_prepend_txrx(spi, &t_prepend);
+ msg->actual_length += (t_prepend.len + bs->prepend_cnt);
+ goto msg_done;
+ }
- /* This controller does not support keeping CS active during idle.
+ /*
+ * This controller does not support keeping CS active during idle.
* To work around this, we use the following ugly hack:
*
* a. Invert the target chip select's polarity so it will be active.
@@ -355,6 +543,7 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
bcm63xx_hsspi_set_cs(bs, spi->chip_select, false);
}
+msg_done:
msg->status = status;
spi_finalize_current_message(master);
@@ -448,6 +637,11 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
bs->speed_hz = rate;
bs->fifo = (u8 __iomem *)(bs->regs + HSSPI_FIFO_REG(0));
bs->irq = irq;
+ bs->prepend_buf = devm_kzalloc(dev, HSSPI_BUFFER_LEN, GFP_KERNEL);
+ if (!bs->prepend_buf) {
+ ret = -ENOMEM;
+ goto out_put_master;
+ }
mutex_init(&bs->bus_mutex);
init_completion(&bs->done);
@@ -462,8 +656,6 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
bs->use_cs_workaround = of_property_read_bool(
dev->of_node, "brcm,use-cs-workaround");
}
- /* tmp hack. hard code to use cs workaround before prepend mode is added */
- bs->use_cs_workaround = true;
of_property_read_u32(dev->of_node, "num-cs", &num_cs);
if (num_cs > 8) {
@@ -474,6 +666,10 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
master->num_chipselect = num_cs;
master->setup = bcm63xx_hsspi_setup;
master->transfer_one_message = bcm63xx_hsspi_transfer_one;
+ if (!bs->use_cs_workaround) {
+ master->max_transfer_size = bcm63xx_hsspi_max_message_size;
+ master->max_message_size = bcm63xx_hsspi_max_message_size;
+ }
master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH |
SPI_RX_DUAL | SPI_TX_DUAL;
master->bits_per_word_mask = SPI_BPW_MASK(8);
--
2.37.3
Add support for HSSPI controller in ARMv8 chip dts files.
Signed-off-by: William Zhang <[email protected]>
---
.../boot/dts/broadcom/bcmbca/bcm4908.dtsi | 17 +++++++++++++++++
.../boot/dts/broadcom/bcmbca/bcm4912.dtsi | 19 +++++++++++++++++++
.../boot/dts/broadcom/bcmbca/bcm63146.dtsi | 18 ++++++++++++++++++
.../boot/dts/broadcom/bcmbca/bcm63158.dtsi | 18 ++++++++++++++++++
.../boot/dts/broadcom/bcmbca/bcm6813.dtsi | 19 +++++++++++++++++++
.../boot/dts/broadcom/bcmbca/bcm6856.dtsi | 17 +++++++++++++++++
.../boot/dts/broadcom/bcmbca/bcm6858.dtsi | 17 +++++++++++++++++
.../boot/dts/broadcom/bcmbca/bcm94908.dts | 4 ++++
.../boot/dts/broadcom/bcmbca/bcm94912.dts | 4 ++++
.../boot/dts/broadcom/bcmbca/bcm963146.dts | 4 ++++
.../boot/dts/broadcom/bcmbca/bcm963158.dts | 4 ++++
.../boot/dts/broadcom/bcmbca/bcm96813.dts | 4 ++++
.../boot/dts/broadcom/bcmbca/bcm96856.dts | 4 ++++
.../boot/dts/broadcom/bcmbca/bcm96858.dts | 4 ++++
14 files changed, 153 insertions(+)
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
index eb2a78f4e033..169045215d91 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
@@ -107,6 +107,12 @@ periph_clk: periph_clk {
clock-frequency = <50000000>;
clock-output-names = "periph";
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <400000000>;
+ };
};
soc {
@@ -531,6 +537,17 @@ leds: leds@800 {
#size-cells = <0>;
};
+ hsspi: spi@1000{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
nand-controller@1800 {
#address-cells = <1>;
#size-cells = <0>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi
index d5bc31980f03..ebacfffc4264 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi
@@ -79,6 +79,7 @@ periph_clk: periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
uart_clk: uart-clk {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
@@ -86,6 +87,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <200000000>;
+ };
};
psci {
@@ -117,6 +124,18 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0x0 0x0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcmbca-hsspi";
+ reg = <0x1000 0x600>, <0x2610 0x4>;
+ reg-names = "hsspi", "spim-ctrl";
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm63146.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm63146.dtsi
index 6f805266d3c9..184f8975d8f2 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm63146.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm63146.dtsi
@@ -60,6 +60,7 @@ periph_clk: periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
uart_clk: uart-clk {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
@@ -67,6 +68,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <200000000>;
+ };
};
psci {
@@ -99,6 +106,17 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0x0 0x0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm63158.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm63158.dtsi
index b982249b80a2..4036ddc3c833 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm63158.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm63158.dtsi
@@ -79,6 +79,7 @@ periph_clk: periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
uart_clk: uart-clk {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
@@ -86,6 +87,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <400000000>;
+ };
};
psci {
@@ -117,6 +124,17 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0x0 0x0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi
index a996d436e977..d29738e6fd67 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi
@@ -79,6 +79,7 @@ periph_clk: periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
uart_clk: uart-clk {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
@@ -86,6 +87,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <200000000>;
+ };
};
psci {
@@ -117,6 +124,18 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0x0 0x0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcmbca-hsspi";
+ reg = <0x1000 0x600>, <0x2610 0x4>;
+ reg-names = "hsspi", "spim-ctrl";
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi
index 62c530d4b103..6c5faf175551 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi
@@ -60,6 +60,12 @@ periph_clk:periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <400000000>;
+ };
};
psci {
@@ -100,5 +106,16 @@ uart0: serial@640 {
clock-names = "refclk";
status = "disabled";
};
+
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
};
};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi
index 34c7b513d363..edc0003457fd 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi
@@ -78,6 +78,12 @@ periph_clk:periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <400000000>;
+ };
};
psci {
@@ -137,5 +143,16 @@ uart0: serial@640 {
clock-names = "refclk";
status = "disabled";
};
+
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
};
};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm94908.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm94908.dts
index fcbd3c430ace..c4e6e71f6310 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm94908.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm94908.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm94912.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm94912.dts
index a3623e6f6919..e69cd683211a 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm94912.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm94912.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm963146.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm963146.dts
index e39f1e6d4774..db2c82d6dfd8 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm963146.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm963146.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm963158.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm963158.dts
index eba07e0b1ca6..25c12bc63545 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm963158.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm963158.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96813.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96813.dts
index af17091ae764..faba21f03120 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96813.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96813.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96856.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96856.dts
index 032aeb75c983..9808331eede2 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96856.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96856.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96858.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96858.dts
index 0cbf582f5d54..1f561c8e13b0 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96858.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96858.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
--
2.37.3
Currently exec_op is always required if controller driver provides
mem_ops. But some controller such as bcm63xx-hsspi may only need to
implement other operation like supports_op and use the default
execution operation. This patch removes this restriction.
Signed-off-by: William Zhang <[email protected]>
---
drivers/spi/spi-mem.c | 2 +-
drivers/spi/spi.c | 13 ++++++-------
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 0c79193d9697..701838b6f0c4 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -325,7 +325,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
if (!spi_mem_internal_supports_op(mem, op))
return -ENOTSUPP;
- if (ctlr->mem_ops && !mem->spi->cs_gpiod) {
+ if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !mem->spi->cs_gpiod) {
ret = spi_mem_access_start(mem);
if (ret)
return ret;
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 3cc7bb4d03de..6faa77592e93 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3051,15 +3051,14 @@ static int spi_controller_check_ops(struct spi_controller *ctlr)
* The controller may implement only the high-level SPI-memory like
* operations if it does not support regular SPI transfers, and this is
* valid use case.
- * If ->mem_ops is NULL, we request that at least one of the
- * ->transfer_xxx() method be implemented.
+ * If ->mem_ops or ->mem_ops->exec_op is NULL, we request that at least
+ * one of the ->transfer_xxx() method be implemented.
*/
- if (ctlr->mem_ops) {
- if (!ctlr->mem_ops->exec_op)
- return -EINVAL;
- } else if (!ctlr->transfer && !ctlr->transfer_one &&
+ if (!ctlr->mem_ops || (ctlr->mem_ops && !ctlr->mem_ops->exec_op)) {
+ if (!ctlr->transfer && !ctlr->transfer_one &&
!ctlr->transfer_one_message) {
- return -EINVAL;
+ return -EINVAL;
+ }
}
return 0;
--
2.37.3
brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific
property for certain SPI device such as Broadcom ISI voice daughtercard
to work properly. It disables the clock gating feature when the chip
select is deasserted for any device that wants to keep the clock
running.
Signed-off-by: William Zhang <[email protected]>
---
.../brcm,bcm63xx-hsspi-peripheral-props.yaml | 27 +++++++++++++++++++
.../bindings/spi/spi-peripheral-props.yaml | 1 +
2 files changed, 28 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml
diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml
new file mode 100644
index 000000000000..81884e2cc42d
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi-peripheral-props.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Peripheral-specific properties for Broadcom Broadband SoC HSSPI controller
+
+description:
+ See spi-peripheral-props.yaml for more info.
+
+maintainers:
+ - William Zhang <[email protected]>
+ - Kursad Oney <[email protected]>
+ - Jonas Gorski <[email protected]>
+
+properties:
+ brcm,no-clk-gate:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ Some SPI device such as Broadcom ISI based voice daughtercard requires SPI
+ clock running even when chip select is deasserted. By default the
+ controller turns off or gate the clock when cs is not active to save
+ power. This flag tells the controller driver to keep the clock running
+ when chip select is not active.
+
+additionalProperties: true
diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index ead2cccf658f..f85d777c7b67 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -108,5 +108,6 @@ allOf:
- $ref: cdns,qspi-nor-peripheral-props.yaml#
- $ref: samsung,spi-peripheral-props.yaml#
- $ref: nvidia,tegra210-quad-peripheral-props.yaml#
+ - $ref: brcm,bcm63xx-hsspi-peripheral-props.yaml#
additionalProperties: true
--
2.37.3
This is the preparation for updates on the bcm63xx hsspi driver. Convert
the text based bindings to json-schema per new dts requirement.
Signed-off-by: William Zhang <[email protected]>
---
.../bindings/spi/brcm,bcm63xx-hsspi.yaml | 52 +++++++++++++++++++
.../bindings/spi/spi-bcm63xx-hsspi.txt | 33 ------------
2 files changed, 52 insertions(+), 33 deletions(-)
create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt
diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
new file mode 100644
index 000000000000..45f1417b1213
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom BCM6328 High Speed SPI controller
+
+maintainers:
+ - Jonas Gorski <[email protected]>
+
+properties:
+ compatible:
+ const: brcm,bcm6328-hsspi
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: spi master reference clock
+ - description: spi master pll clock
+
+ clock-names:
+ items:
+ - const: hsspi
+ - const: pll
+
+ interrupts:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - interrupts
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ spi@10001000 {
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x10001000 0x600>;
+ interrupts = <29>;
+ clocks = <&clkctl 9>, <&hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <2>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
diff --git a/Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt b/Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt
deleted file mode 100644
index 37b29ee13860..000000000000
--- a/Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt
+++ /dev/null
@@ -1,33 +0,0 @@
-Binding for Broadcom BCM6328 High Speed SPI controller
-
-Required properties:
-- compatible: must contain of "brcm,bcm6328-hsspi".
-- reg: Base address and size of the controllers memory area.
-- interrupts: Interrupt for the SPI block.
-- clocks: phandles of the SPI clock and the PLL clock.
-- clock-names: must be "hsspi", "pll".
-- #address-cells: <1>, as required by generic SPI binding.
-- #size-cells: <0>, also as required by generic SPI binding.
-
-Optional properties:
-- num-cs: some controllers have less than 8 cs signals. Defaults to 8
- if absent.
-
-Child nodes as per the generic SPI binding.
-
-Example:
-
- spi@10001000 {
- compatible = "brcm,bcm6328-hsspi";
- reg = <0x10001000 0x600>;
-
- interrupts = <29>;
-
- clocks = <&clkctl 9>, <&hsspi_pll>;
- clock-names = "hsspi", "pll";
-
- num-cs = <2>;
-
- #address-cells = <1>;
- #size-cells = <0>;
- };
--
2.37.3
Add support for HSSPI controller in ARMv7 chip dts files.
Signed-off-by: William Zhang <[email protected]>
---
arch/arm/boot/dts/bcm47622.dtsi | 17 +++++++++++++++++
arch/arm/boot/dts/bcm63138.dtsi | 17 +++++++++++++++++
arch/arm/boot/dts/bcm63148.dtsi | 17 +++++++++++++++++
arch/arm/boot/dts/bcm63178.dtsi | 18 ++++++++++++++++++
arch/arm/boot/dts/bcm6756.dtsi | 18 ++++++++++++++++++
arch/arm/boot/dts/bcm6846.dtsi | 17 +++++++++++++++++
arch/arm/boot/dts/bcm6855.dtsi | 18 ++++++++++++++++++
arch/arm/boot/dts/bcm6878.dtsi | 18 ++++++++++++++++++
arch/arm/boot/dts/bcm947622.dts | 4 ++++
arch/arm/boot/dts/bcm963138.dts | 4 ++++
arch/arm/boot/dts/bcm963138dvt.dts | 4 ++++
arch/arm/boot/dts/bcm963148.dts | 4 ++++
arch/arm/boot/dts/bcm963178.dts | 4 ++++
arch/arm/boot/dts/bcm96756.dts | 4 ++++
arch/arm/boot/dts/bcm96846.dts | 4 ++++
arch/arm/boot/dts/bcm96855.dts | 4 ++++
arch/arm/boot/dts/bcm96878.dts | 4 ++++
17 files changed, 176 insertions(+)
diff --git a/arch/arm/boot/dts/bcm47622.dtsi b/arch/arm/boot/dts/bcm47622.dtsi
index f4b2db9bc4ab..da4b71ef2471 100644
--- a/arch/arm/boot/dts/bcm47622.dtsi
+++ b/arch/arm/boot/dts/bcm47622.dtsi
@@ -88,6 +88,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <200000000>;
+ };
};
psci {
@@ -119,6 +125,17 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/bcm63138.dtsi b/arch/arm/boot/dts/bcm63138.dtsi
index b774a8d63813..1631694c0496 100644
--- a/arch/arm/boot/dts/bcm63138.dtsi
+++ b/arch/arm/boot/dts/bcm63138.dtsi
@@ -66,6 +66,12 @@ apb_clk: apb_clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <400000000>;
+ };
};
/* ARM bus */
@@ -203,6 +209,17 @@ serial1: serial@620 {
status = "disabled";
};
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
nand_controller: nand-controller@2000 {
#address-cells = <1>;
#size-cells = <0>;
diff --git a/arch/arm/boot/dts/bcm63148.dtsi b/arch/arm/boot/dts/bcm63148.dtsi
index 7cd55d64de71..6dccba705f5d 100644
--- a/arch/arm/boot/dts/bcm63148.dtsi
+++ b/arch/arm/boot/dts/bcm63148.dtsi
@@ -60,6 +60,12 @@ periph_clk: periph-clk {
#clock-cells = <0>;
clock-frequency = <50000000>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <400000000>;
+ };
};
psci {
@@ -100,5 +106,16 @@ uart0: serial@600 {
clock-names = "refclk";
status = "disabled";
};
+
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
};
};
diff --git a/arch/arm/boot/dts/bcm63178.dtsi b/arch/arm/boot/dts/bcm63178.dtsi
index 043e699cbc27..8db27e7ac9fd 100644
--- a/arch/arm/boot/dts/bcm63178.dtsi
+++ b/arch/arm/boot/dts/bcm63178.dtsi
@@ -71,6 +71,7 @@ periph_clk: periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
uart_clk: uart-clk {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
@@ -78,6 +79,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <200000000>;
+ };
};
psci {
@@ -109,6 +116,17 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/bcm6756.dtsi b/arch/arm/boot/dts/bcm6756.dtsi
index 5c72219bc194..2af35a48b6c3 100644
--- a/arch/arm/boot/dts/bcm6756.dtsi
+++ b/arch/arm/boot/dts/bcm6756.dtsi
@@ -88,6 +88,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <200000000>;
+ };
};
psci {
@@ -119,6 +125,18 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcmbca-hsspi";
+ reg = <0x1000 0x600>, <0x2610 0x4>;
+ reg-names = "hsspi", "spim-ctrl";
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/bcm6846.dtsi b/arch/arm/boot/dts/bcm6846.dtsi
index 81513a793815..fa26b2107f93 100644
--- a/arch/arm/boot/dts/bcm6846.dtsi
+++ b/arch/arm/boot/dts/bcm6846.dtsi
@@ -61,6 +61,12 @@ periph_clk: periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <400000000>;
+ };
};
psci {
@@ -100,5 +106,16 @@ uart0: serial@640 {
clock-names = "refclk";
status = "disabled";
};
+
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
};
};
diff --git a/arch/arm/boot/dts/bcm6855.dtsi b/arch/arm/boot/dts/bcm6855.dtsi
index 5fa5feac0e29..bf028f0ad84c 100644
--- a/arch/arm/boot/dts/bcm6855.dtsi
+++ b/arch/arm/boot/dts/bcm6855.dtsi
@@ -78,6 +78,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <200000000>;
+ };
};
psci {
@@ -109,6 +115,18 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcmbca-hsspi";
+ reg = <0x1000 0x600>, <0x2610 0x4>;
+ reg-names = "hsspi", "spim-ctrl";
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/bcm6878.dtsi b/arch/arm/boot/dts/bcm6878.dtsi
index 4ec836ac4baf..be7ab5f52da4 100644
--- a/arch/arm/boot/dts/bcm6878.dtsi
+++ b/arch/arm/boot/dts/bcm6878.dtsi
@@ -61,6 +61,7 @@ periph_clk: periph-clk {
#clock-cells = <0>;
clock-frequency = <200000000>;
};
+
uart_clk: uart-clk {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
@@ -68,6 +69,12 @@ uart_clk: uart-clk {
clock-div = <4>;
clock-mult = <1>;
};
+
+ hsspi_pll: hsspi-pll {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <200000000>;
+ };
};
psci {
@@ -100,6 +107,17 @@ bus@ff800000 {
#size-cells = <1>;
ranges = <0 0xff800000 0x800000>;
+ hsspi: spi@1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "brcm,bcm6328-hsspi";
+ reg = <0x1000 0x600>;
+ clocks = <&hsspi_pll &hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ status = "disabled";
+ };
+
uart0: serial@12000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/bcm947622.dts b/arch/arm/boot/dts/bcm947622.dts
index 6f083724ab8e..93b8ce22678d 100644
--- a/arch/arm/boot/dts/bcm947622.dts
+++ b/arch/arm/boot/dts/bcm947622.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm963138.dts b/arch/arm/boot/dts/bcm963138.dts
index d28c4f130ca2..1b405c249213 100644
--- a/arch/arm/boot/dts/bcm963138.dts
+++ b/arch/arm/boot/dts/bcm963138.dts
@@ -25,3 +25,7 @@ memory@0 {
&serial0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm963138dvt.dts b/arch/arm/boot/dts/bcm963138dvt.dts
index 15bec75be74c..b5af61853a07 100644
--- a/arch/arm/boot/dts/bcm963138dvt.dts
+++ b/arch/arm/boot/dts/bcm963138dvt.dts
@@ -50,3 +50,7 @@ &ahci {
&sata_phy {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm963148.dts b/arch/arm/boot/dts/bcm963148.dts
index 98f6a6d09f50..1f5d6d783f09 100644
--- a/arch/arm/boot/dts/bcm963148.dts
+++ b/arch/arm/boot/dts/bcm963148.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm963178.dts b/arch/arm/boot/dts/bcm963178.dts
index fa096e9cde23..d036e99dd8d1 100644
--- a/arch/arm/boot/dts/bcm963178.dts
+++ b/arch/arm/boot/dts/bcm963178.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm96756.dts b/arch/arm/boot/dts/bcm96756.dts
index 9a4a87ba9c8a..8b104f3fb14a 100644
--- a/arch/arm/boot/dts/bcm96756.dts
+++ b/arch/arm/boot/dts/bcm96756.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm96846.dts b/arch/arm/boot/dts/bcm96846.dts
index c70ebccabc19..55852c229608 100644
--- a/arch/arm/boot/dts/bcm96846.dts
+++ b/arch/arm/boot/dts/bcm96846.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm96855.dts b/arch/arm/boot/dts/bcm96855.dts
index 4438152561ac..2ad880af2104 100644
--- a/arch/arm/boot/dts/bcm96855.dts
+++ b/arch/arm/boot/dts/bcm96855.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm96878.dts b/arch/arm/boot/dts/bcm96878.dts
index 8fbc175cb452..b7af8ade7a9d 100644
--- a/arch/arm/boot/dts/bcm96878.dts
+++ b/arch/arm/boot/dts/bcm96878.dts
@@ -28,3 +28,7 @@ memory@0 {
&uart0 {
status = "okay";
};
+
+&hsspi {
+ status = "okay";
+};
--
2.37.3
The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI
controller. Add a new compatible string and required fields for the new
driver. Also add myself and Kursad as the maintainers.
Signed-off-by: William Zhang <[email protected]>
---
.../bindings/spi/brcm,bcm63xx-hsspi.yaml | 84 +++++++++++++++++--
1 file changed, 78 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
index 45f1417b1213..56e69d4a1faf 100644
--- a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
+++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
@@ -4,22 +4,51 @@
$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: Broadcom BCM6328 High Speed SPI controller
+title: Broadcom Broadband SoC High Speed SPI controller
maintainers:
+
+ - William Zhang <[email protected]>
+ - Kursad Oney <[email protected]>
- Jonas Gorski <[email protected]>
+description: |
+ Broadcom Broadband SoC supports High Speed SPI master controller since the
+ early MIPS based chips such as BCM6328 and BCM63268. This controller was
+ carried over to recent ARM based chips, such as BCM63138, BCM4908 and BCM6858.
+
+ It has a limitation that can not keep the chip select line active between
+ the SPI transfers within the same SPI message. This can terminate the
+ transaction to some SPI devices prematurely. The issue can be worked around by
+ either the controller's prepend mode or using the dummy chip select
+ workaround. This controller uses the compatible string brcm,bcm6328-hsspi.
+
+ The newer SoCs such as BCM6756, BCM4912 and BCM6855 include an updated SPI
+ controller that add the capability to allow the driver to control chip select
+ explicitly. This solves the issue in the old controller. This new controller
+ uses the compatible string brcm,bcmbca-hsspi.
+
properties:
compatible:
- const: brcm,bcm6328-hsspi
+ enum:
+ - brcm,bcm6328-hsspi
+ - brcm,bcmbca-hsspi
reg:
- maxItems: 1
+ items:
+ - description: main registers
+ - description: miscellaneous control registers
+ minItems: 1
+
+ reg-names:
+ items:
+ - const: hsspi
+ - const: spim-ctrl
clocks:
items:
- - description: spi master reference clock
- - description: spi master pll clock
+ - description: SPI master reference clock
+ - description: SPI master pll clock
clock-names:
items:
@@ -29,12 +58,43 @@ properties:
interrupts:
maxItems: 1
+ brcm,use-cs-workaround:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description: |
+ Enable dummy chip select workaround for SPI transfers that can not be
+ supported by the default controller's prepend mode, i.e. delay or cs
+ change needed between SPI transfers.
+
required:
- compatible
- reg
- clocks
- clock-names
- - interrupts
+
+allOf:
+ - $ref: "spi-controller.yaml#"
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - brcm,bcm6328-hsspi
+ then:
+ properties:
+ reg:
+ minItems: 1
+ maxItems: 1
+ else:
+ properties:
+ reg:
+ minItems: 2
+ maxItems: 2
+ reg-names:
+ minItems: 2
+ maxItems: 2
+ brcm,use-cs-workaround: false
+ required:
+ - reg-names
unevaluatedProperties: false
@@ -50,3 +110,15 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
};
+ - |
+ spi@ff801000 {
+ compatible = "brcm,bcmbca-hsspi";
+ reg = <0xff801000 0x1000>,
+ <0xff802610 0x4>;
+ reg-names = "hsspi", "spim-ctrl";
+ clocks = <&hsspi>, <&hsspi_pll>;
+ clock-names = "hsspi", "pll";
+ num-cs = <8>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
--
2.37.3
On Fri, Jan 06, 2023 at 12:07:55PM -0800, William Zhang wrote:
> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific
> property for certain SPI device such as Broadcom ISI voice daughtercard
> to work properly. It disables the clock gating feature when the chip
> select is deasserted for any device that wants to keep the clock
> running.
Why would this property be Broadcom specific? Other devices could in
theory implement this.
> +properties:
> + brcm,no-clk-gate:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description:
> + Some SPI device such as Broadcom ISI based voice daughtercard requires SPI
> + clock running even when chip select is deasserted. By default the
> + controller turns off or gate the clock when cs is not active to save
> + power. This flag tells the controller driver to keep the clock running
> + when chip select is not active.
This seems problematic with any host controlled chip select support...
On Fri, Jan 06, 2023 at 12:08:03PM -0800, William Zhang wrote:
> Multiple transfers within a SPI message may be combined into one
> transfer to the controller using its prepend feature. A SPI message is
> prependable only if the following are all true:
> * One or more half duplex write transfer
> * Optional full duplex read/write at the end
> * No delay and cs_change between transfers
There is nothing driver specific here, this should be implemented in the
core - we have existing logic to rewrite messages to match driver
constraints, this could be added there possibly with flags to allow
drivers to disable or enable the merging if they've got special
requirements.
On Fri, Jan 06, 2023 at 12:07:59PM -0800, William Zhang wrote:
> Polling mode provides better throughput in general by avoiding the
> interrupt overhead as the maximum data size one interrupt can handle is
> only 512 bytes.
> When interrupt is not defined in the HSSPI dts node, driver switches to
> polling mode for controller SPI message processing. Also add driver
> banner message when the driver is loaded successfully.
This should not be something the user selects via the DT, if the polling
mode is better then the driver should just use it regardless of there
being an interrupt wired up. Generally there's some point at which the
benefits of polling become minimal (and the costs more impactful) but if
the DMA setup is as bad as it sounds then the driver should just ignore
the interrupt.
Hi Mark,
On 01/06/2023 01:14 PM, Mark Brown wrote:
> On Fri, Jan 06, 2023 at 12:07:55PM -0800, William Zhang wrote:
>
>> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific
>> property for certain SPI device such as Broadcom ISI voice daughtercard
>> to work properly. It disables the clock gating feature when the chip
>> select is deasserted for any device that wants to keep the clock
>> running.
>
> Why would this property be Broadcom specific? Other devices could in
> theory implement this.
>
It does not need to be Broadcom specific if other SoC's SPI bus
controller support such function. I am not aware of such case but
certainly I am no expert on other chips. I can put it in the generic
spi-peripheral-props.yaml if that is what you suggest.
>> +properties:
>> + brcm,no-clk-gate:
>> + $ref: /schemas/types.yaml#/definitions/flag
>> + description:
>> + Some SPI device such as Broadcom ISI based voice daughtercard requires SPI
>> + clock running even when chip select is deasserted. By default the
>> + controller turns off or gate the clock when cs is not active to save
>> + power. This flag tells the controller driver to keep the clock running
>> + when chip select is not active.
>
> This seems problematic with any host controlled chip select support...
>
Yes those ISI chip based voice cards do need such strange requirement
and will not work with other controller. That is one of the reason I
put this as Broadcom specific option.
On 01/06/2023 01:47 PM, Mark Brown wrote:
> On Fri, Jan 06, 2023 at 12:07:59PM -0800, William Zhang wrote:
>
>> Polling mode provides better throughput in general by avoiding the
>> interrupt overhead as the maximum data size one interrupt can handle is
>> only 512 bytes.
>
>> When interrupt is not defined in the HSSPI dts node, driver switches to
>> polling mode for controller SPI message processing. Also add driver
>> banner message when the driver is loaded successfully.
>
> This should not be something the user selects via the DT, if the polling
> mode is better then the driver should just use it regardless of there
> being an interrupt wired up. Generally there's some point at which the
> benefits of polling become minimal (and the costs more impactful) but if
> the DMA setup is as bad as it sounds then the driver should just ignore
> the interrupt.
>
I agree but this is more for backward compatibility as the original
driver uses interrupt to work with MIPS based SoC and I do not want to
change that behavior. For newer devices I added, they work in polling
mode by default with the dts I supplied. IMHO it is also good to have
this fallback option to use interrupt in case user is sensitive to cpu
usage.
On 01/06/2023 02:00 PM, Mark Brown wrote:
> On Fri, Jan 06, 2023 at 12:08:03PM -0800, William Zhang wrote:
>> Multiple transfers within a SPI message may be combined into one
>> transfer to the controller using its prepend feature. A SPI message is
>> prependable only if the following are all true:
>> * One or more half duplex write transfer
>> * Optional full duplex read/write at the end
>> * No delay and cs_change between transfers
>
> There is nothing driver specific here, this should be implemented in the
> core - we have existing logic to rewrite messages to match driver
> constraints, this could be added there possibly with flags to allow
> drivers to disable or enable the merging if they've got special
> requirements.
>
My understanding of combining the spi transfer in the core level does
not quite work out to our controller. For example, for a spi message
with three transfers, tx, tx and rx. We can possibly combine them in
single duplex tx/rx transfer in the core. But this will be treated as
duplex transaction in our controller level which require tx and rx data
happens at the same time. Obviously this won't work when rx depends on
tx happening first. We can not differentiate this combined duplex
transfer from the true duplex transfer unless there is some flag to
indicate that. Also there is limit of max tx length as the prepend
buffer so maybe another parameter. And another reason to be done in the
driver level is this prepend mode has dependency on dummy cs workaround
which is driver level parameter currently. I am not sure how practical
and useful this is to factor them out to the core level? Maybe I didn't
fully understand your comments and appreciate if you can elaborate or
point me to the related core code.
Hi William,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on broonie-spi/for-next krzk/for-next krzk-dt/for-next krzk-mem-ctrl/for-next linus/master v6.2-rc2 next-20230106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230106200809.330769-7-william.zhang%40broadcom.com
patch subject: [PATCH 06/16] spi: bcm63xx-hsspi: Endianness fix for ARM based SoC
config: alpha-randconfig-s052-20230106
compiler: alpha-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/8a82a44b50d88a27e55065e9f1bd75f0fccf479a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
git checkout 8a82a44b50d88a27e55065e9f1bd75f0fccf479a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=alpha olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=alpha SHELL=/bin/bash drivers/spi/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned short [usertype] b @@ got restricted __be16 [usertype] @@
drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: expected unsigned short [usertype] b
drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: got restricted __be16 [usertype]
vim +197 drivers/spi/spi-bcm63xx-hsspi.c
156
157 static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
158 {
159 struct bcm63xx_hsspi *bs = spi_master_get_devdata(spi->master);
160 unsigned int chip_select = spi->chip_select;
161 u16 opcode = 0;
162 int pending = t->len;
163 int step_size = HSSPI_BUFFER_LEN;
164 const u8 *tx = t->tx_buf;
165 u8 *rx = t->rx_buf;
166
167 bcm63xx_hsspi_set_clk(bs, spi, t->speed_hz);
168 bcm63xx_hsspi_set_cs(bs, spi->chip_select, true);
169
170 if (tx && rx)
171 opcode = HSSPI_OP_READ_WRITE;
172 else if (tx)
173 opcode = HSSPI_OP_WRITE;
174 else if (rx)
175 opcode = HSSPI_OP_READ;
176
177 if (opcode != HSSPI_OP_READ)
178 step_size -= HSSPI_OPCODE_LEN;
179
180 if ((opcode == HSSPI_OP_READ && t->rx_nbits == SPI_NBITS_DUAL) ||
181 (opcode == HSSPI_OP_WRITE && t->tx_nbits == SPI_NBITS_DUAL))
182 opcode |= HSSPI_OP_MULTIBIT;
183
184 __raw_writel(1 << MODE_CTRL_MULTIDATA_WR_SIZE_SHIFT |
185 1 << MODE_CTRL_MULTIDATA_RD_SIZE_SHIFT | 0xff,
186 bs->regs + HSSPI_PROFILE_MODE_CTRL_REG(chip_select));
187
188 while (pending > 0) {
189 int curr_step = min_t(int, step_size, pending);
190
191 reinit_completion(&bs->done);
192 if (tx) {
193 memcpy_toio(bs->fifo + HSSPI_OPCODE_LEN, tx, curr_step);
194 tx += curr_step;
195 }
196
> 197 __raw_writew(cpu_to_be16(opcode | curr_step), bs->fifo);
198
199 /* enable interrupt */
200 __raw_writel(HSSPI_PINGx_CMD_DONE(0),
201 bs->regs + HSSPI_INT_MASK_REG);
202
203 /* start the transfer */
204 __raw_writel(!chip_select << PINGPONG_CMD_SS_SHIFT |
205 chip_select << PINGPONG_CMD_PROFILE_SHIFT |
206 PINGPONG_COMMAND_START_NOW,
207 bs->regs + HSSPI_PINGPONG_COMMAND_REG(0));
208
209 if (wait_for_completion_timeout(&bs->done, HZ) == 0) {
210 dev_err(&bs->pdev->dev, "transfer timed out!\n");
211 return -ETIMEDOUT;
212 }
213
214 if (rx) {
215 memcpy_fromio(rx, bs->fifo, curr_step);
216 rx += curr_step;
217 }
218
219 pending -= curr_step;
220 }
221
222 return 0;
223 }
224
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Fri, 06 Jan 2023 12:07:53 -0800, William Zhang wrote:
> This is the preparation for updates on the bcm63xx hsspi driver. Convert
> the text based bindings to json-schema per new dts requirement.
>
> Signed-off-by: William Zhang <[email protected]>
> ---
>
> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 52 +++++++++++++++++++
> .../bindings/spi/spi-bcm63xx-hsspi.txt | 33 ------------
> 2 files changed, 52 insertions(+), 33 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
> delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.example.dtb: spi@10001000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'num-cs' were unexpected)
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On Fri, Jan 6, 2023 at 9:27 PM William Zhang <[email protected]> wrote:
>
> Hi Mark,
>
> On 01/06/2023 01:14 PM, Mark Brown wrote:
> > On Fri, Jan 06, 2023 at 12:07:55PM -0800, William Zhang wrote:
> >
> >> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific
> >> property for certain SPI device such as Broadcom ISI voice daughtercard
> >> to work properly. It disables the clock gating feature when the chip
> >> select is deasserted for any device that wants to keep the clock
> >> running.
> >
> > Why would this property be Broadcom specific? Other devices could in
> > theory implement this.
> >
> It does not need to be Broadcom specific if other SoC's SPI bus
> controller support such function. I am not aware of such case but
> certainly I am no expert on other chips. I can put it in the generic
> spi-peripheral-props.yaml if that is what you suggest.
>
> >> +properties:
> >> + brcm,no-clk-gate:
> >> + $ref: /schemas/types.yaml#/definitions/flag
> >> + description:
> >> + Some SPI device such as Broadcom ISI based voice daughtercard requires SPI
> >> + clock running even when chip select is deasserted. By default the
> >> + controller turns off or gate the clock when cs is not active to save
> >> + power. This flag tells the controller driver to keep the clock running
> >> + when chip select is not active.
> >
> > This seems problematic with any host controlled chip select support...
> >
> Yes those ISI chip based voice cards do need such strange requirement
> and will not work with other controller. That is one of the reason I
> put this as Broadcom specific option.
Keeping the clock on or not would affect all devices unless you have a
per device clock you can gate, so making this a per device flag
doesn't make sense.
If this is a requirement of the slave device, then the device's
compatible string can imply the need for this and its driver can tell
the host controller in some way.
Rob
On 06/01/2023 21:07, William Zhang wrote:
> This is the preparation for updates on the bcm63xx hsspi driver. Convert
> the text based bindings to json-schema per new dts requirement.
>
> Signed-off-by: William Zhang <[email protected]>
> ---
>
> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 52 +++++++++++++++++++
> .../bindings/spi/spi-bcm63xx-hsspi.txt | 33 ------------
> 2 files changed, 52 insertions(+), 33 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
> delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt
>
> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
> new file mode 100644
> index 000000000000..45f1417b1213
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom BCM6328 High Speed SPI controller
> +
> +maintainers:
> + - Jonas Gorski <[email protected]>
> +
Missing reference to spi-controller.
> +properties:
> + compatible:
> + const: brcm,bcm6328-hsspi
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: spi master reference clock
> + - description: spi master pll clock
> +
> + clock-names:
> + items:
> + - const: hsspi
> + - const: pll
> +
> + interrupts:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - interrupts
> +
> +unevaluatedProperties: false
This is for cases when you have reference to other schema.
Best regards,
Krzysztof
Hi William,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on broonie-spi/for-next krzk/for-next krzk-dt/for-next krzk-mem-ctrl/for-next linus/master v6.2-rc2 next-20230106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230106200809.330769-7-william.zhang%40broadcom.com
patch subject: [PATCH 06/16] spi: bcm63xx-hsspi: Endianness fix for ARM based SoC
config: microblaze-randconfig-s041-20230108
compiler: microblaze-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/8a82a44b50d88a27e55065e9f1bd75f0fccf479a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
git checkout 8a82a44b50d88a27e55065e9f1bd75f0fccf479a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/spi/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned short [usertype] value @@ got restricted __be16 [usertype] @@
drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: expected unsigned short [usertype] value
drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: got restricted __be16 [usertype]
vim +197 drivers/spi/spi-bcm63xx-hsspi.c
156
157 static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
158 {
159 struct bcm63xx_hsspi *bs = spi_master_get_devdata(spi->master);
160 unsigned int chip_select = spi->chip_select;
161 u16 opcode = 0;
162 int pending = t->len;
163 int step_size = HSSPI_BUFFER_LEN;
164 const u8 *tx = t->tx_buf;
165 u8 *rx = t->rx_buf;
166
167 bcm63xx_hsspi_set_clk(bs, spi, t->speed_hz);
168 bcm63xx_hsspi_set_cs(bs, spi->chip_select, true);
169
170 if (tx && rx)
171 opcode = HSSPI_OP_READ_WRITE;
172 else if (tx)
173 opcode = HSSPI_OP_WRITE;
174 else if (rx)
175 opcode = HSSPI_OP_READ;
176
177 if (opcode != HSSPI_OP_READ)
178 step_size -= HSSPI_OPCODE_LEN;
179
180 if ((opcode == HSSPI_OP_READ && t->rx_nbits == SPI_NBITS_DUAL) ||
181 (opcode == HSSPI_OP_WRITE && t->tx_nbits == SPI_NBITS_DUAL))
182 opcode |= HSSPI_OP_MULTIBIT;
183
184 __raw_writel(1 << MODE_CTRL_MULTIDATA_WR_SIZE_SHIFT |
185 1 << MODE_CTRL_MULTIDATA_RD_SIZE_SHIFT | 0xff,
186 bs->regs + HSSPI_PROFILE_MODE_CTRL_REG(chip_select));
187
188 while (pending > 0) {
189 int curr_step = min_t(int, step_size, pending);
190
191 reinit_completion(&bs->done);
192 if (tx) {
193 memcpy_toio(bs->fifo + HSSPI_OPCODE_LEN, tx, curr_step);
194 tx += curr_step;
195 }
196
> 197 __raw_writew(cpu_to_be16(opcode | curr_step), bs->fifo);
198
199 /* enable interrupt */
200 __raw_writel(HSSPI_PINGx_CMD_DONE(0),
201 bs->regs + HSSPI_INT_MASK_REG);
202
203 /* start the transfer */
204 __raw_writel(!chip_select << PINGPONG_CMD_SS_SHIFT |
205 chip_select << PINGPONG_CMD_PROFILE_SHIFT |
206 PINGPONG_COMMAND_START_NOW,
207 bs->regs + HSSPI_PINGPONG_COMMAND_REG(0));
208
209 if (wait_for_completion_timeout(&bs->done, HZ) == 0) {
210 dev_err(&bs->pdev->dev, "transfer timed out!\n");
211 return -ETIMEDOUT;
212 }
213
214 if (rx) {
215 memcpy_fromio(rx, bs->fifo, curr_step);
216 rx += curr_step;
217 }
218
219 pending -= curr_step;
220 }
221
222 return 0;
223 }
224
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi William,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on broonie-spi/for-next krzk/for-next krzk-dt/for-next krzk-mem-ctrl/for-next linus/master v6.2-rc2 next-20230106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230106200809.330769-7-william.zhang%40broadcom.com
patch subject: [PATCH 06/16] spi: bcm63xx-hsspi: Endianness fix for ARM based SoC
config: sh-randconfig-s031-20230108
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/8a82a44b50d88a27e55065e9f1bd75f0fccf479a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
git checkout 8a82a44b50d88a27e55065e9f1bd75f0fccf479a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sh olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sh SHELL=/bin/bash drivers/spi/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/spi/spi-bcm63xx-hsspi.c:197:17: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned short volatile [usertype] @@ got restricted __be16 [usertype] @@
drivers/spi/spi-bcm63xx-hsspi.c:197:17: sparse: expected unsigned short volatile [usertype]
drivers/spi/spi-bcm63xx-hsspi.c:197:17: sparse: got restricted __be16 [usertype]
vim +197 drivers/spi/spi-bcm63xx-hsspi.c
156
157 static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
158 {
159 struct bcm63xx_hsspi *bs = spi_master_get_devdata(spi->master);
160 unsigned int chip_select = spi->chip_select;
161 u16 opcode = 0;
162 int pending = t->len;
163 int step_size = HSSPI_BUFFER_LEN;
164 const u8 *tx = t->tx_buf;
165 u8 *rx = t->rx_buf;
166
167 bcm63xx_hsspi_set_clk(bs, spi, t->speed_hz);
168 bcm63xx_hsspi_set_cs(bs, spi->chip_select, true);
169
170 if (tx && rx)
171 opcode = HSSPI_OP_READ_WRITE;
172 else if (tx)
173 opcode = HSSPI_OP_WRITE;
174 else if (rx)
175 opcode = HSSPI_OP_READ;
176
177 if (opcode != HSSPI_OP_READ)
178 step_size -= HSSPI_OPCODE_LEN;
179
180 if ((opcode == HSSPI_OP_READ && t->rx_nbits == SPI_NBITS_DUAL) ||
181 (opcode == HSSPI_OP_WRITE && t->tx_nbits == SPI_NBITS_DUAL))
182 opcode |= HSSPI_OP_MULTIBIT;
183
184 __raw_writel(1 << MODE_CTRL_MULTIDATA_WR_SIZE_SHIFT |
185 1 << MODE_CTRL_MULTIDATA_RD_SIZE_SHIFT | 0xff,
186 bs->regs + HSSPI_PROFILE_MODE_CTRL_REG(chip_select));
187
188 while (pending > 0) {
189 int curr_step = min_t(int, step_size, pending);
190
191 reinit_completion(&bs->done);
192 if (tx) {
193 memcpy_toio(bs->fifo + HSSPI_OPCODE_LEN, tx, curr_step);
194 tx += curr_step;
195 }
196
> 197 __raw_writew(cpu_to_be16(opcode | curr_step), bs->fifo);
198
199 /* enable interrupt */
200 __raw_writel(HSSPI_PINGx_CMD_DONE(0),
201 bs->regs + HSSPI_INT_MASK_REG);
202
203 /* start the transfer */
204 __raw_writel(!chip_select << PINGPONG_CMD_SS_SHIFT |
205 chip_select << PINGPONG_CMD_PROFILE_SHIFT |
206 PINGPONG_COMMAND_START_NOW,
207 bs->regs + HSSPI_PINGPONG_COMMAND_REG(0));
208
209 if (wait_for_completion_timeout(&bs->done, HZ) == 0) {
210 dev_err(&bs->pdev->dev, "transfer timed out!\n");
211 return -ETIMEDOUT;
212 }
213
214 if (rx) {
215 memcpy_fromio(rx, bs->fifo, curr_step);
216 rx += curr_step;
217 }
218
219 pending -= curr_step;
220 }
221
222 return 0;
223 }
224
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi William,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on broonie-spi/for-next krzk/for-next krzk-dt/for-next krzk-mem-ctrl/for-next linus/master v6.2-rc2 next-20230106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230106200809.330769-7-william.zhang%40broadcom.com
patch subject: [PATCH 06/16] spi: bcm63xx-hsspi: Endianness fix for ARM based SoC
config: sparc-randconfig-s051-20230108
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/8a82a44b50d88a27e55065e9f1bd75f0fccf479a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
git checkout 8a82a44b50d88a27e55065e9f1bd75f0fccf479a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc SHELL=/bin/bash drivers/spi/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned short [usertype] w @@ got restricted __be16 [usertype] @@
drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: expected unsigned short [usertype] w
drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: got restricted __be16 [usertype]
vim +197 drivers/spi/spi-bcm63xx-hsspi.c
156
157 static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
158 {
159 struct bcm63xx_hsspi *bs = spi_master_get_devdata(spi->master);
160 unsigned int chip_select = spi->chip_select;
161 u16 opcode = 0;
162 int pending = t->len;
163 int step_size = HSSPI_BUFFER_LEN;
164 const u8 *tx = t->tx_buf;
165 u8 *rx = t->rx_buf;
166
167 bcm63xx_hsspi_set_clk(bs, spi, t->speed_hz);
168 bcm63xx_hsspi_set_cs(bs, spi->chip_select, true);
169
170 if (tx && rx)
171 opcode = HSSPI_OP_READ_WRITE;
172 else if (tx)
173 opcode = HSSPI_OP_WRITE;
174 else if (rx)
175 opcode = HSSPI_OP_READ;
176
177 if (opcode != HSSPI_OP_READ)
178 step_size -= HSSPI_OPCODE_LEN;
179
180 if ((opcode == HSSPI_OP_READ && t->rx_nbits == SPI_NBITS_DUAL) ||
181 (opcode == HSSPI_OP_WRITE && t->tx_nbits == SPI_NBITS_DUAL))
182 opcode |= HSSPI_OP_MULTIBIT;
183
184 __raw_writel(1 << MODE_CTRL_MULTIDATA_WR_SIZE_SHIFT |
185 1 << MODE_CTRL_MULTIDATA_RD_SIZE_SHIFT | 0xff,
186 bs->regs + HSSPI_PROFILE_MODE_CTRL_REG(chip_select));
187
188 while (pending > 0) {
189 int curr_step = min_t(int, step_size, pending);
190
191 reinit_completion(&bs->done);
192 if (tx) {
193 memcpy_toio(bs->fifo + HSSPI_OPCODE_LEN, tx, curr_step);
194 tx += curr_step;
195 }
196
> 197 __raw_writew(cpu_to_be16(opcode | curr_step), bs->fifo);
198
199 /* enable interrupt */
200 __raw_writel(HSSPI_PINGx_CMD_DONE(0),
201 bs->regs + HSSPI_INT_MASK_REG);
202
203 /* start the transfer */
204 __raw_writel(!chip_select << PINGPONG_CMD_SS_SHIFT |
205 chip_select << PINGPONG_CMD_PROFILE_SHIFT |
206 PINGPONG_COMMAND_START_NOW,
207 bs->regs + HSSPI_PINGPONG_COMMAND_REG(0));
208
209 if (wait_for_completion_timeout(&bs->done, HZ) == 0) {
210 dev_err(&bs->pdev->dev, "transfer timed out!\n");
211 return -ETIMEDOUT;
212 }
213
214 if (rx) {
215 memcpy_fromio(rx, bs->fifo, curr_step);
216 rx += curr_step;
217 }
218
219 pending -= curr_step;
220 }
221
222 return 0;
223 }
224
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 06/01/2023 21:07, William Zhang wrote:
> The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI
> controller. Add a new compatible string and required fields for the new
> driver. Also add myself and Kursad as the maintainers.
>
> Signed-off-by: William Zhang <[email protected]>
> ---
>
> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 84 +++++++++++++++++--
> 1 file changed, 78 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
> index 45f1417b1213..56e69d4a1faf 100644
> --- a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
> @@ -4,22 +4,51 @@
> $id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: Broadcom BCM6328 High Speed SPI controller
> +title: Broadcom Broadband SoC High Speed SPI controller
>
> maintainers:
> +
Drop blank line.
> + - William Zhang <[email protected]>
> + - Kursad Oney <[email protected]>
> - Jonas Gorski <[email protected]>
>
> +description: |
> + Broadcom Broadband SoC supports High Speed SPI master controller since the
> + early MIPS based chips such as BCM6328 and BCM63268. This controller was
> + carried over to recent ARM based chips, such as BCM63138, BCM4908 and BCM6858.
> +
> + It has a limitation that can not keep the chip select line active between
> + the SPI transfers within the same SPI message. This can terminate the
> + transaction to some SPI devices prematurely. The issue can be worked around by
> + either the controller's prepend mode or using the dummy chip select
> + workaround. This controller uses the compatible string brcm,bcm6328-hsspi.
> +
> + The newer SoCs such as BCM6756, BCM4912 and BCM6855 include an updated SPI
> + controller that add the capability to allow the driver to control chip select
> + explicitly. This solves the issue in the old controller. This new controller
> + uses the compatible string brcm,bcmbca-hsspi.
> +
> properties:
> compatible:
> - const: brcm,bcm6328-hsspi
> + enum:
> + - brcm,bcm6328-hsspi
> + - brcm,bcmbca-hsspi
bca seems quite unspecific. Your description above mentions several
model numbers and "bca" is not listed as model. Compatibles cannot be
generic.
>
> reg:
> - maxItems: 1
> + items:
> + - description: main registers
> + - description: miscellaneous control registers
> + minItems: 1
> +
> + reg-names:
> + items:
> + - const: hsspi
> + - const: spim-ctrl
This does not match reg
>
> clocks:
> items:
> - - description: spi master reference clock
> - - description: spi master pll clock
> + - description: SPI master reference clock
> + - description: SPI master pll clock
Really? You just added it in previous patch, didn't you?
>
> clock-names:
> items:
> @@ -29,12 +58,43 @@ properties:
> interrupts:
> maxItems: 1
>
> + brcm,use-cs-workaround:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
> + Enable dummy chip select workaround for SPI transfers that can not be
> + supported by the default controller's prepend mode, i.e. delay or cs
> + change needed between SPI transfers.
You need to describe what is the workaround.
> +
> required:
> - compatible
> - reg
> - clocks
> - clock-names
> - - interrupts
> +
> +allOf:
> + - $ref: "spi-controller.yaml#"
No quotes. How this is related to this patch?
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - brcm,bcm6328-hsspi
> + then:
> + properties:
> + reg:
> + minItems: 1
Drop.
reg-names now do not match.
> + maxItems: 1
> + else:
> + properties:
> + reg:
> + minItems: 2
> + maxItems: 2
> + reg-names:
> + minItems: 2
> + maxItems: 2
> + brcm,use-cs-workaround: false
> + required:
> + - reg-names
Best regards,
Krzysztof
On 06/01/2023 21:07, William Zhang wrote:
> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific
> property for certain SPI device such as Broadcom ISI voice daughtercard
> to work properly. It disables the clock gating feature when the chip
> select is deasserted for any device that wants to keep the clock
> running.
> +additionalProperties: true
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index ead2cccf658f..f85d777c7b67 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -108,5 +108,6 @@ allOf:
> - $ref: cdns,qspi-nor-peripheral-props.yaml#
> - $ref: samsung,spi-peripheral-props.yaml#
> - $ref: nvidia,tegra210-quad-peripheral-props.yaml#
> + - $ref: brcm,bcm63xx-hsspi-peripheral-props.yaml#
Don't break the order.
Best regards,
Krzysztof
On 01/07/2023 07:32 AM, Krzysztof Kozlowski wrote:
> On 06/01/2023 21:07, William Zhang wrote:
>> This is the preparation for updates on the bcm63xx hsspi driver. Convert
>> the text based bindings to json-schema per new dts requirement.
>>
>> Signed-off-by: William Zhang <[email protected]>
>> ---
>>
>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 52 +++++++++++++++++++
>> .../bindings/spi/spi-bcm63xx-hsspi.txt | 33 ------------
>> 2 files changed, 52 insertions(+), 33 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>> delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>> new file mode 100644
>> index 000000000000..45f1417b1213
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Broadcom BCM6328 High Speed SPI controller
>> +
>> +maintainers:
>> + - Jonas Gorski <[email protected]>
>> +
>
> Missing reference to spi-controller.
>
This was word to word conversion from the text file. But I will update
with this required reference.
>> +properties:
>> + compatible:
>> + const: brcm,bcm6328-hsspi
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + clocks:
>> + items:
>> + - description: spi master reference clock
>> + - description: spi master pll clock
>> +
>> + clock-names:
>> + items:
>> + - const: hsspi
>> + - const: pll
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - clocks
>> + - clock-names
>> + - interrupts
>> +
>> +unevaluatedProperties: false
>
> This is for cases when you have reference to other schema.
>
Will drop here. But will add back in patch 1 which produces the final
version of this file and need this property.
>
> Best regards,
> Krzysztof
>
Hi Rob,
On 01/07/2023 07:38 AM, Rob Herring wrote:
> On Fri, Jan 6, 2023 at 9:27 PM William Zhang <[email protected]> wrote:
>>
>> Hi Mark,
>>
>> On 01/06/2023 01:14 PM, Mark Brown wrote:
>>> On Fri, Jan 06, 2023 at 12:07:55PM -0800, William Zhang wrote:
>>>
>>>> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific
>>>> property for certain SPI device such as Broadcom ISI voice daughtercard
>>>> to work properly. It disables the clock gating feature when the chip
>>>> select is deasserted for any device that wants to keep the clock
>>>> running.
>>>
>>> Why would this property be Broadcom specific? Other devices could in
>>> theory implement this.
>>>
>> It does not need to be Broadcom specific if other SoC's SPI bus
>> controller support such function. I am not aware of such case but
>> certainly I am no expert on other chips. I can put it in the generic
>> spi-peripheral-props.yaml if that is what you suggest.
>>
>>>> +properties:
>>>> + brcm,no-clk-gate:
>>>> + $ref: /schemas/types.yaml#/definitions/flag
>>>> + description:
>>>> + Some SPI device such as Broadcom ISI based voice daughtercard requires SPI
>>>> + clock running even when chip select is deasserted. By default the
>>>> + controller turns off or gate the clock when cs is not active to save
>>>> + power. This flag tells the controller driver to keep the clock running
>>>> + when chip select is not active.
>>>
>>> This seems problematic with any host controlled chip select support...
>>>
>> Yes those ISI chip based voice cards do need such strange requirement
>> and will not work with other controller. That is one of the reason I
>> put this as Broadcom specific option.
>
> Keeping the clock on or not would affect all devices unless you have a
> per device clock you can gate, so making this a per device flag
> doesn't make sense.
>
This applies only to each chip select. There is only one device under
each chip select. So won't impact any other devices under other cs.
> If this is a requirement of the slave device, then the device's
> compatible string can imply the need for this and its driver can tell
> the host controller in some way.
That is true but spi host controller driver reads and parses these slave
device flag directly.
>
> Rob
>
Hi Krzysztof,
On 01/08/2023 06:51 AM, Krzysztof Kozlowski wrote:
> On 06/01/2023 21:07, William Zhang wrote:
>> The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI
>> controller. Add a new compatible string and required fields for the new
>> driver. Also add myself and Kursad as the maintainers.
>>
>> Signed-off-by: William Zhang <[email protected]>
>> ---
>>
>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 84 +++++++++++++++++--
>> 1 file changed, 78 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>> index 45f1417b1213..56e69d4a1faf 100644
>> --- a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>> @@ -4,22 +4,51 @@
>> $id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
>> $schema: http://devicetree.org/meta-schemas/core.yaml#
>>
>> -title: Broadcom BCM6328 High Speed SPI controller
>> +title: Broadcom Broadband SoC High Speed SPI controller
>>
>> maintainers:
>> +
>
> Drop blank line.
will fix in v2.
>
>> + - William Zhang <[email protected]>
>> + - Kursad Oney <[email protected]>
>> - Jonas Gorski <[email protected]>
>
>>
>> +description: |
>> + Broadcom Broadband SoC supports High Speed SPI master controller since the
>> + early MIPS based chips such as BCM6328 and BCM63268. This controller was
>> + carried over to recent ARM based chips, such as BCM63138, BCM4908 and BCM6858.
>> +
>> + It has a limitation that can not keep the chip select line active between
>> + the SPI transfers within the same SPI message. This can terminate the
>> + transaction to some SPI devices prematurely. The issue can be worked around by
>> + either the controller's prepend mode or using the dummy chip select
>> + workaround. This controller uses the compatible string brcm,bcm6328-hsspi.
>> +
>> + The newer SoCs such as BCM6756, BCM4912 and BCM6855 include an updated SPI
>> + controller that add the capability to allow the driver to control chip select
>> + explicitly. This solves the issue in the old controller. This new controller
>> + uses the compatible string brcm,bcmbca-hsspi.
>> +
>> properties:
>> compatible:
>> - const: brcm,bcm6328-hsspi
>> + enum:
>> + - brcm,bcm6328-hsspi
>> + - brcm,bcmbca-hsspi
>
> bca seems quite unspecific. Your description above mentions several
> model numbers and "bca" is not listed as model. Compatibles cannot be
> generic.
"bca" is not model number, rather it is a group (broadband carrier
access) of chip that share the same spi host controller IP. Agree it is
not particularly specific but it differentiate from other broadcom spi
controller ip used by other groups. We just don't have a specific name
for this spi host controller but can we treat bcmbca as the ip name?
Otherwise we will have to have a compatible string with chip model for
each SoC even they share the same IP. We already have more than ten of
SoCs and the list will increase. I don't see this is a good solution too.
>
>>
>> reg:
>> - maxItems: 1
>> + items:
>> + - description: main registers
>> + - description: miscellaneous control registers
>> + minItems: 1
>> +
>> + reg-names:
>> + items:
>> + - const: hsspi
>> + - const: spim-ctrl
>
> This does not match reg
Do you mean it does not match the description?
>
>>
>> clocks:
>> items:
>> - - description: spi master reference clock
>> - - description: spi master pll clock
>> + - description: SPI master reference clock
>> + - description: SPI master pll clock
>
> Really? You just added it in previous patch, didn't you?
The previous patch was just word to word conversion of the text file. I
will update that patch to include this change.
>
>>
>> clock-names:
>> items:
>> @@ -29,12 +58,43 @@ properties:
>> interrupts:
>> maxItems: 1
>>
>> + brcm,use-cs-workaround:
>> + $ref: /schemas/types.yaml#/definitions/flag
>> + description: |
>> + Enable dummy chip select workaround for SPI transfers that can not be
>> + supported by the default controller's prepend mode, i.e. delay or cs
>> + change needed between SPI transfers.
>
> You need to describe what is the workaround.
Will do.
>
>> +
>> required:
>> - compatible
>> - reg
>> - clocks
>> - clock-names
>> - - interrupts
>> +
>> +allOf:
>> + - $ref: "spi-controller.yaml#"
>
> No quotes. How this is related to this patch?
Will remove quote and put it in patch 1.
>
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - brcm,bcm6328-hsspi
>> + then:
>> + properties:
>> + reg:
>> + minItems: 1
>
> Drop.
>
> reg-names now do not match.
Don't quite understand your comment. What do I need to drop and what is
not matched?
>
>> + maxItems: 1
>> + else:
>> + properties:
>> + reg:
>> + minItems: 2
>> + maxItems: 2
>> + reg-names:
>> + minItems: 2
>> + maxItems: 2
>> + brcm,use-cs-workaround: false
>> + required:
>> + - reg-names
> Best regards,
> Krzysztof
>
On 01/08/2023 06:52 AM, Krzysztof Kozlowski wrote:
> On 06/01/2023 21:07, William Zhang wrote:
>> brcm,no-clk-gate is a Broadcom Broadband HS SPI controller specific
>> property for certain SPI device such as Broadcom ISI voice daughtercard
>> to work properly. It disables the clock gating feature when the chip
>> select is deasserted for any device that wants to keep the clock
>> running.
>
>
>> +additionalProperties: true
>> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>> index ead2cccf658f..f85d777c7b67 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>> @@ -108,5 +108,6 @@ allOf:
>> - $ref: cdns,qspi-nor-peripheral-props.yaml#
>> - $ref: samsung,spi-peripheral-props.yaml#
>> - $ref: nvidia,tegra210-quad-peripheral-props.yaml#
>> + - $ref: brcm,bcm63xx-hsspi-peripheral-props.yaml#
>
> Don't break the order.
>
Will fix in v2
> Best regards,
> Krzysztof
>
On 09/01/2023 08:52, William Zhang wrote:
>
>
> On 01/07/2023 07:32 AM, Krzysztof Kozlowski wrote:
>> On 06/01/2023 21:07, William Zhang wrote:
>>> This is the preparation for updates on the bcm63xx hsspi driver. Convert
>>> the text based bindings to json-schema per new dts requirement.
>>>
>>> Signed-off-by: William Zhang <[email protected]>
>>> ---
>>>
>>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 52 +++++++++++++++++++
>>> .../bindings/spi/spi-bcm63xx-hsspi.txt | 33 ------------
>>> 2 files changed, 52 insertions(+), 33 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>> delete mode 100644 Documentation/devicetree/bindings/spi/spi-bcm63xx-hsspi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>> new file mode 100644
>>> index 000000000000..45f1417b1213
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>> @@ -0,0 +1,52 @@
>>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Broadcom BCM6328 High Speed SPI controller
>>> +
>>> +maintainers:
>>> + - Jonas Gorski <[email protected]>
>>> +
>>
>> Missing reference to spi-controller.
>>
> This was word to word conversion from the text file. But I will update
> with this required reference.
>
>>> +properties:
>>> + compatible:
>>> + const: brcm,bcm6328-hsspi
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + clocks:
>>> + items:
>>> + - description: spi master reference clock
>>> + - description: spi master pll clock
>>> +
>>> + clock-names:
>>> + items:
>>> + - const: hsspi
>>> + - const: pll
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - clocks
>>> + - clock-names
>>> + - interrupts
>>> +
>>> +unevaluatedProperties: false
>>
>> This is for cases when you have reference to other schema.
>>
> Will drop here. But will add back in patch 1 which produces the final
> version of this file and need this property.
When you add reference to spi-controller, keep it here. This was wrong
when the reference was missing.
Best regards,
Krzysztof
On 09/01/2023 09:27, William Zhang wrote:
> Hi Krzysztof,
>
> On 01/08/2023 06:51 AM, Krzysztof Kozlowski wrote:
>> On 06/01/2023 21:07, William Zhang wrote:
>>> The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI
>>> controller. Add a new compatible string and required fields for the new
>>> driver. Also add myself and Kursad as the maintainers.
>>>
>>> Signed-off-by: William Zhang <[email protected]>
>>> ---
>>>
>>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 84 +++++++++++++++++--
>>> 1 file changed, 78 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>> index 45f1417b1213..56e69d4a1faf 100644
>>> --- a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>> @@ -4,22 +4,51 @@
>>> $id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
>>> $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>
>>> -title: Broadcom BCM6328 High Speed SPI controller
>>> +title: Broadcom Broadband SoC High Speed SPI controller
>>>
>>> maintainers:
>>> +
>>
>> Drop blank line.
> will fix in v2.
>
>>
>>> + - William Zhang <[email protected]>
>>> + - Kursad Oney <[email protected]>
>>> - Jonas Gorski <[email protected]>
>>
>>>
>>> +description: |
>>> + Broadcom Broadband SoC supports High Speed SPI master controller since the
>>> + early MIPS based chips such as BCM6328 and BCM63268. This controller was
>>> + carried over to recent ARM based chips, such as BCM63138, BCM4908 and BCM6858.
>>> +
>>> + It has a limitation that can not keep the chip select line active between
>>> + the SPI transfers within the same SPI message. This can terminate the
>>> + transaction to some SPI devices prematurely. The issue can be worked around by
>>> + either the controller's prepend mode or using the dummy chip select
>>> + workaround. This controller uses the compatible string brcm,bcm6328-hsspi.
>>> +
>>> + The newer SoCs such as BCM6756, BCM4912 and BCM6855 include an updated SPI
>>> + controller that add the capability to allow the driver to control chip select
>>> + explicitly. This solves the issue in the old controller. This new controller
>>> + uses the compatible string brcm,bcmbca-hsspi.
>>> +
>>> properties:
>>> compatible:
>>> - const: brcm,bcm6328-hsspi
>>> + enum:
>>> + - brcm,bcm6328-hsspi
>>> + - brcm,bcmbca-hsspi
>>
>> bca seems quite unspecific. Your description above mentions several
>> model numbers and "bca" is not listed as model. Compatibles cannot be
>> generic.
> "bca" is not model number, rather it is a group (broadband carrier
> access) of chip that share the same spi host controller IP. Agree it is
> not particularly specific but it differentiate from other broadcom spi
> controller ip used by other groups. We just don't have a specific name
> for this spi host controller but can we treat bcmbca as the ip name?
No, it is discouraged in such forms. Family or IP block compatibles
should be prepended with a specific compatible. There were many issues
when people insisted on generic or family compatibles...
> Otherwise we will have to have a compatible string with chip model for
> each SoC even they share the same IP. We already have more than ten of
> SoCs and the list will increase. I don't see this is a good solution too.
You will have to do it anyway even with generic fallback, so I don't get
what is here to gain... I also don't get why Broadcom should be here
special, different than others. Why it is not a good solution for
Broadcom SoCs but it is for others?
>
>>
>>>
>>> reg:
>>> - maxItems: 1
>>> + items:
>>> + - description: main registers
>>> + - description: miscellaneous control registers
>>> + minItems: 1
>>> +
>>> + reg-names:
>>> + items:
>>> + - const: hsspi
>>> + - const: spim-ctrl
>>
>> This does not match reg
> Do you mean it does not match the description?
No. reg can be 1 item but you state reg-names cannot. These are always
the same. If one is 1 item, second is as well.
>>
>>>
>>> clocks:
>>> items:
>>> - - description: spi master reference clock
>>> - - description: spi master pll clock
>>> + - description: SPI master reference clock
>>> + - description: SPI master pll clock
>>
>> Really? You just added it in previous patch, didn't you?
> The previous patch was just word to word conversion of the text file. I
> will update that patch to include this change.
>
>>
>>>
>>> clock-names:
>>> items:
>>> @@ -29,12 +58,43 @@ properties:
>>> interrupts:
>>> maxItems: 1
>>>
>>> + brcm,use-cs-workaround:
>>> + $ref: /schemas/types.yaml#/definitions/flag
>>> + description: |
>>> + Enable dummy chip select workaround for SPI transfers that can not be
>>> + supported by the default controller's prepend mode, i.e. delay or cs
>>> + change needed between SPI transfers.
>>
>> You need to describe what is the workaround.
> Will do.
>>
>>> +
>>> required:
>>> - compatible
>>> - reg
>>> - clocks
>>> - clock-names
>>> - - interrupts
>>> +
>>> +allOf:
>>> + - $ref: "spi-controller.yaml#"
>>
>> No quotes. How this is related to this patch?
> Will remove quote and put it in patch 1.
>>
>>> + - if:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + enum:
>>> + - brcm,bcm6328-hsspi
>>> + then:
>>> + properties:
>>> + reg:
>>> + minItems: 1
>>
>> Drop.
>>
>> reg-names now do not match.
> Don't quite understand your comment. What do I need to drop and what is
> not matched?
You need to add constraints for reg-names, same way as for reg.
Disallowing the reg-names also could work, but there won't be benefit in
it. Better to have uniform DTS.
>
Best regards,
Krzysztof
On 01/09/2023 12:56 AM, Krzysztof Kozlowski wrote:
> On 09/01/2023 09:27, William Zhang wrote:
>> Hi Krzysztof,
>>
>> On 01/08/2023 06:51 AM, Krzysztof Kozlowski wrote:
>>> On 06/01/2023 21:07, William Zhang wrote:
>>>> The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI
>>>> controller. Add a new compatible string and required fields for the new
>>>> driver. Also add myself and Kursad as the maintainers.
>>>>
>>>> Signed-off-by: William Zhang <[email protected]>
>>>> ---
>>>>
>>>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 84 +++++++++++++++++--
>>>> 1 file changed, 78 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>> index 45f1417b1213..56e69d4a1faf 100644
>>>> --- a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>> @@ -4,22 +4,51 @@
>>>> $id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
>>>> $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>
>>>> -title: Broadcom BCM6328 High Speed SPI controller
>>>> +title: Broadcom Broadband SoC High Speed SPI controller
>>>>
>>>> maintainers:
>>>> +
>>>
>>> Drop blank line.
>> will fix in v2.
>>
>>>
>>>> + - William Zhang <[email protected]>
>>>> + - Kursad Oney <[email protected]>
>>>> - Jonas Gorski <[email protected]>
>>>
>>>>
>>>> +description: |
>>>> + Broadcom Broadband SoC supports High Speed SPI master controller since the
>>>> + early MIPS based chips such as BCM6328 and BCM63268. This controller was
>>>> + carried over to recent ARM based chips, such as BCM63138, BCM4908 and BCM6858.
>>>> +
>>>> + It has a limitation that can not keep the chip select line active between
>>>> + the SPI transfers within the same SPI message. This can terminate the
>>>> + transaction to some SPI devices prematurely. The issue can be worked around by
>>>> + either the controller's prepend mode or using the dummy chip select
>>>> + workaround. This controller uses the compatible string brcm,bcm6328-hsspi.
>>>> +
>>>> + The newer SoCs such as BCM6756, BCM4912 and BCM6855 include an updated SPI
>>>> + controller that add the capability to allow the driver to control chip select
>>>> + explicitly. This solves the issue in the old controller. This new controller
>>>> + uses the compatible string brcm,bcmbca-hsspi.
>>>> +
>>>> properties:
>>>> compatible:
>>>> - const: brcm,bcm6328-hsspi
>>>> + enum:
>>>> + - brcm,bcm6328-hsspi
>>>> + - brcm,bcmbca-hsspi
>>>
>>> bca seems quite unspecific. Your description above mentions several
>>> model numbers and "bca" is not listed as model. Compatibles cannot be
>>> generic.
>> "bca" is not model number, rather it is a group (broadband carrier
>> access) of chip that share the same spi host controller IP. Agree it is
>> not particularly specific but it differentiate from other broadcom spi
>> controller ip used by other groups. We just don't have a specific name
>> for this spi host controller but can we treat bcmbca as the ip name?
>
> No, it is discouraged in such forms. Family or IP block compatibles
> should be prepended with a specific compatible. There were many issues
> when people insisted on generic or family compatibles...
>
>> Otherwise we will have to have a compatible string with chip model for
>> each SoC even they share the same IP. We already have more than ten of
>> SoCs and the list will increase. I don't see this is a good solution too.
>
> You will have to do it anyway even with generic fallback, so I don't get
> what is here to gain... I also don't get why Broadcom should be here
> special, different than others. Why it is not a good solution for
> Broadcom SoCs but it is for others?
>
I saw a few other vendors like these qcom ones:
qcom,spi-qup.yaml
- qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
- qcom,spi-qup-v2.1.1 # for 8974 and later
- qcom,spi-qup-v2.2.1 # for 8974 v2 and later
qcom,spi-qup.yaml
const: qcom,geni-spi
I guess when individual who only has one particular board/chip and is
not aware of the IP family, it is understandable to use the chip
specific compatible string. But when company works on it, we have the
visibility and access to all the chips and ip blocks in the family and
IMHO it is very reasonable to use the IP family name for the same IP as
these examples shows. Are you saying these are not good example to
follow? What are the issues with generic or family compatibles? Could
you please elaborate?
>
>
>>
>>>
>>>>
>>>> reg:
>>>> - maxItems: 1
>>>> + items:
>>>> + - description: main registers
>>>> + - description: miscellaneous control registers
>>>> + minItems: 1
>>>> +
>>>> + reg-names:
>>>> + items:
>>>> + - const: hsspi
>>>> + - const: spim-ctrl
>>>
>>> This does not match reg
>> Do you mean it does not match the description?
>
> No. reg can be 1 item but you state reg-names cannot. These are always
> the same. If one is 1 item, second is as well.
>
I'll drop the "minItems: 1" from the reg property then.
>>>
>>>>
>>>> clocks:
>>>> items:
>>>> - - description: spi master reference clock
>>>> - - description: spi master pll clock
>>>> + - description: SPI master reference clock
>>>> + - description: SPI master pll clock
>>>
>>> Really? You just added it in previous patch, didn't you?
>> The previous patch was just word to word conversion of the text file. I
>> will update that patch to include this change.
>>
>>>
>>>>
>>>> clock-names:
>>>> items:
>>>> @@ -29,12 +58,43 @@ properties:
>>>> interrupts:
>>>> maxItems: 1
>>>>
>>>> + brcm,use-cs-workaround:
>>>> + $ref: /schemas/types.yaml#/definitions/flag
>>>> + description: |
>>>> + Enable dummy chip select workaround for SPI transfers that can not be
>>>> + supported by the default controller's prepend mode, i.e. delay or cs
>>>> + change needed between SPI transfers.
>>>
>>> You need to describe what is the workaround.
>> Will do.
>>>
>>>> +
>>>> required:
>>>> - compatible
>>>> - reg
>>>> - clocks
>>>> - clock-names
>>>> - - interrupts
>>>> +
>>>> +allOf:
>>>> + - $ref: "spi-controller.yaml#"
>>>
>>> No quotes. How this is related to this patch?
>> Will remove quote and put it in patch 1.
>>>
>>>> + - if:
>>>> + properties:
>>>> + compatible:
>>>> + contains:
>>>> + enum:
>>>> + - brcm,bcm6328-hsspi
>>>> + then:
>>>> + properties:
>>>> + reg:
>>>> + minItems: 1
>>>
>>> Drop.
>>>
>>> reg-names now do not match.
>> Don't quite understand your comment. What do I need to drop and what is
>> not matched?
>
> You need to add constraints for reg-names, same way as for reg.
> Disallowing the reg-names also could work, but there won't be benefit in
> it. Better to have uniform DTS.
>
I agree it is better to have the uniform DTS but the situation here is
that the brcm,bcm6328-hsspi does not require reg name since there is
only one register needed and it was already used in many chip dts for
long time. If I enforce it to have the corresponding reg name, that
could potentially break the compatibility of those old device if the
driver change to use reg name, right?
>>
> Best regards,
> Krzysztof
>
On Mon, Jan 09, 2023 at 12:06:13AM -0800, William Zhang wrote:
> > On Fri, Jan 6, 2023 at 9:27 PM William Zhang <[email protected]> wrote:
> > Keeping the clock on or not would affect all devices unless you have a
> > per device clock you can gate, so making this a per device flag
> > doesn't make sense.
> This applies only to each chip select. There is only one device under each
> chip select. So won't impact any other devices under other cs.
I don't understand how this would work - usually a SPI controller has a
single set of clock, MOSI and MISO lines with the only per device thing
being the chip select. If the clock line is used by all devices then it
must be kept on for all of them if it's to be kept on for one of them.
On Fri, Jan 06, 2023 at 07:52:35PM -0800, William Zhang wrote:
> On 01/06/2023 02:00 PM, Mark Brown wrote:
> > On Fri, Jan 06, 2023 at 12:08:03PM -0800, William Zhang wrote:
> > > Multiple transfers within a SPI message may be combined into one
> > > transfer to the controller using its prepend feature. A SPI message is
> > > prependable only if the following are all true:
> > > * One or more half duplex write transfer
> > > * Optional full duplex read/write at the end
> > > * No delay and cs_change between transfers
> > There is nothing driver specific here, this should be implemented in the
> > core - we have existing logic to rewrite messages to match driver
> > constraints, this could be added there possibly with flags to allow
> > drivers to disable or enable the merging if they've got special
> > requirements.
> My understanding of combining the spi transfer in the core level does not
> quite work out to our controller. For example, for a spi message with three
> transfers, tx, tx and rx. We can possibly combine them in single duplex
> tx/rx transfer in the core. But this will be treated as duplex transaction
> in our controller level which require tx and rx data happens at the same
> time. Obviously this won't work when rx depends on tx happening first. We
I'm saying that if this logic is useful then implement in the core
rather than in the driver.
> can not differentiate this combined duplex transfer from the true duplex
> transfer unless there is some flag to indicate that. Also there is limit of
> max tx length as the prepend buffer so maybe another parameter. And another
> reason to be done in the driver level is this prepend mode has dependency on
> dummy cs workaround which is driver level parameter currently. I am not
> sure how practical and useful this is to factor them out to the core level?
If this relies on software control of the chip select (which is what I
*think* your dummy CS workaround thing is about, the other patch about
that is really hard to understand) then I'm confused about what the
advantage is?
On Fri, Jan 06, 2023 at 07:35:59PM -0800, William Zhang wrote:
> On 01/06/2023 01:47 PM, Mark Brown wrote:
> > On Fri, Jan 06, 2023 at 12:07:59PM -0800, William Zhang wrote:
> > > When interrupt is not defined in the HSSPI dts node, driver switches to
> > > polling mode for controller SPI message processing. Also add driver
> > > banner message when the driver is loaded successfully.
> > This should not be something the user selects via the DT, if the polling
> > mode is better then the driver should just use it regardless of there
> > being an interrupt wired up. Generally there's some point at which the
> > benefits of polling become minimal (and the costs more impactful) but if
> > the DMA setup is as bad as it sounds then the driver should just ignore
> > the interrupt.
> I agree but this is more for backward compatibility as the original driver
> uses interrupt to work with MIPS based SoC and I do not want to change that
> behavior. For newer devices I added, they work in polling mode by default
> with the dts I supplied. IMHO it is also good to have this fallback option
> to use interrupt in case user is sensitive to cpu usage.
You can put whatever logic is needed in the code - for something like
this an architecture based define isn't ideal but is probably good
enough if need be (though I'd not be surprised if it turned out that
there was also some performance benefit for the MIPS systems too, at
least for smaller transfers).
On 01/09/2023 11:06 AM, Mark Brown wrote:
> On Fri, Jan 06, 2023 at 07:35:59PM -0800, William Zhang wrote:
>> On 01/06/2023 01:47 PM, Mark Brown wrote:
>>> On Fri, Jan 06, 2023 at 12:07:59PM -0800, William Zhang wrote:
>
>>>> When interrupt is not defined in the HSSPI dts node, driver switches to
>>>> polling mode for controller SPI message processing. Also add driver
>>>> banner message when the driver is loaded successfully.
>
>>> This should not be something the user selects via the DT, if the polling
>>> mode is better then the driver should just use it regardless of there
>>> being an interrupt wired up. Generally there's some point at which the
>>> benefits of polling become minimal (and the costs more impactful) but if
>>> the DMA setup is as bad as it sounds then the driver should just ignore
>>> the interrupt.
>
>> I agree but this is more for backward compatibility as the original driver
>> uses interrupt to work with MIPS based SoC and I do not want to change that
>> behavior. For newer devices I added, they work in polling mode by default
>> with the dts I supplied. IMHO it is also good to have this fallback option
>> to use interrupt in case user is sensitive to cpu usage.
>
> You can put whatever logic is needed in the code - for something like
> this an architecture based define isn't ideal but is probably good
> enough if need be (though I'd not be surprised if it turned out that
> there was also some performance benefit for the MIPS systems too, at
> least for smaller transfers).
>
I just don't know what other logic I can put in the driver to select
interrupt or polling mode. Only the end user know if performance or cpu
usage is more important to their application.
Hi Mark,
On 01/09/2023 11:19 AM, Mark Brown wrote:
> On Mon, Jan 09, 2023 at 12:06:13AM -0800, William Zhang wrote:
>>> On Fri, Jan 6, 2023 at 9:27 PM William Zhang <[email protected]> wrote:
>
>>> Keeping the clock on or not would affect all devices unless you have a
>>> per device clock you can gate, so making this a per device flag
>>> doesn't make sense.
>
>> This applies only to each chip select. There is only one device under each
>> chip select. So won't impact any other devices under other cs.
>
> I don't understand how this would work - usually a SPI controller has a
> single set of clock, MOSI and MISO lines with the only per device thing
> being the chip select. If the clock line is used by all devices then it
> must be kept on for all of them if it's to be kept on for one of them.
>
This setting is set per spi message for particular chip select of the
device when starting the message through bcm63xx_hsspi_set_clk function
and restore to default(clock gating) when message is done through
bcm63xx_hsspi_restore_clk_gate.
On 01/09/2023 11:31 AM, Mark Brown wrote:
> On Fri, Jan 06, 2023 at 07:52:35PM -0800, William Zhang wrote:
>> On 01/06/2023 02:00 PM, Mark Brown wrote:
>>> On Fri, Jan 06, 2023 at 12:08:03PM -0800, William Zhang wrote:
>
>>>> Multiple transfers within a SPI message may be combined into one
>>>> transfer to the controller using its prepend feature. A SPI message is
>>>> prependable only if the following are all true:
>>>> * One or more half duplex write transfer
>>>> * Optional full duplex read/write at the end
>>>> * No delay and cs_change between transfers
>
>>> There is nothing driver specific here, this should be implemented in the
>>> core - we have existing logic to rewrite messages to match driver
>>> constraints, this could be added there possibly with flags to allow
>>> drivers to disable or enable the merging if they've got special
>>> requirements.
>
>> My understanding of combining the spi transfer in the core level does not
>> quite work out to our controller. For example, for a spi message with three
>> transfers, tx, tx and rx. We can possibly combine them in single duplex
>> tx/rx transfer in the core. But this will be treated as duplex transaction
>> in our controller level which require tx and rx data happens at the same
>> time. Obviously this won't work when rx depends on tx happening first. We
>
> I'm saying that if this logic is useful then implement in the core
> rather than in the driver.
>
>> can not differentiate this combined duplex transfer from the true duplex
>> transfer unless there is some flag to indicate that. Also there is limit of
>> max tx length as the prepend buffer so maybe another parameter. And another
>> reason to be done in the driver level is this prepend mode has dependency on
>> dummy cs workaround which is driver level parameter currently. I am not
>> sure how practical and useful this is to factor them out to the core level?
>
> If this relies on software control of the chip select (which is what I
> *think* your dummy CS workaround thing is about, the other patch about
> that is really hard to understand) then I'm confused about what the
> advantage is?
Dummy CS workaround is implemented by Jonas when he first upstream the
driver. It does not work on all the board designs so prepend mode is
introduced. I have some detail explanation on this in [PATCH 10/16] spi:
bcm63xx-hsspi: Make dummy cs workaround as an option.
The controller only work in one mode and that's why driver code has some
dependency between these two modes. The advantage of the premode is it
works on all hw design however it does not support all types mem_ops
operation. That is why you see the patch 14 to disable the dual io mem
op. But dummy cs workaround can support this and in case there is such
pattern from non mem op spi transaction, dummy cs workaround can be used
as long as it does not have the board design limitation. So neither
one is perfect but hopefully with both options available, we can cover
all the cases.
You mentioned there is some existing logic to rewrite messages to match
driver constraints in the core driver. I didn't see it when I did a
quick search on spi.c. I will take a deep look into the file. But if you
can point me where this logic is so I can be sure that I am looking at
the right place and will double check if this can be done or not in the
core level. Thanks!
>
On 09/01/2023 20:13, William Zhang wrote:
>
>
> On 01/09/2023 12:56 AM, Krzysztof Kozlowski wrote:
>> On 09/01/2023 09:27, William Zhang wrote:
>>> Hi Krzysztof,
>>>
>>> On 01/08/2023 06:51 AM, Krzysztof Kozlowski wrote:
>>>> On 06/01/2023 21:07, William Zhang wrote:
>>>>> The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI
>>>>> controller. Add a new compatible string and required fields for the new
>>>>> driver. Also add myself and Kursad as the maintainers.
>>>>>
>>>>> Signed-off-by: William Zhang <[email protected]>
>>>>> ---
>>>>>
>>>>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 84 +++++++++++++++++--
>>>>> 1 file changed, 78 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>>> index 45f1417b1213..56e69d4a1faf 100644
>>>>> --- a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>>> @@ -4,22 +4,51 @@
>>>>> $id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
>>>>> $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>
>>>>> -title: Broadcom BCM6328 High Speed SPI controller
>>>>> +title: Broadcom Broadband SoC High Speed SPI controller
>>>>>
>>>>> maintainers:
>>>>> +
>>>>
>>>> Drop blank line.
>>> will fix in v2.
>>>
>>>>
>>>>> + - William Zhang <[email protected]>
>>>>> + - Kursad Oney <[email protected]>
>>>>> - Jonas Gorski <[email protected]>
>>>>
>>>>>
>>>>> +description: |
>>>>> + Broadcom Broadband SoC supports High Speed SPI master controller since the
>>>>> + early MIPS based chips such as BCM6328 and BCM63268. This controller was
>>>>> + carried over to recent ARM based chips, such as BCM63138, BCM4908 and BCM6858.
>>>>> +
>>>>> + It has a limitation that can not keep the chip select line active between
>>>>> + the SPI transfers within the same SPI message. This can terminate the
>>>>> + transaction to some SPI devices prematurely. The issue can be worked around by
>>>>> + either the controller's prepend mode or using the dummy chip select
>>>>> + workaround. This controller uses the compatible string brcm,bcm6328-hsspi.
>>>>> +
>>>>> + The newer SoCs such as BCM6756, BCM4912 and BCM6855 include an updated SPI
>>>>> + controller that add the capability to allow the driver to control chip select
>>>>> + explicitly. This solves the issue in the old controller. This new controller
>>>>> + uses the compatible string brcm,bcmbca-hsspi.
>>>>> +
>>>>> properties:
>>>>> compatible:
>>>>> - const: brcm,bcm6328-hsspi
>>>>> + enum:
>>>>> + - brcm,bcm6328-hsspi
>>>>> + - brcm,bcmbca-hsspi
>>>>
>>>> bca seems quite unspecific. Your description above mentions several
>>>> model numbers and "bca" is not listed as model. Compatibles cannot be
>>>> generic.
>>> "bca" is not model number, rather it is a group (broadband carrier
>>> access) of chip that share the same spi host controller IP. Agree it is
>>> not particularly specific but it differentiate from other broadcom spi
>>> controller ip used by other groups. We just don't have a specific name
>>> for this spi host controller but can we treat bcmbca as the ip name?
>>
>> No, it is discouraged in such forms. Family or IP block compatibles
>> should be prepended with a specific compatible. There were many issues
>> when people insisted on generic or family compatibles...
>>
>>> Otherwise we will have to have a compatible string with chip model for
>>> each SoC even they share the same IP. We already have more than ten of
>>> SoCs and the list will increase. I don't see this is a good solution too.
>>
>> You will have to do it anyway even with generic fallback, so I don't get
>> what is here to gain... I also don't get why Broadcom should be here
>> special, different than others. Why it is not a good solution for
>> Broadcom SoCs but it is for others?
>>
> I saw a few other vendors like these qcom ones:
> qcom,spi-qup.yaml
> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
> - qcom,spi-qup-v2.1.1 # for 8974 and later
> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
> qcom,spi-qup.yaml
> const: qcom,geni-spi
IP block version numbers are allowed when there is clear mapping between
version and SoCs using it. This is the case for Qualcomm because there
is such clear mapping documented and available for Qualcomm engineers
and also some of us (although not public).
> I guess when individual who only has one particular board/chip and is
> not aware of the IP family, it is understandable to use the chip
> specific compatible string.
Family of devices is not a versioned IP block.
> But when company works on it, we have the
> visibility and access to all the chips and ip blocks in the family and
> IMHO it is very reasonable to use the IP family name for the same IP as
> these examples shows.
No, because family of devices is not a versioned IP block. I wrote
before that families and wildcards are not allowed.
> Are you saying these are not good example to
> follow?
It's nothing related to your case.
> What are the issues with generic or family compatibles?
> Could
> you please elaborate?
They stop matching and some point and cause ABI breaks. We had several
cases where engineer believed "I have here family of devices" and then
later it turned out that the family is different.
>
>>
>>
>>>
>>>>
>>>>>
>>>>> reg:
>>>>> - maxItems: 1
>>>>> + items:
>>>>> + - description: main registers
>>>>> + - description: miscellaneous control registers
>>>>> + minItems: 1
>>>>> +
>>>>> + reg-names:
>>>>> + items:
>>>>> + - const: hsspi
>>>>> + - const: spim-ctrl
>>>>
>>>> This does not match reg
>>> Do you mean it does not match the description?
>>
>> No. reg can be 1 item but you state reg-names cannot. These are always
>> the same. If one is 1 item, second is as well.
>>
> I'll drop the "minItems: 1" from the reg property then.
Then it won't be correct, because it would mean two items are required
always.
>
>>>>
>>>>>
>>>>> clocks:
>>>>> items:
>>>>> - - description: spi master reference clock
>>>>> - - description: spi master pll clock
>>>>> + - description: SPI master reference clock
>>>>> + - description: SPI master pll clock
>>>>
>>>> Really? You just added it in previous patch, didn't you?
>>> The previous patch was just word to word conversion of the text file. I
>>> will update that patch to include this change.
>>>
>>>>
>>>>>
>>>>> clock-names:
>>>>> items:
>>>>> @@ -29,12 +58,43 @@ properties:
>>>>> interrupts:
>>>>> maxItems: 1
>>>>>
>>>>> + brcm,use-cs-workaround:
>>>>> + $ref: /schemas/types.yaml#/definitions/flag
>>>>> + description: |
>>>>> + Enable dummy chip select workaround for SPI transfers that can not be
>>>>> + supported by the default controller's prepend mode, i.e. delay or cs
>>>>> + change needed between SPI transfers.
>>>>
>>>> You need to describe what is the workaround.
>>> Will do.
>>>>
>>>>> +
>>>>> required:
>>>>> - compatible
>>>>> - reg
>>>>> - clocks
>>>>> - clock-names
>>>>> - - interrupts
>>>>> +
>>>>> +allOf:
>>>>> + - $ref: "spi-controller.yaml#"
>>>>
>>>> No quotes. How this is related to this patch?
>>> Will remove quote and put it in patch 1.
>>>>
>>>>> + - if:
>>>>> + properties:
>>>>> + compatible:
>>>>> + contains:
>>>>> + enum:
>>>>> + - brcm,bcm6328-hsspi
>>>>> + then:
>>>>> + properties:
>>>>> + reg:
>>>>> + minItems: 1
>>>>
>>>> Drop.
>>>>
>>>> reg-names now do not match.
>>> Don't quite understand your comment. What do I need to drop and what is
>>> not matched?
>>
>> You need to add constraints for reg-names, same way as for reg.
>> Disallowing the reg-names also could work, but there won't be benefit in
>> it. Better to have uniform DTS.
>>
> I agree it is better to have the uniform DTS but the situation here is
> that the brcm,bcm6328-hsspi does not require reg name since there is
> only one register needed and it was already used in many chip dts for
> long time. If I enforce it to have the corresponding reg name, that
No one told you to enforce to have a reg-names.
> could potentially break the compatibility of those old device if the
> driver change to use reg name, right?
How compatibility is broken by some optional, unrelated property?
Best regards,
Krzysztof
On Mon, Jan 09, 2023 at 12:43:53PM -0800, William Zhang wrote:
> On 01/09/2023 11:31 AM, Mark Brown wrote:
> > If this relies on software control of the chip select (which is what I
> > *think* your dummy CS workaround thing is about, the other patch about
> > that is really hard to understand) then I'm confused about what the
> > advantage is?
> Dummy CS workaround is implemented by Jonas when he first upstream the
> driver. It does not work on all the board designs so prepend mode is
> introduced. I have some detail explanation on this in [PATCH 10/16] spi:
> bcm63xx-hsspi: Make dummy cs workaround as an option.
Yes, it is the description in patch 10 that I was having a lot of
trouble following.
> The controller only work in one mode and that's why driver code has some
> dependency between these two modes. The advantage of the premode is it works
> on all hw design however it does not support all types mem_ops operation.
> That is why you see the patch 14 to disable the dual io mem op. But dummy cs
> workaround can support this and in case there is such pattern from non mem
> op spi transaction, dummy cs workaround can be used as long as it does not
> have the board design limitation. So neither one is perfect but hopefully
> with both options available, we can cover all the cases.
We can't switch modes per message?
> You mentioned there is some existing logic to rewrite messages to match
> driver constraints in the core driver. I didn't see it when I did a quick
> search on spi.c. I will take a deep look into the file. But if you can point
> me where this logic is so I can be sure that I am looking at the right place
> and will double check if this can be done or not in the core level. Thanks!
spi_replace_transfers().
On Mon, Jan 09, 2023 at 12:18:09PM -0800, William Zhang wrote:
> This setting is set per spi message for particular chip select of the device
> when starting the message through bcm63xx_hsspi_set_clk function and restore
> to default(clock gating) when message is done through
> bcm63xx_hsspi_restore_clk_gate.
In that case I am extremely confused about what the feature is supposed
to do. The description says:
+ brcm,no-clk-gate:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ Some SPI device such as Broadcom ISI based voice daughtercard requires
+SPI
+ clock running even when chip select is deasserted. By default the
+ controller turns off or gate the clock when cs is not active to save
+ power. This flag tells the controller driver to keep the clock running
+ when chip select is not active.
which to me sounds like the clock should never be turned off and instead
left running at all times. Switching back to clock gating after sending
the message doesn't seem to correspond to the above at all, the message
being done would normally also be the point at which chip select is
deasserted.
On 1/10/23 00:40, Krzysztof Kozlowski wrote:
>>> No, it is discouraged in such forms. Family or IP block compatibles
>>> should be prepended with a specific compatible. There were many issues
>>> when people insisted on generic or family compatibles...
>>>
>>>> Otherwise we will have to have a compatible string with chip model for
>>>> each SoC even they share the same IP. We already have more than ten of
>>>> SoCs and the list will increase. I don't see this is a good solution too.
>>>
>>> You will have to do it anyway even with generic fallback, so I don't get
>>> what is here to gain... I also don't get why Broadcom should be here
>>> special, different than others. Why it is not a good solution for
>>> Broadcom SoCs but it is for others?
>>>
>> I saw a few other vendors like these qcom ones:
>> qcom,spi-qup.yaml
>> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>> - qcom,spi-qup-v2.1.1 # for 8974 and later
>> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>> qcom,spi-qup.yaml
>> const: qcom,geni-spi
>
> IP block version numbers are allowed when there is clear mapping between
> version and SoCs using it. This is the case for Qualcomm because there
> is such clear mapping documented and available for Qualcomm engineers
> and also some of us (although not public).
>
>> I guess when individual who only has one particular board/chip and is
>> not aware of the IP family, it is understandable to use the chip
>> specific compatible string.
>
> Family of devices is not a versioned IP block.
Would it be acceptable to define for instance:
- compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";
in which case, having a fallback compatible on the SoC family that sees
this IP being deployed is very useful for client programs of the DT
(u-boot or kernel). As long as the fallback works, we use it, the day it
stops and a quirk needs to be applied because SoC XYZ has a bug, match
the SoC XYZ compatible string.
FWIW, and feel free to rant at me, we have adopted this convention a
while ago for STB chips whereby we want bindings to be defined with:
<chip specific compatible>, <version of the IP>, <fallback>
and the fallback may, or may not be matched, but defining in does not
hurt at all, in fact it dramatically helps with the boot loader looking
for specific nodes because it can search for the fallback.
If the version specific compatible is not available, it does not get used.
--
Florian
On Mon, Jan 09, 2023 at 12:10:30PM -0800, William Zhang wrote:
> On 01/09/2023 11:06 AM, Mark Brown wrote:
> > You can put whatever logic is needed in the code - for something like
> > this an architecture based define isn't ideal but is probably good
> > enough if need be (though I'd not be surprised if it turned out that
> > there was also some performance benefit for the MIPS systems too, at
> > least for smaller transfers).
> I just don't know what other logic I can put in the driver to select
> interrupt or polling mode. Only the end user know if performance or cpu
> usage is more important to their application.
Usually you can take a reasonable guess as to what would be a good point
to start switching, typically for short enough transfers the overhead of
setting up DMA, waiting for interrupts and tearing things down is very
much larger than the cost of just doing PIO - a bunch of other drivers
have pick a number logic of some kind, often things like FIFO sizes are
a good key for where to look. A lot of the time this is good enough,
and it means that users have much better facilities for making tradeoffs
if they have a range of transfer sizes available - it's not an either/or
thing but based on some features of the individual message/transfer.
It is true that for people with heavy SPI traffic or otherwise very
demanding requirements for a specific system and software stack
additional tuning might produce better results, exposing some sysfs
knobs to allow tuning of parameters at runtime would be helpful for them
and I'd certainly be happy to see that added.
On 01/10/2023 12:40 AM, Krzysztof Kozlowski wrote:
> On 09/01/2023 20:13, William Zhang wrote:
>>
>>
>> On 01/09/2023 12:56 AM, Krzysztof Kozlowski wrote:
>>> On 09/01/2023 09:27, William Zhang wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 01/08/2023 06:51 AM, Krzysztof Kozlowski wrote:
>>>>> On 06/01/2023 21:07, William Zhang wrote:
>>>>>> The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI
>>>>>> controller. Add a new compatible string and required fields for the new
>>>>>> driver. Also add myself and Kursad as the maintainers.
>>>>>>
>>>>>> Signed-off-by: William Zhang <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 84 +++++++++++++++++--
>>>>>> 1 file changed, 78 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>>>> index 45f1417b1213..56e69d4a1faf 100644
>>>>>> --- a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml
>>>>>> @@ -4,22 +4,51 @@
>>>>>> $id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml#
>>>>>> $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>
>>>>>> -title: Broadcom BCM6328 High Speed SPI controller
>>>>>> +title: Broadcom Broadband SoC High Speed SPI controller
>>>>>>
>>>>>> maintainers:
>>>>>> +
>>>>>
>>>>> Drop blank line.
>>>> will fix in v2.
>>>>
>>>>>
>>>>>> + - William Zhang <[email protected]>
>>>>>> + - Kursad Oney <[email protected]>
>>>>>> - Jonas Gorski <[email protected]>
>>>>>
>>>>>>
>>>>>> +description: |
>>>>>> + Broadcom Broadband SoC supports High Speed SPI master controller since the
>>>>>> + early MIPS based chips such as BCM6328 and BCM63268. This controller was
>>>>>> + carried over to recent ARM based chips, such as BCM63138, BCM4908 and BCM6858.
>>>>>> +
>>>>>> + It has a limitation that can not keep the chip select line active between
>>>>>> + the SPI transfers within the same SPI message. This can terminate the
>>>>>> + transaction to some SPI devices prematurely. The issue can be worked around by
>>>>>> + either the controller's prepend mode or using the dummy chip select
>>>>>> + workaround. This controller uses the compatible string brcm,bcm6328-hsspi.
>>>>>> +
>>>>>> + The newer SoCs such as BCM6756, BCM4912 and BCM6855 include an updated SPI
>>>>>> + controller that add the capability to allow the driver to control chip select
>>>>>> + explicitly. This solves the issue in the old controller. This new controller
>>>>>> + uses the compatible string brcm,bcmbca-hsspi.
>>>>>> +
>>>>>> properties:
>>>>>> compatible:
>>>>>> - const: brcm,bcm6328-hsspi
>>>>>> + enum:
>>>>>> + - brcm,bcm6328-hsspi
>>>>>> + - brcm,bcmbca-hsspi
>>>>>
>>>>> bca seems quite unspecific. Your description above mentions several
>>>>> model numbers and "bca" is not listed as model. Compatibles cannot be
>>>>> generic.
>>>> "bca" is not model number, rather it is a group (broadband carrier
>>>> access) of chip that share the same spi host controller IP. Agree it is
>>>> not particularly specific but it differentiate from other broadcom spi
>>>> controller ip used by other groups. We just don't have a specific name
>>>> for this spi host controller but can we treat bcmbca as the ip name?
>>>
>>> No, it is discouraged in such forms. Family or IP block compatibles
>>> should be prepended with a specific compatible. There were many issues
>>> when people insisted on generic or family compatibles...
>>>
>>>> Otherwise we will have to have a compatible string with chip model for
>>>> each SoC even they share the same IP. We already have more than ten of
>>>> SoCs and the list will increase. I don't see this is a good solution too.
>>>
>>> You will have to do it anyway even with generic fallback, so I don't get
>>> what is here to gain... I also don't get why Broadcom should be here
>>> special, different than others. Why it is not a good solution for
>>> Broadcom SoCs but it is for others?
>>>
>> I saw a few other vendors like these qcom ones:
>> qcom,spi-qup.yaml
>> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>> - qcom,spi-qup-v2.1.1 # for 8974 and later
>> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>> qcom,spi-qup.yaml
>> const: qcom,geni-spi
>
> IP block version numbers are allowed when there is clear mapping between
> version and SoCs using it. This is the case for Qualcomm because there
> is such clear mapping documented and available for Qualcomm engineers
> and also some of us (although not public).
>
>> I guess when individual who only has one particular board/chip and is
>> not aware of the IP family, it is understandable to use the chip
>> specific compatible string.
>
> Family of devices is not a versioned IP block.
>
>> But when company works on it, we have the
>> visibility and access to all the chips and ip blocks in the family and
>> IMHO it is very reasonable to use the IP family name for the same IP as
>> these examples shows.
>
> No, because family of devices is not a versioned IP block. I wrote
> before that families and wildcards are not allowed.
>
>> Are you saying these are not good example to
>> follow?
>
> It's nothing related to your case.
>
>> What are the issues with generic or family compatibles?
>> Could
>> you please elaborate?
>
> They stop matching and some point and cause ABI breaks. We had several
> cases where engineer believed "I have here family of devices" and then
> later it turned out that the family is different.
>
>
>>
>>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> reg:
>>>>>> - maxItems: 1
>>>>>> + items:
>>>>>> + - description: main registers
>>>>>> + - description: miscellaneous control registers
>>>>>> + minItems: 1
>>>>>> +
>>>>>> + reg-names:
>>>>>> + items:
>>>>>> + - const: hsspi
>>>>>> + - const: spim-ctrl
>>>>>
>>>>> This does not match reg
>>>> Do you mean it does not match the description?
>>>
>>> No. reg can be 1 item but you state reg-names cannot. These are always
>>> the same. If one is 1 item, second is as well.
>>>
>> I'll drop the "minItems: 1" from the reg property then.
>
> Then it won't be correct, because it would mean two items are required
> always.
Ah you are right. Add minItems: 1 for reg-name then.
>
>>
>>>>>
>>>>>>
>>>>>> clocks:
>>>>>> items:
>>>>>> - - description: spi master reference clock
>>>>>> - - description: spi master pll clock
>>>>>> + - description: SPI master reference clock
>>>>>> + - description: SPI master pll clock
>>>>>
>>>>> Really? You just added it in previous patch, didn't you?
>>>> The previous patch was just word to word conversion of the text file. I
>>>> will update that patch to include this change.
>>>>
>>>>>
>>>>>>
>>>>>> clock-names:
>>>>>> items:
>>>>>> @@ -29,12 +58,43 @@ properties:
>>>>>> interrupts:
>>>>>> maxItems: 1
>>>>>>
>>>>>> + brcm,use-cs-workaround:
>>>>>> + $ref: /schemas/types.yaml#/definitions/flag
>>>>>> + description: |
>>>>>> + Enable dummy chip select workaround for SPI transfers that can not be
>>>>>> + supported by the default controller's prepend mode, i.e. delay or cs
>>>>>> + change needed between SPI transfers.
>>>>>
>>>>> You need to describe what is the workaround.
>>>> Will do.
>>>>>
>>>>>> +
>>>>>> required:
>>>>>> - compatible
>>>>>> - reg
>>>>>> - clocks
>>>>>> - clock-names
>>>>>> - - interrupts
>>>>>> +
>>>>>> +allOf:
>>>>>> + - $ref: "spi-controller.yaml#"
>>>>>
>>>>> No quotes. How this is related to this patch?
>>>> Will remove quote and put it in patch 1.
>>>>>
>>>>>> + - if:
>>>>>> + properties:
>>>>>> + compatible:
>>>>>> + contains:
>>>>>> + enum:
>>>>>> + - brcm,bcm6328-hsspi
>>>>>> + then:
>>>>>> + properties:
>>>>>> + reg:
>>>>>> + minItems: 1
>>>>>
>>>>> Drop.
>>>>>
>>>>> reg-names now do not match.
>>>> Don't quite understand your comment. What do I need to drop and what is
>>>> not matched?
>>>
>>> You need to add constraints for reg-names, same way as for reg.
>>> Disallowing the reg-names also could work, but there won't be benefit in
>>> it. Better to have uniform DTS.
>>>
>> I agree it is better to have the uniform DTS but the situation here is
>> that the brcm,bcm6328-hsspi does not require reg name since there is
>> only one register needed and it was already used in many chip dts for
>> long time. If I enforce it to have the corresponding reg name, that
>
> No one told you to enforce to have a reg-names.
>
>> could potentially break the compatibility of those old device if the
>> driver change to use reg name, right?
>
> How compatibility is broken by some optional, unrelated property?
>
I think I misunderstand what you said. You basically want the reg-name
minItem/maxItem constraints for brcm,bcm6328-hsspi compatible as well so
it is consistent for all the compatibles? I was confused and thought it
is not needed as reg-name is not required for brcm,bcm6328-hsspi compatible.
> Best regards,
> Krzysztof
>
On 01/10/2023 02:18 PM, Florian Fainelli wrote:
> On 1/10/23 00:40, Krzysztof Kozlowski wrote:
>>>> No, it is discouraged in such forms. Family or IP block compatibles
>>>> should be prepended with a specific compatible. There were many issues
>>>> when people insisted on generic or family compatibles...
>>>>
>>>>> Otherwise we will have to have a compatible string with chip model for
>>>>> each SoC even they share the same IP. We already have more than ten of
>>>>> SoCs and the list will increase. I don't see this is a good
>>>>> solution too.
>>>>
>>>> You will have to do it anyway even with generic fallback, so I don't
>>>> get
>>>> what is here to gain... I also don't get why Broadcom should be here
>>>> special, different than others. Why it is not a good solution for
>>>> Broadcom SoCs but it is for others?
>>>>
>>> I saw a few other vendors like these qcom ones:
>>> Â Â qcom,spi-qup.yaml
>>> Â Â Â Â Â Â - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>>> Â Â Â Â Â Â - qcom,spi-qup-v2.1.1 # for 8974 and later
>>> Â Â Â Â Â Â - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>>> Â Â qcom,spi-qup.yaml
>>> Â Â Â Â Â Â const: qcom,geni-spi
>>
>> IP block version numbers are allowed when there is clear mapping between
>> version and SoCs using it. This is the case for Qualcomm because there
>> is such clear mapping documented and available for Qualcomm engineers
>> and also some of us (although not public).
>>
>>> I guess when individual who only has one particular board/chip and is
>>> not aware of the IP family, it is understandable to use the chip
>>> specific compatible string.
>>
>> Family of devices is not a versioned IP block.
>
> Would it be acceptable to define for instance:
>
> - compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";
>
> in which case, having a fallback compatible on the SoC family that sees
> this IP being deployed is very useful for client programs of the DT
> (u-boot or kernel). As long as the fallback works, we use it, the day it
> stops and a quirk needs to be applied because SoC XYZ has a bug, match
> the SoC XYZ compatible string.
>
> FWIW, and feel free to rant at me, we have adopted this convention a
> while ago for STB chips whereby we want bindings to be defined with:
>
> <chip specific compatible>, <version of the IP>, <fallback>
>
> and the fallback may, or may not be matched, but defining in does not
> hurt at all, in fact it dramatically helps with the boot loader looking
> for specific nodes because it can search for the fallback.
>
> If the version specific compatible is not available, it does not get used.
Thanks Florian for jumping in! I was thinking to propose something with
version info:
brcm,bcmbca-hsspi-v1.0
brcm,bcmbca-hsspi-v1.1
To meet STB chip convention, then it would be:
compatible = "brcm,bcm63138-hsspi", "brcm,bcmbca-hsspi-v1.0",
"brcm,bcmbca-hsspi";
compatible = "brcm,bcm6756-hsspi", "brcm,bcmbca-hsspi-v1.1",
"brcm,bcmbca-hsspi";
Although I am not a fan of having a chip specific compatible while we
already have IP version, I am okay to have it to be consistent with
Broadcom convention. We will need to remember to update this yaml file
whenever we have a new chip.
Hi William,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on broonie-spi/for-next krzk/for-next krzk-dt/for-next krzk-mem-ctrl/for-next linus/master v6.2-rc3 next-20230111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230106200809.330769-7-william.zhang%40broadcom.com
patch subject: [PATCH 06/16] spi: bcm63xx-hsspi: Endianness fix for ARM based SoC
config: arc-randconfig-s032-20230110
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/8a82a44b50d88a27e55065e9f1bd75f0fccf479a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review William-Zhang/dt-bindings-spi-Convert-bcm63xx-hsspi-bindings-to-json-schema/20230107-042320
git checkout 8a82a44b50d88a27e55065e9f1bd75f0fccf479a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arc SHELL=/bin/bash drivers/spi/ sound/soc/codecs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned short [usertype] s @@ got restricted __be16 [usertype] @@
drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: expected unsigned short [usertype] s
drivers/spi/spi-bcm63xx-hsspi.c:197:30: sparse: got restricted __be16 [usertype]
vim +197 drivers/spi/spi-bcm63xx-hsspi.c
156
157 static int bcm63xx_hsspi_do_txrx(struct spi_device *spi, struct spi_transfer *t)
158 {
159 struct bcm63xx_hsspi *bs = spi_master_get_devdata(spi->master);
160 unsigned int chip_select = spi->chip_select;
161 u16 opcode = 0;
162 int pending = t->len;
163 int step_size = HSSPI_BUFFER_LEN;
164 const u8 *tx = t->tx_buf;
165 u8 *rx = t->rx_buf;
166
167 bcm63xx_hsspi_set_clk(bs, spi, t->speed_hz);
168 bcm63xx_hsspi_set_cs(bs, spi->chip_select, true);
169
170 if (tx && rx)
171 opcode = HSSPI_OP_READ_WRITE;
172 else if (tx)
173 opcode = HSSPI_OP_WRITE;
174 else if (rx)
175 opcode = HSSPI_OP_READ;
176
177 if (opcode != HSSPI_OP_READ)
178 step_size -= HSSPI_OPCODE_LEN;
179
180 if ((opcode == HSSPI_OP_READ && t->rx_nbits == SPI_NBITS_DUAL) ||
181 (opcode == HSSPI_OP_WRITE && t->tx_nbits == SPI_NBITS_DUAL))
182 opcode |= HSSPI_OP_MULTIBIT;
183
184 __raw_writel(1 << MODE_CTRL_MULTIDATA_WR_SIZE_SHIFT |
185 1 << MODE_CTRL_MULTIDATA_RD_SIZE_SHIFT | 0xff,
186 bs->regs + HSSPI_PROFILE_MODE_CTRL_REG(chip_select));
187
188 while (pending > 0) {
189 int curr_step = min_t(int, step_size, pending);
190
191 reinit_completion(&bs->done);
192 if (tx) {
193 memcpy_toio(bs->fifo + HSSPI_OPCODE_LEN, tx, curr_step);
194 tx += curr_step;
195 }
196
> 197 __raw_writew(cpu_to_be16(opcode | curr_step), bs->fifo);
198
199 /* enable interrupt */
200 __raw_writel(HSSPI_PINGx_CMD_DONE(0),
201 bs->regs + HSSPI_INT_MASK_REG);
202
203 /* start the transfer */
204 __raw_writel(!chip_select << PINGPONG_CMD_SS_SHIFT |
205 chip_select << PINGPONG_CMD_PROFILE_SHIFT |
206 PINGPONG_COMMAND_START_NOW,
207 bs->regs + HSSPI_PINGPONG_COMMAND_REG(0));
208
209 if (wait_for_completion_timeout(&bs->done, HZ) == 0) {
210 dev_err(&bs->pdev->dev, "transfer timed out!\n");
211 return -ETIMEDOUT;
212 }
213
214 if (rx) {
215 memcpy_fromio(rx, bs->fifo, curr_step);
216 rx += curr_step;
217 }
218
219 pending -= curr_step;
220 }
221
222 return 0;
223 }
224
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 10/01/2023 23:18, Florian Fainelli wrote:
> On 1/10/23 00:40, Krzysztof Kozlowski wrote:
>>>> No, it is discouraged in such forms. Family or IP block compatibles
>>>> should be prepended with a specific compatible. There were many issues
>>>> when people insisted on generic or family compatibles...
>>>>
>>>>> Otherwise we will have to have a compatible string with chip model for
>>>>> each SoC even they share the same IP. We already have more than ten of
>>>>> SoCs and the list will increase. I don't see this is a good solution too.
>>>>
>>>> You will have to do it anyway even with generic fallback, so I don't get
>>>> what is here to gain... I also don't get why Broadcom should be here
>>>> special, different than others. Why it is not a good solution for
>>>> Broadcom SoCs but it is for others?
>>>>
>>> I saw a few other vendors like these qcom ones:
>>> qcom,spi-qup.yaml
>>> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>>> - qcom,spi-qup-v2.1.1 # for 8974 and later
>>> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>>> qcom,spi-qup.yaml
>>> const: qcom,geni-spi
>>
>> IP block version numbers are allowed when there is clear mapping between
>> version and SoCs using it. This is the case for Qualcomm because there
>> is such clear mapping documented and available for Qualcomm engineers
>> and also some of us (although not public).
>>
>>> I guess when individual who only has one particular board/chip and is
>>> not aware of the IP family, it is understandable to use the chip
>>> specific compatible string.
>>
>> Family of devices is not a versioned IP block.
>
> Would it be acceptable to define for instance:
>
> - compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";
Yes, this is perfectly valid. Although it does not solve William
concerns because it requires defining specific compatibles for all of
the SoCs.
Best regards,
Krzysztof
On 11/01/2023 01:59, William Zhang wrote:
>>>> You need to add constraints for reg-names, same way as for reg.
>>>> Disallowing the reg-names also could work, but there won't be benefit in
>>>> it. Better to have uniform DTS.
>>>>
>>> I agree it is better to have the uniform DTS but the situation here is
>>> that the brcm,bcm6328-hsspi does not require reg name since there is
>>> only one register needed and it was already used in many chip dts for
>>> long time. If I enforce it to have the corresponding reg name, that
>>
>> No one told you to enforce to have a reg-names.
>>
>>> could potentially break the compatibility of those old device if the
>>> driver change to use reg name, right?
>>
>> How compatibility is broken by some optional, unrelated property?
>>
> I think I misunderstand what you said. You basically want the reg-name
> minItem/maxItem constraints for brcm,bcm6328-hsspi compatible as well so
> it is consistent for all the compatibles?
Yes.
Best regards,
Krzysztof
On 01/11/2023 01:02 AM, Krzysztof Kozlowski wrote:
> On 10/01/2023 23:18, Florian Fainelli wrote:
>> On 1/10/23 00:40, Krzysztof Kozlowski wrote:
>>>>> No, it is discouraged in such forms. Family or IP block compatibles
>>>>> should be prepended with a specific compatible. There were many issues
>>>>> when people insisted on generic or family compatibles...
>>>>>
>>>>>> Otherwise we will have to have a compatible string with chip model for
>>>>>> each SoC even they share the same IP. We already have more than ten of
>>>>>> SoCs and the list will increase. I don't see this is a good solution too.
>>>>>
>>>>> You will have to do it anyway even with generic fallback, so I don't get
>>>>> what is here to gain... I also don't get why Broadcom should be here
>>>>> special, different than others. Why it is not a good solution for
>>>>> Broadcom SoCs but it is for others?
>>>>>
>>>> I saw a few other vendors like these qcom ones:
>>>> qcom,spi-qup.yaml
>>>> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>>>> - qcom,spi-qup-v2.1.1 # for 8974 and later
>>>> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>>>> qcom,spi-qup.yaml
>>>> const: qcom,geni-spi
>>>
>>> IP block version numbers are allowed when there is clear mapping between
>>> version and SoCs using it. This is the case for Qualcomm because there
>>> is such clear mapping documented and available for Qualcomm engineers
>>> and also some of us (although not public).
>>>
>>>> I guess when individual who only has one particular board/chip and is
>>>> not aware of the IP family, it is understandable to use the chip
>>>> specific compatible string.
>>>
>>> Family of devices is not a versioned IP block.
>>
>> Would it be acceptable to define for instance:
>>
>> - compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";
>
> Yes, this is perfectly valid. Although it does not solve William
> concerns because it requires defining specific compatibles for all of
> the SoCs.
>
> Best regards,
> Krzysztof
>
As I mentioned in another email, I would be okay to use these
compatibles to differentiate by ip rev and to conforms to brcm convention:
"brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.0", "brcm,bcmbca-hsspi";
"brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.1", "brcm,bcmbca-hsspi";
In the two drivers I included in this series, it will be bound to
brcm,bcmbca-hsspi-v1.0 (in additional to brcm,bcm6328-hsspi) and
brcm,bcmbca-hsspi-v1.1 respectively. This way we don't need to update
the driver with a new soc specific compatible whenever a new chips comes
out.
Does this sound good to you?
On 11/01/2023 19:04, William Zhang wrote:
>
>
> On 01/11/2023 01:02 AM, Krzysztof Kozlowski wrote:
>> On 10/01/2023 23:18, Florian Fainelli wrote:
>>> On 1/10/23 00:40, Krzysztof Kozlowski wrote:
>>>>>> No, it is discouraged in such forms. Family or IP block compatibles
>>>>>> should be prepended with a specific compatible. There were many issues
>>>>>> when people insisted on generic or family compatibles...
>>>>>>
>>>>>>> Otherwise we will have to have a compatible string with chip model for
>>>>>>> each SoC even they share the same IP. We already have more than ten of
>>>>>>> SoCs and the list will increase. I don't see this is a good solution too.
>>>>>>
>>>>>> You will have to do it anyway even with generic fallback, so I don't get
>>>>>> what is here to gain... I also don't get why Broadcom should be here
>>>>>> special, different than others. Why it is not a good solution for
>>>>>> Broadcom SoCs but it is for others?
>>>>>>
>>>>> I saw a few other vendors like these qcom ones:
>>>>> qcom,spi-qup.yaml
>>>>> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>>>>> - qcom,spi-qup-v2.1.1 # for 8974 and later
>>>>> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>>>>> qcom,spi-qup.yaml
>>>>> const: qcom,geni-spi
>>>>
>>>> IP block version numbers are allowed when there is clear mapping between
>>>> version and SoCs using it. This is the case for Qualcomm because there
>>>> is such clear mapping documented and available for Qualcomm engineers
>>>> and also some of us (although not public).
>>>>
>>>>> I guess when individual who only has one particular board/chip and is
>>>>> not aware of the IP family, it is understandable to use the chip
>>>>> specific compatible string.
>>>>
>>>> Family of devices is not a versioned IP block.
>>>
>>> Would it be acceptable to define for instance:
>>>
>>> - compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";
>>
>> Yes, this is perfectly valid. Although it does not solve William
>> concerns because it requires defining specific compatibles for all of
>> the SoCs.
>>
>> Best regards,
>> Krzysztof
>>
> As I mentioned in another email, I would be okay to use these
> compatibles to differentiate by ip rev and to conforms to brcm convention:
> "brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.0", "brcm,bcmbca-hsspi";
> "brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.1", "brcm,bcmbca-hsspi";
Drop the version in such case, no benefits. I assume XYZ is the SoC
model, so for example 6868.
>
> In the two drivers I included in this series, it will be bound to
> brcm,bcmbca-hsspi-v1.0 (in additional to brcm,bcm6328-hsspi) and
> brcm,bcmbca-hsspi-v1.1 respectively. This way we don't need to update
> the driver with a new soc specific compatible whenever a new chips comes
> out.
I don't understand why do you bring it now as an argument. You defined
before that your driver will bind to the generic bcmbca compatible, so
now it is not enough?
Best regards,
Krzysztof
On 01/11/2023 10:12 AM, Krzysztof Kozlowski wrote:
> On 11/01/2023 19:04, William Zhang wrote:
>>
>>
>> On 01/11/2023 01:02 AM, Krzysztof Kozlowski wrote:
>>> On 10/01/2023 23:18, Florian Fainelli wrote:
>>>> On 1/10/23 00:40, Krzysztof Kozlowski wrote:
>>>>>>> No, it is discouraged in such forms. Family or IP block compatibles
>>>>>>> should be prepended with a specific compatible. There were many issues
>>>>>>> when people insisted on generic or family compatibles...
>>>>>>>
>>>>>>>> Otherwise we will have to have a compatible string with chip model for
>>>>>>>> each SoC even they share the same IP. We already have more than ten of
>>>>>>>> SoCs and the list will increase. I don't see this is a good solution too.
>>>>>>>
>>>>>>> You will have to do it anyway even with generic fallback, so I don't get
>>>>>>> what is here to gain... I also don't get why Broadcom should be here
>>>>>>> special, different than others. Why it is not a good solution for
>>>>>>> Broadcom SoCs but it is for others?
>>>>>>>
>>>>>> I saw a few other vendors like these qcom ones:
>>>>>> qcom,spi-qup.yaml
>>>>>> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>>>>>> - qcom,spi-qup-v2.1.1 # for 8974 and later
>>>>>> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>>>>>> qcom,spi-qup.yaml
>>>>>> const: qcom,geni-spi
>>>>>
>>>>> IP block version numbers are allowed when there is clear mapping between
>>>>> version and SoCs using it. This is the case for Qualcomm because there
>>>>> is such clear mapping documented and available for Qualcomm engineers
>>>>> and also some of us (although not public).
>>>>>
>>>>>> I guess when individual who only has one particular board/chip and is
>>>>>> not aware of the IP family, it is understandable to use the chip
>>>>>> specific compatible string.
>>>>>
>>>>> Family of devices is not a versioned IP block.
>>>>
>>>> Would it be acceptable to define for instance:
>>>>
>>>> - compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";
>>>
>>> Yes, this is perfectly valid. Although it does not solve William
>>> concerns because it requires defining specific compatibles for all of
>>> the SoCs.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> As I mentioned in another email, I would be okay to use these
>> compatibles to differentiate by ip rev and to conforms to brcm convention:
>> "brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.0", "brcm,bcmbca-hsspi";
>> "brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.1", "brcm,bcmbca-hsspi";
>
>
> Drop the version in such case, no benefits. I assume XYZ is the SoC
> model, so for example 6868.
>
Yes XYZ is the SoC model
>>
>> In the two drivers I included in this series, it will be bound to
>> brcm,bcmbca-hsspi-v1.0 (in additional to brcm,bcm6328-hsspi) and
>> brcm,bcmbca-hsspi-v1.1 respectively. This way we don't need to update
>> the driver with a new soc specific compatible whenever a new chips comes
>> out.
>
> I don't understand why do you bring it now as an argument. You defined
> before that your driver will bind to the generic bcmbca compatible, so
> now it is not enough?
>
No as we are adding chip model specific info here. The existing driver
spi-bcm63xx-hsspi.c only binds to brcm,bcm6328-hsspi. This driver
supports all the chips with rev1.0 controller so I am using this 6328
string for other chips with v1.0 in the dts patch, which is not ideal.
Now I have to add more compatible to this driver and for each new chip
with 1.0 in the future if any.
With all the thoughts from you and Florian, I think it is better to use
rev compatible in the driver but add on chip model compatible in the dts.
> Best regards,
> Krzysztof
>
On 01/10/2023 02:01 PM, Mark Brown wrote:
> On Mon, Jan 09, 2023 at 12:18:09PM -0800, William Zhang wrote:
>
>> This setting is set per spi message for particular chip select of the device
>> when starting the message through bcm63xx_hsspi_set_clk function and restore
>> to default(clock gating) when message is done through
>> bcm63xx_hsspi_restore_clk_gate.
>
> In that case I am extremely confused about what the feature is supposed
> to do. The description says:
>
> + brcm,no-clk-gate:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description:
> + Some SPI device such as Broadcom ISI based voice daughtercard requires
> +SPI
> + clock running even when chip select is deasserted. By default the
> + controller turns off or gate the clock when cs is not active to save
> + power. This flag tells the controller driver to keep the clock running
> + when chip select is not active.
>
>
> which to me sounds like the clock should never be turned off and instead
> left running at all times. Switching back to clock gating after sending
> the message doesn't seem to correspond to the above at all, the message
> being done would normally also be the point at which chip select is
> deasserted.
>
This feature is used by our voice team and as far I can tell, it is used
to keep clock running between the transfers within the same message.
But now that we have prepend mode to combine to one transfer or dummy
workaround to keep cs always active between transfers, this indeed does
not seems right. I will have to talk to the voice team why this is
still needed and get back here.
On 01/10/2023 02:49 PM, Mark Brown wrote:
> On Mon, Jan 09, 2023 at 12:10:30PM -0800, William Zhang wrote:
>> On 01/09/2023 11:06 AM, Mark Brown wrote:
>
>>> You can put whatever logic is needed in the code - for something like
>>> this an architecture based define isn't ideal but is probably good
>>> enough if need be (though I'd not be surprised if it turned out that
>>> there was also some performance benefit for the MIPS systems too, at
>>> least for smaller transfers).
>
>> I just don't know what other logic I can put in the driver to select
>> interrupt or polling mode. Only the end user know if performance or cpu
>> usage is more important to their application.
>
> Usually you can take a reasonable guess as to what would be a good point
> to start switching, typically for short enough transfers the overhead of
> setting up DMA, waiting for interrupts and tearing things down is very
> much larger than the cost of just doing PIO - a bunch of other drivers
> have pick a number logic of some kind, often things like FIFO sizes are
> a good key for where to look. A lot of the time this is good enough,
> and it means that users have much better facilities for making tradeoffs
> if they have a range of transfer sizes available - it's not an either/or
> thing but based on some features of the individual message/transfer.
>
> It is true that for people with heavy SPI traffic or otherwise very
> demanding requirements for a specific system and software stack
> additional tuning might produce better results, exposing some sysfs
> knobs to allow tuning of parameters at runtime would be helpful for them
> and I'd certainly be happy to see that added.
>
Thanks for the explanation. I saw the spi-uniphier.c and spi-bcm2835.c
doing the trick you mentioned(thanks Kursad for pointing out). In our
case, even the maximum fifo size usage(512bytes), the polling still have
better performance than interrupt. The MTD test result included in this
patch is based on maximum fifo usage. So there is no benefit to switch
to interrupt based on transfer size.
Does the spi framework has any requirement on how much time that the
driver's transfer_one function can spend? I can see the polling
function might take quite some time in busy loop if the clock is low,
for example, at 100Hz(slowest clock this controller can go), it takes
512x8/100Hz ~= 41ms to complete. If this is something in concern, I
can do the interrupt switch based on a time limit value if interrupt is
available.
On 01/10/2023 01:18 PM, Mark Brown wrote:
> On Mon, Jan 09, 2023 at 12:43:53PM -0800, William Zhang wrote:
>> On 01/09/2023 11:31 AM, Mark Brown wrote:
>
>>> If this relies on software control of the chip select (which is what I
>>> *think* your dummy CS workaround thing is about, the other patch about
>>> that is really hard to understand) then I'm confused about what the
>>> advantage is?
>
>> Dummy CS workaround is implemented by Jonas when he first upstream the
>> driver. It does not work on all the board designs so prepend mode is
>> introduced. I have some detail explanation on this in [PATCH 10/16] spi:
>> bcm63xx-hsspi: Make dummy cs workaround as an option.
>
> Yes, it is the description in patch 10 that I was having a lot of
> trouble following.
>
Sorry that my description is not clear... I can certainly improve it if
you can let me know what is not clear.
>> The controller only work in one mode and that's why driver code has some
>> dependency between these two modes. The advantage of the premode is it works
>> on all hw design however it does not support all types mem_ops operation.
>> That is why you see the patch 14 to disable the dual io mem op. But dummy cs
>> workaround can support this and in case there is such pattern from non mem
>> op spi transaction, dummy cs workaround can be used as long as it does not
>> have the board design limitation. So neither one is perfect but hopefully
>> with both options available, we can cover all the cases.
>
> We can't switch modes per message?
>
Technically yes. If the code finds the message is not prependable, it
can try to use dummy cs workaround to transfer the message but it may
also fail if the board design does not work with this workaround. I can
add this if you think this is good to have.
>> You mentioned there is some existing logic to rewrite messages to match
>> driver constraints in the core driver. I didn't see it when I did a quick
>> search on spi.c. I will take a deep look into the file. But if you can point
>> me where this logic is so I can be sure that I am looking at the right place
>> and will double check if this can be done or not in the core level. Thanks!
>
> spi_replace_transfers().
>
Okay I saw this function is used by spi_split_transfers_maxsize which a
few drivers use to limit the transfer size and it make sense. I can
come up something like spi_merge_transfers to be used by my driver's
prepend function. But it has the same issue I mentioned early as the
these tx, rx transfers have the dependency on the order they present in
the original transfer list for my prepend function to work. And for the
same reason, it won't be generally useful for other drivers.
On Wed, Jan 11, 2023 at 12:13:43PM -0800, William Zhang wrote:
> Thanks for the explanation. I saw the spi-uniphier.c and spi-bcm2835.c doing
> the trick you mentioned(thanks Kursad for pointing out). In our case, even
> the maximum fifo size usage(512bytes), the polling still have better
> performance than interrupt. The MTD test result included in this patch is
> based on maximum fifo usage. So there is no benefit to switch to interrupt
> based on transfer size.
> Does the spi framework has any requirement on how much time that the
> driver's transfer_one function can spend? I can see the polling function
> might take quite some time in busy loop if the clock is low, for example, at
> 100Hz(slowest clock this controller can go), it takes 512x8/100Hz ~= 41ms to
> complete. If this is something in concern, I can do the interrupt switch
> based on a time limit value if interrupt is available.
There's no fixed limit, some hardware just doesn't have DMA. Some
doesn't even have interrupts which is even better. If there's always a
clear benefit for using PIO then just do that, perhaps creating a sysfs
hook to allow people to switch to DMA if they need it (rather than
requiring people to update their DT, this is really a runtime
performance tradeoff rather than a description of the hardware). If
there's a point at which the performance is very similar then it's
probably worth switching to DMA there to lower the CPU usage, but if
it's always faster to use PIO then just defaulting to PIO seems like the
best option.
On 01/11/2023 02:41 PM, Mark Brown wrote:
> On Wed, Jan 11, 2023 at 12:13:43PM -0800, William Zhang wrote:
>
>> Thanks for the explanation. I saw the spi-uniphier.c and spi-bcm2835.c doing
>> the trick you mentioned(thanks Kursad for pointing out). In our case, even
>> the maximum fifo size usage(512bytes), the polling still have better
>> performance than interrupt. The MTD test result included in this patch is
>> based on maximum fifo usage. So there is no benefit to switch to interrupt
>> based on transfer size.
>
>> Does the spi framework has any requirement on how much time that the
>> driver's transfer_one function can spend? I can see the polling function
>> might take quite some time in busy loop if the clock is low, for example, at
>> 100Hz(slowest clock this controller can go), it takes 512x8/100Hz ~= 41ms to
>> complete. If this is something in concern, I can do the interrupt switch
>> based on a time limit value if interrupt is available.
>
> There's no fixed limit, some hardware just doesn't have DMA. Some
> doesn't even have interrupts which is even better. If there's always a
> clear benefit for using PIO then just do that, perhaps creating a sysfs
> hook to allow people to switch to DMA if they need it (rather than
> requiring people to update their DT, this is really a runtime
> performance tradeoff rather than a description of the hardware). If
> there's a point at which the performance is very similar then it's
> probably worth switching to DMA there to lower the CPU usage, but if
> it's always faster to use PIO then just defaulting to PIO seems like the
> best option.
>
Sure. I will add the sysfs option. Interrupt node is now required in dts
but by default it will still runs at polling mode until sysfs option is
set to use interrupt.
On 11/01/2023 19:44, William Zhang wrote:
>
>
> On 01/11/2023 10:12 AM, Krzysztof Kozlowski wrote:
>> On 11/01/2023 19:04, William Zhang wrote:
>>>
>>>
>>> On 01/11/2023 01:02 AM, Krzysztof Kozlowski wrote:
>>>> On 10/01/2023 23:18, Florian Fainelli wrote:
>>>>> On 1/10/23 00:40, Krzysztof Kozlowski wrote:
>>>>>>>> No, it is discouraged in such forms. Family or IP block compatibles
>>>>>>>> should be prepended with a specific compatible. There were many issues
>>>>>>>> when people insisted on generic or family compatibles...
>>>>>>>>
>>>>>>>>> Otherwise we will have to have a compatible string with chip model for
>>>>>>>>> each SoC even they share the same IP. We already have more than ten of
>>>>>>>>> SoCs and the list will increase. I don't see this is a good solution too.
>>>>>>>>
>>>>>>>> You will have to do it anyway even with generic fallback, so I don't get
>>>>>>>> what is here to gain... I also don't get why Broadcom should be here
>>>>>>>> special, different than others. Why it is not a good solution for
>>>>>>>> Broadcom SoCs but it is for others?
>>>>>>>>
>>>>>>> I saw a few other vendors like these qcom ones:
>>>>>>> qcom,spi-qup.yaml
>>>>>>> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>>>>>>> - qcom,spi-qup-v2.1.1 # for 8974 and later
>>>>>>> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>>>>>>> qcom,spi-qup.yaml
>>>>>>> const: qcom,geni-spi
>>>>>>
>>>>>> IP block version numbers are allowed when there is clear mapping between
>>>>>> version and SoCs using it. This is the case for Qualcomm because there
>>>>>> is such clear mapping documented and available for Qualcomm engineers
>>>>>> and also some of us (although not public).
>>>>>>
>>>>>>> I guess when individual who only has one particular board/chip and is
>>>>>>> not aware of the IP family, it is understandable to use the chip
>>>>>>> specific compatible string.
>>>>>>
>>>>>> Family of devices is not a versioned IP block.
>>>>>
>>>>> Would it be acceptable to define for instance:
>>>>>
>>>>> - compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";
>>>>
>>>> Yes, this is perfectly valid. Although it does not solve William
>>>> concerns because it requires defining specific compatibles for all of
>>>> the SoCs.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>> As I mentioned in another email, I would be okay to use these
>>> compatibles to differentiate by ip rev and to conforms to brcm convention:
>>> "brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.0", "brcm,bcmbca-hsspi";
>>> "brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.1", "brcm,bcmbca-hsspi";
>>
>>
>> Drop the version in such case, no benefits. I assume XYZ is the SoC
>> model, so for example 6868.
>>
> Yes XYZ is the SoC model
>>>
>>> In the two drivers I included in this series, it will be bound to
>>> brcm,bcmbca-hsspi-v1.0 (in additional to brcm,bcm6328-hsspi) and
>>> brcm,bcmbca-hsspi-v1.1 respectively. This way we don't need to update
>>> the driver with a new soc specific compatible whenever a new chips comes
>>> out.
>>
>> I don't understand why do you bring it now as an argument. You defined
>> before that your driver will bind to the generic bcmbca compatible, so
>> now it is not enough?
>>
> No as we are adding chip model specific info here. The existing driver
> spi-bcm63xx-hsspi.c only binds to brcm,bcm6328-hsspi. This driver
> supports all the chips with rev1.0 controller so I am using this 6328
> string for other chips with v1.0 in the dts patch, which is not ideal.
Why? This is perfectly ideal and usual case. Why changing it?
> Now I have to add more compatible to this driver and for each new chip
> with 1.0 in the future if any.
Why you cannot use compatibility with older chipset?
Best regards,
Krzysztof
On Wed, Jan 11, 2023 at 11:42:57AM -0800, William Zhang wrote:
> On 01/10/2023 01:18 PM, Mark Brown wrote:
> > spi_replace_transfers().
> Okay I saw this function is used by spi_split_transfers_maxsize which a few
> drivers use to limit the transfer size and it make sense. I can come up
> something like spi_merge_transfers to be used by my driver's prepend
> function. But it has the same issue I mentioned early as the these tx, rx
> transfers have the dependency on the order they present in the original
> transfer list for my prepend function to work. And for the same reason, it
> won't be generally useful for other drivers.
I wouldn't be surprised if something else turned up which had similar
constraints, SPI isn't the most complex thing ever so there's a lot of
patterns in controlers that get reproduced.
On 01/12/2023 12:21 AM, Krzysztof Kozlowski wrote:
> On 11/01/2023 19:44, William Zhang wrote:
>>
>>
>> On 01/11/2023 10:12 AM, Krzysztof Kozlowski wrote:
>>> On 11/01/2023 19:04, William Zhang wrote:
>>>>
>>>>
>>>> On 01/11/2023 01:02 AM, Krzysztof Kozlowski wrote:
>>>>> On 10/01/2023 23:18, Florian Fainelli wrote:
>>>>>> On 1/10/23 00:40, Krzysztof Kozlowski wrote:
>>>>>>>>> No, it is discouraged in such forms. Family or IP block compatibles
>>>>>>>>> should be prepended with a specific compatible. There were many issues
>>>>>>>>> when people insisted on generic or family compatibles...
>>>>>>>>>
>>>>>>>>>> Otherwise we will have to have a compatible string with chip model for
>>>>>>>>>> each SoC even they share the same IP. We already have more than ten of
>>>>>>>>>> SoCs and the list will increase. I don't see this is a good solution too.
>>>>>>>>>
>>>>>>>>> You will have to do it anyway even with generic fallback, so I don't get
>>>>>>>>> what is here to gain... I also don't get why Broadcom should be here
>>>>>>>>> special, different than others. Why it is not a good solution for
>>>>>>>>> Broadcom SoCs but it is for others?
>>>>>>>>>
>>>>>>>> I saw a few other vendors like these qcom ones:
>>>>>>>> qcom,spi-qup.yaml
>>>>>>>> - qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
>>>>>>>> - qcom,spi-qup-v2.1.1 # for 8974 and later
>>>>>>>> - qcom,spi-qup-v2.2.1 # for 8974 v2 and later
>>>>>>>> qcom,spi-qup.yaml
>>>>>>>> const: qcom,geni-spi
>>>>>>>
>>>>>>> IP block version numbers are allowed when there is clear mapping between
>>>>>>> version and SoCs using it. This is the case for Qualcomm because there
>>>>>>> is such clear mapping documented and available for Qualcomm engineers
>>>>>>> and also some of us (although not public).
>>>>>>>
>>>>>>>> I guess when individual who only has one particular board/chip and is
>>>>>>>> not aware of the IP family, it is understandable to use the chip
>>>>>>>> specific compatible string.
>>>>>>>
>>>>>>> Family of devices is not a versioned IP block.
>>>>>>
>>>>>> Would it be acceptable to define for instance:
>>>>>>
>>>>>> - compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";
>>>>>
>>>>> Yes, this is perfectly valid. Although it does not solve William
>>>>> concerns because it requires defining specific compatibles for all of
>>>>> the SoCs.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>> As I mentioned in another email, I would be okay to use these
>>>> compatibles to differentiate by ip rev and to conforms to brcm convention:
>>>> "brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.0", "brcm,bcmbca-hsspi";
>>>> "brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.1", "brcm,bcmbca-hsspi";
>>>
>>>
>>> Drop the version in such case, no benefits. I assume XYZ is the SoC
>>> model, so for example 6868.
>>>
>> Yes XYZ is the SoC model
>>>>
>>>> In the two drivers I included in this series, it will be bound to
>>>> brcm,bcmbca-hsspi-v1.0 (in additional to brcm,bcm6328-hsspi) and
>>>> brcm,bcmbca-hsspi-v1.1 respectively. This way we don't need to update
>>>> the driver with a new soc specific compatible whenever a new chips comes
>>>> out.
>>>
>>> I don't understand why do you bring it now as an argument. You defined
>>> before that your driver will bind to the generic bcmbca compatible, so
>>> now it is not enough?
>>>
>> No as we are adding chip model specific info here. The existing driver
>> spi-bcm63xx-hsspi.c only binds to brcm,bcm6328-hsspi. This driver
>> supports all the chips with rev1.0 controller so I am using this 6328
>> string for other chips with v1.0 in the dts patch, which is not ideal.
>
> Why? This is perfectly ideal and usual case. Why changing it?
>
>> Now I have to add more compatible to this driver and for each new chip
>> with 1.0 in the future if any.
>
> Why you cannot use compatibility with older chipset?
>
IMHO it is really confusing that we have all the SoCs but have to bind
to an antique SoC's spi controller compatible and people may think it is
a mistake or typo when they don't know they are actually the same. I
know there are usage like that but when we have clear knowledge of the
IP block with rev info, I think it is much better to have a precise SoC
model number and a general revision info in the compatible. As you know
they are many usage of IP rev info in the compatible too.
brcm,bcm6328-hsspi will stay so it does not break any existing dts
reference to that.
Anyway if you still does not like this idea, I will drop the rev info
and you have it your way.
>
> Best regards,
> Krzysztof
>
On 12/01/2023 20:50, William Zhang wrote:
>>> No as we are adding chip model specific info here. The existing driver
>>> spi-bcm63xx-hsspi.c only binds to brcm,bcm6328-hsspi. This driver
>>> supports all the chips with rev1.0 controller so I am using this 6328
>>> string for other chips with v1.0 in the dts patch, which is not ideal.
>>
>> Why? This is perfectly ideal and usual case. Why changing it?
>>
>>> Now I have to add more compatible to this driver and for each new chip
>>> with 1.0 in the future if any.
>>
>> Why you cannot use compatibility with older chipset?
>>
> IMHO it is really confusing that we have all the SoCs but have to bind
> to an antique SoC's spi controller compatible and people may think it is
> a mistake or typo when they don't know they are actually the same.
I am sorry, this is ridiculous argument. It's like saying - people
cannot understand what they are reading, therefore we need to present
them obfuscated information so they will think something else than their
minds created...
> I
> know there are usage like that but when we have clear knowledge of the
> IP block with rev info, I think it is much better to have a precise SoC
No, it's not particularly better and you were questioning it just before...
> model number and a general revision info in the compatible. As you know
> they are many usage of IP rev info in the compatible too.
> brcm,bcm6328-hsspi will stay so it does not break any existing dts
> reference to that.
Anyway your ship sailed - you already have bindings using SoC versions...
>
> Anyway if you still does not like this idea, I will drop the rev info
> and you have it your way.
Best regards,
Krzysztof
On 01/12/2023 11:41 PM, Krzysztof Kozlowski wrote:
> On 12/01/2023 20:50, William Zhang wrote:
>>>> No as we are adding chip model specific info here. The existing driver
>>>> spi-bcm63xx-hsspi.c only binds to brcm,bcm6328-hsspi. This driver
>>>> supports all the chips with rev1.0 controller so I am using this 6328
>>>> string for other chips with v1.0 in the dts patch, which is not ideal.
>>>
>>> Why? This is perfectly ideal and usual case. Why changing it?
>>>
>>>> Now I have to add more compatible to this driver and for each new chip
>>>> with 1.0 in the future if any.
>>>
>>> Why you cannot use compatibility with older chipset?
>>>
>> IMHO it is really confusing that we have all the SoCs but have to bind
>> to an antique SoC's spi controller compatible and people may think it is
>> a mistake or typo when they don't know they are actually the same.
>
> I am sorry, this is ridiculous argument. It's like saying - people
> cannot understand what they are reading, therefore we need to present
> them obfuscated information so they will think something else than their
> minds created...
>
This is clearly not to obfuscate. Rather it provide more accurate info
about the binding. Is it a problem to have the correct and precise info
to make it easier for people to understand?
>> I
>> know there are usage like that but when we have clear knowledge of the
>> IP block with rev info, I think it is much better to have a precise SoC
>
> No, it's not particularly better and you were questioning it just before...
>
Better than using the very old specific chip model number to bind all
other new chips while I have a chance to update the doc now. I guess we
have to agree to disagree. Enough discussion and I will send out v2 next
week. Thanks for the review.
>> model number and a general revision info in the compatible. As you know
>> they are many usage of IP rev info in the compatible too.
>> brcm,bcm6328-hsspi will stay so it does not break any existing dts
>> reference to that.
>
> Anyway your ship sailed - you already have bindings using SoC versions...
>
>>
>> Anyway if you still does not like this idea, I will drop the rev info
>> and you have it your way.
>
> Best regards,
> Krzysztof
>
On 14/01/2023 04:17, William Zhang wrote:
>>> I
>>> know there are usage like that but when we have clear knowledge of the
>>> IP block with rev info, I think it is much better to have a precise SoC
>>
>> No, it's not particularly better and you were questioning it just before...
>>
> Better than using the very old specific chip model number to bind all
> other new chips while I have a chance to update the doc now.
It will be used to bind them anyway, it's already an ABI.
> I guess we
> have to agree to disagree. Enough discussion and I will send out v2 next
> week. Thanks for the review.
>
>>> model number and a general revision info in the compatible. As you know
>>> they are many usage of IP rev info in the compatible too.
>>> brcm,bcm6328-hsspi will stay so it does not break any existing dts
>>> reference to that.
>>
>> Anyway your ship sailed - you already have bindings using SoC versions...
As I said here...
Best regards,
Krzysztof