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

[Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > Co-developed-by: Shreeya Patel 
> > <shreeya.patel-ZGY8ohtN/8qB+jHODAdFcQ@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Shreeya Patel 
> > <shreeya.patel-ZGY8ohtN/8qB+jHODAdFcQ@xxxxxxxxxxxxxxxx>
> > ---
> >
> > 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;