Thanks for the review, Arnd! On 12/6/24 6:47 AM, Arnd Bergmann wrote: > On Thu, Dec 5, 2024, at 18:53, Tudor Ambarus wrote: > >> +#define exynos_acpm_set_bulk(data, i) \ >> + (((data) & ACPM_BULK_MASK) << (ACPM_BULK_SHIFT * (i))) >> +#define exynos_acpm_read_bulk(data, i) \ >> + (((data) >> (ACPM_BULK_SHIFT * (i))) & ACPM_BULK_MASK) > > Could these be inline functions for readability? sure, will do. > >> + cmd[3] = (u32)(sched_clock() / 1000000); /*record ktime ms*/ > > The comment does not match the implementation, sched_clock() > is probably not what you want here because of its limitiations. > > Maybe ktime_to_ms(ktime_get())? > Indeed, will use, thanks. >> +/** >> + * acpm_get_rx() - get response from RX queue. >> + * @achan: ACPM channel info. >> + * @xfer: reference to the transfer to get response for. >> + * >> + * Return: 0 on success, -errno otherwise. >> + */ >> +static int acpm_get_rx(struct acpm_chan *achan, struct acpm_xfer *xfer) >> +{ >> + struct acpm_msg *tx = &xfer->tx; >> + struct acpm_msg *rx = &xfer->rx; >> + struct acpm_rx_data *rx_data; >> + const void __iomem *base, *addr; >> + u32 rx_front, rx_seqnum, tx_seqnum, seqnum; >> + u32 i, val, mlen; >> + bool rx_set = false; >> + >> + rx_front = readl_relaxed(achan->rx.front); >> + i = readl_relaxed(achan->rx.rear); > > If you have to use readl_relaxed(), please annotate why, > otherwise just use the normal readl(). Is this access to > the SRAM? all IO accesses in this driver are to SRAM, yes. There are no DMA accesses involved in the driver and the _relaxed() accessors are fully ordered for accesses to the same endpoint, so I thought _relaxed are fine. > >> + spin_lock_irqsave(&achan->tx_lock, flags); >> + >> + tx_front = readl_relaxed(achan->tx.front); >> + idx = (tx_front + 1) % achan->qlen; >> + >> + ret = acpm_wait_for_queue_slots(achan, idx); >> + if (ret) { >> + spin_unlock_irqrestore(&achan->tx_lock, flags); >> + return ret; >> + } > > It looks like you are calling a busy loop function inside > of a hardirq handler here, with a 500ms timeout. This is > not ok. That's true, the code assumes that the method can be called from hard irq context. Can't tell whether that timeout is accurate, it comes from downstream and the resources that I have do not specify anything about what would be an acceptable timeout. I see arm_scmi typically uses 30 ms for transport layers. > > If you may need to wait for a long timeout, I would suggest > changing the interface so that this function is not allowed > to be called from irq-disabled context, change the spinlock > to a mutex and polling read to a sleeping version. I think the question I need to answer here is whether the acpm interface can be called from atomic context or not. On a first look, all downstream drivers call it from process context. Curios there's no clock enable though in downstream, which would require atomic context. I'll get back to you on this. If there's at least a client that calls the interface in hard/soft irq context (clocks?) then I don't think I'll be able to implement your suggestion. Each channel has its own TX/RX rings in SRAM. If there are requests from both hard irq and process context for the same channel, then I need to protect the accesses to the rings via spin_lock_irqsave. This is what the code assumes, because downstream allows calls from atomic context even if I can't pinpoint one right now. I guess I can switch everything to sleeping version, and worry about atomic context when such a client gets proposed? > >> + /* Advance TX front. */ >> + writel_relaxed(idx, achan->tx.front); >> + >> + /* Flush SRAM posted writes. */ >> + readl_relaxed(achan->tx.front); >> + >> + spin_unlock_irqrestore(&achan->tx_lock, flags); > > I don't think this sequence guarantees the serialization > you want. By making the access _relaxed() you explicitly > say you don't want serialization, so the store does > not have to complete before the unlock. > I could benefit of the non relaxed versions indeed. >> + .of_match_table = of_match_ptr(acpm_match), > > Remove the stray of_match_ptr() here. okay >> --- /dev/null >> +++ b/include/linux/soc/samsung/exynos-acpm-protocol.h > > Why is this in include/linux/soc, and not in the firmware > header? right, will move. Thanks! ta