Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller
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
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
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
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
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
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
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
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
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
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
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
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
>+ 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
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
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
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
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
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/
Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller
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
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
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
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/
Re: [PATCH linux-next v4 5/5] mtd: atmel-quadspi: add driver for Atmel QSPI controller
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
+ 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/
