Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

Hi Mark,
    Thanks for your advice.

On 2025/7/7 21:05, Mark Brown wrote:
> Subject:
> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver
> From:
> Mark Brown <broonie@kernel.org>
> Date:
> 2025/7/7 21:05
> 
> To:
> xianwei.zhao@amlogic.com
> CC:
> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, 
> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley 
> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, 
> linux-spi@vger.kernel.org, devicetree@vger.kernel.org, 
> linux-kernel@vger.kernel.org
> 
> 
> 
> On Fri, Jul 04, 2025 at 10:59:33AM +0800, Xianwei Zhao via B4 Relay wrote:
> 
>> Introduced support for the new SPI IP (SPISG) driver. The SPISG is
>> a communication-oriented SPI controller from Amlogic,supporting
>> three operation modes: PIO, block DMA, and scatter-gather DMA.
> This looks good, a few small things below but nothing major.
> 
>> +static bool aml_spisg_can_dma(struct spi_controller *ctlr,
>> +			      struct spi_device *spi,
>> +			      struct spi_transfer *xfer)
>> +{
>> +	return true;
>> +}
> Is it worth having a copybreak such that smaller transfers are done
> using PIO?  With a lot of controllers that increases performance due to
> the extra overhead of setting up DMA, talking to the DMA and interrupt
> controllers can be as expensive as directly accessing the FIFOs.
> 

If the data volume of a single transfer (xfer) is small, PIO mode does 
offer some advantages. However, since PIO requires the CPU to wait in a 
busy loop for the transfer to complete, it continuously occupies CPU 
resources. As a result, its advantages are not particularly significant.

If PIO is to be implemented, it can only handle one transfer at a time 
(via transfer_one), and not entire messages (which consist of multiple 
transfers). In contrast, when processing messages, the SPI controller 
can handle the entire sequence in one go, which also provides certain 
benefits.

Taking all factors into account, it may be better not to add PIO support.

>> +static irqreturn_t aml_spisg_irq(int irq, void *data)
>> +{
>> +	struct spisg_device *spisg = (void *)data;
>> +	u32 sts;
>> +
>> +	spisg->status = 0;
>> +	regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts);
>> +	regmap_write(spisg->map, SPISG_REG_IRQ_STS, sts);
>> +	if (sts & (IRQ_RCH_DESC_INVALID |
>> +		   IRQ_RCH_DESC_RESP |
>> +		   IRQ_RCH_DATA_RESP |
>> +		   IRQ_WCH_DESC_INVALID |
>> +		   IRQ_WCH_DESC_RESP |
>> +		   IRQ_WCH_DATA_RESP |
>> +		   IRQ_DESC_ERR))
>> +		spisg->status = sts;
>> +
>> +	complete(&spisg->completion);
>> +
>> +	return IRQ_HANDLED;
> It'd be better to check if there's an interrupt actually flagged and
> return IRQ_NONE if not, as well as supporting sharing that means that
> the interrupt core can handle any errors that cause the interrupt to
> latch on.
>

Will do,unreasonable values will be returned IRQ_NONE.

>> +	ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "irq request failed\n");
>> +		goto out_controller;
>> +	}
>> +
>> +	ret = aml_spisg_clk_init(spisg, base);
>> +	if (ret)
>> +		goto out_controller;
> Do we need the clocks for register access - if so what happens if the
> interrupt fires as soon as it is registered?  I'd have expected
> requesting the interrupt to be one of the last things done.

Will move irq request of the processing to a bit further back.

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic