Hi, > >> +/** > >> + * qcom_offset_to_nandc_reg() - Get the actual offset > >> + * @regs: pointer to nandc_reg structure > >> + * @offset: register offset > >> + * > >> + * This function will reurn the actual offset for qpic controller register > >> + */ > >> +__le32 *qcom_offset_to_nandc_reg(struct nandc_regs *regs, int offset) > >> +{ > >> + switch (offset) { > >> + case NAND_FLASH_CMD: > >> + return ®s->cmd; > >> + case NAND_ADDR0: > >> + return ®s->addr0; > >> + case NAND_ADDR1: > >> + return ®s->addr1; > >> + case NAND_FLASH_CHIP_SELECT: > >> + return ®s->chip_sel; > >> + case NAND_EXEC_CMD: > >> + return ®s->exec; > >> + case NAND_FLASH_STATUS: > >> + return ®s->clrflashstatus; > >> + case NAND_DEV0_CFG0: > >> + return ®s->cfg0; > >> + case NAND_DEV0_CFG1: > >> + return ®s->cfg1; > >> + case NAND_DEV0_ECC_CFG: > >> + return ®s->ecc_bch_cfg; > >> + case NAND_READ_STATUS: > >> + return ®s->clrreadstatus; > >> + case NAND_DEV_CMD1: > >> + return ®s->cmd1; > >> + case NAND_DEV_CMD1_RESTORE: > >> + return ®s->orig_cmd1; > >> + case NAND_DEV_CMD_VLD: > >> + return ®s->vld; > >> + case NAND_DEV_CMD_VLD_RESTORE: > >> + return ®s->orig_vld; > >> + case NAND_EBI2_ECC_BUF_CFG: > >> + return ®s->ecc_buf_cfg; > >> + case NAND_READ_LOCATION_0: > >> + return ®s->read_location0; > >> + case NAND_READ_LOCATION_1: > >> + return ®s->read_location1; > >> + case NAND_READ_LOCATION_2: > >> + return ®s->read_location2; > >> + case NAND_READ_LOCATION_3: > >> + return ®s->read_location3; > >> + case NAND_READ_LOCATION_LAST_CW_0: > >> + return ®s->read_location_last0; > >> + case NAND_READ_LOCATION_LAST_CW_1: > >> + return ®s->read_location_last1; > >> + case NAND_READ_LOCATION_LAST_CW_2: > >> + return ®s->read_location_last2; > >> + case NAND_READ_LOCATION_LAST_CW_3: > >> + return ®s->read_location_last3; > > > > Why do you need this indirection? > > This indirection I believe is needed by the write_reg_dma function, > wherein a bunch of registers are modified based on a starting register. > Can I change this in a separate cleanup series as a follow up to this? I think it would be cleaner to make the changes I requested first and then make a copy. I understand it is more work on your side, so if you really prefer you can (1) make the copy and then (2) clean it all. But please do it all in this series. > >> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h > >> new file mode 100644 > >> index 000000000000..aced15866627 > >> --- /dev/null > >> +++ b/include/linux/mtd/nand-qpic-common.h > >> @@ -0,0 +1,486 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +/* > >> + * QCOM QPIC common APIs header file > >> + * > >> + * Copyright (c) 2023 Qualcomm Inc. > >> + * Authors: Md sadre Alam <quic_mdalam@quicinc.com> > >> + * Sricharan R <quic_srichara@quicinc.com> > >> + * Varadarajan Narayanan <quic_varada@quicinc.com> > >> + * > >> + */ > >> +#ifndef __MTD_NAND_QPIC_COMMON_H__ > >> +#define __MTD_NAND_QPIC_COMMON_H__ > >> + > >> +#include <linux/bitops.h> > >> +#include <linux/clk.h> > >> +#include <linux/delay.h> > >> +#include <linux/dmaengine.h> > >> +#include <linux/dma-mapping.h> > >> +#include <linux/dma/qcom_adm.h> > >> +#include <linux/dma/qcom_bam_dma.h> > >> +#include <linux/module.h> > >> +#include <linux/mtd/partitions.h> > >> +#include <linux/mtd/rawnand.h> > > > > You really need this? > Yes , since some generic structure used here. Which ones? If this is a common file, you probably should not. Thanks, Miquèl
On 3/15/2024 5:38 PM, Miquel Raynal wrote: > Hi, > > quic_mdalam@quicinc.com wrote on Fri, 8 Mar 2024 14:47:50 +0530: > >> Add qpic spi nand driver support. The spi nand >> driver currently supported the below commands. >> >> -- RESET >> -- READ ID >> -- SET FEATURE >> -- GET FEATURE >> -- READ PAGE >> -- WRITE PAGE >> -- ERASE PAGE >> >> Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Co-developed-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > > I don't like the "spi nand driver" wording here. It is a spi > controller, for spi-memories. Ok > > Plus, I'd expect some kind of check to see whether you support the > requested operation, I don't see any in the code. Ok, will add in next patch. > > >> --- >> Change in [v4] >> >> * No change >> >> Change in [v3] >> >> * Set SPI_QPIC_SNAND to n and added COMPILE_TEST in Kconfig >> >> * Made driver name sorted in Make file >> >> * Made comment like c++ >> >> * Changed macro to functions, snandc_set_read_loc_last() >> and snandc_set_read_loc_first() >> >> * Added error handling in snandc_set_reg() >> >> * Changed into normal conditional statement for >> return snandc->ecc_stats.failed ? -EBADMSG : >> snandc->ecc_stats.bitflips; >> >> * Remove cast of wbuf in qpic_snand_program_execute() >> function >> >> * Made num_cw variable instead hardcoded value >> >> * changed if..else condition of function qpic_snand_io_op() >> to switch..case statement >> >> * Added __devm_spi_alloc_controller() api instead of >> devm_spi_alloc_master() >> >> * Disabling clock in remove path >> >> Change in [v2] >> >> * Added initial support for SPI-NAND driver >> >> Change in [v1] >> >> * Added RFC patch for design review >> >> drivers/mtd/nand/qpic_common.c | 8 + >> drivers/spi/Kconfig | 8 + >> drivers/spi/Makefile | 1 + >> drivers/spi/spi-qpic-snand.c | 1041 ++++++++++++++++++++++++++ >> include/linux/mtd/nand-qpic-common.h | 61 ++ >> 5 files changed, 1119 insertions(+) >> create mode 100644 drivers/spi/spi-qpic-snand.c >> >> diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c >> index 5b7c0d119d9a..67ccb3d05f20 100644 >> --- a/drivers/mtd/nand/qpic_common.c >> +++ b/drivers/mtd/nand/qpic_common.c >> @@ -134,6 +134,14 @@ __le32 *qcom_offset_to_nandc_reg(struct nandc_regs *regs, int offset) >> return ®s->read_location_last2; >> case NAND_READ_LOCATION_LAST_CW_3: >> return ®s->read_location_last3; >> + case NAND_FLASH_SPI_CFG: >> + return ®s->spi_cfg; >> + case NAND_NUM_ADDR_CYCLES: >> + return ®s->num_addr_cycle; >> + case NAND_BUSY_CHECK_WAIT_CNT: >> + return ®s->busy_wait_cnt; >> + case NAND_FLASH_FEATURES: >> + return ®s->flash_feature; > > I am still not convinced about these. I don't understand who you have > this indirection. This indirection I believe is needed by the write_reg_dma function, wherein a bunch of registers are modified based on a starting register.Can I change this in a separate cleanup series as a follow up to this? > >> default: >> return NULL; >> } >> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig >> index bc7021da2fe9..63764e943d82 100644 >> --- a/drivers/spi/Kconfig >> +++ b/drivers/spi/Kconfig >> @@ -882,6 +882,14 @@ config SPI_QCOM_QSPI >> help >> QSPI(Quad SPI) driver for Qualcomm QSPI controller. >> >> +config SPI_QPIC_SNAND >> + tristate "QPIC SNAND controller" >> + depends on ARCH_QCOM || COMPILE_TEST >> + help >> + QPIC_SNAND (QPIC SPI NAND) driver for Qualcomm QPIC controller. >> + QPIC controller supports both parallel nand and serial nand. >> + This config will enable serial nand driver for QPIC controller. >> + >> config SPI_QUP >> tristate "Qualcomm SPI controller with QUP interface" >> depends on ARCH_QCOM || COMPILE_TEST >> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile >> index 4ff8d725ba5e..9015368f8c73 100644 >> --- a/drivers/spi/Makefile >> +++ b/drivers/spi/Makefile >> @@ -111,6 +111,7 @@ obj-$(CONFIG_SPI_PXA2XX) += spi-pxa2xx-platform.o >> obj-$(CONFIG_SPI_PXA2XX_PCI) += spi-pxa2xx-pci.o >> obj-$(CONFIG_SPI_QCOM_GENI) += spi-geni-qcom.o >> obj-$(CONFIG_SPI_QCOM_QSPI) += spi-qcom-qspi.o >> +obj-$(CONFIG_SPI_QPIC_SNAND) += spi-qpic-snand.o >> obj-$(CONFIG_SPI_QUP) += spi-qup.o >> obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o >> obj-$(CONFIG_SPI_ROCKCHIP_SFC) += spi-rockchip-sfc.o >> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c >> new file mode 100644 >> index 000000000000..df7d5d8d4db2 >> --- /dev/null >> +++ b/drivers/spi/spi-qpic-snand.c >> @@ -0,0 +1,1041 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0 >> + * >> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. >> + * >> + * Authors: >> + * Md Sadre Alam <quic_mdalam@quicinc.com> >> + * Sricharan R <quic_srichara@quicinc.com> >> + * Varadarajan Narayanan <quic_varada@quicinc.com> >> + */ >> + >> +#include <linux/mtd/spinand.h> >> +#include <linux/mtd/nand-qpic-common.h> >> + >> +/* QSPI NAND config reg bits */ >> +#define LOAD_CLK_CNTR_INIT_EN BIT(28) >> +#define CLK_CNTR_INIT_VAL_VEC 0x924 >> +#define FEA_STATUS_DEV_ADDR 0xc0 >> +#define SPI_CFG BIT(0) >> +#define SPI_NUM_ADDR 0xDA4DB >> +#define SPI_WAIT_CNT 0x10 >> +#define QPIC_QSPI_NUM_CS 1 >> +#define SPI_TRANSFER_MODE_x1 BIT(29) >> +#define SPI_TRANSFER_MODE_x4 (3 << 29) >> +#define SPI_WP BIT(28) >> +#define SPI_HOLD BIT(27) >> +#define QPIC_SET_FEATURE BIT(31) >> + >> +#define SPINAND_RESET 0xff >> +#define SPINAND_READID 0x9f >> +#define SPINAND_GET_FEATURE 0x0f >> +#define SPINAND_SET_FEATURE 0x1f >> +#define SPINAND_READ 0x13 >> +#define SPINAND_ERASE 0xd8 >> +#define SPINAND_WRITE_EN 0x06 >> +#define SPINAND_PROGRAM_EXECUTE 0x10 >> +#define SPINAND_PROGRAM_LOAD 0x84 >> + >> +struct qpic_snand_op { >> + u32 cmd_reg; >> + u32 addr1_reg; >> + u32 addr2_reg; >> +}; >> + >> +struct snandc_read_status { >> + __le32 snandc_flash; >> + __le32 snandc_buffer; >> + __le32 snandc_erased_cw; >> +}; >> + >> +static void snandc_set_reg(struct qcom_nand_controller *snandc, int offset, u32 val) > > qcom_spi_ would be a better prefix maybe? Ok > >> +{ >> + struct nandc_regs *regs = snandc->regs; >> + __le32 *reg; >> + >> + reg = qcom_offset_to_nandc_reg(regs, offset); >> + >> + if (reg) >> + *reg = cpu_to_le32(val); >> + >> + if (WARN_ON(!reg)) >> + return; > > This whole logic really seems suboptimal. > >> +} >> + >> +static void snandc_set_read_loc_first(struct qcom_nand_controller *snandc, >> + int reg, int cw_offset, int read_size, >> + int is_last_read_loc) >> +{ >> + snandc_set_reg(snandc, reg, ((cw_offset) << READ_LOCATION_OFFSET) | >> + ((read_size) << READ_LOCATION_SIZE) | ((is_last_read_loc) >> + << READ_LOCATION_LAST)); > > FIELD_GET, FIELD_PREP ? Ok > >> +} >> + >> +static void snandc_set_read_loc_last(struct qcom_nand_controller *snandc, >> + int reg, int cw_offset, int read_size, >> + int is_last_read_loc) >> +{ >> + snandc_set_reg(snandc, reg, ((cw_offset) << READ_LOCATION_OFFSET) | >> + ((read_size) << READ_LOCATION_SIZE) | ((is_last_read_loc) >> + << READ_LOCATION_LAST)); >> +} >> + >> +static struct qcom_nand_controller *nand_to_qcom_snand(struct nand_device *nand) >> +{ >> + struct nand_ecc_engine *eng = nand->ecc.engine; >> + >> + return container_of(eng, struct qcom_nand_controller, ecc_eng); >> +} >> + >> +static int qcom_snand_init(struct qcom_nand_controller *snandc) >> +{ >> + u32 snand_cfg_val = 0x0; >> + int ret; >> + >> + snand_cfg_val |= (LOAD_CLK_CNTR_INIT_EN | (CLK_CNTR_INIT_VAL_VEC << 16) >> + | (FEA_STATUS_DEV_ADDR << 8) | SPI_CFG); > > ^ > the | should be on the previous line. Ok > >> + >> + snandc_set_reg(snandc, NAND_FLASH_SPI_CFG, 0); >> + snandc_set_reg(snandc, NAND_FLASH_SPI_CFG, snand_cfg_val); >> + snandc_set_reg(snandc, NAND_NUM_ADDR_CYCLES, SPI_NUM_ADDR); >> + snandc_set_reg(snandc, NAND_BUSY_CHECK_WAIT_CNT, SPI_WAIT_CNT); >> + >> + qcom_write_reg_dma(snandc, NAND_FLASH_SPI_CFG, 1, 0); >> + qcom_write_reg_dma(snandc, NAND_FLASH_SPI_CFG, 1, 0); >> + >> + snand_cfg_val &= ~LOAD_CLK_CNTR_INIT_EN; >> + snandc_set_reg(snandc, NAND_FLASH_SPI_CFG, snand_cfg_val); >> + >> + qcom_write_reg_dma(snandc, NAND_FLASH_SPI_CFG, 1, 0); >> + >> + qcom_write_reg_dma(snandc, NAND_NUM_ADDR_CYCLES, 1, 0); >> + qcom_write_reg_dma(snandc, NAND_BUSY_CHECK_WAIT_CNT, 1, NAND_BAM_NEXT_SGL); >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) >> + dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n"); > > Typos... Ok > >> + >> + return 0; > > return ret ? Ok > >> +} >> + >> +static int qcom_snand_ooblayout_ecc(struct mtd_info *mtd, int section, >> + struct mtd_oob_region *oobregion) >> +{ >> + struct nand_device *nand = mtd_to_nanddev(mtd); >> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand); >> + struct qpic_ecc *qecc = snandc->ecc; >> + >> + if (section > 1) >> + return -ERANGE; >> + >> + if (!section) { >> + oobregion->length = (qecc->bytes * (qecc->steps - 1)) + qecc->bbm_size; >> + oobregion->offset = 0; > > No BBM? Ok > >> + } else { >> + oobregion->length = qecc->ecc_bytes_hw + qecc->spare_bytes; >> + oobregion->offset = mtd->oobsize - oobregion->length; >> + } >> + >> + return 0; >> +} >> + >> +static int qcom_snand_ooblayout_free(struct mtd_info *mtd, int section, >> + struct mtd_oob_region *oobregion) >> +{ >> + struct nand_device *nand = mtd_to_nanddev(mtd); >> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand); >> + struct qpic_ecc *qecc = snandc->ecc; >> + >> + if (section) >> + return -ERANGE; >> + >> + oobregion->length = qecc->steps * 4; >> + oobregion->offset = ((qecc->steps - 1) * qecc->bytes) + qecc->bbm_size; >> + > > Using the same order would be easier to compare with the above version Not able to understand. Could you please elaborate order of what has to be change Will change it in the next version. >> + return 0; >> +} >> + >> +static const struct mtd_ooblayout_ops qcom_snand_ooblayout = { >> + .ecc = qcom_snand_ooblayout_ecc, >> + .free = qcom_snand_ooblayout_free, >> +}; >> + >> +static int qpic_snand_ecc_init_ctx_pipelined(struct nand_device *nand) >> +{ >> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand); >> + struct nand_ecc_props *conf = &nand->ecc.ctx.conf; >> + struct nand_ecc_props *reqs = &nand->ecc.requirements; >> + struct nand_ecc_props *user = &nand->ecc.user_conf; >> + struct mtd_info *mtd = nanddev_to_mtd(nand); >> + int step_size = 0, strength = 0, steps; >> + int cwperpage, bad_block_byte; >> + struct qpic_ecc *ecc_cfg; >> + >> + cwperpage = mtd->writesize / NANDC_STEP_SIZE; >> + snandc->num_cw = cwperpage; >> + >> + ecc_cfg = kzalloc(sizeof(*ecc_cfg), GFP_KERNEL); >> + if (!ecc_cfg) >> + return -ENOMEM; >> + >> + nand->ecc.ctx.priv = ecc_cfg; >> + >> + if (user->step_size && user->strength) { >> + step_size = user->step_size; >> + strength = user->strength; >> + } else if (reqs->step_size && reqs->strength) { >> + step_size = reqs->step_size; >> + strength = reqs->strength; >> + } >> + >> + if (step_size && strength) >> + steps = mtd->writesize / step_size; >> + >> + ecc_cfg->ecc_bytes_hw = 7; >> + ecc_cfg->spare_bytes = 4; >> + ecc_cfg->bbm_size = 1; >> + ecc_cfg->bch_enabled = true; >> + ecc_cfg->bytes = ecc_cfg->ecc_bytes_hw + ecc_cfg->spare_bytes + ecc_cfg->bbm_size; >> + >> + ecc_cfg->steps = 4; >> + ecc_cfg->strength = 4; >> + ecc_cfg->step_size = 512; >> + >> + mtd_set_ooblayout(mtd, &qcom_snand_ooblayout); >> + >> + ecc_cfg->cw_data = 516; >> + ecc_cfg->cw_size = ecc_cfg->cw_data + ecc_cfg->bytes; >> + bad_block_byte = mtd->writesize - ecc_cfg->cw_size * (cwperpage - 1) + 1; >> + >> + ecc_cfg->cfg0 = (cwperpage - 1) << CW_PER_PAGE >> + | ecc_cfg->cw_data << UD_SIZE_BYTES >> + | 1 << DISABLE_STATUS_AFTER_WRITE >> + | 3 << NUM_ADDR_CYCLES >> + | ecc_cfg->ecc_bytes_hw << ECC_PARITY_SIZE_BYTES_RS >> + | 0 << STATUS_BFR_READ >> + | 1 << SET_RD_MODE_AFTER_STATUS >> + | ecc_cfg->spare_bytes << SPARE_SIZE_BYTES; >> + >> + ecc_cfg->cfg1 = 0 << NAND_RECOVERY_CYCLES >> + | 0 << CS_ACTIVE_BSY >> + | bad_block_byte << BAD_BLOCK_BYTE_NUM >> + | 0 << BAD_BLOCK_IN_SPARE_AREA >> + | 20 << WR_RD_BSY_GAP >> + | 0 << WIDE_FLASH >> + | ecc_cfg->bch_enabled << ENABLE_BCH_ECC; >> + >> + ecc_cfg->cfg0_raw = (cwperpage - 1) << CW_PER_PAGE >> + | ecc_cfg->cw_size << UD_SIZE_BYTES >> + | 3 << NUM_ADDR_CYCLES >> + | 0 << SPARE_SIZE_BYTES; >> + >> + ecc_cfg->cfg1_raw = 0 << NAND_RECOVERY_CYCLES >> + | 0 << CS_ACTIVE_BSY >> + | 17 << BAD_BLOCK_BYTE_NUM >> + | 1 << BAD_BLOCK_IN_SPARE_AREA >> + | 20 << WR_RD_BSY_GAP >> + | 0 << WIDE_FLASH >> + | 1 << DEV0_CFG1_ECC_DISABLE; >> + >> + ecc_cfg->ecc_bch_cfg = !ecc_cfg->bch_enabled << ECC_CFG_ECC_DISABLE >> + | 0 << ECC_SW_RESET >> + | ecc_cfg->cw_data << ECC_NUM_DATA_BYTES >> + | 1 << ECC_FORCE_CLK_OPEN >> + | 0 << ECC_MODE >> + | ecc_cfg->ecc_bytes_hw << ECC_PARITY_SIZE_BYTES_BCH; >> + >> + ecc_cfg->ecc_buf_cfg = 0x203 << NUM_STEPS; >> + ecc_cfg->clrflashstatus = FS_READY_BSY_N; >> + ecc_cfg->clrreadstatus = 0xc0; >> + >> + conf->step_size = ecc_cfg->step_size; >> + conf->strength = ecc_cfg->strength; >> + >> + if (ecc_cfg->strength < strength) >> + dev_warn(snandc->dev, "Unable to fulfill ECC requirements of %u bits.\n", strength); > > Not needed I guess. Somewhat redundant with the core? Ok > >> + >> + dev_info(snandc->dev, "ECC strength: %u bits per %u bytes\n", >> + ecc_cfg->strength, ecc_cfg->step_size); > > Debug? Ok > >> + >> + return 0; >> +} >> + >> +static void qpic_snand_ecc_cleanup_ctx_pipelined(struct nand_device *nand) >> +{ >> + struct qpic_ecc *ecc_cfg = nand_to_ecc_ctx(nand); >> + >> + kfree(ecc_cfg); >> +} >> + >> +static int qpic_snand_ecc_prepare_io_req_pipelined(struct nand_device *nand, >> + struct nand_page_io_req *req) >> +{ >> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand); >> + struct qpic_ecc *ecc_cfg = nand_to_ecc_ctx(nand); >> + >> + snandc->ecc = ecc_cfg; >> + snandc->raw = false; >> + snandc->oob_read = false; >> + >> + if (req->mode == MTD_OPS_RAW) { >> + if (req->ooblen) >> + snandc->oob_read = true; >> + snandc->raw = true; >> + } >> + >> + return 0; >> +} >> + >> +static int qpic_snand_ecc_finish_io_req_pipelined(struct nand_device *nand, >> + struct nand_page_io_req *req) >> +{ >> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand); >> + struct mtd_info *mtd = nanddev_to_mtd(nand); >> + >> + if (req->mode == MTD_OPS_RAW || req->type != NAND_PAGE_READ) >> + return 0; >> + >> + if (snandc->ecc_stats.failed) >> + mtd->ecc_stats.failed += snandc->ecc_stats.failed; >> + mtd->ecc_stats.corrected += snandc->ecc_stats.corrected; >> + >> + if (snandc->ecc_stats.failed) > > I hope you reset this counter at some point. Yes > > Did you run nandbiterrs -i ? No. I tested with mtd write/erase and dd commands etc. Will include nandbiterrs from next version. > >> + return -EBADMSG; >> + else >> + return snandc->ecc_stats.bitflips; >> +} >> + >> +static struct nand_ecc_engine_ops qcom_snand_ecc_engine_ops_pipelined = { >> + .init_ctx = qpic_snand_ecc_init_ctx_pipelined, >> + .cleanup_ctx = qpic_snand_ecc_cleanup_ctx_pipelined, >> + .prepare_io_req = qpic_snand_ecc_prepare_io_req_pipelined, >> + .finish_io_req = qpic_snand_ecc_finish_io_req_pipelined, >> +}; >> + >> +/* helper to configure location register values */ >> +static void snandc_set_read_loc(struct qcom_nand_controller *snandc, int cw, int reg, >> + int cw_offset, int read_size, int is_last_read_loc) >> +{ >> + int reg_base = NAND_READ_LOCATION_0; >> + >> + if (cw == 3) >> + reg_base = NAND_READ_LOCATION_LAST_CW_0; >> + >> + reg_base += reg * 4; >> + >> + if (cw == 3) >> + return snandc_set_read_loc_last(snandc, reg_base, cw_offset, >> + read_size, is_last_read_loc); > > Alignments are still wrong, again. Ok > >> + else >> + return snandc_set_read_loc_first(snandc, reg_base, cw_offset, >> + read_size, is_last_read_loc); >> +} >> + >> +static void >> +snandc_config_cw_read(struct qcom_nand_controller *snandc, bool use_ecc, int cw) >> +{ >> + int reg = NAND_READ_LOCATION_0; >> + >> + if (cw == 3) >> + reg = NAND_READ_LOCATION_LAST_CW_0; >> + >> + if (snandc->props->is_bam) >> + qcom_write_reg_dma(snandc, reg, 4, NAND_BAM_NEXT_SGL); >> + >> + qcom_write_reg_dma(snandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL); >> + qcom_write_reg_dma(snandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); >> + >> + qcom_read_reg_dma(snandc, NAND_FLASH_STATUS, 2, 0); >> + qcom_read_reg_dma(snandc, NAND_ERASED_CW_DETECT_STATUS, 1, >> + NAND_BAM_NEXT_SGL); >> +} >> + >> +static int qpic_snand_block_erase(struct qcom_nand_controller *snandc) >> +{ >> + struct qpic_ecc *ecc_cfg = snandc->ecc; >> + int ret; >> + >> + snandc->buf_count = 0; >> + snandc->buf_start = 0; >> + qcom_clear_read_regs(snandc); >> + qcom_clear_bam_transaction(snandc); >> + >> + snandc_set_reg(snandc, NAND_FLASH_CMD, snandc->cmd); >> + snandc_set_reg(snandc, NAND_ADDR0, snandc->addr1); >> + snandc_set_reg(snandc, NAND_ADDR1, snandc->addr2); >> + snandc_set_reg(snandc, NAND_DEV0_CFG0, ecc_cfg->cfg0_raw & ~(7 << CW_PER_PAGE)); >> + snandc_set_reg(snandc, NAND_DEV0_CFG1, ecc_cfg->cfg1_raw); >> + snandc_set_reg(snandc, NAND_EXEC_CMD, 1); >> + >> + qcom_write_reg_dma(snandc, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL); >> + qcom_write_reg_dma(snandc, NAND_DEV0_CFG0, 2, NAND_BAM_NEXT_SGL); >> + qcom_write_reg_dma(snandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) { >> + dev_err(snandc->dev, "failure to erase block\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static void config_snand_single_cw_page_read(struct qcom_nand_controller *snandc, >> + bool use_ecc, int cw) >> +{ >> + int reg; >> + >> + qcom_write_reg_dma(snandc, NAND_ADDR0, 2, 0); >> + qcom_write_reg_dma(snandc, NAND_DEV0_CFG0, 3, 0); >> + qcom_write_reg_dma(snandc, NAND_ERASED_CW_DETECT_CFG, 1, 0); >> + qcom_write_reg_dma(snandc, NAND_ERASED_CW_DETECT_CFG, 1, >> + NAND_ERASED_CW_SET | NAND_BAM_NEXT_SGL); >> + >> + reg = NAND_READ_LOCATION_0; >> + if (cw == 3) > > This is hardcoded everywhere, I am not a big fan. Ok will fix in next patch. > >> + reg = NAND_READ_LOCATION_LAST_CW_0; >> + qcom_write_reg_dma(snandc, reg, 4, NAND_BAM_NEXT_SGL); >> + qcom_write_reg_dma(snandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL); >> + qcom_write_reg_dma(snandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); >> + >> + qcom_read_reg_dma(snandc, NAND_FLASH_STATUS, 2, 0); >> + qcom_read_reg_dma(snandc, NAND_ERASED_CW_DETECT_STATUS, 1, NAND_BAM_NEXT_SGL); >> +} >> + >> +static int qpic_snand_read_oob(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) >> +{ >> + struct qpic_ecc *ecc_cfg = snandc->ecc; >> + int size, ret; >> + int col, bbpos; >> + u32 cfg0, cfg1, ecc_bch_cfg; >> + u32 num_cw = snandc->num_cw; >> + >> + qcom_clear_bam_transaction(snandc); >> + qcom_clear_read_regs(snandc); >> + >> + size = ecc_cfg->cw_size; >> + col = ecc_cfg->cw_size * (num_cw - 1); >> + >> + /* prepare a clean read buffer */ >> + memset(snandc->data_buffer, 0xff, size); >> + snandc_set_reg(snandc, NAND_ADDR0, (snandc->addr1 | col)); >> + snandc_set_reg(snandc, NAND_ADDR1, snandc->addr2); >> + >> + cfg0 = (ecc_cfg->cfg0_raw & ~(7U << CW_PER_PAGE)) | >> + 0 << CW_PER_PAGE; >> + cfg1 = ecc_cfg->cfg1_raw; >> + ecc_bch_cfg = 1 << ECC_CFG_ECC_DISABLE; >> + >> + snandc_set_reg(snandc, NAND_FLASH_CMD, snandc->cmd); >> + snandc_set_reg(snandc, NAND_DEV0_CFG0, cfg0); >> + snandc_set_reg(snandc, NAND_DEV0_CFG1, cfg1); >> + snandc_set_reg(snandc, NAND_DEV0_ECC_CFG, ecc_bch_cfg); >> + snandc_set_reg(snandc, NAND_EXEC_CMD, 1); >> + >> + config_snand_single_cw_page_read(snandc, false, num_cw - 1); >> + >> + qcom_read_data_dma(snandc, FLASH_BUF_ACC, snandc->data_buffer, size, 0); >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) >> + dev_err(snandc->dev, "failed to read oob\n"); > > Why don't you return here? Ok > >> + >> + qcom_nandc_read_buffer_sync(snandc, true); >> + u32 flash = le32_to_cpu(snandc->reg_read_buf[0]); > > No compiler warning here? Ok will check and fix in next patch. > >> + >> + if (flash & (FS_OP_ERR | FS_MPU_ERR)) >> + return -EIO; >> + >> + bbpos = 2048 - ecc_cfg->cw_size * (num_cw - 1); > > Why is this size hardcoded?! That cannot work! Ok will fix in next patch. > >> + memcpy(op->data.buf.in, snandc->data_buffer + bbpos, op->data.nbytes); >> + >> + return ret; >> +} >> + >> +static int snandc_check_error(struct qcom_nand_controller *snandc) >> +{ >> + struct snandc_read_status *buf; >> + int i, num_cw = snandc->num_cw; >> + bool serial_op_err = false, erased; >> + >> + qcom_nandc_read_buffer_sync(snandc, true); >> + buf = (struct snandc_read_status *)snandc->reg_read_buf; >> + >> + for (i = 0; i < num_cw; i++, buf++) { >> + u32 flash, buffer, erased_cw; >> + >> + flash = le32_to_cpu(buf->snandc_flash); >> + buffer = le32_to_cpu(buf->snandc_buffer); >> + erased_cw = le32_to_cpu(buf->snandc_erased_cw); >> + >> + if ((flash & FS_OP_ERR) && (buffer & BS_UNCORRECTABLE_BIT)) { >> + erased = (erased_cw & ERASED_CW) == ERASED_CW ? >> + true : false; > > This ternary operation is useless Ok > >> + } else if (flash & (FS_OP_ERR | FS_MPU_ERR)) { >> + serial_op_err = true; >> + } >> + } >> + >> + if (serial_op_err) >> + return -EIO; >> + >> + return 0; >> +} >> + >> +static int qpic_snand_read_page_cache(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) >> +{ >> + struct qpic_ecc *ecc_cfg = snandc->ecc; >> + u8 *data_buf; >> + int ret, i; >> + u32 cfg0, cfg1, ecc_bch_cfg, num_cw = snandc->num_cw; >> + >> + data_buf = op->data.buf.in; >> + >> + if (snandc->oob_read) { >> + return qpic_snand_read_oob(snandc, op); >> + snandc->oob_read = false; >> + } >> + >> + snandc->buf_count = 0; >> + snandc->buf_start = 0; >> + qcom_clear_read_regs(snandc); >> + >> + cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) | >> + (num_cw - 1) << CW_PER_PAGE; >> + cfg1 = ecc_cfg->cfg1; >> + ecc_bch_cfg = ecc_cfg->ecc_bch_cfg; >> + >> + snandc_set_reg(snandc, NAND_ADDR0, snandc->addr1); >> + snandc_set_reg(snandc, NAND_ADDR1, snandc->addr2); >> + snandc_set_reg(snandc, NAND_FLASH_CMD, snandc->cmd); >> + snandc_set_reg(snandc, NAND_DEV0_CFG0, cfg0); >> + snandc_set_reg(snandc, NAND_DEV0_CFG1, cfg1); >> + snandc_set_reg(snandc, NAND_DEV0_ECC_CFG, ecc_bch_cfg); >> + snandc_set_reg(snandc, NAND_FLASH_STATUS, ecc_cfg->clrflashstatus); >> + snandc_set_reg(snandc, NAND_READ_STATUS, ecc_cfg->clrreadstatus); >> + snandc_set_reg(snandc, NAND_EXEC_CMD, 1); >> + snandc_set_read_loc(snandc, 0, 0, 0, ecc_cfg->cw_data, 1); >> + >> + qcom_clear_bam_transaction(snandc); >> + >> + qcom_write_reg_dma(snandc, NAND_ADDR0, 2, 0); >> + qcom_write_reg_dma(snandc, NAND_DEV0_CFG0, 3, 0); >> + qcom_write_reg_dma(snandc, NAND_ERASED_CW_DETECT_CFG, 1, 0); >> + qcom_write_reg_dma(snandc, NAND_ERASED_CW_DETECT_CFG, 1, >> + NAND_ERASED_CW_SET | NAND_BAM_NEXT_SGL); >> + >> + for (i = 0; i < num_cw; i++) { >> + int data_size; >> + >> + if (i == (num_cw - 1)) >> + data_size = 512 - ((num_cw - 1) << 2); >> + else >> + data_size = ecc_cfg->cw_data; >> + >> + if (data_buf) >> + snandc_set_read_loc(snandc, i, 0, 0, data_size, 1); >> + >> + snandc_config_cw_read(snandc, true, i); >> + >> + if (data_buf) >> + qcom_read_data_dma(snandc, FLASH_BUF_ACC, data_buf, >> + data_size, 0); >> + >> + if (data_buf) >> + data_buf += data_size; >> + } >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) { >> + dev_err(snandc->dev, "failure to read page/oob\n"); >> + return ret; >> + } >> + >> + return snandc_check_error(snandc); >> +} >> + >> +static void config_snand_page_write(struct qcom_nand_controller *snandc) >> +{ >> + qcom_write_reg_dma(snandc, NAND_ADDR0, 2, 0); >> + qcom_write_reg_dma(snandc, NAND_DEV0_CFG0, 3, 0); >> + qcom_write_reg_dma(snandc, NAND_EBI2_ECC_BUF_CFG, 1, NAND_BAM_NEXT_SGL); >> +} >> + >> +static void config_snand_cw_write(struct qcom_nand_controller *snandc) >> +{ >> + qcom_write_reg_dma(snandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL); >> + qcom_write_reg_dma(snandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); >> +} >> + >> +static int qpic_snand_program_execute(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) >> +{ >> + struct qpic_ecc *ecc_cfg = snandc->ecc; >> + u8 *data_buf; >> + int i, ret; >> + int num_cw = snandc->num_cw; >> + u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg; >> + >> + cfg0 = (ecc_cfg->cfg0 & ~(7U << CW_PER_PAGE)) | >> + (num_cw - 1) << CW_PER_PAGE; >> + cfg1 = ecc_cfg->cfg1; >> + ecc_bch_cfg = ecc_cfg->ecc_bch_cfg; >> + ecc_buf_cfg = ecc_cfg->ecc_buf_cfg; >> + >> + data_buf = snandc->wbuf; >> + >> + snandc->buf_count = 0; >> + snandc->buf_start = 0; >> + qcom_clear_read_regs(snandc); >> + qcom_clear_bam_transaction(snandc); >> + >> + snandc_set_reg(snandc, NAND_ADDR0, snandc->addr1); >> + snandc_set_reg(snandc, NAND_ADDR1, snandc->addr2); >> + snandc_set_reg(snandc, NAND_FLASH_CMD, snandc->cmd); >> + >> + snandc_set_reg(snandc, NAND_DEV0_CFG0, cfg0); >> + snandc_set_reg(snandc, NAND_DEV0_CFG1, cfg1); >> + snandc_set_reg(snandc, NAND_DEV0_ECC_CFG, ecc_bch_cfg); >> + >> + snandc_set_reg(snandc, NAND_EBI2_ECC_BUF_CFG, ecc_buf_cfg); >> + >> + snandc_set_reg(snandc, NAND_EXEC_CMD, 1); >> + >> + config_snand_page_write(snandc); >> + >> + for (i = 0; i < num_cw; i++) { >> + int data_size; >> + >> + if (i == (num_cw - 1)) >> + data_size = NANDC_STEP_SIZE - ((num_cw - 1) << 2); >> + else >> + data_size = ecc_cfg->cw_data; >> + >> + qcom_write_data_dma(snandc, FLASH_BUF_ACC, data_buf, data_size, >> + i == (num_cw - 1) ? NAND_BAM_NO_EOT : 0); >> + >> + config_snand_cw_write(snandc); >> + if (data_buf) >> + data_buf += data_size; >> + } >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) { >> + dev_err(snandc->dev, "failure to write page\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static u32 qpic_snand_cmd_mapping(struct qcom_nand_controller *snandc, u32 opcode) >> +{ >> + u32 cmd = 0x0; >> + >> + switch (opcode) { >> + case SPINAND_RESET: >> + cmd = (SPI_WP | SPI_HOLD | SPI_TRANSFER_MODE_x1 | OP_RESET_DEVICE); >> + break; >> + case SPINAND_READID: >> + cmd = (SPI_WP | SPI_HOLD | SPI_TRANSFER_MODE_x1 | OP_FETCH_ID); >> + break; >> + case SPINAND_GET_FEATURE: >> + cmd = (SPI_TRANSFER_MODE_x1 | SPI_WP | SPI_HOLD | ACC_FEATURE); >> + break; >> + case SPINAND_SET_FEATURE: >> + cmd = (SPI_TRANSFER_MODE_x1 | SPI_WP | SPI_HOLD | ACC_FEATURE | >> + QPIC_SET_FEATURE); >> + break; >> + case SPINAND_READ: >> + if (snandc->raw) >> + cmd = (PAGE_ACC | LAST_PAGE | SPI_TRANSFER_MODE_x1 | >> + SPI_WP | SPI_HOLD | OP_PAGE_READ); >> + else >> + cmd = (PAGE_ACC | LAST_PAGE | SPI_TRANSFER_MODE_x1 | >> + SPI_WP | SPI_HOLD | OP_PAGE_READ_WITH_ECC); >> + break; >> + case SPINAND_ERASE: >> + cmd = OP_BLOCK_ERASE | PAGE_ACC | LAST_PAGE | SPI_WP | >> + SPI_HOLD | SPI_TRANSFER_MODE_x1; >> + break; >> + case SPINAND_WRITE_EN: >> + cmd = SPINAND_WRITE_EN; >> + break; >> + case SPINAND_PROGRAM_EXECUTE: >> + cmd = (PAGE_ACC | LAST_PAGE | SPI_TRANSFER_MODE_x1 | >> + SPI_WP | SPI_HOLD | OP_PROGRAM_PAGE); >> + break; >> + case SPINAND_PROGRAM_LOAD: >> + cmd = SPINAND_PROGRAM_LOAD; >> + break; >> + default: >> + break; > > No, no, no and no again. You've been sending NAND contributions for > years, and you still continue to assume all the commands are defined > and we will never check for supported ops. Please. Sorry, I missed that check for unsupported command. Will fix in next patch. > >> + } >> + >> + return cmd; >> +} >> + >> +static int qpic_snand_write_page_cache(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) >> +{ >> + struct qpic_snand_op s_op = {}; >> + u32 cmd; >> + >> + cmd = qpic_snand_cmd_mapping(snandc, op->cmd.opcode); >> + s_op.cmd_reg = cmd; >> + >> + if (op->cmd.opcode == SPINAND_PROGRAM_LOAD) { >> + snandc->wbuf = (u8 *)op->data.buf.out; >> + snandc->wlen = op->data.nbytes; >> + } >> + >> + return 0; >> +} >> + >> +static int qpic_snand_send_cmdaddr(struct qcom_nand_controller *snandc, >> + const struct spi_mem_op *op) >> +{ >> + struct qpic_snand_op s_op = {}; >> + u32 cmd; >> + int ret; >> + >> + cmd = qpic_snand_cmd_mapping(snandc, op->cmd.opcode); >> + >> + s_op.cmd_reg = cmd; >> + s_op.addr1_reg = op->addr.val; >> + s_op.addr2_reg = 0; >> + > Would a switch case be more appropriate? Ok > >> + if (op->cmd.opcode == SPINAND_WRITE_EN) >> + return 0; >> + >> + if (op->cmd.opcode == SPINAND_PROGRAM_EXECUTE) { >> + s_op.addr1_reg = op->addr.val << 16; >> + s_op.addr2_reg = op->addr.val >> 16 & 0xff; >> + snandc->addr1 = s_op.addr1_reg; >> + snandc->addr2 = s_op.addr2_reg; >> + snandc->cmd = cmd; >> + return qpic_snand_program_execute(snandc, op); >> + } >> + >> + if (op->cmd.opcode == SPINAND_READ) { >> + s_op.addr1_reg = (op->addr.val << 16); >> + s_op.addr2_reg = op->addr.val >> 16 & 0xff; >> + snandc->addr1 = s_op.addr1_reg; >> + snandc->addr2 = s_op.addr2_reg; >> + snandc->cmd = cmd; >> + return 0; >> + } >> + >> + if (op->cmd.opcode == SPINAND_ERASE) { >> + s_op.addr2_reg = (op->addr.val >> 16) & 0xffff; >> + s_op.addr1_reg = op->addr.val; >> + snandc->addr1 = s_op.addr1_reg; >> + snandc->addr1 <<= 16; >> + snandc->addr2 = s_op.addr2_reg; >> + snandc->cmd = cmd; >> + qpic_snand_block_erase(snandc); >> + return 0; >> + } >> + >> + snandc->buf_count = 0; >> + snandc->buf_start = 0; >> + qcom_clear_read_regs(snandc); >> + qcom_clear_bam_transaction(snandc); >> + >> + snandc_set_reg(snandc, NAND_FLASH_CMD, s_op.cmd_reg); >> + snandc_set_reg(snandc, NAND_EXEC_CMD, 0x1); >> + snandc_set_reg(snandc, NAND_ADDR0, s_op.addr1_reg); >> + snandc_set_reg(snandc, NAND_ADDR1, s_op.addr2_reg); >> + >> + qcom_write_reg_dma(snandc, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL); >> + qcom_write_reg_dma(snandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) >> + dev_err(snandc->dev, "failure in sbumitting cmd descriptor\n"); >> + >> + return ret; >> +} >> + >> +static int qpic_snand_io_op(struct qcom_nand_controller *snandc, const struct spi_mem_op *op) >> +{ >> + int ret, val, opcode; >> + bool copy = false, copy_ftr = false; >> + >> + ret = qpic_snand_send_cmdaddr(snandc, op); >> + if (ret) >> + return ret; >> + >> + snandc->buf_count = 0; >> + snandc->buf_start = 0; >> + qcom_clear_read_regs(snandc); >> + qcom_clear_bam_transaction(snandc); >> + opcode = op->cmd.opcode; >> + >> + switch (opcode) { >> + case SPINAND_READID: >> + snandc->buf_count = 4; >> + qcom_read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL); >> + copy = true; >> + break; >> + case SPINAND_GET_FEATURE: >> + snandc->buf_count = 4; >> + qcom_read_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); >> + copy_ftr = true; >> + break; >> + case SPINAND_SET_FEATURE: >> + snandc_set_reg(snandc, NAND_FLASH_FEATURES, *(u32 *)op->data.buf.out); >> + qcom_write_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL); >> + break; >> + default: >> + return 0; >> + } >> + >> + ret = qcom_submit_descs(snandc); >> + if (ret) >> + dev_err(snandc->dev, "failure in submitting descriptor for:%d\n", opcode); >> + >> + if (copy) { >> + qcom_nandc_read_buffer_sync(snandc, true); >> + memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count); >> + } >> + >> + if (copy_ftr) { >> + qcom_nandc_read_buffer_sync(snandc, true); >> + val = le32_to_cpu(*(__le32 *)snandc->reg_read_buf); >> + val >>= 8; >> + memcpy(op->data.buf.in, &val, snandc->buf_count); >> + } >> + >> + return ret; >> +} >> + >> +static bool qpic_snand_is_page_op(const struct spi_mem_op *op) >> +{ >> + if (op->addr.buswidth != 1 && op->addr.buswidth != 2 && op->addr.buswidth != 4) >> + return false; >> + >> + if (op->data.dir == SPI_MEM_DATA_IN) { >> + if (op->addr.buswidth == 4 && op->data.buswidth == 4) >> + return true; >> + >> + if (op->addr.nbytes == 2 && op->addr.buswidth == 1) >> + return true; >> + >> + } else if (op->data.dir == SPI_MEM_DATA_OUT) { >> + if (op->data.buswidth == 4) >> + return true; >> + if (op->addr.nbytes == 2 && op->addr.buswidth == 1) >> + return true; >> + } >> + >> + return false; >> +} >> + >> +static bool qpic_snand_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) >> +{ >> + if (!spi_mem_default_supports_op(mem, op)) >> + return false; >> + >> + if (op->cmd.nbytes != 1 || op->cmd.buswidth != 1) >> + return false; >> + >> + if (qpic_snand_is_page_op(op)) >> + return true; >> + >> + return ((op->addr.nbytes == 0 || op->addr.buswidth == 1) && > > !op->addr.nbytes and so on Ok > >> + (op->dummy.nbytes == 0 || op->dummy.buswidth == 1) && >> + (op->data.nbytes == 0 || op->data.buswidth == 1)); >> +} >> + >> +static int qpic_snand_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) >> +{ >> + struct qcom_nand_controller *snandc = spi_controller_get_devdata(mem->spi->controller); >> + >> + dev_dbg(snandc->dev, "OP %02x ADDR %08llX@%d:%u DATA %d:%u", op->cmd.opcode, >> + op->addr.val, op->addr.buswidth, op->addr.nbytes, >> + op->data.buswidth, op->data.nbytes); >> + >> + if (qpic_snand_is_page_op(op)) { >> + if (op->data.dir == SPI_MEM_DATA_IN) >> + return qpic_snand_read_page_cache(snandc, op); >> + if (op->data.dir == SPI_MEM_DATA_OUT) >> + return qpic_snand_write_page_cache(snandc, op); >> + } else { >> + return qpic_snand_io_op(snandc, op); >> + } >> + >> + return 0; >> +} >> + >> +static const struct spi_controller_mem_ops qcom_spi_mem_ops = { >> + .supports_op = qpic_snand_supports_op, >> + .exec_op = qpic_snand_exec_op, >> +}; >> + >> +static const struct spi_controller_mem_caps qcom_snand_mem_caps = { >> + .ecc = true, >> +}; >> + >> +static int qcom_snand_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct spi_controller *ctlr; >> + struct qcom_nand_controller *snandc; >> + struct resource *res; >> + const void *dev_data; >> + struct qpic_ecc *ecc; >> + int ret; >> + >> + ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL); >> + if (!ecc) >> + return -ENOMEM; >> + >> + ctlr = __devm_spi_alloc_controller(dev, sizeof(*snandc), false); > > I don't know if that is legitimate, why using __devm? This one I have added as per Mark's comment. > >> + if (!ctlr) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, ctlr); >> + >> + snandc = spi_controller_get_devdata(ctlr); >> + >> + snandc->ctlr = ctlr; >> + snandc->dev = dev; >> + snandc->ecc = ecc; >> + >> + dev_data = of_device_get_match_data(dev); >> + if (!dev_data) { >> + dev_err(&pdev->dev, "failed to get device data\n"); >> + return -ENODEV; >> + } >> + >> + snandc->props = dev_data; >> + snandc->dev = &pdev->dev; >> + >> + snandc->core_clk = devm_clk_get(dev, "core"); >> + if (IS_ERR(snandc->core_clk)) >> + return PTR_ERR(snandc->core_clk); >> + >> + snandc->aon_clk = devm_clk_get(dev, "aon"); >> + if (IS_ERR(snandc->aon_clk)) >> + return PTR_ERR(snandc->aon_clk); >> + >> + snandc->iomacro_clk = devm_clk_get(dev, "iom"); >> + if (IS_ERR(snandc->iomacro_clk)) >> + return PTR_ERR(snandc->iomacro_clk); >> + >> + snandc->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); >> + if (IS_ERR(snandc->base)) >> + return PTR_ERR(snandc->base); >> + >> + snandc->base_phys = res->start; >> + snandc->base_dma = dma_map_resource(dev, res->start, resource_size(res), >> + DMA_BIDIRECTIONAL, 0); >> + if (dma_mapping_error(dev, snandc->base_dma)) >> + return -ENXIO; >> + >> + ret = clk_prepare_enable(snandc->core_clk); >> + if (ret) >> + goto err_core_clk; >> + >> + ret = clk_prepare_enable(snandc->aon_clk); >> + if (ret) >> + goto err_aon_clk; >> + >> + ret = clk_prepare_enable(snandc->iomacro_clk); >> + if (ret) >> + goto err_snandc_alloc; >> + >> + ret = qcom_nandc_alloc(snandc); >> + if (ret) >> + goto err_snandc_alloc; >> + >> + ret = qcom_snand_init(snandc); >> + if (ret) >> + goto err_init; >> + >> + /* setup ECC engine */ >> + snandc->ecc_eng.dev = &pdev->dev; >> + snandc->ecc_eng.integration = NAND_ECC_ENGINE_INTEGRATION_PIPELINED; >> + snandc->ecc_eng.ops = &qcom_snand_ecc_engine_ops_pipelined; >> + snandc->ecc_eng.priv = snandc; >> + >> + ret = nand_ecc_register_on_host_hw_engine(&snandc->ecc_eng); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to register ecc engine.\n"); >> + goto err_init; >> + } >> + >> + ctlr->num_chipselect = QPIC_QSPI_NUM_CS; >> + ctlr->mem_ops = &qcom_spi_mem_ops; >> + ctlr->mem_caps = &qcom_snand_mem_caps; >> + ctlr->dev.of_node = pdev->dev.of_node; >> + ctlr->mode_bits = SPI_TX_DUAL | SPI_RX_DUAL | >> + SPI_TX_QUAD | SPI_RX_QUAD; >> + >> + ret = spi_register_controller(ctlr); >> + if (ret) { >> + dev_err(&pdev->dev, "spi_register_controller failed.\n"); >> + goto err_init; >> + } >> + >> + return 0; >> + >> +err_init: >> + qcom_nandc_unalloc(snandc); >> +err_snandc_alloc: >> + clk_disable_unprepare(snandc->aon_clk); >> +err_aon_clk: >> + clk_disable_unprepare(snandc->core_clk); > > Don't you miss one clock ? Ok will fix in next patch > >> +err_core_clk: >> + dma_unmap_resource(dev, res->start, resource_size(res), >> + DMA_BIDIRECTIONAL, 0); >> + return ret; >> +} >> + >> +static int qcom_snand_remove(struct platform_device *pdev) >> +{ >> + struct spi_controller *ctlr = platform_get_drvdata(pdev); >> + struct qcom_nand_controller *snandc = spi_controller_get_devdata(ctlr); >> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + >> + spi_unregister_controller(ctlr); >> + >> + clk_disable_unprepare(snandc->aon_clk); >> + clk_disable_unprepare(snandc->core_clk); >> + clk_disable_unprepare(snandc->iomacro_clk); >> + >> + dma_unmap_resource(&pdev->dev, snandc->base_dma, resource_size(res), >> + DMA_BIDIRECTIONAL, 0); >> + return 0; >> +} >> + >> +static const struct qcom_nandc_props ipq9574_snandc_props = { >> + .dev_cmd_reg_start = 0x7000, >> + .is_bam = true, > > Same comment about "is_bam" Ok > >> +}; >> + >> +static const struct of_device_id qcom_snandc_of_match[] = { >> + { >> + .compatible = "qcom,spi-qpic-snand", >> + .data = &ipq9574_snandc_props, >> + }, >> + {} >> +} >> +MODULE_DEVICE_TABLE(of, qcom_snandc_of_match); >> + >> +static struct platform_driver qcom_snand_driver = { >> + .driver = { >> + .name = "qcom_snand", >> + .of_match_table = qcom_snandc_of_match, >> + }, >> + .probe = qcom_snand_probe, >> + .remove = qcom_snand_remove, >> +}; >> +module_platform_driver(qcom_snand_driver); >> + >> +MODULE_DESCRIPTION("SPI driver for QPIC QSPI cores"); >> +MODULE_AUTHOR("Md Sadre Alam <quic_mdalam@quicinc.com>"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h >> index aced15866627..4673cd36ff0a 100644 >> --- a/include/linux/mtd/nand-qpic-common.h >> +++ b/include/linux/mtd/nand-qpic-common.h >> @@ -45,6 +45,9 @@ >> #define NAND_DEV_CMD1 0xa4 >> #define NAND_DEV_CMD2 0xa8 >> #define NAND_DEV_CMD_VLD 0xac >> +#define NAND_FLASH_SPI_CFG 0xc0 >> +#define NAND_NUM_ADDR_CYCLES 0xc4 >> +#define NAND_BUSY_CHECK_WAIT_CNT 0xc8 > > If this is specific to the spi-nand implementation, then it's not > common and has no reason to be shared here. Same for the other > definitions of course. Ok > >> #define SFLASHC_BURST_CFG 0xe0 >> #define NAND_ERASED_CW_DETECT_CFG 0xe8 >> #define NAND_ERASED_CW_DETECT_STATUS 0xec >> @@ -61,6 +64,7 @@ >> #define NAND_READ_LOCATION_LAST_CW_1 0xf44 >> #define NAND_READ_LOCATION_LAST_CW_2 0xf48 >> #define NAND_READ_LOCATION_LAST_CW_3 0xf4c >> +#define NAND_FLASH_FEATURES 0xf64 >> >> /* dummy register offsets, used by write_reg_dma */ >> #define NAND_DEV_CMD1_RESTORE 0xdead >> @@ -169,6 +173,7 @@ >> #define OP_CHECK_STATUS 0xc >> #define OP_FETCH_ID 0xb >> #define OP_RESET_DEVICE 0xd >> +#define ACC_FEATURE 0xe >> >> /* Default Value for NAND_DEV_CMD_VLD */ >> #define NAND_DEV_CMD_VLD_VAL (READ_START_VLD | WRITE_START_VLD | \ >> @@ -329,11 +334,53 @@ struct nandc_regs { >> __le32 read_location_last1; >> __le32 read_location_last2; >> __le32 read_location_last3; >> + __le32 spi_cfg; >> + __le32 num_addr_cycle; >> + __le32 busy_wait_cnt; >> + __le32 flash_feature; >> >> __le32 erased_cw_detect_cfg_clr; >> __le32 erased_cw_detect_cfg_set; >> }; >> >> +struct qcom_ecc_stats { >> + u32 corrected; >> + u32 bitflips; >> + u32 failed; >> +}; >> + >> +/* >> + * QPIC ECC data struct >> + * >> + */ >> +struct qpic_ecc { >> + struct device *dev; >> + const struct qpic_ecc_caps *caps; >> + struct completion done; >> + u32 sectors; >> + u8 *eccdata; >> + bool use_ecc; >> + u32 ecc_modes; >> + int ecc_bytes_hw; >> + int spare_bytes; >> + int bbm_size; >> + int ecc_mode; >> + int bytes; >> + int steps; >> + int step_size; >> + int strength; >> + int cw_size; >> + int cw_data; >> + u32 cfg0, cfg1; >> + u32 cfg0_raw, cfg1_raw; >> + u32 ecc_buf_cfg; >> + u32 ecc_bch_cfg; >> + u32 clrflashstatus; >> + u32 clrreadstatus; >> + bool bch_enabled; >> +}; >> + >> +struct qpic_ecc; >> /* >> * NAND controller data struct >> * >> @@ -352,6 +399,7 @@ struct nandc_regs { >> * initialized via DT match data >> * >> * @controller: base controller structure >> + * @ctlr: spi controller structure >> * @host_list: list containing all the chips attached to the >> * controller >> * >> @@ -389,6 +437,7 @@ struct qcom_nand_controller { >> >> struct clk *core_clk; >> struct clk *aon_clk; >> + struct clk *iomacro_clk; >> >> struct nandc_regs *regs; >> struct bam_transaction *bam_txn; >> @@ -396,6 +445,7 @@ struct qcom_nand_controller { >> const struct qcom_nandc_props *props; >> >> struct nand_controller controller; >> + struct spi_controller *ctlr; >> struct list_head host_list; >> >> union { >> @@ -432,6 +482,17 @@ struct qcom_nand_controller { >> >> u32 cmd1, vld; >> bool exec_opwrite; >> + struct qpic_ecc *ecc; >> + struct qcom_ecc_stats ecc_stats; >> + struct nand_ecc_engine ecc_eng; >> + u8 *wbuf; >> + u32 wlen; >> + u32 addr1; >> + u32 addr2; >> + u32 cmd; >> + u32 num_cw; >> + bool oob_read; >> + bool raw; >> }; >> >> /* > > > Thanks, > Miquèl Thanks for reviewing. Will address all the comments in next patch series. Regards, Alam.
On 3/15/2024 5:15 PM, Miquel Raynal wrote: > Hello, > >> +/** >> + * qcom_qpic_bam_dma_done() - Callback for DMA descriptor completion >> + * @data: data pointer >> + * >> + * This function is a callback for DMA descriptor completion >> + */ >> +void qcom_qpic_bam_dma_done(void *data) >> +{ >> + struct bam_transaction *bam_txn = data; >> + >> + /* >> + * In case of data transfer with NAND, 2 callbacks will be generated. >> + * One for command channel and another one for data channel. >> + * If current transaction has data descriptors >> + * (i.e. wait_second_completion is true), then set this to false >> + * and wait for second DMA descriptor completion. >> + */ >> + if (bam_txn->wait_second_completion) >> + bam_txn->wait_second_completion = false; >> + else >> + complete(&bam_txn->txn_done); > > Can't you just call "wait" and "complete" twice? It's supposed to be > handled by the API. This is totally racy. Ok > >> +} >> + >> +/** >> + * qcom_nandc_read_buffer_sync() - Check for dma sync for cpu or device >> + * @nandc: qpic nand controller >> + * @is_cpu: cpu or Device > > ? the naming is really strange dev_to_mem or something like that would > probably be more helpful. Ok > >> + * >> + * This function will check for dma sync for cpu or device >> + */ >> +void qcom_nandc_read_buffer_sync(struct qcom_nand_controller *nandc, >> + bool is_cpu) >> +{ >> + if (!nandc->props->is_bam) >> + return; >> + >> + if (is_cpu) >> + dma_sync_single_for_cpu(nandc->dev, nandc->reg_read_dma, >> + MAX_REG_RD * >> + sizeof(*nandc->reg_read_buf), >> + DMA_FROM_DEVICE); >> + else >> + dma_sync_single_for_device(nandc->dev, nandc->reg_read_dma, >> + MAX_REG_RD * >> + sizeof(*nandc->reg_read_buf), >> + DMA_FROM_DEVICE); >> +} >> + >> +/** >> + * qcom_offset_to_nandc_reg() - Get the actual offset >> + * @regs: pointer to nandc_reg structure >> + * @offset: register offset >> + * >> + * This function will reurn the actual offset for qpic controller register >> + */ >> +__le32 *qcom_offset_to_nandc_reg(struct nandc_regs *regs, int offset) >> +{ >> + switch (offset) { >> + case NAND_FLASH_CMD: >> + return ®s->cmd; >> + case NAND_ADDR0: >> + return ®s->addr0; >> + case NAND_ADDR1: >> + return ®s->addr1; >> + case NAND_FLASH_CHIP_SELECT: >> + return ®s->chip_sel; >> + case NAND_EXEC_CMD: >> + return ®s->exec; >> + case NAND_FLASH_STATUS: >> + return ®s->clrflashstatus; >> + case NAND_DEV0_CFG0: >> + return ®s->cfg0; >> + case NAND_DEV0_CFG1: >> + return ®s->cfg1; >> + case NAND_DEV0_ECC_CFG: >> + return ®s->ecc_bch_cfg; >> + case NAND_READ_STATUS: >> + return ®s->clrreadstatus; >> + case NAND_DEV_CMD1: >> + return ®s->cmd1; >> + case NAND_DEV_CMD1_RESTORE: >> + return ®s->orig_cmd1; >> + case NAND_DEV_CMD_VLD: >> + return ®s->vld; >> + case NAND_DEV_CMD_VLD_RESTORE: >> + return ®s->orig_vld; >> + case NAND_EBI2_ECC_BUF_CFG: >> + return ®s->ecc_buf_cfg; >> + case NAND_READ_LOCATION_0: >> + return ®s->read_location0; >> + case NAND_READ_LOCATION_1: >> + return ®s->read_location1; >> + case NAND_READ_LOCATION_2: >> + return ®s->read_location2; >> + case NAND_READ_LOCATION_3: >> + return ®s->read_location3; >> + case NAND_READ_LOCATION_LAST_CW_0: >> + return ®s->read_location_last0; >> + case NAND_READ_LOCATION_LAST_CW_1: >> + return ®s->read_location_last1; >> + case NAND_READ_LOCATION_LAST_CW_2: >> + return ®s->read_location_last2; >> + case NAND_READ_LOCATION_LAST_CW_3: >> + return ®s->read_location_last3; > > Why do you need this indirection? This indirection I believe is needed by the write_reg_dma function, wherein a bunch of registers are modified based on a starting register. Can I change this in a separate cleanup series as a follow up to this? > >> + default: >> + return NULL; >> + } >> +} >> + > > ... > >> +/** >> + * qcom_clear_bam_transaction() - Clears the BAM transaction >> + * @nandc: qpic nand controller >> + * >> + * This function will clear the BAM transaction indexes. >> + */ >> +void qcom_clear_bam_transaction(struct qcom_nand_controller *nandc) >> +{ >> + struct bam_transaction *bam_txn = nandc->bam_txn; >> + >> + if (!nandc->props->is_bam) >> + return; >> + >> + bam_txn->bam_ce_pos = 0; >> + bam_txn->bam_ce_start = 0; >> + bam_txn->cmd_sgl_pos = 0; >> + bam_txn->cmd_sgl_start = 0; >> + bam_txn->tx_sgl_pos = 0; >> + bam_txn->tx_sgl_start = 0; >> + bam_txn->rx_sgl_pos = 0; >> + bam_txn->rx_sgl_start = 0; >> + bam_txn->last_data_desc = NULL; >> + bam_txn->wait_second_completion = false; > > What about using memset here? Ok > >> + >> + sg_init_table(bam_txn->cmd_sgl, nandc->max_cwperpage * >> + QPIC_PER_CW_CMD_SGL); >> + sg_init_table(bam_txn->data_sgl, nandc->max_cwperpage * >> + QPIC_PER_CW_DATA_SGL); >> + >> + reinit_completion(&bam_txn->txn_done); >> +} > > ... > >> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h >> new file mode 100644 >> index 000000000000..aced15866627 >> --- /dev/null >> +++ b/include/linux/mtd/nand-qpic-common.h >> @@ -0,0 +1,486 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * QCOM QPIC common APIs header file >> + * >> + * Copyright (c) 2023 Qualcomm Inc. >> + * Authors: Md sadre Alam <quic_mdalam@quicinc.com> >> + * Sricharan R <quic_srichara@quicinc.com> >> + * Varadarajan Narayanan <quic_varada@quicinc.com> >> + * >> + */ >> +#ifndef __MTD_NAND_QPIC_COMMON_H__ >> +#define __MTD_NAND_QPIC_COMMON_H__ >> + >> +#include <linux/bitops.h> >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/dmaengine.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/dma/qcom_adm.h> >> +#include <linux/dma/qcom_bam_dma.h> >> +#include <linux/module.h> >> +#include <linux/mtd/partitions.h> >> +#include <linux/mtd/rawnand.h> > > You really need this? Yes , since some generic structure used here. > >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> + >> +/* NANDc reg offsets */ >> +#define NAND_FLASH_CMD 0x00 >> +#define NAND_ADDR0 0x04 >> +#define NAND_ADDR1 0x08 >> +#define NAND_FLASH_CHIP_SELECT 0x0c >> +#define NAND_EXEC_CMD 0x10 >> +#define NAND_FLASH_STATUS 0x14 >> +#define NAND_BUFFER_STATUS 0x18 >> +#define NAND_DEV0_CFG0 0x20 >> +#define NAND_DEV0_CFG1 0x24 >> +#define NAND_DEV0_ECC_CFG 0x28 >> +#define NAND_AUTO_STATUS_EN 0x2c >> +#define NAND_DEV1_CFG0 0x30 >> +#define NAND_DEV1_CFG1 0x34 >> +#define NAND_READ_ID 0x40 >> +#define NAND_READ_STATUS 0x44 >> +#define NAND_DEV_CMD0 0xa0 >> +#define NAND_DEV_CMD1 0xa4 >> +#define NAND_DEV_CMD2 0xa8 >> +#define NAND_DEV_CMD_VLD 0xac >> +#define SFLASHC_BURST_CFG 0xe0 >> +#define NAND_ERASED_CW_DETECT_CFG 0xe8 >> +#define NAND_ERASED_CW_DETECT_STATUS 0xec >> +#define NAND_EBI2_ECC_BUF_CFG 0xf0 >> +#define FLASH_BUF_ACC 0x100 >> + > > ... > >> +/* >> + * This data type corresponds to the NAND controller properties which varies >> + * among different NAND controllers. >> + * @ecc_modes - ecc mode for NAND > > Should this member be an enum? Ok , Will fix in next patch > >> + * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset >> + * @is_bam - whether NAND controller is using BAM > > has_bam_support? supports_bam? Ok > >> + * @is_qpic - whether NAND CTRL is part of qpic IP > > CTRL? do you mean controller? Yes. > >> + * @qpic_v2 - flag to indicate QPIC IP version 2 >> + * @use_codeword_fixup - whether NAND has different layout for boot partitions > > The doc is clear but the member name is terrible. Please clarify the > naming. Ok > >> + */ >> +struct qcom_nandc_props { >> + u32 ecc_modes; >> + u32 dev_cmd_reg_start; >> + bool is_bam; >> + bool is_qpic; >> + bool qpic_v2; >> + bool use_codeword_fixup; >> +}; >> + >> +void config_nand_page_read(struct nand_chip *chip); >> +void qcom_qpic_bam_dma_done(void *data); >> +void qcom_nandc_read_buffer_sync(struct qcom_nand_controller *nandc, bool is_cpu); >> +__le32 *qcom_offset_to_nandc_reg(struct nandc_regs *regs, int offset); >> +int qcom_prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read, >> + int reg_off, const void *vaddr, int size, >> + bool flow_control); >> +int qcom_submit_descs(struct qcom_nand_controller *nandc); >> +int qcom_prepare_bam_async_desc(struct qcom_nand_controller *nandc, >> + struct dma_chan *chan, unsigned long flags); >> +int qcom_prep_bam_dma_desc_cmd(struct qcom_nand_controller *nandc, bool read, >> + int reg_off, const void *vaddr, >> + int size, unsigned int flags); >> +int qcom_prep_bam_dma_desc_data(struct qcom_nand_controller *nandc, bool read, >> + const void *vaddr, >> + int size, unsigned int flags); >> +int qcom_read_reg_dma(struct qcom_nand_controller *nandc, int first, >> + int num_regs, unsigned int flags); >> +int qcom_write_reg_dma(struct qcom_nand_controller *nandc, int first, >> + int num_regs, unsigned int flags); >> +int qcom_read_data_dma(struct qcom_nand_controller *nandc, int reg_off, >> + const u8 *vaddr, int size, unsigned int flags); >> +int qcom_write_data_dma(struct qcom_nand_controller *nandc, int reg_off, >> + const u8 *vaddr, int size, unsigned int flags); >> +struct bam_transaction *qcom_alloc_bam_transaction(struct qcom_nand_controller *nandc); >> +void qcom_clear_bam_transaction(struct qcom_nand_controller *nandc); >> +void qcom_nandc_unalloc(struct qcom_nand_controller *nandc); >> +int qcom_nandc_alloc(struct qcom_nand_controller *nandc); >> +void qcom_clear_read_regs(struct qcom_nand_controller *nandc); >> +void qcom_free_bam_transaction(struct qcom_nand_controller *nandc); >> +#endif > > I made several requests on code that already exists, please add these > changes to your series. ok > > > Also, this patching being big, please split: > 1- rename your all your symbols to start with the same prefix > (qcom_nand_ instead of nothing or just qcom) Ok > 2- then perform the move, which should not require changing the names > of all the functions everywhere. Ok > > Thanks, > Miquèl Thanks for reviewing. Will address all the comments in next patch series. Reagrds, Alam.
[-- Attachment #1: Type: text/plain, Size: 734 bytes --] Hi, > Regardless, this patch actually does not contain any code for EEPROM > support I have just mentioned it to give more context on why mikroBUS > manifest is the focus of this patch instead of DT overlay or something > else. Right, and I think this is the crux here. Why can't you use DT overlays? The manifest files, seem to be yet another hardware description (method) and we already have DT. Can't we have some kind of userspace helper that could translate them to DT overlays? That way, you could also handle the EEPROM vs non-EEPROM case, or have some other kind of method to load a DT overlay. Admittedly, I've never worked with in-kernel overlays, but AFAIK they work with some subsystems. -michael [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 252 bytes --]
Hi Ayush, On 18/03/24 01:07, Ayush Singh wrote: > DONOTMERGE > > this patch depends on Patch 1, 2, 3 > > mikroBUS driver aims to split the platform aspects of mikroBUS (pinmux, > SPI/I2C/GPIO controller .etc) from the add-on board information, thus > requiring one device tree overlay per port and just a single manifest > describing the add-on board. > > The driver exposes a sysfs entry that allows passing mikroBUS manifest of > add-on board to the driver. The driver then parses this manifest, sets up > the pins and protocols and allows using the appropriate Linux driver. Here > is an example: In v3 series, Krzysztof mentioned that you need to document the ABI, the feedback was not considered in the series, see: https://www.kernel.org/doc/Documentation/ABI/README Also he mentioned that the mechanism of adding devices from userspace needs to be discussed first, that feedback also was not responded to. These add-on boards are not electrically hot-pluggable and this kind of sysfs interface seems to give the idea that you can add an add-on board on a running system (which is not recommended due to latchup/other electrical failures), I will recommend you discuss the pending feedback and discuss on acceptable solutions before spinning future revisions. > > ``` > cat /lib/firmware/mikrobus/AMBIENT-2-CLICK.mnfb > /sys/bus/mikrobus/devices/mikrobus-0/new_device > ``` > > Another sysfs entry is exposed that allows removing a previously > registered mikrobus add-on board: > > ``` > echo " " > /sys/bus/mikrobus/devices/mikrobus-0/delete_device > ``` > > 100s of mikroBUS addon board manifests can be found in the clickID > repository. > > In the future the driver also aims to support plug and play support > using 1-wire EEPROM and mikroBUS over greybus. > > Link: https://www.mikroe.com/clickid ClickID > > Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org> > Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org> > Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> > --- > MAINTAINERS | 1 + > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/mikrobus/Kconfig | 15 + > drivers/misc/mikrobus/Makefile | 5 + > drivers/misc/mikrobus/mikrobus_core.c | 696 ++++++++++++++++++++++ > drivers/misc/mikrobus/mikrobus_core.h | 151 +++++ > drivers/misc/mikrobus/mikrobus_manifest.c | 503 ++++++++++++++++ > drivers/misc/mikrobus/mikrobus_manifest.h | 29 + > 9 files changed, 1402 insertions(+) > create mode 100644 drivers/misc/mikrobus/Kconfig > create mode 100644 drivers/misc/mikrobus/Makefile > create mode 100644 drivers/misc/mikrobus/mikrobus_core.c > create mode 100644 drivers/misc/mikrobus/mikrobus_core.h > create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.c > create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 69418a058c6b..83bc5e48bef9 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14772,6 +14772,7 @@ M: Ayush Singh <ayushdevel1325@gmail.com> > M: Vaishnav M A <vaishnav@beagleboard.org> > S: Maintained > F: Documentation/devicetree/bindings/misc/mikrobus-connector.yaml > +F: drivers/misc/mikrobus/* > > MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT > M: Luka Kovacic <luka.kovacic@sartura.hr> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 4fb291f0bf7c..3d5c36205c6c 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -591,4 +591,5 @@ source "drivers/misc/cardreader/Kconfig" > source "drivers/misc/uacce/Kconfig" > source "drivers/misc/pvpanic/Kconfig" > source "drivers/misc/mchp_pci1xxxx/Kconfig" > +source "drivers/misc/mikrobus/Kconfig" > endmenu > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index ea6ea5bbbc9c..b9ac88055b87 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -68,3 +68,4 @@ obj-$(CONFIG_TMR_INJECT) += xilinx_tmr_inject.o > obj-$(CONFIG_TPS6594_ESM) += tps6594-esm.o > obj-$(CONFIG_TPS6594_PFSM) += tps6594-pfsm.o > obj-$(CONFIG_NSM) += nsm.o > +obj-y += mikrobus/ > diff --git a/drivers/misc/mikrobus/Kconfig b/drivers/misc/mikrobus/Kconfig > new file mode 100644 > index 000000000000..aa57b994dc66 > --- /dev/null > +++ b/drivers/misc/mikrobus/Kconfig > @@ -0,0 +1,15 @@ > +menuconfig MIKROBUS > + tristate "Module for instantiating devices on mikroBUS ports" > + depends on GPIOLIB > + help > + This option enables the mikroBUS driver. mikroBUS is an add-on > + board socket standard that offers maximum expandability with > + the smallest number of pins. The mikroBUS driver instantiates > + devices on a mikroBUS port described mikroBUS manifest which is > + passed using a sysfs interface. > + > + > + Say Y here to enable support for this driver. > + > + To compile this code as a module, chose M here: the module > + will be called mikrobus.ko > diff --git a/drivers/misc/mikrobus/Makefile b/drivers/misc/mikrobus/Makefile > new file mode 100644 > index 000000000000..0e51c5a7db4b > --- /dev/null > +++ b/drivers/misc/mikrobus/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# mikroBUS Core > + > +obj-$(CONFIG_MIKROBUS) += mikrobus.o > +mikrobus-y := mikrobus_core.o mikrobus_manifest.o > diff --git a/drivers/misc/mikrobus/mikrobus_core.c b/drivers/misc/mikrobus/mikrobus_core.c > new file mode 100644 > index 000000000000..6aa20cef8e3b > --- /dev/null > +++ b/drivers/misc/mikrobus/mikrobus_core.c > @@ -0,0 +1,696 @@ > +// SPDX-License-Identifier: GPL-2.0: > +/* > + * mikroBUS driver for instantiating add-on > + * board devices with an identifier EEPROM > + * > + * Copyright 2020 Vaishnav M A, BeagleBoard.org Foundation. > + * Copyright 2024 Ayush Singh <ayushdevel1325@gmail.com> > + */ > + > +#define pr_fmt(fmt) "mikrobus:%s: " fmt, __func__ > + > +#include "linux/gpio/driver.h" > +#include "linux/gpio/machine.h" > +#include "linux/gpio/consumer.h" > +#include "linux/greybus/greybus_manifest.h" > +#include "linux/i2c.h" > +#include "linux/irq.h" > +#include "linux/pinctrl/consumer.h" > +#include "linux/platform_device.h" > +#include "linux/spi/spi.h" > + > +#include "mikrobus_core.h" > +#include "mikrobus_manifest.h" > + > +static struct class_compat *mikrobus_port_compat_class; > + > +static const struct bus_type mikrobus_bus_type = { > + .name = "mikrobus", > +}; > + > +static int mikrobus_board_register(struct mikrobus_port *port, > + struct addon_board_info *board); > +static void mikrobus_board_unregister(struct mikrobus_port *port, > + struct addon_board_info *board); > + > +/* > + * mikrobus_pinctrl_select: Select pinctrl state for mikrobus pin > + * > + * @port: mikrobus port > + * @pinctrl_selected: pinctrl state to be selected > + */ > +static int mikrobus_pinctrl_select(struct mikrobus_port *port, > + const char *pinctrl_selected) > +{ > + struct pinctrl_state *state; > + int ret; > + > + state = pinctrl_lookup_state(port->pinctrl, pinctrl_selected); > + if (IS_ERR(state)) { > + return dev_err_probe(&port->dev, PTR_ERR(state), > + "failed to find state %s", > + pinctrl_selected); > + } > + > + ret = pinctrl_select_state(port->pinctrl, state); > + if (ret) { > + return dev_err_probe(&port->dev, ret, > + "failed to select state %s", > + pinctrl_selected); > + } > + dev_dbg(&port->dev, "setting pinctrl %s", pinctrl_selected); > + > + return 0; > +} > + > +/* > + * mikrobus_pinctrl_setup: Setup mikrobus pins to either default of gpio > + * > + * @port: mikrobus port > + * @board: mikrobus board or NULL for default state > + * > + * returns 0 on success, negative error code on failure > + */ > +static int mikrobus_pinctrl_setup(struct mikrobus_port *port, > + struct addon_board_info *board) > +{ > + int ret; > + > + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM) > + ret = mikrobus_pinctrl_select(port, "pwm_default"); > + else > + ret = mikrobus_pinctrl_select(port, "pwm_gpio"); > + if (ret) > + return ret; > + > + if (!board || board->pin_state[MIKROBUS_PIN_RX] == MIKROBUS_STATE_UART) > + ret = mikrobus_pinctrl_select(port, "uart_default"); > + else > + ret = mikrobus_pinctrl_select(port, "uart_gpio"); > + if (ret) > + return ret; > + > + if (!board || board->pin_state[MIKROBUS_PIN_SCL] == MIKROBUS_STATE_I2C) > + ret = mikrobus_pinctrl_select(port, "i2c_default"); > + else > + ret = mikrobus_pinctrl_select(port, "i2c_gpio"); > + if (ret) > + return ret; > + > + if (!board || board->pin_state[MIKROBUS_PIN_MOSI] == MIKROBUS_STATE_SPI) > + ret = mikrobus_pinctrl_select(port, "spi_default"); > + else > + ret = mikrobus_pinctrl_select(port, "spi_gpio"); > + > + return ret; > +} > + > +/* > + * new_device_store: Expose sysfs entry for adding new board > + * > + * new_device_store: Allows userspace to add mikroBUS boards that lack 1-wire > + * EEPROM for board identification by manually passing mikroBUS manifest > + */ > +static ssize_t new_device_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t count) > +{ > + struct mikrobus_port *port = to_mikrobus_port(dev); > + struct addon_board_info *board; > + int ret; > + > + if (port->board) > + return dev_err_probe(dev, -EBUSY, > + "already has board registered"); > + > + board = devm_kzalloc(&port->dev, sizeof(*board), GFP_KERNEL); > + if (!board) > + return -ENOMEM; > + > + INIT_LIST_HEAD(&board->manifest_descs); > + INIT_LIST_HEAD(&board->devices); > + > + ret = mikrobus_manifest_parse(board, (void *)buf, count); > + if (ret < 0) { > + ret = dev_err_probe(dev, -EINVAL, "failed to parse manifest"); > + goto err_free_board; > + } > + > + ret = mikrobus_board_register(port, board); > + if (ret) { > + ret = dev_err_probe(dev, -EINVAL, "failed to register board %s", > + board->name); > + goto err_free_board; > + } > + > + return count; > + > +err_free_board: > + devm_kfree(&port->dev, board); > + return ret; > +} > +static DEVICE_ATTR_WO(new_device); > + > +/* > + * delete_device_store: Expose sysfs entry for deleting board > + */ > +static ssize_t delete_device_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct mikrobus_port *port = to_mikrobus_port(dev); > + > + if (!port->board) > + return dev_err_probe(dev, -ENODEV, > + "does not have registered boards"); > + > + mikrobus_board_unregister(port, port->board); > + return count; > +} > +static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, 0200, NULL, > + delete_device_store); > + > +static struct attribute *mikrobus_port_attrs[] = { &dev_attr_new_device.attr, > + &dev_attr_delete_device.attr, > + NULL }; > +ATTRIBUTE_GROUPS(mikrobus_port); > + > +static void mikrobus_port_release(struct device *dev) > +{ > +} > + > +static const struct device_type mikrobus_port_type = { > + .groups = mikrobus_port_groups, > + .release = mikrobus_port_release, > +}; > + > +static int mikrobus_irq_get(struct mikrobus_port *port, int irqno, int irq_type) > +{ > + int irq; > + > + if (irqno > port->gpios->ndescs - 1) > + return dev_err_probe(&port->dev, -ENODEV, > + "GPIO %d does not exist", irqno); > + > + irq = gpiod_to_irq(port->gpios->desc[irqno]); > + if (irq < 0) > + return dev_err_probe(&port->dev, -EINVAL, > + "could not get irq %d", irqno); > + > + irq_set_irq_type(irq, irq_type); > + > + return irq; > +} > + > +static int mikrobus_gpio_setup(struct gpio_desc *gpio, int gpio_state) > +{ > + switch (gpio_state) { > + case MIKROBUS_STATE_INPUT: > + return gpiod_direction_input(gpio); > + case MIKROBUS_STATE_OUTPUT_HIGH: > + return gpiod_direction_output(gpio, 1); > + case MIKROBUS_STATE_OUTPUT_LOW: > + return gpiod_direction_output(gpio, 0); > + case MIKROBUS_STATE_PWM: > + case MIKROBUS_STATE_SPI: > + case MIKROBUS_STATE_I2C: > + default: > + return 0; > + } > +} > + > +static char *mikrobus_gpio_chip_name_get(struct mikrobus_port *port, int gpio) > +{ > + struct gpio_chip *gpiochip; > + > + if (gpio > port->gpios->ndescs - 1) > + return NULL; > + > + gpiochip = gpiod_to_chip(port->gpios->desc[gpio]); > + return kmemdup(gpiochip->label, strlen(gpiochip->label), GFP_KERNEL); > +} > + > +static int mikrobus_gpio_hwnum_get(struct mikrobus_port *port, int gpio) > +{ > + struct gpio_chip *gpiochip; > + > + if (gpio > port->gpios->ndescs - 1) > + return -ENODEV; > + > + gpiochip = gpiod_to_chip(port->gpios->desc[gpio]); > + return desc_to_gpio(port->gpios->desc[gpio]) - gpiochip->base; > +} > + > +static void mikrobus_board_device_release_all(struct addon_board_info *info) > +{ > + struct board_device_info *dev, *next; > + > + list_for_each_entry_safe(dev, next, &info->devices, links) { > + list_del(&dev->links); > + kfree(dev); > + } > +} > + > +static int mikrobus_device_register(struct mikrobus_port *port, > + struct board_device_info *dev, > + char *board_name) > +{ > + struct gpiod_lookup_table *lookup; > + struct spi_board_info *spi_info; > + struct i2c_board_info *i2c_info; > + struct platform_device *pdev; > + struct fwnode_handle *fwnode; > + struct spi_device *spi; > + struct i2c_client *i2c; > + int i, ret; > + > + dev_info(&port->dev, "registering device : %s", dev->drv_name); > + > + if (dev->gpio_lookup) { > + lookup = dev->gpio_lookup; > + > + switch (dev->protocol) { > + case GREYBUS_PROTOCOL_SPI: > + lookup->dev_id = kasprintf(GFP_KERNEL, "%s.%u", > + dev->drv_name, > + port->chip_select[dev->reg]); > + break; > + case GREYBUS_PROTOCOL_RAW: > + lookup->dev_id = kasprintf(GFP_KERNEL, "%s.%u", > + dev->drv_name, dev->reg); > + break; > + default: > + lookup->dev_id = kmemdup(dev->drv_name, > + strlen(dev->drv_name), > + GFP_KERNEL); > + } > + > + dev_info(&port->dev, "adding lookup table : %s", > + lookup->dev_id); > + > + for (i = 0; i < dev->num_gpio_resources; i++) { > + lookup->table[i].key = mikrobus_gpio_chip_name_get( > + port, lookup->table[i].chip_hwnum); > + lookup->table[i].chip_hwnum = mikrobus_gpio_hwnum_get( > + port, lookup->table[i].chip_hwnum); > + } > + > + gpiod_add_lookup_table(lookup); > + } > + > + switch (dev->protocol) { > + case GREYBUS_PROTOCOL_SPI: > + spi_info = > + devm_kzalloc(&port->dev, sizeof(*spi_info), GFP_KERNEL); > + strscpy_pad(spi_info->modalias, dev->drv_name, > + sizeof(spi_info->modalias)); > + if (dev->irq) > + spi_info->irq = > + mikrobus_irq_get(port, dev->irq, dev->irq_type); > + if (dev->properties) { > + fwnode = fwnode_create_software_node(dev->properties, > + NULL); > + spi_info->swnode = to_software_node(fwnode); > + } > + spi_info->chip_select = port->chip_select[dev->reg]; > + spi_info->max_speed_hz = dev->max_speed_hz; > + spi_info->mode = dev->mode; > + > + spi = spi_new_device(port->spi_ctrl, spi_info); > + devm_kfree(&port->dev, spi_info); > + if (!spi) > + return dev_err_probe(&port->dev, -ENODEV, > + "failed to register spi device"); > + dev->dev_client = (void *)spi; > + break; > + case GREYBUS_PROTOCOL_I2C: > + i2c_info = > + devm_kzalloc(&port->dev, sizeof(*i2c_info), GFP_KERNEL); > + if (!i2c_info) > + return -ENOMEM; > + > + strscpy_pad(i2c_info->type, dev->drv_name, > + sizeof(i2c_info->type)); > + if (dev->irq) > + i2c_info->irq = > + mikrobus_irq_get(port, dev->irq, dev->irq_type); > + if (dev->properties) { > + fwnode = fwnode_create_software_node(dev->properties, > + NULL); > + i2c_info->swnode = to_software_node(fwnode); > + } > + i2c_info->addr = dev->reg; > + > + i2c = i2c_new_client_device(port->i2c_adap, i2c_info); > + devm_kfree(&port->dev, i2c_info); > + if (IS_ERR(dev->dev_client)) > + return dev_err_probe(&port->dev, > + PTR_ERR(dev->dev_client), > + "failed to register i2c device"); > + dev->dev_client = (void *)i2c; > + break; > + case GREYBUS_PROTOCOL_RAW: > + pdev = platform_device_alloc(dev->drv_name, 0); > + if (!pdev) > + return -ENOMEM; > + > + if (dev->properties) { > + ret = device_create_managed_software_node( > + &pdev->dev, dev->properties, NULL); > + if (ret) > + return dev_err_probe( > + &port->dev, ret, > + "failed to create software node"); > + } > + ret = platform_device_add(dev->dev_client); > + if (ret) > + return dev_err_probe( > + &port->dev, ret, > + "failed to register platform device"); > + dev->dev_client = (void *)pdev; > + break; > + case GREYBUS_PROTOCOL_UART: > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static void mikrobus_device_unregister(struct mikrobus_port *port, > + struct board_device_info *dev, > + char *board_name) > +{ > + dev_info(&port->dev, "removing device %s", dev->drv_name); > + if (dev->gpio_lookup) { > + gpiod_remove_lookup_table(dev->gpio_lookup); > + kfree(dev->gpio_lookup->dev_id); > + kfree(dev->gpio_lookup); > + } > + > + kfree(dev->properties); > + > + switch (dev->protocol) { > + case GREYBUS_PROTOCOL_SPI: > + spi_unregister_device((struct spi_device *)dev->dev_client); > + break; > + case GREYBUS_PROTOCOL_I2C: > + i2c_unregister_device((struct i2c_client *)dev->dev_client); > + break; > + case GREYBUS_PROTOCOL_RAW: > + platform_device_unregister( > + (struct platform_device *)dev->dev_client); > + break; > + case GREYBUS_PROTOCOL_UART: > + break; > + } > +} > + > +static int mikrobus_board_register(struct mikrobus_port *port, > + struct addon_board_info *board) > +{ > + struct board_device_info *devinfo, *next; > + int ret, i; > + > + if (WARN_ON(list_empty(&board->devices))) > + return false; > + > + if (port->pinctrl) { > + ret = mikrobus_pinctrl_setup(port, board); > + if (ret) > + dev_err(&port->dev, > + "failed to setup pinctrl state [%d]", ret); > + } > + > + if (port->gpios) { > + for (i = 0; i < port->gpios->ndescs; i++) { > + ret = mikrobus_gpio_setup(port->gpios->desc[i], > + board->pin_state[i]); > + if (ret) > + dev_err(&port->dev, > + "failed to setup gpio %d, state %d", i, > + board->pin_state[i]); > + > + gpiochip_free_own_desc(port->gpios->desc[i]); > + } > + } > + > + list_for_each_entry_safe(devinfo, next, &board->devices, links) > + mikrobus_device_register(port, devinfo, board->name); > + > + port->board = board; > + return 0; > +} > + > +static void mikrobus_board_unregister(struct mikrobus_port *port, > + struct addon_board_info *board) > +{ > + struct board_device_info *devinfo, *next; > + > + if (WARN_ON(list_empty(&board->devices))) > + return; > + > + list_for_each_entry_safe(devinfo, next, &board->devices, links) > + mikrobus_device_unregister(port, devinfo, board->name); > + > + mikrobus_board_device_release_all(board); > + devm_kfree(&port->dev, board); > + port->board = NULL; > +} > + > +static int mikrobus_port_register(struct mikrobus_port *port) > +{ > + int ret; > + > + port->dev.bus = &mikrobus_bus_type; > + port->dev.type = &mikrobus_port_type; > + dev_set_name(&port->dev, "mikrobus-%d", port->id); > + > + dev_info(&port->dev, "registering port %s", dev_name(&port->dev)); > + > + ret = device_register(&port->dev); > + if (ret) > + return dev_err_probe(&port->dev, ret, > + "port '%d': can't register device (%d)", > + port->id, ret); > + > + ret = class_compat_create_link(mikrobus_port_compat_class, &port->dev, > + port->dev.parent); > + if (ret) > + dev_warn(&port->dev, > + "failed to create compatibility class link"); > + > + return ret; > +} > + > +static void mikrobus_port_delete(struct mikrobus_port *port) > +{ > + if (port->board) > + return dev_err( > + &port->dev, > + "attempting to delete port with registered boards, port [%s]", > + dev_name(&port->dev)); > + > + class_compat_remove_link(mikrobus_port_compat_class, &port->dev, > + port->dev.parent); > + > + devm_pinctrl_put(port->pinctrl); > + put_device(&port->spi_ctrl->dev); > + gpiod_put_array(port->gpios); > + put_device(&port->i2c_adap->dev); > + > + device_unregister(&port->dev); > + memset(&port->dev, 0, sizeof(port->dev)); > +} > + > +static int mikrobus_port_probe_pinctrl_setup(struct mikrobus_port *port) > +{ > + struct device *dev = port->dev.parent; > + struct pinctrl_state *state; > + int ret; > + > + state = pinctrl_lookup_state(port->pinctrl, PINCTRL_STATE_DEFAULT); > + if (IS_ERR(state)) > + return dev_err_probe(dev, PTR_ERR(state), > + "failed to find state %s", > + PINCTRL_STATE_DEFAULT); > + > + ret = pinctrl_select_state(port->pinctrl, state); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to select state %s", > + PINCTRL_STATE_DEFAULT); > + > + ret = mikrobus_pinctrl_setup(port, NULL); > + if (ret) > + dev_err(dev, "failed to select pinctrl states [%d]", ret); > + > + return ret; > +} > + > +static int mikrobus_port_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mikrobus_port *port; > + struct device_node *np; > + int ret; > + > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); > + if (!port) > + return -ENOMEM; > + > + port->dev.parent = dev; > + port->dev.of_node = pdev->dev.of_node; > + > + /* port id */ > + port->id = of_alias_get_id(dev->of_node, "mikrobus"); > + if (port->id) { > + ret = dev_err_probe(dev, -EINVAL, "invalid mikrobus id"); > + goto err_port; > + } > + > + /* I2C setup */ > + np = of_parse_phandle(dev->of_node, "i2c-adapter", 0); > + if (!np) { > + ret = dev_err_probe(dev, -ENODEV, "cannot parse i2c-adapter"); > + goto err_port; > + } > + port->i2c_adap = of_find_i2c_adapter_by_node(np); > + of_node_put(np); > + if (!port->i2c_adap) { > + ret = dev_err_probe(dev, -ENODEV, "cannot find i2c adapter"); > + goto err_port; > + } > + > + /* GPIO setup */ > + port->gpios = gpiod_get_array(dev, "mikrobus", GPIOD_OUT_LOW); > + if (IS_ERR(port->gpios)) { > + ret = dev_err_probe(dev, PTR_ERR(port->gpios), > + "failed to get gpio array [%ld]", > + PTR_ERR(port->gpios)); > + goto free_i2c; > + } > + > + /* SPI setup */ > + np = of_parse_phandle(dev->of_node, "spi-controller", 0); > + if (!np) { > + ret = dev_err_probe(dev, -ENODEV, > + "cannot parse spi-controller"); > + goto free_gpio; > + } > + port->spi_ctrl = of_find_spi_controller_by_node(np); > + of_node_put(np); > + if (!port->spi_ctrl) { > + ret = dev_err_probe(dev, -ENODEV, "cannot find spi controller"); > + goto free_gpio; > + } > + ret = device_property_read_u32_array(dev, "spi-cs", port->chip_select, > + MIKROBUS_NUM_CS); > + if (ret) { > + dev_err(dev, "failed to get spi-cs [%d]", ret); > + goto free_spi; > + } > + > + /* pinctrl setup */ > + port->pinctrl = devm_pinctrl_get(dev); > + if (IS_ERR(port->pinctrl)) { > + ret = dev_err_probe(dev, PTR_ERR(port->pinctrl), > + "failed to get pinctrl [%ld]", > + PTR_ERR(port->pinctrl)); > + goto free_spi; > + } > + ret = mikrobus_port_probe_pinctrl_setup(port); > + if (ret) { > + dev_err(dev, "failed to setup pinctrl [%d]", ret); > + goto free_pinctrl; > + } > + > + /* TODO: UART */ > + /* TODO: PWM */ > + > + ret = mikrobus_port_register(port); > + if (ret) { > + dev_err(dev, "port : can't register port [%d]", ret); > + goto free_pinctrl; > + } > + > + platform_set_drvdata(pdev, port); > + > + return 0; > + > +free_pinctrl: > + devm_pinctrl_put(port->pinctrl); > +free_spi: > + put_device(&port->spi_ctrl->dev); > +free_gpio: > + gpiod_put_array(port->gpios); > +free_i2c: > + put_device(&port->i2c_adap->dev); > +err_port: > + put_device(&port->dev); > + return ret; > +} > + > +static int mikrobus_port_remove(struct platform_device *pdev) > +{ > + struct mikrobus_port *port = platform_get_drvdata(pdev); > + > + mikrobus_port_delete(port); > + return 0; > +} > + > +static const struct of_device_id mikrobus_port_of_match[] = { > + { .compatible = "mikrobus-connector" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, mikrobus_port_of_match); > + > +static struct platform_driver mikrobus_port_driver = { > + .probe = mikrobus_port_probe, > + .remove = mikrobus_port_remove, > + .driver = { > + .name = "mikrobus", > + .of_match_table = mikrobus_port_of_match, > + }, > +}; > + > +static int mikrobus_init(void) > +{ > + int ret; > + > + ret = bus_register(&mikrobus_bus_type); > + if (ret) { > + pr_err("bus_register failed (%d)", ret); > + return ret; > + } > + > + mikrobus_port_compat_class = class_compat_register("mikrobus-port"); > + if (!mikrobus_port_compat_class) { > + ret = -ENOMEM; > + pr_err("class_compat register failed (%d)", ret); > + goto class_err; > + } > + > + ret = platform_driver_register(&mikrobus_port_driver); > + if (ret) > + pr_err("driver register failed [%d]", ret); > + > + return ret; > + > +class_err: > + bus_unregister(&mikrobus_bus_type); > + return ret; > +} > +subsys_initcall(mikrobus_init); > + > +static void mikrobus_exit(void) > +{ > + platform_driver_unregister(&mikrobus_port_driver); > + bus_unregister(&mikrobus_bus_type); > + class_compat_unregister(mikrobus_port_compat_class); > +} > +module_exit(mikrobus_exit); > + > +MODULE_AUTHOR("Vaishnav M A <vaishnav@beagleboard.org>"); > +MODULE_AUTHOR("Ayush Singh <ayushdevel1325@beagleboard.org>"); This e-mail is different from your Signed-off-by:/ MAINTAINERS entry, was this intentional? Thanks and Regards, Vaishnav > +MODULE_DESCRIPTION("mikroBUS main module"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/misc/mikrobus/mikrobus_core.h b/drivers/misc/mikrobus/mikrobus_core.h > new file mode 100644 > index 000000000000..1d41ee32ca94 > --- /dev/null > +++ b/drivers/misc/mikrobus/mikrobus_core.h > @@ -0,0 +1,151 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * mikroBUS Driver for instantiating add-on board devices > + * > + * Copyright 2020 Vaishnav M A, BeagleBoard.org Foundation. > + * Copyright 2024 Ayush Singh <ayushdevel1325@gmail.com> > + */ > + > +#ifndef __MIKROBUS_H > +#define __MIKROBUS_H > + > +#include "linux/device.h" > + > +#define MIKROBUS_VERSION_MAJOR 0x00 > +#define MIKROBUS_VERSION_MINOR 0x03 > + > +#define MIKROBUS_NUM_PINCTRL_STATE 4 > +#define MIKROBUS_NUM_CS 2 > + > +#define MIKROBUS_PINCTRL_PWM 0 > +#define MIKROBUS_PINCTRL_UART 1 > +#define MIKROBUS_PINCTRL_I2C 2 > +#define MIKROBUS_PINCTRL_SPI 3 > + > +enum mikrobus_property_type { > + MIKROBUS_PROPERTY_TYPE_MIKROBUS = 0x00, > + MIKROBUS_PROPERTY_TYPE_PROPERTY, > + MIKROBUS_PROPERTY_TYPE_GPIO, > + MIKROBUS_PROPERTY_TYPE_U8, > + MIKROBUS_PROPERTY_TYPE_U16, > + MIKROBUS_PROPERTY_TYPE_U32, > + MIKROBUS_PROPERTY_TYPE_U64, > +}; > + > +enum mikrobus_pin { > + MIKROBUS_PIN_PWM = 0x00, > + MIKROBUS_PIN_INT, > + MIKROBUS_PIN_RX, > + MIKROBUS_PIN_TX, > + MIKROBUS_PIN_SCL, > + MIKROBUS_PIN_SDA, > + MIKROBUS_PIN_MOSI, > + MIKROBUS_PIN_MISO, > + MIKROBUS_PIN_SCK, > + MIKROBUS_PIN_CS, > + MIKROBUS_PIN_RST, > + MIKROBUS_PIN_AN, > + MIKROBUS_PORT_PIN_COUNT, > +}; > + > +enum mikrobus_pin_state { > + MIKROBUS_STATE_INPUT = 0x01, > + MIKROBUS_STATE_OUTPUT_HIGH, > + MIKROBUS_STATE_OUTPUT_LOW, > + MIKROBUS_STATE_PWM, > + MIKROBUS_STATE_SPI, > + MIKROBUS_STATE_I2C, > + MIKROBUS_STATE_UART, > +}; > + > +/* > + * board_device_info describes a single device on a mikrobus add-on > + * board, an add-on board can present one or more device to the host > + * > + * @gpio_lookup: used to provide the GPIO lookup table for > + * passing the named GPIOs to device drivers. > + * @properties: used to provide the property_entry to pass named > + * properties to device drivers, applicable only when driver uses > + * device_property_read_* calls to fetch the properties. > + * @num_gpio_resources: number of named gpio resources for the device, > + * used mainly for gpiod_lookup_table memory allocation. > + * @num_properties: number of custom properties for the device, > + * used mainly for property_entry memory allocation. > + * @protocol: used to know the type of the device and it should > + * contain one of the values defined under 'enum greybus_class_type' > + * under linux/greybus/greybus_manifest.h > + * @reg: I2C address for the device, for devices on the SPI bus > + * this field is the chip select address relative to the mikrobus > + * port:0->device chip select connected to CS pin on mikroBUS port > + * 1->device chip select connected to RST Pin on mikroBUS port > + * @mode: SPI mode > + * @max_speed_hz: SPI max speed(Hz) > + * @drv_name: device_id to match with the driver > + * @irq_type: type of IRQ trigger , match with defines in linux/interrupt.h > + * @irq: irq number relative to the mikrobus port should contain one of the > + * values defined under 'enum mikrobus_pin' > + * @id: device id starting from 1 > + */ > +struct board_device_info { > + struct gpiod_lookup_table *gpio_lookup; > + struct property_entry *properties; > + struct list_head links; > + unsigned short num_gpio_resources; > + unsigned short num_properties; > + unsigned short protocol; > + unsigned short reg; > + unsigned int mode; > + void *dev_client; > + u32 max_speed_hz; > + char *drv_name; > + int irq_type; > + int irq; > + int id; > +}; > + > +/* > + * addon_board_info describes a mikrobus add-on device the add-on > + * board, an add-on board can present one or more device to the host > + * > + * @manifest_descs: list of manifest descriptors > + * @devices: list of devices on the board > + * @pin_state: the state of each pin on the mikrobus port required > + * for the add-on board should contain one of the values defined under > + * 'enum mikrobus_pin_state' restrictions are as per mikrobus standard > + * specifications. > + * @name: add-on board name > + */ > +struct addon_board_info { > + struct list_head manifest_descs; > + struct list_head devices; > + u8 pin_state[MIKROBUS_PORT_PIN_COUNT]; > + char *name; > +}; > + > +/* > + * mikrobus_port describes the peripherals mapped to a mikrobus port. > + * > + * @chip_select: chip select number mapped to the SPI CS pin on the > + * mikrobus port and the RST pin on the mikrobus port > + * @board: pointer to the attached add-on board. > + * @spi_ctrl: SPI controller attached to the mikrobus port. > + * @i2c_adap: I2C adapter attached to the mikrobus port. > + * @gpios: GPIOs attached to the mikrobus port. > + * @pinctrl: pinctrl attached to the mikrobus port. > + * @dev: device structure for the mikrobus port. > + * @id: port id starting from 1 > + */ > +struct mikrobus_port { > + u32 chip_select[MIKROBUS_NUM_CS]; > + struct addon_board_info *board; > + struct spi_controller *spi_ctrl; > + struct i2c_adapter *i2c_adap; > + struct gpio_descs *gpios; > + struct pinctrl *pinctrl; > + struct device dev; > + int id; > +}; > + > +#define to_mikrobus_port(d) container_of(d, struct mikrobus_port, dev) > + > +#endif /* __MIKROBUS_H */ > diff --git a/drivers/misc/mikrobus/mikrobus_manifest.c b/drivers/misc/mikrobus/mikrobus_manifest.c > new file mode 100644 > index 000000000000..5f30620277be > --- /dev/null > +++ b/drivers/misc/mikrobus/mikrobus_manifest.c > @@ -0,0 +1,503 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * mikroBUS manifest parsing, an > + * extension to Greybus Manifest Parsing > + * under drivers/greybus/manifest.c > + * > + * Copyright 2014-2015 Google Inc. > + * Copyright 2014-2015 Linaro Ltd. > + * Copyright 2024 Ayush Singh <ayushdevel1325@gmail.com> > + */ > + > +#define pr_fmt(fmt) "mikrobus_manifest:%s: " fmt, __func__ > + > +#include "linux/gpio/machine.h" > +#include "linux/greybus/greybus_manifest.h" > +#include "linux/property.h" > +#include "mikrobus_manifest.h" > + > +struct manifest_desc { > + struct list_head links; > + size_t size; > + void *data; > + enum greybus_descriptor_type type; > +}; > + > +static void manifest_descriptor_release_all(struct addon_board_info *board) > +{ > + struct manifest_desc *descriptor, *next; > + > + list_for_each_entry_safe(descriptor, next, &board->manifest_descs, > + links) { > + list_del(&descriptor->links); > + kfree(descriptor); > + } > +} > + > +static int board_descriptor_add(struct addon_board_info *board, > + struct greybus_descriptor *desc, size_t size) > +{ > + struct greybus_descriptor_header *desc_header = &desc->header; > + struct manifest_desc *descriptor; > + size_t desc_size, expected_size; > + > + if (size < sizeof(*desc_header)) { > + pr_err("short descriptor (%zu < %zu)", size, > + sizeof(*desc_header)); > + return -EINVAL; > + } > + > + desc_size = le16_to_cpu(desc_header->size); > + if (desc_size > size) { > + pr_err("incorrect descriptor size (%zu != %zu)", size, > + desc_size); > + return -EINVAL; > + } > + > + expected_size = sizeof(*desc_header); > + switch (desc_header->type) { > + case GREYBUS_TYPE_STRING: > + expected_size += sizeof(struct greybus_descriptor_string); > + expected_size += desc->string.length; > + expected_size = ALIGN(expected_size, 4); > + break; > + case GREYBUS_TYPE_PROPERTY: > + expected_size += sizeof(struct greybus_descriptor_property); > + expected_size += desc->property.length; > + expected_size = ALIGN(expected_size, 4); > + break; > + case GREYBUS_TYPE_DEVICE: > + expected_size += sizeof(struct greybus_descriptor_device); > + break; > + case GREYBUS_TYPE_MIKROBUS: > + expected_size += sizeof(struct greybus_descriptor_mikrobus); > + break; > + case GREYBUS_TYPE_INTERFACE: > + expected_size += sizeof(struct greybus_descriptor_interface); > + break; > + case GREYBUS_TYPE_CPORT: > + expected_size += sizeof(struct greybus_descriptor_cport); > + break; > + case GREYBUS_TYPE_BUNDLE: > + expected_size += sizeof(struct greybus_descriptor_bundle); > + break; > + case GREYBUS_TYPE_INVALID: > + default: > + pr_err("invalid descriptor type %d", desc_header->type); > + return -EINVAL; > + } > + > + descriptor = kzalloc(sizeof(*descriptor), GFP_KERNEL); > + if (!descriptor) > + return -ENOMEM; > + > + descriptor->size = desc_size; > + descriptor->data = (char *)desc + sizeof(*desc_header); > + descriptor->type = desc_header->type; > + list_add_tail(&descriptor->links, &board->manifest_descs); > + > + return desc_size; > +} > + > +static char *mikrobus_string_get(struct addon_board_info *board, u8 string_id) > +{ > + struct greybus_descriptor_string *desc_string; > + struct manifest_desc *descriptor; > + bool found = false; > + char *string; > + > + if (!string_id) > + return NULL; > + > + list_for_each_entry(descriptor, &board->manifest_descs, links) { > + if (descriptor->type != GREYBUS_TYPE_STRING) > + continue; > + > + desc_string = descriptor->data; > + if (desc_string->id == string_id) { > + found = true; > + break; > + } > + } > + > + if (!found) > + return ERR_PTR(-ENOENT); > + > + string = kmemdup(&desc_string->string, desc_string->length + 1, > + GFP_KERNEL); > + if (!string) > + return ERR_PTR(-ENOMEM); > + > + string[desc_string->length] = '\0'; > + > + return string; > +} > + > +static void mikrobus_state_get(struct addon_board_info *board) > +{ > + struct greybus_descriptor_mikrobus *mikrobus; > + struct greybus_descriptor_interface *interface; > + struct manifest_desc *descriptor; > + bool found = false; > + size_t i; > + > + list_for_each_entry(descriptor, &board->manifest_descs, links) { > + if (descriptor->type == GREYBUS_TYPE_MIKROBUS) { > + mikrobus = descriptor->data; > + found = true; > + break; > + } > + } > + > + if (!found) { > + pr_err("mikrobus descriptor not found"); > + return; > + } > + > + for (i = 0; i < MIKROBUS_PORT_PIN_COUNT; i++) > + board->pin_state[i] = mikrobus->pin_state[i]; > + > + found = false; > + list_for_each_entry(descriptor, &board->manifest_descs, links) { > + if (descriptor->type == GREYBUS_TYPE_INTERFACE) { > + interface = descriptor->data; > + found = true; > + break; > + } > + } > + > + if (!found) { > + pr_err("interface descriptor not found"); > + return; > + } > + > + board->name = mikrobus_string_get(board, interface->product_stringid); > +} > + > +static struct property_entry * > +mikrobus_property_entry_get(struct addon_board_info *board, u8 *prop_link, > + int num_properties) > +{ > + struct greybus_descriptor_property *desc_property; > + struct manifest_desc *descriptor; > + struct property_entry *properties; > + bool found = false; > + char *prop_name; > + int i, ret; > + u64 *val_u64; > + u32 *val_u32; > + u16 *val_u16; > + u8 *val_u8; > + > + properties = kcalloc(num_properties, sizeof(*properties), GFP_KERNEL); > + if (!properties) > + return ERR_PTR(-ENOMEM); > + > + for (i = 0; i < num_properties; i++) { > + list_for_each_entry(descriptor, &board->manifest_descs, links) { > + if (descriptor->type != GREYBUS_TYPE_PROPERTY) > + continue; > + > + desc_property = descriptor->data; > + if (desc_property->id == prop_link[i]) { > + found = true; > + break; > + } > + } > + > + if (!found) { > + ret = -ENOENT; > + goto early_exit; > + } > + > + prop_name = mikrobus_string_get( > + board, desc_property->propname_stringid); > + if (!prop_name) { > + ret = -ENOENT; > + goto early_exit; > + } > + > + switch (desc_property->type) { > + case MIKROBUS_PROPERTY_TYPE_U8: > + val_u8 = kmemdup(&desc_property->value, > + (desc_property->length) * sizeof(u8), > + GFP_KERNEL); > + if (desc_property->length == 1) > + properties[i] = > + PROPERTY_ENTRY_U8(prop_name, *val_u8); > + else > + properties[i] = PROPERTY_ENTRY_U8_ARRAY_LEN( > + prop_name, (void *)desc_property->value, > + desc_property->length); > + break; > + case MIKROBUS_PROPERTY_TYPE_U16: > + val_u16 = kmemdup(&desc_property->value, > + (desc_property->length) * sizeof(u16), > + GFP_KERNEL); > + if (desc_property->length == 1) > + properties[i] = > + PROPERTY_ENTRY_U16(prop_name, *val_u16); > + else > + properties[i] = PROPERTY_ENTRY_U16_ARRAY_LEN( > + prop_name, (void *)desc_property->value, > + desc_property->length); > + break; > + case MIKROBUS_PROPERTY_TYPE_U32: > + val_u32 = kmemdup(&desc_property->value, > + (desc_property->length) * sizeof(u32), > + GFP_KERNEL); > + if (desc_property->length == 1) > + properties[i] = > + PROPERTY_ENTRY_U32(prop_name, *val_u32); > + else > + properties[i] = PROPERTY_ENTRY_U32_ARRAY_LEN( > + prop_name, (void *)desc_property->value, > + desc_property->length); > + break; > + case MIKROBUS_PROPERTY_TYPE_U64: > + val_u64 = kmemdup(&desc_property->value, > + (desc_property->length) * sizeof(u64), > + GFP_KERNEL); > + if (desc_property->length == 1) > + properties[i] = > + PROPERTY_ENTRY_U64(prop_name, *val_u64); > + else > + properties[i] = PROPERTY_ENTRY_U64_ARRAY_LEN( > + prop_name, (void *)desc_property->value, > + desc_property->length); > + break; > + default: > + ret = -EINVAL; > + goto early_exit; > + } > + } > + return properties; > + > +early_exit: > + kfree(properties); > + return ERR_PTR(ret); > +} > + > +static u8 *mikrobus_property_link_get(struct addon_board_info *board, > + u8 prop_id, > + struct board_device_info *board_dev, > + u8 prop_type) > +{ > + struct greybus_descriptor_property *desc_property; > + struct manifest_desc *descriptor; > + bool found = false; > + u8 *val_u8; > + > + if (!prop_id) > + return NULL; > + > + list_for_each_entry(descriptor, &board->manifest_descs, links) { > + if (descriptor->type != GREYBUS_TYPE_PROPERTY) > + continue; > + > + desc_property = descriptor->data; > + if (desc_property->id == prop_id && > + desc_property->type == prop_type) { > + found = true; > + break; > + } > + } > + > + if (!found) > + return ERR_PTR(-ENOENT); > + > + val_u8 = kmemdup(&desc_property->value, desc_property->length, > + GFP_KERNEL); > + if (prop_type == MIKROBUS_PROPERTY_TYPE_GPIO) > + board_dev->num_gpio_resources = desc_property->length; > + else if (prop_type == MIKROBUS_PROPERTY_TYPE_PROPERTY) > + board_dev->num_properties = desc_property->length; > + > + return val_u8; > +} > + > +static int > +mikrobus_manifest_attach_device(struct addon_board_info *board, > + struct greybus_descriptor_device *dev_desc) > +{ > + struct greybus_descriptor_property *desc_property; > + u8 *gpio_desc_link, *prop_link, *gpioval; > + struct board_device_info *board_dev; > + struct gpiod_lookup_table *lookup; > + struct manifest_desc *descriptor; > + int ret, i; > + > + board_dev = kzalloc(sizeof(*board_dev), GFP_KERNEL); > + if (!board_dev) > + return -ENOMEM; > + > + board_dev->id = dev_desc->id; > + board_dev->drv_name = > + mikrobus_string_get(board, dev_desc->driver_stringid); > + if (!board_dev->drv_name) { > + ret = -ENOENT; > + goto err_free_board_dev; > + } > + > + board_dev->protocol = dev_desc->protocol; > + board_dev->reg = dev_desc->reg; > + board_dev->irq = dev_desc->irq; > + board_dev->irq_type = dev_desc->irq_type; > + board_dev->max_speed_hz = le32_to_cpu(dev_desc->max_speed_hz); > + board_dev->mode = dev_desc->mode; > + pr_info("parsed device %d, driver=%s", board_dev->id, > + board_dev->drv_name); > + > + if (dev_desc->prop_link > 0) { > + prop_link = mikrobus_property_link_get( > + board, dev_desc->prop_link, board_dev, > + MIKROBUS_PROPERTY_TYPE_PROPERTY); > + if (!prop_link) { > + ret = -ENOENT; > + goto err_free_board_dev; > + } > + > + pr_info("device %d, number of properties=%d", board_dev->id, > + board_dev->num_properties); > + board_dev->properties = mikrobus_property_entry_get( > + board, prop_link, board_dev->num_properties); > + } > + > + if (dev_desc->gpio_link > 0) { > + gpio_desc_link = mikrobus_property_link_get( > + board, dev_desc->gpio_link, board_dev, > + MIKROBUS_PROPERTY_TYPE_GPIO); > + if (!gpio_desc_link) { > + ret = -ENOENT; > + goto err_free_board_dev; > + } > + > + pr_info("device %d, number of gpio resource=%d", board_dev->id, > + board_dev->num_gpio_resources); > + lookup = kzalloc(struct_size(lookup, table, > + board_dev->num_gpio_resources), > + GFP_KERNEL); > + if (!lookup) { > + ret = -ENOMEM; > + goto err_free_board_dev; > + } > + > + for (i = 0; i < board_dev->num_gpio_resources; i++) { > + list_for_each_entry(descriptor, &board->manifest_descs, > + links) { > + if (descriptor->type != GREYBUS_TYPE_PROPERTY) > + continue; > + > + desc_property = descriptor->data; > + if (desc_property->id == gpio_desc_link[i]) { > + gpioval = desc_property->value; > + lookup->table[i].chip_hwnum = > + gpioval[0]; > + lookup->table[i].flags = gpioval[1]; > + lookup->table[i] > + .con_id = mikrobus_string_get( > + board, > + desc_property > + ->propname_stringid); > + break; > + } > + } > + } > + board_dev->gpio_lookup = lookup; > + } > + > + list_add_tail(&board_dev->links, &board->devices); > + return 0; > + > +err_free_board_dev: > + kfree(board_dev); > + return ret; > +} > + > +static int mikrobus_manifest_parse_devices(struct addon_board_info *board) > +{ > + struct greybus_descriptor_device *desc_device; > + struct manifest_desc *desc, *next; > + int ret, devcount = 0; > + > + list_for_each_entry_safe(desc, next, &board->manifest_descs, links) { > + if (desc->type != GREYBUS_TYPE_DEVICE) > + continue; > + > + desc_device = desc->data; > + ret = mikrobus_manifest_attach_device(board, desc_device); > + devcount++; > + } > + > + return devcount; > +} > + > +static size_t mikrobus_manifest_header_validate(void *data, size_t size) > +{ > + struct greybus_manifest_header *header = data; > + u16 manifest_size = le16_to_cpu(header->size); > + > + if (manifest_size < sizeof(*header)) { > + pr_err("short manifest (%zu < %zu)", size, sizeof(*header)); > + return -EINVAL; > + } > + > + if (header->version_major > MIKROBUS_VERSION_MAJOR) { > + pr_err("manifest version too new (%u.%u > %u.%u)", > + header->version_major, header->version_minor, > + MIKROBUS_VERSION_MAJOR, MIKROBUS_VERSION_MINOR); > + return -EINVAL; > + } > + > + return manifest_size; > +} > + > +int mikrobus_manifest_parse(struct addon_board_info *board, void *data, > + size_t size) > +{ > + struct greybus_manifest_header *header; > + struct greybus_manifest *manifest; > + struct greybus_descriptor *desc; > + int dev_count, desc_size, ret; > + u16 manifest_size; > + > + if (size < sizeof(*header)) { > + pr_err("short manifest (%zu < %zu)", size, sizeof(*header)); > + return -EINVAL; > + } > + > + ret = mikrobus_manifest_header_validate(data, size); > + if (ret < 0) { > + pr_err("invalid manifest header: %u", manifest_size); > + return ret; > + } > + > + manifest = data; > + manifest_size = ret; > + > + if (manifest_size != size) { > + pr_err("invalid manifest size(%zu < %u)", size, manifest_size); > + return -EINVAL; > + } > + > + desc = manifest->descriptors; > + size -= sizeof(*header); > + while (size) { > + desc_size = board_descriptor_add(board, desc, size); > + if (desc_size < 0) { > + pr_err("invalid manifest descriptor, size: %u", > + desc_size); > + return -EINVAL; > + } > + > + desc = (void *)desc + desc_size; > + size -= desc_size; > + } > + > + mikrobus_state_get(board); > + dev_count = mikrobus_manifest_parse_devices(board); > + pr_info("%s manifest parsed with %d devices", board->name, dev_count); > + manifest_descriptor_release_all(board); > + > + return dev_count; > +} > diff --git a/drivers/misc/mikrobus/mikrobus_manifest.h b/drivers/misc/mikrobus/mikrobus_manifest.h > new file mode 100644 > index 000000000000..39ae53a25fc4 > --- /dev/null > +++ b/drivers/misc/mikrobus/mikrobus_manifest.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * mikroBUS manifest definition > + * extension to Greybus Manifest Definition > + * > + * Copyright 2014-2015 Google Inc. > + * Copyright 2014-2015 Linaro Ltd. > + * > + * Released under the GPLv2 and BSD licenses. > + */ > + > +#ifndef __MIKROBUS_MANIFEST_H > +#define __MIKROBUS_MANIFEST_H > + > +#include "mikrobus_core.h" > + > +/* > + * mikrobus_manifest_header - parse mikroBUS manifest > + * > + * @info: addon board info structure to populate with parsed information > + * @data: pointer to the manifest blob > + * @size: size of the manifest blob > + * > + * returns: number of devices on success, negative error code on failure > + */ > +int mikrobus_manifest_parse(struct addon_board_info *info, void *data, > + size_t size); > + > +#endif /* __MIKROBUS_MANIFEST_H */
On 18/03/24 01:07, Ayush Singh wrote: > DONOTMERGE > Why? > mikroBUS addon boards allow using same mikroBUS connector for a wide > range of peripherals. It is also possible for the addon board not to use > all the pins in mikroBUS socket (marked by NC or Not Connected). This > would require the need to create an almost new overlays for each > permutation of the hardware. > > To overcome this, a manifest format based on Greybus manifest > specification was created which allows describing mikroBUS addon boards. > The reason for choosing greybus manifest for the identifier is that so far > we discussed only about physical mikroBUS ports on the board, but we can you will need to reword the commit message properly in imperative mood, here and in multiple other places. > also have mikroBUS ports on a remote microcontroller appearing on host > over greybus and there a device tree overlay solution does not work as the > standard identifier mechanism. > > The patch introduces 3 new greybus descriptor types: > 1. mikrobus-descriptor: > Is a fixed-length descriptor (12 bytes), and the manifest shall have > precisely one mikroBUS descriptor. Each byte describes a configuration > of the corresponding pin on the mikroBUS addon board in a clockwise > direction starting from the PWM pin omitting power (VCC and ground) > pins as same as the default state of the pin. > There are mikroBUS addon boards that use some dedicated SPI, UART, PWM, > and I2C pins as GPIO pins, so it is necessary to redefine the default > pin configuration of that pins on the host system. Also, sometimes it is > required the pull-up on the host pin for correct functionality > 2. property-descriptor: > Are used to pass named properties or named GPIOs to the host. The host > system uses this information to properly configure specific board > drivers by passing the properties and GPIO name. There can be multiple > instances of property descriptors per add-on board manifest. > 3. device-descriptor: > Describes a device on the mikroBUS port. The device descriptor is a > fixed-length descriptor, and there can be multiple instances of device > descriptors in an add-on board manifest in cases where the add-on board > presents more than one device to the host. > > New mikroBUS addon boards also sometimes contain a 1-wire EEPROM with > the mikroBUS manifest, thus enabling plug and play support. > new mikroBUS sometimes contain an EEPROM? aren't these called Click ID compliant add-on boards? there should be clarity in the commit message. > I have also created PR to add mikrobus descriptors in Greybus spec and I > think the old PR on manifesto by Vaishnav should also work. However, > both of these repositories seem to be inactive. I am guessing the > greybus mailing list can provide more information on how I should go > about these. Why is information like these inside the commit message, these go below the tear line. > > Here is a sample mikroBUS manifest: > ``` > ;; > ; PRESSURE CLICK > ; https://www.mikroe.com/pressure-click > ; CONFIG_IIO_ST_PRESS > ; > ; Copyright 2020 BeagleBoard.org Foundation > ; Copyright 2020 Texas Instruments > ; > > [manifest-header] > version-major = 0 > version-minor = 1 > > [interface-descriptor] > vendor-string-id = 1 > product-string-id = 2 > > [string-descriptor 1] > string = MIKROE > > [string-descriptor 2] > string = Pressure > > [mikrobus-descriptor] > pwm-state = 4 > int-state = 1 > rx-state = 7 > tx-state = 7 > scl-state = 6 > sda-state = 6 > mosi-state = 5 > miso-state = 5 > sck-state = 5 > cs-state = 5 > rst-state = 2 > an-state = 1 > > [device-descriptor 1] > driver-string-id = 3 > protocol = 0x3 > reg = 0x5d > > [string-descriptor 3] > string = lps331ap > ``` > > Link: https://www.mikroe.com/clickid ClickID > Link: > https://docs.beagleboard.org/latest/projects/beagleconnect/index.html > beagleconnect > Link: https://github.com/projectara/greybus-spec Greybus Spec > Link: https://github.com/projectara/greybus-spec/pull/4 Greybus Spec PR > Link: https://github.com/projectara/manifesto/pull/2 manifesto PR > The manifesto PR might not be updated. Thanks and Regards, Vaishnav > Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org> > Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org> > Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> > --- > include/linux/greybus/greybus_manifest.h | 49 ++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/include/linux/greybus/greybus_manifest.h b/include/linux/greybus/greybus_manifest.h > index bef9eb2093e9..83241e19d9b3 100644 > --- a/include/linux/greybus/greybus_manifest.h > +++ b/include/linux/greybus/greybus_manifest.h > @@ -23,6 +23,9 @@ enum greybus_descriptor_type { > GREYBUS_TYPE_STRING = 0x02, > GREYBUS_TYPE_BUNDLE = 0x03, > GREYBUS_TYPE_CPORT = 0x04, > + GREYBUS_TYPE_MIKROBUS = 0x05, > + GREYBUS_TYPE_PROPERTY = 0x06, > + GREYBUS_TYPE_DEVICE = 0x07, > }; > > enum greybus_protocol { > @@ -151,6 +154,49 @@ struct greybus_descriptor_cport { > __u8 protocol_id; /* enum greybus_protocol */ > } __packed; > > +/* > + * A mikrobus descriptor is used to describe the details > + * about the bus ocnfiguration for the add-on board > + * connected to the mikrobus port. > + */ > +struct greybus_descriptor_mikrobus { > + __u8 pin_state[12]; > +} __packed; > + > +/* > + * A property descriptor is used to pass named properties > + * to device drivers through the unified device properties > + * interface under linux/property.h > + */ > +struct greybus_descriptor_property { > + __u8 length; > + __u8 id; > + __u8 propname_stringid; > + __u8 type; > + __u8 value[]; > +} __packed; > + > +/* > + * A device descriptor is used to describe the > + * details required by a add-on board device > + * driver. > + */ > +struct greybus_descriptor_device { > + __u8 id; > + __u8 driver_stringid; > + __u8 protocol; > + __u8 reg; > + __le32 max_speed_hz; > + __u8 irq; > + __u8 irq_type; > + __u8 mode; > + __u8 prop_link; > + __u8 gpio_link; > + __u8 reg_link; > + __u8 clock_link; > + __u8 pad[1]; > +} __packed; > + > struct greybus_descriptor_header { > __le16 size; > __u8 type; /* enum greybus_descriptor_type */ > @@ -164,6 +210,9 @@ struct greybus_descriptor { > struct greybus_descriptor_interface interface; > struct greybus_descriptor_bundle bundle; > struct greybus_descriptor_cport cport; > + struct greybus_descriptor_mikrobus mikrobus; > + struct greybus_descriptor_property property; > + struct greybus_descriptor_device device; > }; > } __packed; >
> This externalizes and exports the symbol > of_find_spi_controller_by_node() from the SPI core akin to how > of_find_i2c_adapter_by_node() is already available. As we will > need this also for non-dynamic OF setups, we move it under a > CONFIG_OF check. Would a change description be more desirable with a corresponding imperative wording? See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.8#n94 Regards, Markus
Hi Ayush, On 19/03/24 12:17, Ayush Singh wrote: > On 3/19/24 11:34, Krzysztof Kozlowski wrote: > >> On 17/03/2024 20:37, Ayush Singh wrote: >>> DONOTMERGE >>> >>> this patch depends on Patch 1, 2, 3 >> So none of your work should be reviewed? I don't understand this, but in >> such case I am not going to review it. >> >> Best regards, >> Krzysztof >> > I am a bit lost here. It was mentioned in the patch v3 that I should > specify the interdependence of patches in v3. And now you are saying I > should not? > It was mentioned in v3 that patches that are independent should be sent separately to the particular subsytem list and the dependencies should be mentioned in this series, still in this series you have combined SPI patches/platform DT changes along with the mikroBUS driver patches which creates confusion. This is what I mentioned as a response to your v3 series regarding adding the patches "The reasoning behind this is that these patches go in to separate maintainer trees and without the bindings merged first the device tree changes cannot be validated, thus it is a usual practice to get the bindings and driver merged first and the device tree changes to go in the next cycle. Another alternative is you can point to your fork with all the changes together." My suggestion was to get your series with the bindings and the base driver support accepted/ready first and the send the platform specific DT changes later. The rationale behind pointing to your fork with all changes is to have all the changes (w1 EEPROM, instantiating remote mikrobus ports over greybus .etc) together and ensure there are no conflicts with the base series. It looks like you have put DONOTMERGE on random patches (why is patch 3 and 4 marked as do not merge?) Thanks and Regards, Vaishnav > Here is the rationale for the dependence: > > 1. Any changes to the property names in dt-bindings patch 1 will need an > appropriate change here. > > 2. This patch will fail to build without patch 2. > > 3. This patch will fail to build without patch 3. > > > Ayush Singh >
On 3/19/24 11:28, Krzysztof Kozlowski wrote: > On 18/03/2024 18:20, Ayush Singh wrote: >> On 3/18/24 17:52, Michael Walle wrote: >> >>> On Sun Mar 17, 2024 at 8:37 PM CET, Ayush Singh wrote: >>>> Add DT bindings for mikroBUS interface. MikroBUS is an open standard >>>> developed by MikroElektronika for connecting add-on boards to >>>> microcontrollers or microprocessors. >>>> >>>> mikroBUS is a connector and does not have a controller. Instead the >>>> software is responsible for identification of board and setting up / >>>> registering uart, spi, i2c, pwm and other buses. Thus it needs a way to >>>> get uart, spi, i2c, pwm and gpio controllers / adapters. >>>> >>>> A mikroBUS addon board is free to leave some of the pins unused which >>>> are marked as NC or Not Connected. >>>> >>>> Some of the pins might need to be configured as GPIOs deviating from their >>>> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be >>>> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work. >>>> >>>> For some add-on boards the driver may not take care of some additional >>>> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line >>>> (RST pin on the mikrobus port) needs to be pulled high. >>>> >>>> Here's the list of pins in mikroBUS connector: >>>> Analog - AN >>>> Reset - RST >>>> SPI Chip Select - CS >>>> SPI Clock - SCK >>>> SPI Master Input Slave Output - MISO >>>> SPI Master Output Slave Input - MOSI >>>> VCC-3.3V power - +3.3V >>>> Reference Ground - GND >>>> PWM - PWM output >>>> INT - Hardware Interrupt >>>> RX - UART Receive >>>> TX - UART Transmit >>>> SCL - I2C Clock >>>> SDA - I2C Data >>>> +5V - VCC-5V power >>>> GND - Reference Ground >>>> >>>> Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains >>>> a manifest to describe the addon board to provide plug and play >>>> capabilities. >>>> >>>> Link: https://www.mikroe.com/mikrobus >>>> Link: >>>> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf >>>> mikroBUS specification >>>> Link: https://www.mikroe.com/sht1x-click SHT15 Click >>>> Link: https://www.mikroe.com/eth-click ENC28J60 Click >>>> Link: https://www.mikroe.com/clickid ClickID >>>> >>>> Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org> >>>> Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org> >>>> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> >>>> --- >>>> .../connector/mikrobus-connector.yaml | 113 ++++++++++++++++++ >>> See also >>> https://lore.kernel.org/r/YmFo+EntwxIsco%2Ft@robh.at.kernel.org/ >>> >>> Looks like this proposal doesn't have the subnodes. How do you >>> attach a kernel driver to it's spi port for example? Only through >>> the manifest files? >>> >>> -michael >> >> So I looked at the Patch, and it seems the approach of fundamentally >> different than this PR. So, let me try to explain what this patch set >> does for an add-on board using SPI. >> >> The device tree defines the SPI controller associated with mikroBUS SPI >> pins. The driver on match queries and takes a reference to the SPI >> controller but does nothing with it. Once a mikroBUS add-on board is >> detected (by passing manifest using sysfs or reading from 1-wire >> EEPROM), the driver parses the manifest, and if it detects an SPI device > As I understood Mikrobus does not have EEPROM. mikroBUS add-on boards do not need to have EEPROM, but they can have it. Simply put, EEPROM is not part of mikroBUS specification, but you will find a lot (especially newer) addon boards with support for EEPROM manifest. Regardless, this patch actually does not contain any code for EEPROM support I have just mentioned it to give more context on why mikroBUS manifest is the focus of this patch instead of DT overlay or something else. >> in manifest, it registers SPI device along with setting properties such >> as `chip_select`, `max_speed_hz`, `mode`, etc., which are defined in the >> manifest. On board removal, it unregisters the SPI device and waits for >> a new mikroBUS board to be detected again. > You explained drivers, not hardware for DT. Yes, I was replying to the question posed by Michael. Since this happens in the driver and not in the devicetree, I needed to explain the working of the driver: > How do you attach a kernel driver to it's spi port for example? For more hardware side, the bindings are for mikrobus connector rather than for any addon board. Thus, while an addon board might not use some of the pins, the connector still needs to have all the pins and associated controllers. >> It is also possible for SPI not to be used by a device, in which case, >> no SPI device is registered to the controller. It is also possible that >> the SPI pins will be used as normal GPIOs. Everything is identified from >> the manifest. > > Best regards, > Krzysztof > Ayush Singh
On 3/19/24 11:02, Krzysztof Kozlowski wrote:
> On 16/03/2024 14:06, Ayush Singh wrote:
>> > Are you sure this fits in Linux coding style limit (not checkpatch
>> limit, but the limit expressed by Linux coding style)?
>>
>>
>> Well, I am just using clang-format with column width of 100 instead of
>> 80. The docs now say 80 is prefered rather than mandatory, so well I was
> So you introduce your own style? Then consider it mandatory...
>
>> using 100 since I prefer that. If 80 is necessary or would make review
>> easier than I can just switch to it.
> You do not choose your own coding style.
>
>>
>> I will remove serdev, pwm, clickID and send a new patch with the minimal
>> driver and better commit messages as suggested with Vaishnav. It is
>> important to have good support for mikroBUS boards without clickID as well.
> Best regards,
> Krzysztof
>
I mean after the whole discussion about 80 vs 100 column line limit a
few years ago, and change in checkpatch behavior, I thought 100 was an
acceptable column length in the kernel, but I guess was mistaken, and 80
character is still mandatory? Not sure why there was a change in
checkpatch and docs though.
Regardless, I have switched 80 in the next patch since it is mandatory,
and I do not care as long as I can format using a formatter.
Ayush Singh
On 3/19/24 11:34, Krzysztof Kozlowski wrote:
> On 17/03/2024 20:37, Ayush Singh wrote:
>> DONOTMERGE
>>
>> this patch depends on Patch 1, 2, 3
> So none of your work should be reviewed? I don't understand this, but in
> such case I am not going to review it.
>
> Best regards,
> Krzysztof
>
I am a bit lost here. It was mentioned in the patch v3 that I should
specify the interdependence of patches in v3. And now you are saying I
should not?
Here is the rationale for the dependence:
1. Any changes to the property names in dt-bindings patch 1 will need an
appropriate change here.
2. This patch will fail to build without patch 2.
3. This patch will fail to build without patch 3.
Ayush Singh
On 3/19/24 11:33, Krzysztof Kozlowski wrote: > On 17/03/2024 20:37, Ayush Singh wrote: >> Add DT bindings for mikroBUS interface. MikroBUS is an open standard >> developed by MikroElektronika for connecting add-on boards to >> microcontrollers or microprocessors. >> > ... > >> +title: mikroBUS add-on board socket >> + >> +maintainers: >> + - Ayush Singh <ayushdevel1325@gmail.com> >> + >> +properties: >> + compatible: >> + const: mikrobus-connector >> + >> + pinctrl-0: true >> + pinctrl-1: true >> + pinctrl-2: true >> + pinctrl-3: true >> + pinctrl-4: true >> + pinctrl-5: true >> + pinctrl-6: true >> + pinctrl-7: true >> + pinctrl-8: true >> + >> + pinctrl-names: >> + items: >> + - const: default >> + - const: pwm_default >> + - const: pwm_gpio >> + - const: uart_default >> + - const: uart_gpio >> + - const: i2c_default >> + - const: i2c_gpio >> + - const: spi_default >> + - const: spi_gpio >> + >> + mikrobus-gpios: >> + minItems: 11 >> + maxItems: 12 > I don't see any of the issues resolved which I raised at v3. I think > Russell pointed that you do not have EEPROM and that some pins are > optional. You do not allow that. So this patchset does not contain any EEPROM code. The bindings describe mikroBUS connector and not mikroBUS addon board. While it is optional for the mikroBUS addon board to not use sone pins (aka NC), the pins still exist on the connector on the device side. It is not optional to have pins in the host device. > Plus I don't see him being Cced but he had quite detailed look and > comments at your patchset, so *you are supposed to Cc* him. > > I also do not see Rob's comments fully addressed. > > Do not send next versions before resolving previous discusssion. I apologize, I thought he was on the list by get_maintainers.pl, but it seems I was mistaken. I will try to remember going forward. >> + >> + i2c-adapter: >> + description: i2c adapter attached to the mikrobus socket. >> + $ref: /schemas/types.yaml#/definitions/phandle >> + >> + spi-controller: >> + description: spi bus number of the spi-master attached to the mikrobus socket. >> + $ref: /schemas/types.yaml#/definitions/phandle >> + >> + uart: >> + description: uart port attached to the mikrobus socket >> + $ref: /schemas/types.yaml#/definitions/phandle >> + >> + pwms: >> + description: the pwm-controller corresponding to the mikroBUS PWM pin. >> + maxItems: 1 >> + >> + spi-cs: >> + description: spi chip-select numbers corresponding to the chip-selects on the mikrobus socket. >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + items: >> + - description: chip select corresponding to CS pin >> + - description: chip select corresponding to RST pin >> + >> +required: >> + - compatible >> + - pinctrl-0 >> + - pinctrl-1 >> + - pinctrl-2 >> + - pinctrl-3 >> + - pinctrl-4 >> + - pinctrl-5 >> + - pinctrl-6 >> + - pinctrl-7 >> + - pinctrl-8 >> + - i2c-adapter >> + - spi-controller >> + - spi-cs >> + - uart >> + - pwms >> + - mikrobus-gpios >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/gpio/gpio.h> >> + >> + mikrobus { >> + compatible = "mikrobus-connector"; >> + pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default", > Please properly wrap your code according to Linux and DTS coding style > documents. > > > Best regards, > Krzysztof > Ayush Singh
On 3/19/24 11:29, Krzysztof Kozlowski wrote: > On 17/03/2024 20:37, Ayush Singh wrote: >> DONOTMERGE > Why? Explain then the purpose of this patch. Well, it was suggested to have DONOTMERGE by Vaishnav in the patches until dt bindings have been approved, since this patch touches different subsystems. Here is the full context from v3: > The reasoning behind this is that these patches go in to separate maintainer trees and without the bindings merged first the device tree changes cannot be validated, thus it is a usual practice to get the bindings and driver merged first and the device tree changes to go in the next cycle. Another alternative is you can point to your fork with all the changes together. >> this patch depends on patch 1 > How? I don't see any dependency. I think it is not allowed to have code in device tree unless a corresponding dt-binding exists for the device. And thus every time the dt-binding changes, this patch also needs to change.So I thought it is dependent on patch 1. >> Add mikroBUS connector support for Beagleplay. >> >> Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org> >> Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org> >> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> >> --- >> .../arm64/boot/dts/ti/k3-am625-beagleplay.dts | 80 +++++++++++++++++-- >> 1 file changed, 72 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts >> index a34e0df2ab86..e1dce1b80153 100644 >> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts >> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts >> @@ -29,6 +29,7 @@ aliases { >> i2c3 = &main_i2c3; >> i2c4 = &wkup_i2c0; >> i2c5 = &mcu_i2c0; >> + mikrobus0 = &mikrobus0; >> mmc0 = &sdhci0; >> mmc1 = &sdhci1; >> mmc2 = &sdhci2; >> @@ -230,6 +231,38 @@ simple-audio-card,codec { >> }; >> }; >> > > Best regards, > Krzysztof Link: https://lore.kernel.org/lkml/CALudOK5v_uCUffxHGKS-jA-DKLNV7xwmKkxJwjHaMWWgDdPDqA@mail.gmail.com/ Patch v3 Ayush Singh
On 17/03/2024 20:37, Ayush Singh wrote:
> DONOTMERGE
>
> this patch depends on Patch 1, 2, 3
So none of your work should be reviewed? I don't understand this, but in
such case I am not going to review it.
Best regards,
Krzysztof
On 17/03/2024 20:37, Ayush Singh wrote: > Add DT bindings for mikroBUS interface. MikroBUS is an open standard > developed by MikroElektronika for connecting add-on boards to > microcontrollers or microprocessors. > ... > +title: mikroBUS add-on board socket > + > +maintainers: > + - Ayush Singh <ayushdevel1325@gmail.com> > + > +properties: > + compatible: > + const: mikrobus-connector > + > + pinctrl-0: true > + pinctrl-1: true > + pinctrl-2: true > + pinctrl-3: true > + pinctrl-4: true > + pinctrl-5: true > + pinctrl-6: true > + pinctrl-7: true > + pinctrl-8: true > + > + pinctrl-names: > + items: > + - const: default > + - const: pwm_default > + - const: pwm_gpio > + - const: uart_default > + - const: uart_gpio > + - const: i2c_default > + - const: i2c_gpio > + - const: spi_default > + - const: spi_gpio > + > + mikrobus-gpios: > + minItems: 11 > + maxItems: 12 I don't see any of the issues resolved which I raised at v3. I think Russell pointed that you do not have EEPROM and that some pins are optional. You do not allow that. Plus I don't see him being Cced but he had quite detailed look and comments at your patchset, so *you are supposed to Cc* him. I also do not see Rob's comments fully addressed. Do not send next versions before resolving previous discusssion. > + > + i2c-adapter: > + description: i2c adapter attached to the mikrobus socket. > + $ref: /schemas/types.yaml#/definitions/phandle > + > + spi-controller: > + description: spi bus number of the spi-master attached to the mikrobus socket. > + $ref: /schemas/types.yaml#/definitions/phandle > + > + uart: > + description: uart port attached to the mikrobus socket > + $ref: /schemas/types.yaml#/definitions/phandle > + > + pwms: > + description: the pwm-controller corresponding to the mikroBUS PWM pin. > + maxItems: 1 > + > + spi-cs: > + description: spi chip-select numbers corresponding to the chip-selects on the mikrobus socket. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + items: > + - description: chip select corresponding to CS pin > + - description: chip select corresponding to RST pin > + > +required: > + - compatible > + - pinctrl-0 > + - pinctrl-1 > + - pinctrl-2 > + - pinctrl-3 > + - pinctrl-4 > + - pinctrl-5 > + - pinctrl-6 > + - pinctrl-7 > + - pinctrl-8 > + - i2c-adapter > + - spi-controller > + - spi-cs > + - uart > + - pwms > + - mikrobus-gpios > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + mikrobus { > + compatible = "mikrobus-connector"; > + pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default", Please properly wrap your code according to Linux and DTS coding style documents. Best regards, Krzysztof
On 17/03/2024 20:37, Ayush Singh wrote: > DONOTMERGE Why? Explain then the purpose of this patch. > > this patch depends on patch 1 How? I don't see any dependency. > > Add mikroBUS connector support for Beagleplay. > > Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org> > Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org> > Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> > --- > .../arm64/boot/dts/ti/k3-am625-beagleplay.dts | 80 +++++++++++++++++-- > 1 file changed, 72 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts > index a34e0df2ab86..e1dce1b80153 100644 > --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts > +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts > @@ -29,6 +29,7 @@ aliases { > i2c3 = &main_i2c3; > i2c4 = &wkup_i2c0; > i2c5 = &mcu_i2c0; > + mikrobus0 = &mikrobus0; > mmc0 = &sdhci0; > mmc1 = &sdhci1; > mmc2 = &sdhci2; > @@ -230,6 +231,38 @@ simple-audio-card,codec { > }; > }; > Best regards, Krzysztof
On 18/03/2024 18:20, Ayush Singh wrote: > On 3/18/24 17:52, Michael Walle wrote: > >> On Sun Mar 17, 2024 at 8:37 PM CET, Ayush Singh wrote: >>> Add DT bindings for mikroBUS interface. MikroBUS is an open standard >>> developed by MikroElektronika for connecting add-on boards to >>> microcontrollers or microprocessors. >>> >>> mikroBUS is a connector and does not have a controller. Instead the >>> software is responsible for identification of board and setting up / >>> registering uart, spi, i2c, pwm and other buses. Thus it needs a way to >>> get uart, spi, i2c, pwm and gpio controllers / adapters. >>> >>> A mikroBUS addon board is free to leave some of the pins unused which >>> are marked as NC or Not Connected. >>> >>> Some of the pins might need to be configured as GPIOs deviating from their >>> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be >>> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work. >>> >>> For some add-on boards the driver may not take care of some additional >>> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line >>> (RST pin on the mikrobus port) needs to be pulled high. >>> >>> Here's the list of pins in mikroBUS connector: >>> Analog - AN >>> Reset - RST >>> SPI Chip Select - CS >>> SPI Clock - SCK >>> SPI Master Input Slave Output - MISO >>> SPI Master Output Slave Input - MOSI >>> VCC-3.3V power - +3.3V >>> Reference Ground - GND >>> PWM - PWM output >>> INT - Hardware Interrupt >>> RX - UART Receive >>> TX - UART Transmit >>> SCL - I2C Clock >>> SDA - I2C Data >>> +5V - VCC-5V power >>> GND - Reference Ground >>> >>> Additionally, some new mikroBUS boards contain 1-wire EEPROM that contains >>> a manifest to describe the addon board to provide plug and play >>> capabilities. >>> >>> Link: https://www.mikroe.com/mikrobus >>> Link: >>> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf >>> mikroBUS specification >>> Link: https://www.mikroe.com/sht1x-click SHT15 Click >>> Link: https://www.mikroe.com/eth-click ENC28J60 Click >>> Link: https://www.mikroe.com/clickid ClickID >>> >>> Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org> >>> Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org> >>> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> >>> --- >>> .../connector/mikrobus-connector.yaml | 113 ++++++++++++++++++ >> See also >> https://lore.kernel.org/r/YmFo+EntwxIsco%2Ft@robh.at.kernel.org/ >> >> Looks like this proposal doesn't have the subnodes. How do you >> attach a kernel driver to it's spi port for example? Only through >> the manifest files? >> >> -michael > > > So I looked at the Patch, and it seems the approach of fundamentally > different than this PR. So, let me try to explain what this patch set > does for an add-on board using SPI. > > The device tree defines the SPI controller associated with mikroBUS SPI > pins. The driver on match queries and takes a reference to the SPI > controller but does nothing with it. Once a mikroBUS add-on board is > detected (by passing manifest using sysfs or reading from 1-wire > EEPROM), the driver parses the manifest, and if it detects an SPI device As I understood Mikrobus does not have EEPROM. > in manifest, it registers SPI device along with setting properties such > as `chip_select`, `max_speed_hz`, `mode`, etc., which are defined in the > manifest. On board removal, it unregisters the SPI device and waits for > a new mikroBUS board to be detected again. You explained drivers, not hardware for DT. > > It is also possible for SPI not to be used by a device, in which case, > no SPI device is registered to the controller. It is also possible that > the SPI pins will be used as normal GPIOs. Everything is identified from > the manifest. Best regards, Krzysztof
On 16/03/2024 14:06, Ayush Singh wrote: > > Are you sure this fits in Linux coding style limit (not checkpatch > limit, but the limit expressed by Linux coding style)? > > > Well, I am just using clang-format with column width of 100 instead of > 80. The docs now say 80 is prefered rather than mandatory, so well I was So you introduce your own style? Then consider it mandatory... > using 100 since I prefer that. If 80 is necessary or would make review > easier than I can just switch to it. You do not choose your own coding style. > > > I will remove serdev, pwm, clickID and send a new patch with the minimal > driver and better commit messages as suggested with Vaishnav. It is > important to have good support for mikroBUS boards without clickID as well. Best regards, Krzysztof
Latest series: [v3] spi: spi-imx: fix off-by-one in mx51 CPU mode burst length (2024-03-18T17:50:52) Superseding: [v2] spi: spi-imx: fix off-by-one in mx51 CPU mode burst length (2024-03-13T20:58:19): [v2] spi: spi-imx: fix off-by-one in mx51 CPU mode burst length -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Mon, Mar 18, 2024 at 01:41:38PM -0500, Alex Elder wrote:
> On 3/18/24 12:58 PM, Markus Elfring wrote:
> > …
> > > +++ b/drivers/misc/mikrobus/mikrobus_core.c
> > …
> > > +static int mikrobus_pinctrl_setup(struct mikrobus_port *port,
> > > + struct addon_board_info *board)
> > > +{
> > > + int ret;
> > > +
> > > + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> > > + ret = mikrobus_pinctrl_select(port, "pwm_default");
> > > + else
> > > + ret = mikrobus_pinctrl_select(port, "pwm_gpio");
> > …
> >
> > How do you think about to avoid the specification of a bit of duplicate source code here
> > by using conditional operator expressions?
> >
> > ret = mikrobus_pinctrl_select(port,
> > ((!board ||
> > board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM)
> > ? "pwm_default"
> > : "pwm_gpio"));
>
> No.
>
> It's a complex enough bit of logic without trying to bury
> it inside the parameters passed to the function.
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list. I strongly suggest that you not do this anymore. Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.
thanks,
greg k-h's patch email bot
On 3/18/24 11:29, Kousik Sanagavarapu wrote:
> On Mon, Mar 18, 2024 at 11:11:29AM -0700, Guenter Roeck wrote:
>> On Mon, Mar 18, 2024 at 09:08:35PM +0530, Kousik Sanagavarapu wrote:
>>> Update links in the documentation and in-code comments which point to
>>> the datasheet.
>>>
>>> The current links don't work because National Semiconductor (which is
>>> the manufacturer of this board and lm70) has been a part of Texas
>> ^^^^^^^^^^
>>
>> Is this a leftover from the other patch ? The lm70 driver supports
>> the LM70 chip, not a specific board.
>
> Yeah, it should be "the manufacturer of lm70". Thanks for spotting.
>
> Should I fix and resend this specific patch as v3 or would you edit it
> while pulling?
>
I'll edit it.
Thanks,
Guenter
On 3/18/24 12:58 PM, Markus Elfring wrote: > … >> +++ b/drivers/misc/mikrobus/mikrobus_core.c > … >> +static int mikrobus_pinctrl_setup(struct mikrobus_port *port, >> + struct addon_board_info *board) >> +{ >> + int ret; >> + >> + if (!board || board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM) >> + ret = mikrobus_pinctrl_select(port, "pwm_default"); >> + else >> + ret = mikrobus_pinctrl_select(port, "pwm_gpio"); > … > > How do you think about to avoid the specification of a bit of duplicate source code here > by using conditional operator expressions? > > ret = mikrobus_pinctrl_select(port, > ((!board || > board->pin_state[MIKROBUS_PIN_PWM] == MIKROBUS_STATE_PWM) > ? "pwm_default" > : "pwm_gpio")); No. It's a complex enough bit of logic without trying to bury it inside the parameters passed to the function. -Alex > > > Regards, > Markus >
On Mon, Mar 18, 2024 at 11:11:29AM -0700, Guenter Roeck wrote:
> On Mon, Mar 18, 2024 at 09:08:35PM +0530, Kousik Sanagavarapu wrote:
> > Update links in the documentation and in-code comments which point to
> > the datasheet.
> >
> > The current links don't work because National Semiconductor (which is
> > the manufacturer of this board and lm70) has been a part of Texas
> ^^^^^^^^^^
>
> Is this a leftover from the other patch ? The lm70 driver supports
> the LM70 chip, not a specific board.
Yeah, it should be "the manufacturer of lm70". Thanks for spotting.
Should I fix and resend this specific patch as v3 or would you edit it
while pulling?
… > +++ b/drivers/misc/mikrobus/mikrobus_core.h … > +#ifndef __MIKROBUS_H > +#define __MIKROBUS_H … I suggest to avoid the specification of leading underscores for include guards. See also: https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier#DCL37C.Donotdeclareordefineareservedidentifier-NoncompliantCodeExample%28IncludeGuard%29 Regards, Markus
On Mon, Mar 18, 2024 at 09:08:35PM +0530, Kousik Sanagavarapu wrote:
> Update links in the documentation and in-code comments which point to
> the datasheet.
>
> The current links don't work because National Semiconductor (which is
> the manufacturer of this board and lm70) has been a part of Texas
^^^^^^^^^^
Is this a leftover from the other patch ? The lm70 driver supports
the LM70 chip, not a specific board.
Guenter