Re: [PATCH v2 10/10] spi: atmel-quadspi: add support for sam9x60 qspi controller
On Fri, 1 Feb 2019 14:49:27 + wrote: > > +#define QSPI_IFR_APBTFRTYP_READ BIT(24) > > > > And this one would be > > > > define QSPI_IFR_SAM9X60_READ_TRSFR BIT(24) > > I prefer letting this bit named as in the datasheet, QSPI_IFR_APBTFRTYP_READ, > and change it if future versions of the IP will modify its sense. It is a READ > transfer done on APB, it is more generic this way. If you have a strong > opinion > on this, please let me know. As you wish.
Re: [PATCH v2 10/10] spi: atmel-quadspi: add support for sam9x60 qspi controller
On 02/01/2019 09:07 AM, tudor.amba...@microchip.com wrote:
cut
>>> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
cut
>>> +static int atmel_sam9x60_qspi_set_cfg(void __iomem *base,
>>> + const struct spi_mem_op *op,
>>> + struct atmel_qspi_cfg *cfg)
>>> +{
>>> + int ret = atmel_qspi_set_mode(cfg, op);
>>> +
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = atmel_qspi_set_address_mode(cfg, op);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + cfg->ifr |= QSPI_IFR_INSTEN;
>>> + cfg->icr |= QSPI_ICR_INST(op->cmd.opcode);
>>> +
>>> + /* Set data enable */
>>> + if (op->data.nbytes)
>>> + cfg->ifr |= QSPI_IFR_DATAEN;
>>> +
>>> + if (!op->addr.nbytes) {
>>> + cfg->ifr |= QSPI_IFR_TFRTYP_TRSFR_REG;
>>> + if (op->data.dir == SPI_MEM_DATA_OUT)
>>> + cfg->ifr |= QSPI_IFR_APBTFRTYP_WRITE;
>>> + else
>>> + cfg->ifr |= QSPI_IFR_APBTFRTYP_READ;
>>> + } else {
>>> + cfg->ifr |= QSPI_IFR_TFRTYP_TRSFR_MEM;
>>
>> Can you try doing only regular transfers and let me know if it still
>> works. Support for mem transfers can then be added along with dirmap
>> support.
>
> should work. Will try and let you know.
you were right, it works. I will let mem transfer logic for dirmap support.
Cheers,
ta
Re: [PATCH v2 10/10] spi: atmel-quadspi: add support for sam9x60 qspi controller
On 02/01/2019 09:57 AM, Boris Brezillon wrote: > On Fri, 1 Feb 2019 07:07:40 + > wrote: > >>> #define QSPI_IFR_TFRTYP_MASKGENMASK(13, 12) #define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12) #define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12) >>> >>> Looks like the read/write flag is on bit 13. Can we just add >> >> for sama5d2 only > > Feel free to prefix macros with the SoC name to make it clear: > > #define QSPI_IFR_SAMA5D2_WRITE_TRSFR BIT(13) agreed > cut +#define QSPI_IFR_APBTFRTYP_READ BIT(24) > > And this one would be > > define QSPI_IFR_SAM9X60_READ_TRSFRBIT(24) I prefer letting this bit named as in the datasheet, QSPI_IFR_APBTFRTYP_READ, and change it if future versions of the IP will modify its sense. It is a READ transfer done on APB, it is more generic this way. If you have a strong opinion on this, please let me know. ta
Re: [PATCH v2 10/10] spi: atmel-quadspi: add support for sam9x60 qspi controller
On Fri, 1 Feb 2019 07:07:40 +
wrote:
> >
> >> #define QSPI_IFR_TFRTYP_MASKGENMASK(13, 12)
> >> #define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12)
> >> #define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12)
> >
> > Looks like the read/write flag is on bit 13. Can we just add
>
> for sama5d2 only
Feel free to prefix macros with the SoC name to make it clear:
#define QSPI_IFR_SAMA5D2_WRITE_TRSFRBIT(13)
>
> >
> > #define QSPI_IFR_TFRTYP_TRSFR_WRITE BIT(13)
> >
> > and drop all others def? This way the implementation is consistent
> > between sam9x60 and sama5d2.
>
> BIT(13) has no meaning for sam9x60. I can drop the macros with zero value for
> sama5d2 in a separate patch.
> >
> >> +#define QSPI_IFR_APBTFRTYP_READ BIT(24)
And this one would be
define QSPI_IFR_SAM9X60_READ_TRSFR BIT(24)
> >>
> >> /* Bitfields in QSPI_SMR (Scrambling Mode Register) */
> >> #define QSPI_SMR_SCREN BIT(0)
> >> @@ -137,16 +144,37 @@
> >> #define QSPI_WPSR_WPVSRC(src) (((src) << 8) & QSPI_WPSR_WPVSRC)
> >>
> >>
> >> +/* Describes register values. */
> >> +struct atmel_qspi_cfg {
> >> + u32 icr;
> >> + u32 iar;
> >> + u32 ifr;
> >> +};
> >> +
> >> +struct atmel_qspi_caps;
> >> +
> >> struct atmel_qspi {
> >>void __iomem*regs;
> >>void __iomem*mem;
> >>struct clk *clk;
> >
> > Can we rename that on pclk?
>
> will rename it, together with the support for unnamed clock of sama5d2 in a
> separate
> patch. The dt-bindings patch that imposes "pclk" for sama5d2 should be
> separated too.
Sounds good.
Re: [PATCH v2 10/10] spi: atmel-quadspi: add support for sam9x60 qspi controller
On 01/31/2019 06:32 PM, Boris Brezillon wrote:
> On Thu, 31 Jan 2019 16:15:51 +
> wrote:
>
>> From: Tudor Ambarus
>>
>> The sam9x60 qspi controller uses 2 clocks, one for the peripheral register
>> access, the other for the qspi core and phy. Both are mandatory. It uses
>> dedicated register for Read Instruction Code Register (RICR) and
>> Write Instruction Code Register (WICR). ICR/RICR/WICR have identical
>> fields.
>>
>> Tested with sst26vf064b jedec,spi-nor flash. Backward compatibility test
>> done on sama5d2 qspi controller and mx25l25635e jedec,spi-nor flash.
>>
>> Signed-off-by: Tudor Ambarus
>> ---
>> v2:
>> - rework clock handling
>> - reorder setting of register values in set_cfg() calls -> move functions
>> that can fail in the upper part of the function body.
>>
>> drivers/spi/atmel-quadspi.c | 296
>> +++-
>> 1 file changed, 239 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
>> index d3e76acf8517..80c934f3e479 100644
>> --- a/drivers/spi/atmel-quadspi.c
>> +++ b/drivers/spi/atmel-quadspi.c
>> @@ -19,6 +19,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -35,7 +36,9 @@
>>
>> #define QSPI_IAR 0x0030 /* Instruction Address Register */
>> #define QSPI_ICR 0x0034 /* Instruction Code Register */
>> +#define QSPI_WICR0x0034 /* Write Instruction Code Register */
>> #define QSPI_IFR 0x0038 /* Instruction Frame Register */
>> +#define QSPI_RICR0x003C /* Read Instruction Code Register */
>>
>> #define QSPI_SMR 0x0040 /* Scrambling Mode Register */
>> #define QSPI_SKR 0x0044 /* Scrambling Key Register */
>> @@ -88,7 +91,7 @@
>> #define QSPI_SCR_DLYBS_MASK GENMASK(23, 16)
>> #define QSPI_SCR_DLYBS(n) (((n) << 16) & QSPI_SCR_DLYBS_MASK)
>>
>> -/* Bitfields in QSPI_ICR (Instruction Code Register) */
>> +/* Bitfields in QSPI_ICR (Read/Write Instruction Code Register) */
>> #define QSPI_ICR_INST_MASK GENMASK(7, 0)
>> #define QSPI_ICR_INST(inst) (((inst) << 0) & QSPI_ICR_INST_MASK)
>> #define QSPI_ICR_OPT_MASK GENMASK(23, 16)
>> @@ -113,6 +116,8 @@
>> #define QSPI_IFR_OPTL_4BIT (2 << 8)
>> #define QSPI_IFR_OPTL_8BIT (3 << 8)
>> #define QSPI_IFR_ADDRL BIT(10)
>> +#define QSPI_IFR_TFRTYP_TRSFR_MEM BIT(12)
>> +#define QSPI_IFR_TFRTYP_TRSFR_REG (0 << 12)
>
> You don't need to define TRSFR_REG, just set QSPI_IFR_TFRTYP_TRSFR_MEM
> when you do a mem transfer and do nothing when this is a regular
> transfer.
I chose to introduce macros with zero value for better code readability. I would
expect that the NOP operations to be optimized at compile time. I will remove
them
if you prefer, it will result in fewer lines of code.
>
>> #define QSPI_IFR_TFRTYP_MASKGENMASK(13, 12)
>> #define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12)
>> #define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12)
>
> Looks like the read/write flag is on bit 13. Can we just add
for sama5d2 only
>
> #define QSPI_IFR_TFRTYP_TRSFR_WRITE BIT(13)
>
> and drop all others def? This way the implementation is consistent
> between sam9x60 and sama5d2.
BIT(13) has no meaning for sam9x60. I can drop the macros with zero value for
sama5d2 in a separate patch.
>
>> @@ -121,6 +126,8 @@
>> #define QSPI_IFR_CRMBIT(14)
>> #define QSPI_IFR_NBDUM_MASK GENMASK(20, 16)
>> #define QSPI_IFR_NBDUM(n) (((n) << 16) & QSPI_IFR_NBDUM_MASK)
>> +#define QSPI_IFR_APBTFRTYP_WRITE(0 << 24)
>
> As for the other defs, I don't think you need to define _WRITE.
understood
>
>> +#define QSPI_IFR_APBTFRTYP_READ BIT(24)
>>
>> /* Bitfields in QSPI_SMR (Scrambling Mode Register) */
>> #define QSPI_SMR_SCREN BIT(0)
>> @@ -137,16 +144,37 @@
>> #define QSPI_WPSR_WPVSRC(src) (((src) << 8) & QSPI_WPSR_WPVSRC)
>>
>>
>> +/* Describes register values. */
>> +struct atmel_qspi_cfg {
>> +u32 icr;
>> +u32 iar;
>> +u32 ifr;
>> +};
>> +
>> +struct atmel_qspi_caps;
>> +
>> struct atmel_qspi {
>> void __iomem*regs;
>> void __iomem*mem;
>> struct clk *clk;
>
> Can we rename that on pclk?
will rename it, together with the support for unnamed clock of sama5d2 in a
separate
patch. The dt-bindings patch that imposes "pclk" for sama5d2 should be
separated too.
>
>> +struct clk *qspick;
>> struct platform_device *pdev;
>> +const struct atmel_qspi_caps *caps;
>> u32 pending;
>> u32 mr;
>> struct completion cmd_completion;
>> };
>>
>
> ...
>
>> +
>> +static int atmel_sam9x60_qspi_set_cfg(void __iomem *base,
>> + const struct spi_mem_op *op,
>> +
Re: [PATCH v2 10/10] spi: atmel-quadspi: add support for sam9x60 qspi controller
On Thu, 31 Jan 2019 16:15:51 +
wrote:
> From: Tudor Ambarus
>
> The sam9x60 qspi controller uses 2 clocks, one for the peripheral register
> access, the other for the qspi core and phy. Both are mandatory. It uses
> dedicated register for Read Instruction Code Register (RICR) and
> Write Instruction Code Register (WICR). ICR/RICR/WICR have identical
> fields.
>
> Tested with sst26vf064b jedec,spi-nor flash. Backward compatibility test
> done on sama5d2 qspi controller and mx25l25635e jedec,spi-nor flash.
>
> Signed-off-by: Tudor Ambarus
> ---
> v2:
> - rework clock handling
> - reorder setting of register values in set_cfg() calls -> move functions
> that can fail in the upper part of the function body.
>
> drivers/spi/atmel-quadspi.c | 296
> +++-
> 1 file changed, 239 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index d3e76acf8517..80c934f3e479 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -35,7 +36,9 @@
>
> #define QSPI_IAR 0x0030 /* Instruction Address Register */
> #define QSPI_ICR 0x0034 /* Instruction Code Register */
> +#define QSPI_WICR0x0034 /* Write Instruction Code Register */
> #define QSPI_IFR 0x0038 /* Instruction Frame Register */
> +#define QSPI_RICR0x003C /* Read Instruction Code Register */
>
> #define QSPI_SMR 0x0040 /* Scrambling Mode Register */
> #define QSPI_SKR 0x0044 /* Scrambling Key Register */
> @@ -88,7 +91,7 @@
> #define QSPI_SCR_DLYBS_MASK GENMASK(23, 16)
> #define QSPI_SCR_DLYBS(n) (((n) << 16) & QSPI_SCR_DLYBS_MASK)
>
> -/* Bitfields in QSPI_ICR (Instruction Code Register) */
> +/* Bitfields in QSPI_ICR (Read/Write Instruction Code Register) */
> #define QSPI_ICR_INST_MASK GENMASK(7, 0)
> #define QSPI_ICR_INST(inst) (((inst) << 0) & QSPI_ICR_INST_MASK)
> #define QSPI_ICR_OPT_MASK GENMASK(23, 16)
> @@ -113,6 +116,8 @@
> #define QSPI_IFR_OPTL_4BIT (2 << 8)
> #define QSPI_IFR_OPTL_8BIT (3 << 8)
> #define QSPI_IFR_ADDRL BIT(10)
> +#define QSPI_IFR_TFRTYP_TRSFR_MEMBIT(12)
> +#define QSPI_IFR_TFRTYP_TRSFR_REG(0 << 12)
You don't need to define TRSFR_REG, just set QSPI_IFR_TFRTYP_TRSFR_MEM
when you do a mem transfer and do nothing when this is a regular
transfer.
> #define QSPI_IFR_TFRTYP_MASKGENMASK(13, 12)
> #define QSPI_IFR_TFRTYP_TRSFR_READ (0 << 12)
> #define QSPI_IFR_TFRTYP_TRSFR_READ_MEM (1 << 12)
Looks like the read/write flag is on bit 13. Can we just add
#define QSPI_IFR_TFRTYP_TRSFR_WRITE BIT(13)
and drop all others def? This way the implementation is consistent
between sam9x60 and sama5d2.
> @@ -121,6 +126,8 @@
> #define QSPI_IFR_CRMBIT(14)
> #define QSPI_IFR_NBDUM_MASK GENMASK(20, 16)
> #define QSPI_IFR_NBDUM(n) (((n) << 16) & QSPI_IFR_NBDUM_MASK)
> +#define QSPI_IFR_APBTFRTYP_WRITE (0 << 24)
As for the other defs, I don't think you need to define _WRITE.
> +#define QSPI_IFR_APBTFRTYP_READ BIT(24)
>
> /* Bitfields in QSPI_SMR (Scrambling Mode Register) */
> #define QSPI_SMR_SCREN BIT(0)
> @@ -137,16 +144,37 @@
> #define QSPI_WPSR_WPVSRC(src) (((src) << 8) & QSPI_WPSR_WPVSRC)
>
>
> +/* Describes register values. */
> +struct atmel_qspi_cfg {
> + u32 icr;
> + u32 iar;
> + u32 ifr;
> +};
> +
> +struct atmel_qspi_caps;
> +
> struct atmel_qspi {
> void __iomem*regs;
> void __iomem*mem;
> struct clk *clk;
Can we rename that on pclk?
> + struct clk *qspick;
> struct platform_device *pdev;
> + const struct atmel_qspi_caps *caps;
> u32 pending;
> u32 mr;
> struct completion cmd_completion;
> };
>
...
> +
> +static int atmel_sam9x60_qspi_set_cfg(void __iomem *base,
> + const struct spi_mem_op *op,
> + struct atmel_qspi_cfg *cfg)
> +{
> + int ret = atmel_qspi_set_mode(cfg, op);
> +
> + if (ret)
> + return ret;
> +
> + ret = atmel_qspi_set_address_mode(cfg, op);
> + if (ret)
> + return ret;
> +
> + cfg->ifr |= QSPI_IFR_INSTEN;
> + cfg->icr |= QSPI_ICR_INST(op->cmd.opcode);
> +
> + /* Set data enable */
> + if (op->data.nbytes)
> + cfg->ifr |= QSPI_IFR_DATAEN;
> +
> + if (!op->addr.nbytes) {
> + cfg->ifr |= QSPI_IFR_TFRTYP_TRSFR_REG;
> + if (op->data.dir == SPI_MEM_DATA_OUT)
> + cfg->ifr |= QSPI_IFR_APBTFRTYP_WRITE;
> + else
> + cfg->ifr |= QSPI_IFR_A
