Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Brian Norris 
On Tue, Aug 25, 2015 at 12:22:10PM +0200, Marek Vasut wrote:
> On Tuesday, August 25, 2015 at 12:17:37 PM, Cyrille Pitchen wrote:
> > If you don't mind, I'd rather keep some of these inline functions. I have
> > no strong justification, it's more a personal taste: it makes lines
> > shorter as it avoids the need to add "->regs + ".
> > Also it makes the code consistent with other Atmel drivers which already
> > use such wrappers.
> > 
> > However I'll fix the comment and remove the byte and word versions, which
> > are not used. So only qspi_readl() and qspi_writel() are left.
> > 
> > Does it sound good to you?
> 
> In my mind, seeing explicit readl_relaxed() somewhere is much easier to
> digest than seeing some wrapper, which I have to look up. But please do
> wait for others to voice their concern too, I might not be the best person
> to tell you what to do when it comes to wrapping IO accessors ;-)

I could go either way, but there are times where local wrapper I/O
accessors are useful. Case in point: it makes it really easy to make the
choice between readl() and readl_relaxed() in one place (i.e., the
discussion you had in another branch of this thread). That's been useful
for me on brcmnand, where certain platforms (big-endian MIPS) have
different assumptions about endianness than your average platform. Also,
it helps with things like what Robert Jarzmik is trying to do on
pxa3xx_nand -- add debug info to print every register read/write.

Also as Cyrille mentioned, personal taste is a factor.

Anyway, I'll go with whatever makes sense between y'all. I don't mind.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Cyrille Pitchen 
Hi!

Le 25/08/2015 03:44, Bean Huo 霍斌斌 (beanhuo) a écrit :
> 
>> +nor->read_reg = atmel_qspi_read_reg;
>> +nor->write_reg = atmel_qspi_write_reg;
>> +nor->read = atmel_qspi_read;
>> +nor->write = atmel_qspi_write;
>> +nor->erase = atmel_qspi_erase;
>> +nor->set_protocol = atmel_qspi_set_protocol;
> 
> This is very good, the structure of spi_nor should add a hook function to 
> notify spi controller 
> That spi nor transfer protocol already changed.
> 
>> +
>> +if (of_modalias_node(child, modalias, sizeof(modalias)) < 0) {
>> +err = -ENODEV;
>> +goto release_channel;
>> +}
>> +
>> +err = of_property_read_u32(child, "spi-max-frequency", >clk_rate);
>> +if (err < 0)
>> +goto release_channel;
>> +
>> +err = atmel_qspi_init(aq);
>> +if (err)
>> +goto release_channel;
>> +
>> +nor->dev->of_node = child;
>> +err = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
>>  goto release_channel;
>> +
> 
> 
> ...
> 
>> static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)  {
>   return mtd->priv;
>> @@ -944,6 +960,11 @@ static int micron_quad_enable(struct spi_nor *nor)
>>  return ret;
>>  }
>>
>> +/* switch protocol to Quad CMD 4-4-4 */
>> +ret = spi_nor_set_protocol(nor, SPI_PROTO_4_4_4);
>> +if (ret)
>> +return ret;
>> +
> 
> This make sense,from spi nor side,once its protocol being changed,
> Mtd layer must notify this status to spi nor controller immediately,
> And spi nor controller also should re-adjust its protocol.
> Otherwise, following reading SR operation will fail. 
> 
>>
>>  ret = spi_nor_wait_till_ready(nor);
>>  if (ret)
>>  return ret;
> 
> If my ack has any value in here, feel free to add it.
> 
> Acked-by: Bean Huo 
> 

Since your comments deal with the protocol change, I'll add your ack to the
first patch of the series:
"mtd: spi-nor: notify (Q)SPI controller about protocol change"

Thanks for your review!

Best regards,

Cyrille

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Marek Vasut 
On Tuesday, August 25, 2015 at 12:17:37 PM, Cyrille Pitchen wrote:
> Hi Marek,

Hi!

> Le 24/08/2015 13:03, Marek Vasut a écrit :
> > On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
> >> This driver add support to the new Atmel QSPI controller embedded into
> >> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
> >> controller.
> >> 
> >> Signed-off-by: Cyrille Pitchen 
> >> Acked-by: Nicolas Ferre 
> > 
> > Hi,
> > 
> > [...]
> > 
> >> +/* Register access macros */
> > 
> > These are functions, not macros :)
> > 
> > btw is there any reason for these ? I'd say, just put the read*() and
> > write*() functions directly into the code and be done with it, it is
> > much less confusing.
> 
> If you don't mind, I'd rather keep some of these inline functions. I have
> no strong justification, it's more a personal taste: it makes lines
> shorter as it avoids the need to add "->regs + ".
> Also it makes the code consistent with other Atmel drivers which already
> use such wrappers.
> 
> However I'll fix the comment and remove the byte and word versions, which
> are not used. So only qspi_readl() and qspi_writel() are left.
> 
> Does it sound good to you?

In my mind, seeing explicit readl_relaxed() somewhere is much easier to
digest than seeing some wrapper, which I have to look up. But please do
wait for others to voice their concern too, I might not be the best person
to tell you what to do when it comes to wrapping IO accessors ;-)

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Cyrille Pitchen 
Le 25/08/2015 11:46, Jonas Gorski a écrit :
> On Mon, Aug 24, 2015 at 7:45 PM, Marek Vasut  wrote:
>> On Monday, August 24, 2015 at 07:04:38 PM, Cyrille Pitchen wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> Le 24/08/2015 13:03, Marek Vasut a écrit :
 On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
> This driver add support to the new Atmel QSPI controller embedded into
> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
> controller.
>>
>> [...]
>>
> +  /* Compute address parameters */
> +  switch (cmd->enable.bits.address) {
> +  case 4:
> +  ifr |= QSPI_IFR_ADDRL;
> +  /*break;*/ /* fallback to the 24bit address case */

 What's this commented out bit of code for ? :-)
>>>
>>> I just wanted to stress out there was no missing "break;".
>>> I've reworded the comment to:
>>> /* No "break" on purpose: fallback to the 24bit address case. */
>>
>> Oh, the address is in bytes . I see, yes, it makes sense to be more
>> explicit here about the purpose of the fallback. I think this change
>> in the comment will make it easier for everyone who comes back in a
>> few years and reads this code.
> 
> I think you are looking for the term "(switch case) fallthrough", not
> "fallback". "Fallback" makes it sound like there is something missing,
> or an invalid state.
> 
> 
> Jonas
> 

will be modified in the next series, thanks for the review!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Cyrille Pitchen 
Hi Marek,

Le 24/08/2015 13:03, Marek Vasut a écrit :
> On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
>> This driver add support to the new Atmel QSPI controller embedded into
>> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
>> controller.
>>
>> Signed-off-by: Cyrille Pitchen 
>> Acked-by: Nicolas Ferre 
> 
> Hi,
> 
> [...]
> 
>> +/* Register access macros */
> 
> These are functions, not macros :)
> 
> btw is there any reason for these ? I'd say, just put the read*() and
> write*() functions directly into the code and be done with it, it is
> much less confusing.

If you don't mind, I'd rather keep some of these inline functions. I have
no strong justification, it's more a personal taste: it makes lines
shorter as it avoids the need to add "->regs + ".
Also it makes the code consistent with other Atmel drivers which already
use such wrappers.

However I'll fix the comment and remove the byte and word versions, which
are not used. So only qspi_readl() and qspi_writel() are left.

Does it sound good to you?

> 
> Also, why do you use the _relaxed() versions of the functions ?
> 
>> +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
>> +{
>> +return readl_relaxed(aq->regs + reg);
>> +}
>> +
>> +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
>> +{
>> +writel_relaxed(value, aq->regs + reg);
>> +}
>> +
>> +static inline u16 qspi_readw(struct atmel_qspi *aq, u32 reg)
>> +{
>> +return readw_relaxed(aq->regs + reg);
>> +}
>> +
>> +static inline void qspi_writew(struct atmel_qspi *aq, u32 reg, u16 value)
>> +{
>> +writew_relaxed(value, aq->regs + reg);
>> +}
>> +
>> +static inline u8 qspi_readb(struct atmel_qspi *aq, u32 reg)
>> +{
>> +return readb_relaxed(aq->regs + reg);
>> +}
>> +
>> +static inline void qspi_writeb(struct atmel_qspi *aq, u32 reg, u8 value)
>> +{
>> +writeb_relaxed(value, aq->regs + reg);
>> +}
> 
> [...]
> 
> Hope this helps :)
> 

Best regards,

Cyrille
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Jonas Gorski 
On Mon, Aug 24, 2015 at 7:45 PM, Marek Vasut  wrote:
> On Monday, August 24, 2015 at 07:04:38 PM, Cyrille Pitchen wrote:
>> Hi Marek,
>
> Hi!
>
>> Le 24/08/2015 13:03, Marek Vasut a écrit :
>> > On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
>> >> This driver add support to the new Atmel QSPI controller embedded into
>> >> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
>> >> controller.
>
> [...]
>
>> >> +  /* Compute address parameters */
>> >> +  switch (cmd->enable.bits.address) {
>> >> +  case 4:
>> >> +  ifr |= QSPI_IFR_ADDRL;
>> >> +  /*break;*/ /* fallback to the 24bit address case */
>> >
>> > What's this commented out bit of code for ? :-)
>>
>> I just wanted to stress out there was no missing "break;".
>> I've reworded the comment to:
>> /* No "break" on purpose: fallback to the 24bit address case. */
>
> Oh, the address is in bytes . I see, yes, it makes sense to be more
> explicit here about the purpose of the fallback. I think this change
> in the comment will make it easier for everyone who comes back in a
> few years and reads this code.

I think you are looking for the term "(switch case) fallthrough", not
"fallback". "Fallback" makes it sound like there is something missing,
or an invalid state.


Jonas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Brian Norris 
On Tue, Aug 25, 2015 at 12:22:10PM +0200, Marek Vasut wrote:
 On Tuesday, August 25, 2015 at 12:17:37 PM, Cyrille Pitchen wrote:
  If you don't mind, I'd rather keep some of these inline functions. I have
  no strong justification, it's more a personal taste: it makes lines
  shorter as it avoids the need to add -regs + .
  Also it makes the code consistent with other Atmel drivers which already
  use such wrappers.
  
  However I'll fix the comment and remove the byte and word versions, which
  are not used. So only qspi_readl() and qspi_writel() are left.
  
  Does it sound good to you?
 
 In my mind, seeing explicit readl_relaxed() somewhere is much easier to
 digest than seeing some wrapper, which I have to look up. But please do
 wait for others to voice their concern too, I might not be the best person
 to tell you what to do when it comes to wrapping IO accessors ;-)

I could go either way, but there are times where local wrapper I/O
accessors are useful. Case in point: it makes it really easy to make the
choice between readl() and readl_relaxed() in one place (i.e., the
discussion you had in another branch of this thread). That's been useful
for me on brcmnand, where certain platforms (big-endian MIPS) have
different assumptions about endianness than your average platform. Also,
it helps with things like what Robert Jarzmik is trying to do on
pxa3xx_nand -- add debug info to print every register read/write.

Also as Cyrille mentioned, personal taste is a factor.

Anyway, I'll go with whatever makes sense between y'all. I don't mind.

Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Jonas Gorski 
On Mon, Aug 24, 2015 at 7:45 PM, Marek Vasut ma...@denx.de wrote:
 On Monday, August 24, 2015 at 07:04:38 PM, Cyrille Pitchen wrote:
 Hi Marek,

 Hi!

 Le 24/08/2015 13:03, Marek Vasut a écrit :
  On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
  This driver add support to the new Atmel QSPI controller embedded into
  sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
  controller.

 [...]

  +  /* Compute address parameters */
  +  switch (cmd-enable.bits.address) {
  +  case 4:
  +  ifr |= QSPI_IFR_ADDRL;
  +  /*break;*/ /* fallback to the 24bit address case */
 
  What's this commented out bit of code for ? :-)

 I just wanted to stress out there was no missing break;.
 I've reworded the comment to:
 /* No break on purpose: fallback to the 24bit address case. */

 Oh, the address is in bytes . I see, yes, it makes sense to be more
 explicit here about the purpose of the fallback. I think this change
 in the comment will make it easier for everyone who comes back in a
 few years and reads this code.

I think you are looking for the term (switch case) fallthrough, not
fallback. Fallback makes it sound like there is something missing,
or an invalid state.


Jonas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Cyrille Pitchen 
Hi Marek,

Le 24/08/2015 13:03, Marek Vasut a écrit :
 On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
 This driver add support to the new Atmel QSPI controller embedded into
 sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
 controller.

 Signed-off-by: Cyrille Pitchen cyrille.pitc...@atmel.com
 Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
 
 Hi,
 
 [...]
 
 +/* Register access macros */
 
 These are functions, not macros :)
 
 btw is there any reason for these ? I'd say, just put the read*() and
 write*() functions directly into the code and be done with it, it is
 much less confusing.

If you don't mind, I'd rather keep some of these inline functions. I have
no strong justification, it's more a personal taste: it makes lines
shorter as it avoids the need to add -regs + .
Also it makes the code consistent with other Atmel drivers which already
use such wrappers.

However I'll fix the comment and remove the byte and word versions, which
are not used. So only qspi_readl() and qspi_writel() are left.

Does it sound good to you?

 
 Also, why do you use the _relaxed() versions of the functions ?
 
 +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
 +{
 +return readl_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
 +{
 +writel_relaxed(value, aq-regs + reg);
 +}
 +
 +static inline u16 qspi_readw(struct atmel_qspi *aq, u32 reg)
 +{
 +return readw_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writew(struct atmel_qspi *aq, u32 reg, u16 value)
 +{
 +writew_relaxed(value, aq-regs + reg);
 +}
 +
 +static inline u8 qspi_readb(struct atmel_qspi *aq, u32 reg)
 +{
 +return readb_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writeb(struct atmel_qspi *aq, u32 reg, u8 value)
 +{
 +writeb_relaxed(value, aq-regs + reg);
 +}
 
 [...]
 
 Hope this helps :)
 

Best regards,

Cyrille
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Marek Vasut 
On Tuesday, August 25, 2015 at 12:17:37 PM, Cyrille Pitchen wrote:
 Hi Marek,

Hi!

 Le 24/08/2015 13:03, Marek Vasut a écrit :
  On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
  This driver add support to the new Atmel QSPI controller embedded into
  sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
  controller.
  
  Signed-off-by: Cyrille Pitchen cyrille.pitc...@atmel.com
  Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
  
  Hi,
  
  [...]
  
  +/* Register access macros */
  
  These are functions, not macros :)
  
  btw is there any reason for these ? I'd say, just put the read*() and
  write*() functions directly into the code and be done with it, it is
  much less confusing.
 
 If you don't mind, I'd rather keep some of these inline functions. I have
 no strong justification, it's more a personal taste: it makes lines
 shorter as it avoids the need to add -regs + .
 Also it makes the code consistent with other Atmel drivers which already
 use such wrappers.
 
 However I'll fix the comment and remove the byte and word versions, which
 are not used. So only qspi_readl() and qspi_writel() are left.
 
 Does it sound good to you?

In my mind, seeing explicit readl_relaxed() somewhere is much easier to
digest than seeing some wrapper, which I have to look up. But please do
wait for others to voice their concern too, I might not be the best person
to tell you what to do when it comes to wrapping IO accessors ;-)

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Cyrille Pitchen 
Le 25/08/2015 11:46, Jonas Gorski a écrit :
 On Mon, Aug 24, 2015 at 7:45 PM, Marek Vasut ma...@denx.de wrote:
 On Monday, August 24, 2015 at 07:04:38 PM, Cyrille Pitchen wrote:
 Hi Marek,

 Hi!

 Le 24/08/2015 13:03, Marek Vasut a écrit :
 On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
 This driver add support to the new Atmel QSPI controller embedded into
 sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
 controller.

 [...]

 +  /* Compute address parameters */
 +  switch (cmd-enable.bits.address) {
 +  case 4:
 +  ifr |= QSPI_IFR_ADDRL;
 +  /*break;*/ /* fallback to the 24bit address case */

 What's this commented out bit of code for ? :-)

 I just wanted to stress out there was no missing break;.
 I've reworded the comment to:
 /* No break on purpose: fallback to the 24bit address case. */

 Oh, the address is in bytes . I see, yes, it makes sense to be more
 explicit here about the purpose of the fallback. I think this change
 in the comment will make it easier for everyone who comes back in a
 few years and reads this code.
 
 I think you are looking for the term (switch case) fallthrough, not
 fallback. Fallback makes it sound like there is something missing,
 or an invalid state.
 
 
 Jonas
 

will be modified in the next series, thanks for the review!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-25 Thread Cyrille Pitchen 
Hi!

Le 25/08/2015 03:44, Bean Huo 霍斌斌 (beanhuo) a écrit :
 
 +nor-read_reg = atmel_qspi_read_reg;
 +nor-write_reg = atmel_qspi_write_reg;
 +nor-read = atmel_qspi_read;
 +nor-write = atmel_qspi_write;
 +nor-erase = atmel_qspi_erase;
 +nor-set_protocol = atmel_qspi_set_protocol;
 
 This is very good, the structure of spi_nor should add a hook function to 
 notify spi controller 
 That spi nor transfer protocol already changed.
 
 +
 +if (of_modalias_node(child, modalias, sizeof(modalias))  0) {
 +err = -ENODEV;
 +goto release_channel;
 +}
 +
 +err = of_property_read_u32(child, spi-max-frequency, aq-clk_rate);
 +if (err  0)
 +goto release_channel;
 +
 +err = atmel_qspi_init(aq);
 +if (err)
 +goto release_channel;
 +
 +nor-dev-of_node = child;
 +err = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
  goto release_channel;
 +
 
 
 ...
 
 static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)  {
   return mtd-priv;
 @@ -944,6 +960,11 @@ static int micron_quad_enable(struct spi_nor *nor)
  return ret;
  }

 +/* switch protocol to Quad CMD 4-4-4 */
 +ret = spi_nor_set_protocol(nor, SPI_PROTO_4_4_4);
 +if (ret)
 +return ret;
 +
 
 This make sense,from spi nor side,once its protocol being changed,
 Mtd layer must notify this status to spi nor controller immediately,
 And spi nor controller also should re-adjust its protocol.
 Otherwise, following reading SR operation will fail. 
 

  ret = spi_nor_wait_till_ready(nor);
  if (ret)
  return ret;
 
 If my ack has any value in here, feel free to add it.
 
 Acked-by: Bean Huo bean...@micron.com
 

Since your comments deal with the protocol change, I'll add your ack to the
first patch of the series:
mtd: spi-nor: notify (Q)SPI controller about protocol change

Thanks for your review!

Best regards,

Cyrille

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread beanhuo 

>+  nor->read_reg = atmel_qspi_read_reg;
>+  nor->write_reg = atmel_qspi_write_reg;
>+  nor->read = atmel_qspi_read;
>+  nor->write = atmel_qspi_write;
>+  nor->erase = atmel_qspi_erase;
>+  nor->set_protocol = atmel_qspi_set_protocol;

This is very good, the structure of spi_nor should add a hook function to 
notify spi controller 
That spi nor transfer protocol already changed.

>+
>+  if (of_modalias_node(child, modalias, sizeof(modalias)) < 0) {
>+  err = -ENODEV;
>+  goto release_channel;
>+  }
>+
>+  err = of_property_read_u32(child, "spi-max-frequency", >clk_rate);
>+  if (err < 0)
>+  goto release_channel;
>+
>+  err = atmel_qspi_init(aq);
>+  if (err)
>+  goto release_channel;
>+
>+  nor->dev->of_node = child;
>+  err = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
>   goto release_channel;
>+


...

>static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)  {
return mtd->priv;
>@@ -944,6 +960,11 @@ static int micron_quad_enable(struct spi_nor *nor)
>   return ret;
>   }
> 
>+  /* switch protocol to Quad CMD 4-4-4 */
>+  ret = spi_nor_set_protocol(nor, SPI_PROTO_4_4_4);
>+  if (ret)
>+  return ret;
>+

This make sense,from spi nor side,once its protocol being changed,
Mtd layer must notify this status to spi nor controller immediately,
And spi nor controller also should re-adjust its protocol.
Otherwise, following reading SR operation will fail. 

>
>   ret = spi_nor_wait_till_ready(nor);
>   if (ret)
>   return ret;

If my ack has any value in here, feel free to add it.

Acked-by: Bean Huo 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Marek Vasut 
On Monday, August 24, 2015 at 07:04:38 PM, Cyrille Pitchen wrote:
> Hi Marek,

Hi!

> Le 24/08/2015 13:03, Marek Vasut a écrit :
> > On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
> >> This driver add support to the new Atmel QSPI controller embedded into
> >> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
> >> controller.

[...]

> >> +  /* Compute address parameters */
> >> +  switch (cmd->enable.bits.address) {
> >> +  case 4:
> >> +  ifr |= QSPI_IFR_ADDRL;
> >> +  /*break;*/ /* fallback to the 24bit address case */
> > 
> > What's this commented out bit of code for ? :-)
> 
> I just wanted to stress out there was no missing "break;".
> I've reworded the comment to:
> /* No "break" on purpose: fallback to the 24bit address case. */

Oh, the address is in bytes . I see, yes, it makes sense to be more
explicit here about the purpose of the fallback. I think this change
in the comment will make it easier for everyone who comes back in a
few years and reads this code.

> >> +  case 3:
> >> +  iar = (cmd->enable.bits.data) ? 0 : cmd->address;
> >> +  ifr |= QSPI_IFR_ADDREN;
> >> +  break;
> >> +  case 0:
> >> +  break;
> >> +  default:
> >> +  return -EINVAL;
> >> +  }
> > 
> > [...]
> > 
> >> +no_data:
> >> +  /* Poll INSTRuction End status */
> >> +  sr = qspi_readl(aq, QSPI_SR);
> >> +  if (sr & QSPI_SR_INSTRE)
> >> +  return err;
> >> +
> >> +  /* Wait for INSTRuction End interrupt */
> >> +  init_completion(>completion);
> > 
> > You should use reinit_completion() in the code. init_completion()
> > should be used only in the probe() function and nowhere else.
> 
> Alright. In the next version I'll rename the "completion" member of
> struct atmel_qspi into "cmd_completion". Also I'll add another
> dma_completion member in this very same structure to replace the local
> "struct completion completion" in atmel_qspi_run_dma_transfer().
> 
> Then I'll call init_completion() on both cmd_completion and dma_completion
> only from atmel_qspi_probe() and reinit_completion() elsewhere.
> 
> >> +  aq->pending = 0;
> >> +  qspi_writel(aq, QSPI_IER, QSPI_SR_INSTRE);
> >> +  if (!wait_for_completion_timeout(>completion,
> >> +   msecs_to_jiffies(1000)))
> >> +  err = -ETIMEDOUT;
> >> +  qspi_writel(aq, QSPI_IDR, QSPI_SR_INSTRE);
> >> +
> >> +  return err;
> >> +}
> > 
> > [...]
> > 
> > Hope this helps :)
> 
> Indeed, it does! I still work on the next version of this series to take
> all your comments into account.

Thanks :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Cyrille Pitchen 
Hi Marek,

Le 24/08/2015 13:03, Marek Vasut a écrit :
> On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
>> This driver add support to the new Atmel QSPI controller embedded into
>> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
>> controller.
>>
>> Signed-off-by: Cyrille Pitchen 
>> Acked-by: Nicolas Ferre 
> 
> Hi,
> 
> [...]
> 
>> +/* Register access macros */
> 
> These are functions, not macros :)
> 
> btw is there any reason for these ? I'd say, just put the read*() and
> write*() functions directly into the code and be done with it, it is
> much less confusing.
> 
> Also, why do you use the _relaxed() versions of the functions ?
> 
>> +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
>> +{
>> +return readl_relaxed(aq->regs + reg);
>> +}
>> +
>> +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
>> +{
>> +writel_relaxed(value, aq->regs + reg);
>> +}
>> +
>> +static inline u16 qspi_readw(struct atmel_qspi *aq, u32 reg)
>> +{
>> +return readw_relaxed(aq->regs + reg);
>> +}
>> +
>> +static inline void qspi_writew(struct atmel_qspi *aq, u32 reg, u16 value)
>> +{
>> +writew_relaxed(value, aq->regs + reg);
>> +}
>> +
>> +static inline u8 qspi_readb(struct atmel_qspi *aq, u32 reg)
>> +{
>> +return readb_relaxed(aq->regs + reg);
>> +}
>> +
>> +static inline void qspi_writeb(struct atmel_qspi *aq, u32 reg, u8 value)
>> +{
>> +writeb_relaxed(value, aq->regs + reg);
>> +}
> 
> [...]
> 
>> +static int atmel_qspi_run_command(struct atmel_qspi *aq,
>> +  const struct atmel_qspi_command *cmd)
>> +{
>> +u32 iar, icr, ifr, sr;
>> +int err = 0;
>> +
>> +iar = 0;
>> +icr = 0;
>> +ifr = aq->ifr_width | cmd->ifr_tfrtyp;
>> +
>> +/* Compute instruction parameters */
>> +if (cmd->enable.bits.instruction) {
>> +icr |= QSPI_ICR_INST(cmd->instruction);
>> +ifr |= QSPI_IFR_INSTEN;
>> +}
>> +
>> +/* Compute address parameters */
>> +switch (cmd->enable.bits.address) {
>> +case 4:
>> +ifr |= QSPI_IFR_ADDRL;
>> +/*break;*/ /* fallback to the 24bit address case */
> 
> What's this commented out bit of code for ? :-)

I just wanted to stress out there was no missing "break;".
I've reworded the comment to:
/* No "break" on purpose: fallback to the 24bit address case. */

> 
>> +case 3:
>> +iar = (cmd->enable.bits.data) ? 0 : cmd->address;
>> +ifr |= QSPI_IFR_ADDREN;
>> +break;
>> +case 0:
>> +break;
>> +default:
>> +return -EINVAL;
>> +}
> 
> [...]
> 
>> +no_data:
>> +/* Poll INSTRuction End status */
>> +sr = qspi_readl(aq, QSPI_SR);
>> +if (sr & QSPI_SR_INSTRE)
>> +return err;
>> +
>> +/* Wait for INSTRuction End interrupt */
>> +init_completion(>completion);
> 
> You should use reinit_completion() in the code. init_completion()
> should be used only in the probe() function and nowhere else.

Alright. In the next version I'll rename the "completion" member of
struct atmel_qspi into "cmd_completion". Also I'll add another dma_completion
member in this very same structure to replace the local
"struct completion completion" in atmel_qspi_run_dma_transfer().

Then I'll call init_completion() on both cmd_completion and dma_completion only
from atmel_qspi_probe() and reinit_completion() elsewhere.

> 
>> +aq->pending = 0;
>> +qspi_writel(aq, QSPI_IER, QSPI_SR_INSTRE);
>> +if (!wait_for_completion_timeout(>completion,
>> + msecs_to_jiffies(1000)))
>> +err = -ETIMEDOUT;
>> +qspi_writel(aq, QSPI_IDR, QSPI_SR_INSTRE);
>> +
>> +return err;
>> +}
> 
> [...]
> 
> Hope this helps :)
> 

Indeed, it does! I still work on the next version of this series to take all 
your
comments into account.

Best regards,

Cyrille
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Marek Vasut 
On Monday, August 24, 2015 at 02:49:24 PM, Russell King - ARM Linux wrote:

Hi Russell,

> On Mon, Aug 24, 2015 at 01:03:51PM +0200, Marek Vasut wrote:
> > These are functions, not macros :)
> > 
> > btw is there any reason for these ? I'd say, just put the read*() and
> > write*() functions directly into the code and be done with it, it is
> > much less confusing.
> > 
> > Also, why do you use the _relaxed() versions of the functions ?
> 
> Now that the _relaxed() accessors are available throughout the kernel,
> everyone should be using the _relaxed() versions unless they need the
> properties of the non-relaxed versions.

You mean the memory barrier, right ?

> Remember that the non-relaxed
> versions are rather expensive on ARM due to the need to go all the way
> out to the L2 cache - it at least doubles the number of accesses for
> every read*/write*().

I think in case of this driver, we don't need the non-relaxed version
anywhere, right ? Thanks for the educational writeup :)

btw. is [1] still the current study material on the I/O accessor best
practices please ?

[1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/117644

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Russell King - ARM Linux 
On Mon, Aug 24, 2015 at 01:03:51PM +0200, Marek Vasut wrote:
> These are functions, not macros :)
> 
> btw is there any reason for these ? I'd say, just put the read*() and
> write*() functions directly into the code and be done with it, it is
> much less confusing.
> 
> Also, why do you use the _relaxed() versions of the functions ?

Now that the _relaxed() accessors are available throughout the kernel,
everyone should be using the _relaxed() versions unless they need the
properties of the non-relaxed versions.  Remember that the non-relaxed
versions are rather expensive on ARM due to the need to go all the way
out to the L2 cache - it at least doubles the number of accesses for
every read*/write*().

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Marek Vasut 
On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
> This driver add support to the new Atmel QSPI controller embedded into
> sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
> controller.
> 
> Signed-off-by: Cyrille Pitchen 
> Acked-by: Nicolas Ferre 

Hi,

[...]

> +/* Register access macros */

These are functions, not macros :)

btw is there any reason for these ? I'd say, just put the read*() and
write*() functions directly into the code and be done with it, it is
much less confusing.

Also, why do you use the _relaxed() versions of the functions ?

> +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
> +{
> + return readl_relaxed(aq->regs + reg);
> +}
> +
> +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
> +{
> + writel_relaxed(value, aq->regs + reg);
> +}
> +
> +static inline u16 qspi_readw(struct atmel_qspi *aq, u32 reg)
> +{
> + return readw_relaxed(aq->regs + reg);
> +}
> +
> +static inline void qspi_writew(struct atmel_qspi *aq, u32 reg, u16 value)
> +{
> + writew_relaxed(value, aq->regs + reg);
> +}
> +
> +static inline u8 qspi_readb(struct atmel_qspi *aq, u32 reg)
> +{
> + return readb_relaxed(aq->regs + reg);
> +}
> +
> +static inline void qspi_writeb(struct atmel_qspi *aq, u32 reg, u8 value)
> +{
> + writeb_relaxed(value, aq->regs + reg);
> +}

[...]

> +static int atmel_qspi_run_command(struct atmel_qspi *aq,
> +   const struct atmel_qspi_command *cmd)
> +{
> + u32 iar, icr, ifr, sr;
> + int err = 0;
> +
> + iar = 0;
> + icr = 0;
> + ifr = aq->ifr_width | cmd->ifr_tfrtyp;
> +
> + /* Compute instruction parameters */
> + if (cmd->enable.bits.instruction) {
> + icr |= QSPI_ICR_INST(cmd->instruction);
> + ifr |= QSPI_IFR_INSTEN;
> + }
> +
> + /* Compute address parameters */
> + switch (cmd->enable.bits.address) {
> + case 4:
> + ifr |= QSPI_IFR_ADDRL;
> + /*break;*/ /* fallback to the 24bit address case */

What's this commented out bit of code for ? :-)

> + case 3:
> + iar = (cmd->enable.bits.data) ? 0 : cmd->address;
> + ifr |= QSPI_IFR_ADDREN;
> + break;
> + case 0:
> + break;
> + default:
> + return -EINVAL;
> + }

[...]

> +no_data:
> + /* Poll INSTRuction End status */
> + sr = qspi_readl(aq, QSPI_SR);
> + if (sr & QSPI_SR_INSTRE)
> + return err;
> +
> + /* Wait for INSTRuction End interrupt */
> + init_completion(>completion);

You should use reinit_completion() in the code. init_completion()
should be used only in the probe() function and nowhere else.

> + aq->pending = 0;
> + qspi_writel(aq, QSPI_IER, QSPI_SR_INSTRE);
> + if (!wait_for_completion_timeout(>completion,
> +  msecs_to_jiffies(1000)))
> + err = -ETIMEDOUT;
> + qspi_writel(aq, QSPI_IDR, QSPI_SR_INSTRE);
> +
> + return err;
> +}

[...]

Hope this helps :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Cyrille Pitchen 
This driver add support to the new Atmel QSPI controller embedded into
sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
controller.

Signed-off-by: Cyrille Pitchen 
Acked-by: Nicolas Ferre 
---
 drivers/mtd/spi-nor/Kconfig |   7 +
 drivers/mtd/spi-nor/Makefile|   1 +
 drivers/mtd/spi-nor/atmel-quadspi.c | 876 
 3 files changed, 884 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/atmel-quadspi.c

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 89bf4c1faa2b..7a3d55429550 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -29,6 +29,13 @@ config SPI_FSL_QUADSPI
  This controller does not support generic SPI. It only supports
  SPI NOR.
 
+config SPI_ATMEL_QUADSPI
+   tristate "Atmel Quad SPI Controller"
+   depends on OF && HAS_DMA && (ARCH_AT91 || COMPILE_TEST)
+   help
+ This enables support for the Quad SPI controller in master mode.
+ We only connect the NOR to this controller now.
+
 config SPI_NXP_SPIFI
tristate "NXP SPI Flash Interface (SPIFI)"
depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index e5ef8582..f5d23d7379bb 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_MTD_SPI_NOR)  += spi-nor.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)  += fsl-quadspi.o
+obj-$(CONFIG_SPI_ATMEL_QUADSPI)+= atmel-quadspi.o
 obj-$(CONFIG_SPI_NXP_SPIFI)+= nxp-spifi.o
diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c 
b/drivers/mtd/spi-nor/atmel-quadspi.c
new file mode 100644
index ..ada6f95782e4
--- /dev/null
+++ b/drivers/mtd/spi-nor/atmel-quadspi.c
@@ -0,0 +1,876 @@
+/*
+ * Driver for Atmel QSPI Controller
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *
+ * Author: Cyrille Pitchen 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ *
+ * This driver is based on drivers/mtd/spi-nor/fsl-quadspi.c from Freescale.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/* QSPI register offsets */
+#define QSPI_CR  0x  /* Control Register */
+#define QSPI_MR  0x0004  /* Mode Register */
+#define QSPI_RD  0x0008  /* Receive Data Register */
+#define QSPI_TD  0x000c  /* Transmit Data Register */
+#define QSPI_SR  0x0010  /* Status Register */
+#define QSPI_IER 0x0014  /* Interrupt Enable Register */
+#define QSPI_IDR 0x0018  /* Interrupt Disable Register */
+#define QSPI_IMR 0x001c  /* Interrupt Mask Register */
+#define QSPI_SCR 0x0020  /* Serial Clock Register */
+
+#define QSPI_IAR 0x0030  /* Instruction Address Register */
+#define QSPI_ICR 0x0034  /* Instruction Code Register */
+#define QSPI_IFR 0x0038  /* Instruction Frame Register */
+
+#define QSPI_SMR 0x0040  /* Scrambling Mode Register */
+#define QSPI_SKR 0x0044  /* Scrambling Key Register */
+
+#define QSPI_WPMR0x00E4  /* Write Protection Mode Register */
+#define QSPI_WPSR0x00E8  /* Write Protection Status Register */
+
+#define QSPI_VERSION 0x00FC  /* Version Register */
+
+
+/* Bitfields in QSPI_CR (Control Register) */
+#define QSPI_CR_QSPIEN  BIT(0)
+#define QSPI_CR_QSPIDIS BIT(1)
+#define QSPI_CR_SWRST   BIT(7)
+#define QSPI_CR_LASTXFERBIT(24)
+
+/* Bitfields in QSPI_MR (Mode Register) */
+#define QSPI_MR_SSM BIT(0)
+#define QSPI_MR_LLB BIT(1)
+#define QSPI_MR_WDRBT   BIT(2)
+#define QSPI_MR_SMRMBIT(3)
+#define QSPI_MR_CSMODE_MASK GENMASK(5, 4)
+#define QSPI_MR_CSMODE_NOT_RELOADED (0 << 4)
+#define QSPI_MR_CSMODE_LASTXFER (1 << 4)
+#define QSPI_MR_CSMODE_SYSTEMATICALLY   (2 << 4)
+#define QSPI_MR_NBBITS_MASK GENMASK(11, 8)
+#define QSPI_MR_NBBITS(n)   n) - 8) << 8) & 
QSPI_MR_NBBITS_MASK)
+#define QSPI_MR_DLYBCT_MASK GENMASK(23, 16)
+#define QSPI_MR_DLYBCT(n)   (((n) << 16) & QSPI_MR_DLYBCT_MASK)
+#define QSPI_MR_DLYCS_MASK  GENMASK(31, 24)
+#define QSPI_MR_DLYCS(n)(((n) << 24) & QSPI_MR_DLYCS_MASK)
+
+/* Bitfields in

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Russell King - ARM Linux 
On Mon, Aug 24, 2015 at 01:03:51PM +0200, Marek Vasut wrote:
 These are functions, not macros :)
 
 btw is there any reason for these ? I'd say, just put the read*() and
 write*() functions directly into the code and be done with it, it is
 much less confusing.
 
 Also, why do you use the _relaxed() versions of the functions ?

Now that the _relaxed() accessors are available throughout the kernel,
everyone should be using the _relaxed() versions unless they need the
properties of the non-relaxed versions.  Remember that the non-relaxed
versions are rather expensive on ARM due to the need to go all the way
out to the L2 cache - it at least doubles the number of accesses for
every read*/write*().

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Marek Vasut 
On Monday, August 24, 2015 at 02:49:24 PM, Russell King - ARM Linux wrote:

Hi Russell,

 On Mon, Aug 24, 2015 at 01:03:51PM +0200, Marek Vasut wrote:
  These are functions, not macros :)
  
  btw is there any reason for these ? I'd say, just put the read*() and
  write*() functions directly into the code and be done with it, it is
  much less confusing.
  
  Also, why do you use the _relaxed() versions of the functions ?
 
 Now that the _relaxed() accessors are available throughout the kernel,
 everyone should be using the _relaxed() versions unless they need the
 properties of the non-relaxed versions.

You mean the memory barrier, right ?

 Remember that the non-relaxed
 versions are rather expensive on ARM due to the need to go all the way
 out to the L2 cache - it at least doubles the number of accesses for
 every read*/write*().

I think in case of this driver, we don't need the non-relaxed version
anywhere, right ? Thanks for the educational writeup :)

btw. is [1] still the current study material on the I/O accessor best
practices please ?

[1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/117644

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Cyrille Pitchen 
Hi Marek,

Le 24/08/2015 13:03, Marek Vasut a écrit :
 On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
 This driver add support to the new Atmel QSPI controller embedded into
 sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
 controller.

 Signed-off-by: Cyrille Pitchen cyrille.pitc...@atmel.com
 Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
 
 Hi,
 
 [...]
 
 +/* Register access macros */
 
 These are functions, not macros :)
 
 btw is there any reason for these ? I'd say, just put the read*() and
 write*() functions directly into the code and be done with it, it is
 much less confusing.
 
 Also, why do you use the _relaxed() versions of the functions ?
 
 +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
 +{
 +return readl_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
 +{
 +writel_relaxed(value, aq-regs + reg);
 +}
 +
 +static inline u16 qspi_readw(struct atmel_qspi *aq, u32 reg)
 +{
 +return readw_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writew(struct atmel_qspi *aq, u32 reg, u16 value)
 +{
 +writew_relaxed(value, aq-regs + reg);
 +}
 +
 +static inline u8 qspi_readb(struct atmel_qspi *aq, u32 reg)
 +{
 +return readb_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writeb(struct atmel_qspi *aq, u32 reg, u8 value)
 +{
 +writeb_relaxed(value, aq-regs + reg);
 +}
 
 [...]
 
 +static int atmel_qspi_run_command(struct atmel_qspi *aq,
 +  const struct atmel_qspi_command *cmd)
 +{
 +u32 iar, icr, ifr, sr;
 +int err = 0;
 +
 +iar = 0;
 +icr = 0;
 +ifr = aq-ifr_width | cmd-ifr_tfrtyp;
 +
 +/* Compute instruction parameters */
 +if (cmd-enable.bits.instruction) {
 +icr |= QSPI_ICR_INST(cmd-instruction);
 +ifr |= QSPI_IFR_INSTEN;
 +}
 +
 +/* Compute address parameters */
 +switch (cmd-enable.bits.address) {
 +case 4:
 +ifr |= QSPI_IFR_ADDRL;
 +/*break;*/ /* fallback to the 24bit address case */
 
 What's this commented out bit of code for ? :-)

I just wanted to stress out there was no missing break;.
I've reworded the comment to:
/* No break on purpose: fallback to the 24bit address case. */

 
 +case 3:
 +iar = (cmd-enable.bits.data) ? 0 : cmd-address;
 +ifr |= QSPI_IFR_ADDREN;
 +break;
 +case 0:
 +break;
 +default:
 +return -EINVAL;
 +}
 
 [...]
 
 +no_data:
 +/* Poll INSTRuction End status */
 +sr = qspi_readl(aq, QSPI_SR);
 +if (sr  QSPI_SR_INSTRE)
 +return err;
 +
 +/* Wait for INSTRuction End interrupt */
 +init_completion(aq-completion);
 
 You should use reinit_completion() in the code. init_completion()
 should be used only in the probe() function and nowhere else.

Alright. In the next version I'll rename the completion member of
struct atmel_qspi into cmd_completion. Also I'll add another dma_completion
member in this very same structure to replace the local
struct completion completion in atmel_qspi_run_dma_transfer().

Then I'll call init_completion() on both cmd_completion and dma_completion only
from atmel_qspi_probe() and reinit_completion() elsewhere.

 
 +aq-pending = 0;
 +qspi_writel(aq, QSPI_IER, QSPI_SR_INSTRE);
 +if (!wait_for_completion_timeout(aq-completion,
 + msecs_to_jiffies(1000)))
 +err = -ETIMEDOUT;
 +qspi_writel(aq, QSPI_IDR, QSPI_SR_INSTRE);
 +
 +return err;
 +}
 
 [...]
 
 Hope this helps :)
 

Indeed, it does! I still work on the next version of this series to take all 
your
comments into account.

Best regards,

Cyrille
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Marek Vasut 
On Monday, August 24, 2015 at 07:04:38 PM, Cyrille Pitchen wrote:
 Hi Marek,

Hi!

 Le 24/08/2015 13:03, Marek Vasut a écrit :
  On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
  This driver add support to the new Atmel QSPI controller embedded into
  sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
  controller.

[...]

  +  /* Compute address parameters */
  +  switch (cmd-enable.bits.address) {
  +  case 4:
  +  ifr |= QSPI_IFR_ADDRL;
  +  /*break;*/ /* fallback to the 24bit address case */
  
  What's this commented out bit of code for ? :-)
 
 I just wanted to stress out there was no missing break;.
 I've reworded the comment to:
 /* No break on purpose: fallback to the 24bit address case. */

Oh, the address is in bytes . I see, yes, it makes sense to be more
explicit here about the purpose of the fallback. I think this change
in the comment will make it easier for everyone who comes back in a
few years and reads this code.

  +  case 3:
  +  iar = (cmd-enable.bits.data) ? 0 : cmd-address;
  +  ifr |= QSPI_IFR_ADDREN;
  +  break;
  +  case 0:
  +  break;
  +  default:
  +  return -EINVAL;
  +  }
  
  [...]
  
  +no_data:
  +  /* Poll INSTRuction End status */
  +  sr = qspi_readl(aq, QSPI_SR);
  +  if (sr  QSPI_SR_INSTRE)
  +  return err;
  +
  +  /* Wait for INSTRuction End interrupt */
  +  init_completion(aq-completion);
  
  You should use reinit_completion() in the code. init_completion()
  should be used only in the probe() function and nowhere else.
 
 Alright. In the next version I'll rename the completion member of
 struct atmel_qspi into cmd_completion. Also I'll add another
 dma_completion member in this very same structure to replace the local
 struct completion completion in atmel_qspi_run_dma_transfer().
 
 Then I'll call init_completion() on both cmd_completion and dma_completion
 only from atmel_qspi_probe() and reinit_completion() elsewhere.
 
  +  aq-pending = 0;
  +  qspi_writel(aq, QSPI_IER, QSPI_SR_INSTRE);
  +  if (!wait_for_completion_timeout(aq-completion,
  +   msecs_to_jiffies(1000)))
  +  err = -ETIMEDOUT;
  +  qspi_writel(aq, QSPI_IDR, QSPI_SR_INSTRE);
  +
  +  return err;
  +}
  
  [...]
  
  Hope this helps :)
 
 Indeed, it does! I still work on the next version of this series to take
 all your comments into account.

Thanks :)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Cyrille Pitchen 
This driver add support to the new Atmel QSPI controller embedded into
sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
controller.

Signed-off-by: Cyrille Pitchen cyrille.pitc...@atmel.com
Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
---
 drivers/mtd/spi-nor/Kconfig |   7 +
 drivers/mtd/spi-nor/Makefile|   1 +
 drivers/mtd/spi-nor/atmel-quadspi.c | 876 
 3 files changed, 884 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/atmel-quadspi.c

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 89bf4c1faa2b..7a3d55429550 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -29,6 +29,13 @@ config SPI_FSL_QUADSPI
  This controller does not support generic SPI. It only supports
  SPI NOR.
 
+config SPI_ATMEL_QUADSPI
+   tristate Atmel Quad SPI Controller
+   depends on OF  HAS_DMA  (ARCH_AT91 || COMPILE_TEST)
+   help
+ This enables support for the Quad SPI controller in master mode.
+ We only connect the NOR to this controller now.
+
 config SPI_NXP_SPIFI
tristate NXP SPI Flash Interface (SPIFI)
depends on OF  (ARCH_LPC18XX || COMPILE_TEST)
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index e5ef8582..f5d23d7379bb 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_MTD_SPI_NOR)  += spi-nor.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)  += fsl-quadspi.o
+obj-$(CONFIG_SPI_ATMEL_QUADSPI)+= atmel-quadspi.o
 obj-$(CONFIG_SPI_NXP_SPIFI)+= nxp-spifi.o
diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c 
b/drivers/mtd/spi-nor/atmel-quadspi.c
new file mode 100644
index ..ada6f95782e4
--- /dev/null
+++ b/drivers/mtd/spi-nor/atmel-quadspi.c
@@ -0,0 +1,876 @@
+/*
+ * Driver for Atmel QSPI Controller
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *
+ * Author: Cyrille Pitchen cyrille.pitc...@atmel.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see http://www.gnu.org/licenses/.
+ *
+ * This driver is based on drivers/mtd/spi-nor/fsl-quadspi.c from Freescale.
+ */
+
+#include linux/kernel.h
+#include linux/clk.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/delay.h
+#include linux/dma-mapping.h
+#include linux/dmaengine.h
+#include linux/err.h
+#include linux/interrupt.h
+#include linux/mtd/mtd.h
+#include linux/mtd/partitions.h
+#include linux/mtd/spi-nor.h
+#include linux/platform_data/atmel.h
+#include linux/platform_data/dma-atmel.h
+#include linux/of.h
+
+#include linux/io.h
+#include linux/gpio.h
+#include linux/pinctrl/consumer.h
+
+/* QSPI register offsets */
+#define QSPI_CR  0x  /* Control Register */
+#define QSPI_MR  0x0004  /* Mode Register */
+#define QSPI_RD  0x0008  /* Receive Data Register */
+#define QSPI_TD  0x000c  /* Transmit Data Register */
+#define QSPI_SR  0x0010  /* Status Register */
+#define QSPI_IER 0x0014  /* Interrupt Enable Register */
+#define QSPI_IDR 0x0018  /* Interrupt Disable Register */
+#define QSPI_IMR 0x001c  /* Interrupt Mask Register */
+#define QSPI_SCR 0x0020  /* Serial Clock Register */
+
+#define QSPI_IAR 0x0030  /* Instruction Address Register */
+#define QSPI_ICR 0x0034  /* Instruction Code Register */
+#define QSPI_IFR 0x0038  /* Instruction Frame Register */
+
+#define QSPI_SMR 0x0040  /* Scrambling Mode Register */
+#define QSPI_SKR 0x0044  /* Scrambling Key Register */
+
+#define QSPI_WPMR0x00E4  /* Write Protection Mode Register */
+#define QSPI_WPSR0x00E8  /* Write Protection Status Register */
+
+#define QSPI_VERSION 0x00FC  /* Version Register */
+
+
+/* Bitfields in QSPI_CR (Control Register) */
+#define QSPI_CR_QSPIEN  BIT(0)
+#define QSPI_CR_QSPIDIS BIT(1)
+#define QSPI_CR_SWRST   BIT(7)
+#define QSPI_CR_LASTXFERBIT(24)
+
+/* Bitfields in QSPI_MR (Mode Register) */
+#define QSPI_MR_SSM BIT(0)
+#define QSPI_MR_LLB BIT(1)
+#define QSPI_MR_WDRBT   BIT(2)
+#define QSPI_MR_SMRMBIT(3)
+#define QSPI_MR_CSMODE_MASK GENMASK(5, 4)
+#define QSPI_MR_CSMODE_NOT_RELOADED (0  4)
+#define QSPI_MR_CSMODE_LASTXFER (1  4)
+#define QSPI_MR_CSMODE_SYSTEMATICALLY   (2  4)
+#define QSPI_MR_NBBITS_MASK GENMASK(11, 8)
+#define

Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread Marek Vasut 
On Monday, August 24, 2015 at 12:14:00 PM, Cyrille Pitchen wrote:
 This driver add support to the new Atmel QSPI controller embedded into
 sama5d2x SoCs. It expects a NOR memory to be connected to the QSPI
 controller.
 
 Signed-off-by: Cyrille Pitchen cyrille.pitc...@atmel.com
 Acked-by: Nicolas Ferre nicolas.fe...@atmel.com

Hi,

[...]

 +/* Register access macros */

These are functions, not macros :)

btw is there any reason for these ? I'd say, just put the read*() and
write*() functions directly into the code and be done with it, it is
much less confusing.

Also, why do you use the _relaxed() versions of the functions ?

 +static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
 +{
 + return readl_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
 +{
 + writel_relaxed(value, aq-regs + reg);
 +}
 +
 +static inline u16 qspi_readw(struct atmel_qspi *aq, u32 reg)
 +{
 + return readw_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writew(struct atmel_qspi *aq, u32 reg, u16 value)
 +{
 + writew_relaxed(value, aq-regs + reg);
 +}
 +
 +static inline u8 qspi_readb(struct atmel_qspi *aq, u32 reg)
 +{
 + return readb_relaxed(aq-regs + reg);
 +}
 +
 +static inline void qspi_writeb(struct atmel_qspi *aq, u32 reg, u8 value)
 +{
 + writeb_relaxed(value, aq-regs + reg);
 +}

[...]

 +static int atmel_qspi_run_command(struct atmel_qspi *aq,
 +   const struct atmel_qspi_command *cmd)
 +{
 + u32 iar, icr, ifr, sr;
 + int err = 0;
 +
 + iar = 0;
 + icr = 0;
 + ifr = aq-ifr_width | cmd-ifr_tfrtyp;
 +
 + /* Compute instruction parameters */
 + if (cmd-enable.bits.instruction) {
 + icr |= QSPI_ICR_INST(cmd-instruction);
 + ifr |= QSPI_IFR_INSTEN;
 + }
 +
 + /* Compute address parameters */
 + switch (cmd-enable.bits.address) {
 + case 4:
 + ifr |= QSPI_IFR_ADDRL;
 + /*break;*/ /* fallback to the 24bit address case */

What's this commented out bit of code for ? :-)

 + case 3:
 + iar = (cmd-enable.bits.data) ? 0 : cmd-address;
 + ifr |= QSPI_IFR_ADDREN;
 + break;
 + case 0:
 + break;
 + default:
 + return -EINVAL;
 + }

[...]

 +no_data:
 + /* Poll INSTRuction End status */
 + sr = qspi_readl(aq, QSPI_SR);
 + if (sr  QSPI_SR_INSTRE)
 + return err;
 +
 + /* Wait for INSTRuction End interrupt */
 + init_completion(aq-completion);

You should use reinit_completion() in the code. init_completion()
should be used only in the probe() function and nowhere else.

 + aq-pending = 0;
 + qspi_writel(aq, QSPI_IER, QSPI_SR_INSTRE);
 + if (!wait_for_completion_timeout(aq-completion,
 +  msecs_to_jiffies(1000)))
 + err = -ETIMEDOUT;
 + qspi_writel(aq, QSPI_IDR, QSPI_SR_INSTRE);
 +
 + return err;
 +}

[...]

Hope this helps :)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller

2015-08-24 Thread beanhuo 

+  nor-read_reg = atmel_qspi_read_reg;
+  nor-write_reg = atmel_qspi_write_reg;
+  nor-read = atmel_qspi_read;
+  nor-write = atmel_qspi_write;
+  nor-erase = atmel_qspi_erase;
+  nor-set_protocol = atmel_qspi_set_protocol;

This is very good, the structure of spi_nor should add a hook function to 
notify spi controller 
That spi nor transfer protocol already changed.

+
+  if (of_modalias_node(child, modalias, sizeof(modalias))  0) {
+  err = -ENODEV;
+  goto release_channel;
+  }
+
+  err = of_property_read_u32(child, spi-max-frequency, aq-clk_rate);
+  if (err  0)
+  goto release_channel;
+
+  err = atmel_qspi_init(aq);
+  if (err)
+  goto release_channel;
+
+  nor-dev-of_node = child;
+  err = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
   goto release_channel;
+


...

static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)  {
return mtd-priv;
@@ -944,6 +960,11 @@ static int micron_quad_enable(struct spi_nor *nor)
   return ret;
   }
 
+  /* switch protocol to Quad CMD 4-4-4 */
+  ret = spi_nor_set_protocol(nor, SPI_PROTO_4_4_4);
+  if (ret)
+  return ret;
+

This make sense,from spi nor side,once its protocol being changed,
Mtd layer must notify this status to spi nor controller immediately,
And spi nor controller also should re-adjust its protocol.
Otherwise, following reading SR operation will fail. 


   ret = spi_nor_wait_till_ready(nor);
   if (ret)
   return ret;

If my ack has any value in here, feel free to add it.

Acked-by: Bean Huo bean...@micron.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/