| Message ID | 20241030084445.2438750-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] spi: atmel-quadspi: Create `atmel_qspi_ops` to support newer SoC families | expand |
On 10/30/24 8:44 AM, Csókás, Bence wrote: > Refactor the code to introduce an ops struct, to prepare for merging > support for later SoCs, such as SAMA7G5. This code was based on the > vendor's kernel (linux4microchip). Cc'ing original contributors. I think it's fine to split sama7g5 addition in smaller steps. But please add the sama7g5 support in the same patch set, otherwise this patch doesn't make sense on its own. Also, if you think you significantly changed the code of authors, I think it's fine to overwrite the authorship. Otherwise, try to keep the authorship and specify your contributions above your S-o-b tag. Cheers, ta
Hi, On 2024. 10. 30. 12:09, Tudor Ambarus wrote: > I think it's fine to split sama7g5 addition in smaller steps. But please > add the sama7g5 support in the same patch set, otherwise this patch > doesn't make sense on its own. Well, actually, we're using SAMA5D2. My goal was just to somewhat harmonize upstream with the vendor kernel so that we may contribute other patches that we have made on top of the latter, or in the future, take patches from upstream and apply it to our vendor kernel-based tree. This patch was only meant to lay the groundworks for future SAMA7G5 support. I can of course send the "other half" of the original patch if needed, but I wouldn't want it to hold up this refactor. > Also, if you think you significantly changed the code of authors, I > think it's fine to overwrite the authorship. Otherwise, try to keep the > authorship and specify your contributions above your S-o-b tag. I don't know if it counts as "significantly changed", I split out parts of a patch that were relevant for our device, and made small adjustments to make it correctly apply to master. I didn't find a descriptive enough tag for this, so I just went with Cc:, but if so desired, I could change it to a S-o-b, Co-authored-by, Suggested-by etc. Bence
On 10/30/24 12:37 PM, Csókás Bence wrote: > Hi, Hi! > > On 2024. 10. 30. 12:09, Tudor Ambarus wrote: >> I think it's fine to split sama7g5 addition in smaller steps. But please >> add the sama7g5 support in the same patch set, otherwise this patch >> doesn't make sense on its own. > > Well, actually, we're using SAMA5D2. My goal was just to somewhat > harmonize upstream with the vendor kernel so that we may contribute > other patches that we have made on top of the latter, or in the future, > take patches from upstream and apply it to our vendor kernel-based tree. > This patch was only meant to lay the groundworks for future SAMA7G5 > support. I can of course send the "other half" of the original patch if > needed, but I wouldn't want it to hold up this refactor. Do you have a sama7g5 at hand? If not and unable to test it, it's probably better to not touch that code, unless you get support from someone to do the testing for you. I still think that this patch on its own doesn't make sense without the sama7g5 addition because nothing guarantees that sama7g5 will be added on top of this. But I won't stand against your patch. Maybe others from Cc find it fine. > >> Also, if you think you significantly changed the code of authors, I >> think it's fine to overwrite the authorship. Otherwise, try to keep the >> authorship and specify your contributions above your S-o-b tag. > > I don't know if it counts as "significantly changed", I split out parts > of a patch that were relevant for our device, and made small adjustments > to make it correctly apply to master. I didn't find a descriptive enough > tag for this, so I just went with Cc:, but if so desired, I could change > it to a S-o-b, Co-authored-by, Suggested-by etc. > I think it's fine if you keep your authorship for this refactoring patch, I was thinking more of the bulk of the sama7g5 changes. Cheers and good luck! ta
Hi, Am Wed, Oct 30, 2024 at 01:37:52PM +0100 schrieb Csókás Bence: > Hi, > > On 2024. 10. 30. 12:09, Tudor Ambarus wrote: > > I think it's fine to split sama7g5 addition in smaller steps. But please > > add the sama7g5 support in the same patch set, otherwise this patch > > doesn't make sense on its own. > > Well, actually, we're using SAMA5D2. My goal was just to somewhat harmonize > upstream with the vendor kernel so that we may contribute other patches that > we have made on top of the latter, or in the future, take patches from > upstream and apply it to our vendor kernel-based tree. This patch was only > meant to lay the groundworks for future SAMA7G5 support. I can of course > send the "other half" of the original patch if needed, but I wouldn't want > it to hold up this refactor. It would actually be better if vendor would bring their stuff upstream, so there's no need for a vendor kernel. Did you talk to Microchip about their upstreaming efforts? What was the answer? Greets Alex > > > Also, if you think you significantly changed the code of authors, I > > think it's fine to overwrite the authorship. Otherwise, try to keep the > > authorship and specify your contributions above your S-o-b tag. > > I don't know if it counts as "significantly changed", I split out parts of a > patch that were relevant for our device, and made small adjustments to make > it correctly apply to master. I didn't find a descriptive enough tag for > this, so I just went with Cc:, but if so desired, I could change it to a > S-o-b, Co-authored-by, Suggested-by etc. > > Bence > >
Hi! On 2024. 11. 04. 13:48, Alexander Dahl wrote: > Hi, > > Am Wed, Oct 30, 2024 at 01:37:52PM +0100 schrieb Csókás Bence: >> Hi, >> >> On 2024. 10. 30. 12:09, Tudor Ambarus wrote: >>> I think it's fine to split sama7g5 addition in smaller steps. But please >>> add the sama7g5 support in the same patch set, otherwise this patch >>> doesn't make sense on its own. >> >> Well, actually, we're using SAMA5D2. My goal was just to somewhat harmonize >> upstream with the vendor kernel so that we may contribute other patches that >> we have made on top of the latter, or in the future, take patches from >> upstream and apply it to our vendor kernel-based tree. This patch was only >> meant to lay the groundworks for future SAMA7G5 support. I can of course >> send the "other half" of the original patch if needed, but I wouldn't want >> it to hold up this refactor. > > It would actually be better if vendor would bring their stuff > upstream, so there's no need for a vendor kernel. Did you talk to > Microchip about their upstreaming efforts? What was the answer? > > Greets > Alex Agreed. Though in this case, the original patch *was* submitted by Microchip (by Tudor, originally) for upstream inclusion, but it was not merged. Hence this forward-port. Link: https://lore.kernel.org/linux-spi/20211214133404.121739-1-tudor.ambarus@microchip.com/ Bence
Hello Bence, On 11/4/24 6:26 PM, Csókás Bence wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know > the content is safe > > Hi! > > On 2024. 11. 04. 13:48, Alexander Dahl wrote: >> Hi, >> >> Am Wed, Oct 30, 2024 at 01:37:52PM +0100 schrieb Csókás Bence: >>> Hi, >>> >>> On 2024. 10. 30. 12:09, Tudor Ambarus wrote: >>>> I think it's fine to split sama7g5 addition in smaller steps. But >>>> please >>>> add the sama7g5 support in the same patch set, otherwise this patch >>>> doesn't make sense on its own. >>> >>> Well, actually, we're using SAMA5D2. My goal was just to somewhat >>> harmonize >>> upstream with the vendor kernel so that we may contribute other >>> patches that >>> we have made on top of the latter, or in the future, take patches from >>> upstream and apply it to our vendor kernel-based tree. This patch was >>> only >>> meant to lay the groundworks for future SAMA7G5 support. I can of course >>> send the "other half" of the original patch if needed, but I wouldn't >>> want >>> it to hold up this refactor. >> >> It would actually be better if vendor would bring their stuff >> upstream, so there's no need for a vendor kernel. Did you talk to >> Microchip about their upstreaming efforts? What was the answer? >> >> Greets >> Alex > > Agreed. Though in this case, the original patch *was* submitted by > Microchip (by Tudor, originally) for upstream inclusion, but it was not > merged. Hence this forward-port. > Link: > https://lore.kernel.org/linux-spi/20211214133404.121739-1-tudor.ambarus@microchip.com/ Thanks for your patch. We are planning to revive this work at the earliest. While I don't have specific timeline for this, we at Microchip are fully aware of this gap and doing everything we could to keep the delta between the upstream kernel and vendor kernel as minimal as possible. We will discuss internally and provide you the feedback. Thanks again for your efforts. Regards, Hari > > Bence > >
Hi, On 2024. 11. 05. 8:47, Hari.PrasathGE@microchip.com wrote: > Hello Bence, > > On 11/4/24 6:26 PM, Csókás Bence wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Hi! >> >> On 2024. 11. 04. 13:48, Alexander Dahl wrote: >>> It would actually be better if vendor would bring their stuff >>> upstream, so there's no need for a vendor kernel. Did you talk to >>> Microchip about their upstreaming efforts? What was the answer? >>> >>> Greets >>> Alex >> >> Agreed. Though in this case, the original patch *was* submitted by >> Microchip (by Tudor, originally) for upstream inclusion, but it was not >> merged. Hence this forward-port. >> Link: >> https://lore.kernel.org/linux-spi/20211214133404.121739-1-tudor.ambarus@microchip.com/ > > > Thanks for your patch. We are planning to revive this work at the > earliest. While I don't have specific timeline for this, we at Microchip > are fully aware of this gap and doing everything we could to keep the > delta between the upstream kernel and vendor kernel as minimal as possible. > > We will discuss internally and provide you the feedback. Thanks again > for your efforts. > > Regards, > Hari Did you reach a conclusion internally regarding whether to support this patch? Since then, I opened a ticket with Microchip, but haven't got a response yet. I have also been in face-to-face contact with some of the engineers from the Rousset office, and they have expressed their support, and even the possibility of lending us a SAMA7G5 to test with. So really, all I'm waiting for is this patch to be merged, and then I can submit the SAMA7G5 parts, at worst as an RFC, if we don't get the real hardware in time. Bence
diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c index 95cdfc28361e..1d256ab3f7f2 100644 --- a/drivers/spi/atmel-quadspi.c +++ b/drivers/spi/atmel-quadspi.c @@ -138,11 +138,15 @@ #define QSPI_WPSR_WPVSRC_MASK GENMASK(15, 8) #define QSPI_WPSR_WPVSRC(src) (((src) << 8) & QSPI_WPSR_WPVSRC) +#define ATMEL_QSPI_TIMEOUT 1000 /* ms */ + struct atmel_qspi_caps { bool has_qspick; bool has_ricr; }; +struct atmel_qspi_ops; + struct atmel_qspi { void __iomem *regs; void __iomem *mem; @@ -150,13 +154,22 @@ struct atmel_qspi { struct clk *qspick; struct platform_device *pdev; const struct atmel_qspi_caps *caps; + const struct atmel_qspi_ops *ops; resource_size_t mmap_size; u32 pending; + u32 irq_mask; u32 mr; u32 scr; struct completion cmd_completion; }; +struct atmel_qspi_ops { + int (*set_cfg)(struct atmel_qspi *aq, const struct spi_mem_op *op, + u32 *offset); + int (*transfer)(struct spi_mem *mem, const struct spi_mem_op *op, + u32 offset); +}; + struct atmel_qspi_mode { u8 cmd_buswidth; u8 addr_buswidth; @@ -404,10 +417,60 @@ static int atmel_qspi_set_cfg(struct atmel_qspi *aq, return 0; } +static int atmel_qspi_wait_for_completion(struct atmel_qspi *aq, u32 irq_mask) +{ + int err = 0; + u32 sr; + + /* Poll INSTRuction End status */ + sr = atmel_qspi_read(aq, QSPI_SR); + if ((sr & irq_mask) == irq_mask) + return 0; + + /* Wait for INSTRuction End interrupt */ + reinit_completion(&aq->cmd_completion); + aq->pending = sr & irq_mask; + aq->irq_mask = irq_mask; + atmel_qspi_write(irq_mask, aq, QSPI_IER); + if (!wait_for_completion_timeout(&aq->cmd_completion, + msecs_to_jiffies(ATMEL_QSPI_TIMEOUT))) + err = -ETIMEDOUT; + atmel_qspi_write(irq_mask, aq, QSPI_IDR); + + return err; +} + +static int atmel_qspi_transfer(struct spi_mem *mem, + const struct spi_mem_op *op, u32 offset) +{ + struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->controller); + + /* Skip to the final steps if there is no data */ + if (!op->data.nbytes) + return atmel_qspi_wait_for_completion(aq, + QSPI_SR_CMD_COMPLETED); + + /* Dummy read of QSPI_IFR to synchronize APB and AHB accesses */ + (void)atmel_qspi_read(aq, QSPI_IFR); + + /* Send/Receive data */ + if (op->data.dir == SPI_MEM_DATA_IN) + memcpy_fromio(op->data.buf.in, aq->mem + offset, + op->data.nbytes); + else + memcpy_toio(aq->mem + offset, op->data.buf.out, + op->data.nbytes); + + /* Release the chip-select */ + atmel_qspi_write(QSPI_CR_LASTXFER, aq, QSPI_CR); + + return atmel_qspi_wait_for_completion(aq, QSPI_SR_CMD_COMPLETED); +} + static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) { struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->controller); - u32 sr, offset; + u32 offset; int err; /* @@ -416,46 +479,20 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) * when the flash memories overrun the controller's memory space. */ if (op->addr.val + op->data.nbytes > aq->mmap_size) - return -ENOTSUPP; + return -EOPNOTSUPP; + + if (op->addr.nbytes > 4) + return -EOPNOTSUPP; err = pm_runtime_resume_and_get(&aq->pdev->dev); if (err < 0) return err; - err = atmel_qspi_set_cfg(aq, op, &offset); + err = aq->ops->set_cfg(aq, op, &offset); if (err) goto pm_runtime_put; - /* Skip to the final steps if there is no data */ - if (op->data.nbytes) { - /* Dummy read of QSPI_IFR to synchronize APB and AHB accesses */ - (void)atmel_qspi_read(aq, QSPI_IFR); - - /* Send/Receive data */ - if (op->data.dir == SPI_MEM_DATA_IN) - memcpy_fromio(op->data.buf.in, aq->mem + offset, - op->data.nbytes); - else - memcpy_toio(aq->mem + offset, op->data.buf.out, - op->data.nbytes); - - /* Release the chip-select */ - atmel_qspi_write(QSPI_CR_LASTXFER, aq, QSPI_CR); - } - - /* Poll INSTRuction End status */ - sr = atmel_qspi_read(aq, QSPI_SR); - if ((sr & QSPI_SR_CMD_COMPLETED) == QSPI_SR_CMD_COMPLETED) - goto pm_runtime_put; - - /* Wait for INSTRuction End interrupt */ - reinit_completion(&aq->cmd_completion); - aq->pending = sr & QSPI_SR_CMD_COMPLETED; - atmel_qspi_write(QSPI_SR_CMD_COMPLETED, aq, QSPI_IER); - if (!wait_for_completion_timeout(&aq->cmd_completion, - msecs_to_jiffies(1000))) - err = -ETIMEDOUT; - atmel_qspi_write(QSPI_SR_CMD_COMPLETED, aq, QSPI_IDR); + err = aq->ops->transfer(mem, op, offset); pm_runtime_put: pm_runtime_mark_last_busy(&aq->pdev->dev); @@ -571,12 +608,17 @@ static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id) return IRQ_NONE; aq->pending |= pending; - if ((aq->pending & QSPI_SR_CMD_COMPLETED) == QSPI_SR_CMD_COMPLETED) + if ((aq->pending & aq->irq_mask) == aq->irq_mask) complete(&aq->cmd_completion); return IRQ_HANDLED; } +static const struct atmel_qspi_ops atmel_qspi_ops = { + .set_cfg = atmel_qspi_set_cfg, + .transfer = atmel_qspi_transfer, +}; + static int atmel_qspi_probe(struct platform_device *pdev) { struct spi_controller *ctrl; @@ -601,6 +643,7 @@ static int atmel_qspi_probe(struct platform_device *pdev) init_completion(&aq->cmd_completion); aq->pdev = pdev; + aq->ops = &atmel_qspi_ops; /* Map the registers */ aq->regs = devm_platform_ioremap_resource_byname(pdev, "qspi_base");
Refactor the code to introduce an ops struct, to prepare for merging support for later SoCs, such as SAMA7G5. This code was based on the vendor's kernel (linux4microchip). Cc'ing original contributors. Cc: Tudor Ambarus <tudor.ambarus@linaro.org> Cc: Varshini Rajendran <varshini.rajendran@microchip.com> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> --- Notes: Changes in v2: * Remove obsolete SPI naming drivers/spi/atmel-quadspi.c | 111 +++++++++++++++++++++++++----------- 1 file changed, 77 insertions(+), 34 deletions(-)