| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <90912685-33e3-d94e-c83f-2d11b967e3a1@collabora.com>
Date: Fri, 26 Aug 2022 16:49:11 +0530
From: Shreeya Patel <shreeya.patel@...labora.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: alvaro.soliverez@...labora.com, broonie@...nel.org,
kernel@...labora.com, krisman@...labora.com,
linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org,
sanju.mehta@....com, tanureal@...nsource.cirrus.com
Subject: Re: [PATCH v3] spi: amd: Configure device speed
On 26/08/22 12:14, Christophe JAILLET wrote:
> Le 25/08/2022 à 16:31, Shreeya Patel a écrit :
>> From: Lucas Tanure
>> <tanureal-yzvPICuk2AA4QjBA90+/kJqQE7yCjDx5@...lic.gmane.org>
>>
>> Number of clock frequencies are supported by AMD controller
>> which are mentioned in the amd_spi_freq structure table.
>>
>> Create mechanism to configure device clock frequency such
>> that it is strictly less than the requested frequency.
>>
>> Give priority to the device transfer speed and in case
>> it is not set then use the max clock speed supported
>> by the device.
>>
>> Signed-off-by: Lucas Tanure
>> <tanureal-yzvPICuk2AA4QjBA90+/kJqQE7yCjDx5@...lic.gmane.org>
>> Co-developed-by: Shreeya Patel
>> <shreeya.patel-ZGY8ohtN/8qB+jHODAdFcQ@...lic.gmane.org>
>> Signed-off-by: Shreeya Patel
>> <shreeya.patel-ZGY8ohtN/8qB+jHODAdFcQ@...lic.gmane.org>
>> ---
>>
>> Changes in v3
>> - Make changes as per the current code and resend.
>>
>> Changes in v2
>> - Improve the commit message.
>>
>> drivers/spi/spi-amd.c | 97 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 97 insertions(+)
>>
>> diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
>> index b3b3f5167c4c..264633c5ce0b 100644
>> --- a/drivers/spi/spi-amd.c
>> +++ b/drivers/spi/spi-amd.c
>> @@ -36,6 +36,18 @@
>> #define AMD_SPI_FIFO_SIZE 70
>> #define AMD_SPI_MEM_SIZE 200
>> +#define AMD_SPI_ENA_REG 0x20
>> +#define AMD_SPI_ALT_SPD_SHIFT 20
>> +#define AMD_SPI_ALT_SPD_MASK GENMASK(23, AMD_SPI_ALT_SPD_SHIFT)
>> +#define AMD_SPI_SPI100_SHIFT 0
>> +#define AMD_SPI_SPI100_MASK GENMASK(AMD_SPI_SPI100_SHIFT,
>> AMD_SPI_SPI100_SHIFT)
>> +#define AMD_SPI_SPEED_REG 0x6C
>> +#define AMD_SPI_SPD7_SHIFT 8
>> +#define AMD_SPI_SPD7_MASK GENMASK(13, AMD_SPI_SPD7_SHIFT)
>> +
>> +#define AMD_SPI_MAX_HZ 100000000
>> +#define AMD_SPI_MIN_HZ 800000
>> +
>> /**
>> * enum amd_spi_versions - SPI controller versions
>> * @AMD_SPI_V1: AMDI0061 hardware version
>> @@ -46,14 +58,41 @@ enum amd_spi_versions {
>> AMD_SPI_V2,
>> };
>> +enum amd_spi_speed {
>> + F_66_66MHz,
>> + F_33_33MHz,
>> + F_22_22MHz,
>> + F_16_66MHz,
>> + F_100MHz,
>> + F_800KHz,
>> + SPI_SPD7,
>> + F_50MHz = 0x4,
>> + F_4MHz = 0x32,
>> + F_3_17MHz = 0x3F
>> +};
>> +
>> +/**
>> + * struct amd_spi_freq - Matches device speed with values to write
>> in regs
>> + * @speed_hz: Device frequency
>> + * @enable_val: Value to be written to "enable register"
>> + * @spd7_val: Some frequencies requires to have a value written at
>> SPISPEED register
>> + */
>> +struct amd_spi_freq {
>> + u32 speed_hz;
>> + u32 enable_val;
>> + u32 spd7_val;
>> +};
>> +
>> /**
>> * struct amd_spi - SPI driver instance
>> * @io_remap_addr: Start address of the SPI controller registers
>> * @version: SPI controller hardware version
>> + * @speed_hz: Device frequency
>> */
>> struct amd_spi {
>> void __iomem *io_remap_addr;
>> enum amd_spi_versions version;
>> + unsigned int speed_hz;
>> };
>> static inline u8 amd_spi_readreg8(struct amd_spi *amd_spi, int idx)
>> @@ -185,11 +224,62 @@ static int amd_spi_master_setup(struct
>> spi_device *spi)
>> return 0;
>> }
>> +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},
>> +};
>> +
>
> Hi,
> I'm not sure to understand the logic here:
>
>> +static int amd_set_spi_freq(struct amd_spi *amd_spi, u32 speed_hz)
>> +{
>> + unsigned int i, spd7_val, alt_spd;
>> +
>> + if (speed_hz == amd_spi->speed_hz)
>> + return 0;
>
> If we set a speed that is already the one in use --> return success
>
>> +
>> + if (speed_hz < AMD_SPI_MIN_HZ)
>> + return -EINVAL;
>
> If we request a speed below the lower possible --> return error.
>
>> +
>> + for (i = 0; i < ARRAY_SIZE(amd_spi_freq); i++)
>> + if (speed_hz >= amd_spi_freq[i].speed_hz)
>> + break;
>
> Search the array for the speed that is just below or equal to the
> requested one.
>
>> +
>> + if (speed_hz == amd_spi_freq[i].speed_hz)
>> + return 0;
>
> If it is an exact match --> return success
> ???
>
> So, if the requested value is one of the table, nothing is done and it
> can't be selected.
>
> Is it what is expected?
> Without looking at the rest of the code or at the driver, I don't see
> the point of this test.
>
>
> Should this be:
> if (amd_spi->speed_hz == amd_spi_freq[i].speed_hz)
> ^^^^^^^^^
> ?
>
Yes you are right, it should be amd_spi->speed_hz instead of speed_hz.
Thanks for pointing it out.
I'll send a fix for it.
Thanks,
Shreeya Patel
>
> If so, the first 'if (speed_hz == amd_spi->speed_hz)' looks redundant.
> (I guess it is a slow path, so should it be an optimization, it
> should'nt really matter)
>
> CJ
>
>> +
>> + amd_spi->speed_hz = amd_spi_freq[i].speed_hz;
>> +
>> + alt_spd = (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, alt_spd,
>> + AMD_SPI_ALT_SPD_MASK);
>> +
>> + if (amd_spi->speed_hz == AMD_SPI_MAX_HZ)
>> + amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, 1,
>> + AMD_SPI_SPI100_MASK);
>> +
>> + if (amd_spi_freq[i].spd7_val) {
>> + spd7_val = (amd_spi_freq[i].spd7_val << AMD_SPI_SPD7_SHIFT)
>> + & AMD_SPI_SPD7_MASK;
>> + amd_spi_setclear_reg32(amd_spi, AMD_SPI_SPEED_REG, spd7_val,
>> + AMD_SPI_SPD7_MASK);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi,
>> struct spi_master *master,
>> struct spi_message *message)
>> {
>> struct spi_transfer *xfer = NULL;
>> + struct spi_device *spi = message->spi;
>> u8 cmd_opcode = 0, fifo_pos = AMD_SPI_FIFO_BASE;
>> u8 *buf = NULL;
>> u32 i = 0;
>> @@ -197,6 +287,11 @@ static inline int amd_spi_fifo_xfer(struct
>> amd_spi *amd_spi,
>> list_for_each_entry(xfer, &message->transfers,
>> transfer_list) {
>> + if (xfer->speed_hz)
>> + amd_set_spi_freq(amd_spi, xfer->speed_hz);
>> + else
>> + amd_set_spi_freq(amd_spi, spi->max_speed_hz);
>> +
>> if (xfer->tx_buf) {
>> buf = (u8 *)xfer->tx_buf;
>> if (!tx_len) {
>> @@ -313,6 +408,8 @@ static int amd_spi_probe(struct platform_device
>> *pdev)
>> master->num_chipselect = 4;
>> master->mode_bits = 0;
>> master->flags = SPI_MASTER_HALF_DUPLEX;
>> + master->max_speed_hz = AMD_SPI_MAX_HZ;
>> + master->min_speed_hz = AMD_SPI_MIN_HZ;
>> master->setup = amd_spi_master_setup;
>> master->transfer_one_message = amd_spi_master_transfer;
>> master->max_transfer_size = amd_spi_max_transfer_size;
>
>
Powered by blists - more mailing lists