Re: [PATCH] spi: amd: Configure device speed

[Mark Brown]

On Thu, Mar 17, 2022 at 11:14:53AM -0300, André Almeida wrote:

> Create mechanism to configure device clock frequency. For now, it can
> only set the device to use the maximum speed.

This changelog doesn't really tell me what the patch does - what is the
mechanism?  What exactly do you mean by "use the maximum speed" and why
aren't the normal speed setting interfaces supported?

> +/* Set device speed to the maximum frequency supported */
> +static bool max_speed;
> +module_param(max_speed, bool, 0644);

It s very surprising to have a module parameter for this, we should be
setting it by quirk.  If this is system specific presumably it's
possibly per controller system specific so a module parameter doesn't
cut it, and in general we don't have device specific command line quirks
like this.

> +static const struct amd_spi_freq amd_spi_freq[] = {
> +	{ AMD_SPI_MAX_HZ,   F_100MHz,         0},
> +	{       66660000, F_66_66MHz,         0},
> +	{       50000000,   SPI_SPD7,   F_50MHz},
> +	{       33330000, F_33_33MHz,         0},
> +	{       22220000, F_22_22MHz,         0},
> +	{       16660000, F_16_66MHz,         0},
> +	{        4000000,   SPI_SPD7,    F_4MHz},
> +	{        3170000,   SPI_SPD7, F_3_17MHz},
> +	{ AMD_SPI_MIN_HZ,   F_800KHz,         0},
> +};

This looks like a table of specific clock frequencies.  Why not just
implement the standard interface for setting the speed of SPI transfers?

> +static int amd_set_spi_freq(struct amd_spi *amd_spi, u32 speed_hz)
> +{
> +	unsigned int i, spd7_val, enable_val;
> +
> +	if (speed_hz == amd_spi->speed_hz)
> +		return 0;
> +
> +	if (speed_hz < AMD_SPI_MIN_HZ)
> +		return -EINVAL;

Why not just check to see if we don't find a matching value in the
table?

> +	for (i = 0; i < ARRAY_SIZE(amd_spi_freq); i++) {
> +		if (speed_hz >= amd_spi_freq[i].speed_hz) {
> +			if (amd_spi->speed_hz == amd_spi_freq[i].speed_hz)
> +				return 0;
> +
> +			amd_spi->speed_hz = amd_spi_freq[i].speed_hz;

This would be easier to read if it were refactored so that the code
applying the setting is factored out of the loop looking for a matching
entry in the table - that way most of the code would have less
indentation.

> +			enable_val = (amd_spi_freq[i].enable_val << AMD_SPI_ALT_SPD_SHIFT)
> +				     & AMD_SPI_ALT_SPD_MASK;
> +			amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, enable_val,
> +					       AMD_SPI_ALT_SPD_MASK);

So enable_val is actually the value to write to _ALT_SPD in _ENA_REG?
It might be clearer to call it alt_spd or something.  I have to say that
the use of set and clear on the same bitfield in the same operation is
surprising and I'd have to go and check the implementation to verify
that the set happens after the clear - it would be helpful to have an
_update_bits() style helper for these operations, it's more directly
what's being done.

> +
> +			if (amd_spi->speed_hz == AMD_SPI_MAX_HZ)
> +				amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, 1,
> +						       AMD_SPI_SPI100_MASK);

This is the same register as above - could things be combined, and also
doesn't the operation need to be reversed if we ever set a speed other
than the maximum?

Attachment: signature.asc
Description: PGP signature