Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1264107 > unrolled thread

[PATCH v6 0/3] Mediatek SPI-NOR flash driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 0/3] Mediatek SPI-NOR flash driver  Bayi Cheng <bayi.cheng@mediatek.com> - 2015-11-06 16:50 +0100
    Re: [PATCH v6 0/3] Mediatek SPI-NOR flash driver Brian Norris <computersforpeace@gmail.com> - 2015-11-10 03:50 +0100
      Re: [PATCH v6 0/3] Mediatek SPI-NOR flash driver bayi cheng <bayi.cheng@mediatek.com> - 2015-11-11 15:10 +0100
        Re: [PATCH v6 0/3] Mediatek SPI-NOR flash driver Brian Norris <computersforpeace@gmail.com> - 2015-11-11 21:30 +0100
          Re: [PATCH v6 0/3] Mediatek SPI-NOR flash driver bayi cheng <bayi.cheng@mediatek.com> - 2015-11-13 07:50 +0100

#1264107 — [PATCH v6 0/3] Mediatek SPI-NOR flash driver

This series is based on v4.3-rc1 and l2-mtd.git [0] and erase_sector
implementation patch [1]

[0]: git://git.infradead.org/l2-mtd.git
[1]: http://lists.infradead.org/pipermail/linux-mtd/2015-October//062959.html

Change in v6:
1: delete mt8173_nor_do_rx
2: delete mt8173_nor_do_rx
3: add mt8173_nor_do_tx_rx for general usage
4: support nor flash with 6 IDs
5: delete mt8173_nor_erase_sector and use "nor->erase_opcode"
6: add mt8173_nor_set_addr to programming the address register
7: initialize the ppdata in mtk_nor_init

Change in v5:
1: add "status = "disable"" to device tree
2: add document "flash" node
3: fix some statement error in Kconfig file
4: fix alphabetical order error in makefile
5: delete the parament "mtd_info *mtd" in mt8173_nor structure
6: delete SPINOR_OP_WREN repeated calls
7: add mt8173_nor_do_tx & mt8173_nor_do_rx for them full potential
8: use a subnode to represent the flash

Change in v4:
1: delete the parament "write_enable" for mt8173_nor_write_reg
2: fix the build warning for calling mt8173_nor_write_single_byte

Change in v3:
1: use switch() to replace some if-else statement
2: use shifts to replace endianness statement
3: delete some unused macros
4: use auto-increment mechanism for single write
5: write address added to 32bytes

Change in v2:
1. Rebase to 4.3-rc1
2. propagate error code
3. delete mux clock and axi clock in dts file
4. descripts more exactly for binding file
5. change file names from mtk-nor.c to mtk_quadspi.c
6. delete some functions witch were used once time

Bayi Cheng (3):
  doc: dt: add documentation for Mediatek spi-nor controller
  mtd: mtk-nor: mtk serial flash controller driver
  arm64: dts: mt8173: Add nor flash node

 .../devicetree/bindings/mtd/mtk-quadspi.txt        |  41 ++
 arch/arm64/boot/dts/mediatek/mt8173.dtsi           |  16 +
 drivers/mtd/spi-nor/Kconfig                        |   7 +
 drivers/mtd/spi-nor/Makefile                       |   1 +
 drivers/mtd/spi-nor/mtk-quadspi.c                  | 475 +++++++++++++++++++++
 5 files changed, 540 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
 create mode 100644 drivers/mtd/spi-nor/mtk-quadspi.c

--
1.8.1.1.dirty 

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

[toc] | [next] | [standalone]

#1266189

Hi Bayi,

On Fri, Nov 06, 2015 at 11:48:06PM +0800, Bayi Cheng wrote:
> This series is based on v4.3-rc1 and l2-mtd.git [0] and erase_sector
> implementation patch [1]
> 
> [0]: git://git.infradead.org/l2-mtd.git
> [1]: http://lists.infradead.org/pipermail/linux-mtd/2015-October//062959.html
> 
> Change in v6:
> 1: delete mt8173_nor_do_rx
> 2: delete mt8173_nor_do_rx
> 3: add mt8173_nor_do_tx_rx for general usage
> 4: support nor flash with 6 IDs
> 5: delete mt8173_nor_erase_sector and use "nor->erase_opcode"
> 6: add mt8173_nor_set_addr to programming the address register
> 7: initialize the ppdata in mtk_nor_init

This series is looking a lot better to me. Thanks for incorporating (and
I hope fully reviewing and testing!) my suggested changes. I have a just
a few small comments that I might post to the driver patch, and if
that's all that's outstanding, I can fix them up myself before applying.

I believe you didn't completely answer all my questions from v5 though.
I'll repeat a bit here. Particularly, refer to [1].
I'll summarize; I understand that your common transmit/receive operation
works something like this:

Quoting from [1]:
> (1) total number of bits to send/receive goes in the COUNT register (so
>     far, as many as 7*8=56?)
> (2) opcode is written to PRGDATA5
> (3) other "transmit" data (like addresses), if any, are placed on PRGDATA4..0
> (4) command is sent (execute_cmd())
> (5) data is read back in SHREG{X..0}, if needed

My questions were:

(a) Why does mt8173_nor_set_read_mode() use PRGDATA3? That's not
    mentioned in the SoC manual, and it doesn't really match any of the
    steps above. Perhaps it's just a quirk of the controller's
    programming model?

(b) How do you determine X from step (5)?

Right now, your code seems to answer that X is "rxlen - 1". Correct?

If that's correct and if I put all of my understanding together
correctly, this means that you can actually shift out (in PRGDATA) up to
6 bytes (that is, 1 opcode and 5 tx bytes) and shift in (in SHREG) up to
7 bytes, except that the first byte is received during the opcode cycle,
and so it is discarded, and we effectively receive only 6 bytes.

Is that all correct? If so, then I think you still need to adjust the
boundary conditions in your do_tx_rx() function. (I'll comment on the
driver to point out the specifics.)

Regards,
Brian

[1] http://lists.infradead.org/pipermail/linux-mtd/2015-October/062951.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267209

On Mon, 2015-11-09 at 18:46 -0800, Brian Norris wrote:
> Hi Bayi,
> 
> On Fri, Nov 06, 2015 at 11:48:06PM +0800, Bayi Cheng wrote:
> > This series is based on v4.3-rc1 and l2-mtd.git [0] and erase_sector
> > implementation patch [1]
> > 
> > [0]: git://git.infradead.org/l2-mtd.git
> > [1]: http://lists.infradead.org/pipermail/linux-mtd/2015-October//062959.html
> > 
> > Change in v6:
> > 1: delete mt8173_nor_do_rx
> > 2: delete mt8173_nor_do_rx
> > 3: add mt8173_nor_do_tx_rx for general usage
> > 4: support nor flash with 6 IDs
> > 5: delete mt8173_nor_erase_sector and use "nor->erase_opcode"
> > 6: add mt8173_nor_set_addr to programming the address register
> > 7: initialize the ppdata in mtk_nor_init
> 
> This series is looking a lot better to me. Thanks for incorporating (and
> I hope fully reviewing and testing!) my suggested changes. I have a just
> a few small comments that I might post to the driver patch, and if
> that's all that's outstanding, I can fix them up myself before applying.
> 
> I believe you didn't completely answer all my questions from v5 though.
> I'll repeat a bit here. Particularly, refer to [1].
> I'll summarize; I understand that your common transmit/receive operation
> works something like this:
> 
> Quoting from [1]:
> > (1) total number of bits to send/receive goes in the COUNT register (so
> >     far, as many as 7*8=56?)
> > (2) opcode is written to PRGDATA5
> > (3) other "transmit" data (like addresses), if any, are placed on PRGDATA4..0
> > (4) command is sent (execute_cmd())
> > (5) data is read back in SHREG{X..0}, if needed
> 
> My questions were:
> 
> (a) Why does mt8173_nor_set_read_mode() use PRGDATA3? That's not
>     mentioned in the SoC manual, and it doesn't really match any of the
>     steps above. Perhaps it's just a quirk of the controller's
>     programming model?
> 
yes, for this question, I have done some testes, If I change the
PRGDATA3 to PRGDATA5 for mt8173_nor_set_read_mode() like others
functions, then the controller will be hanged, and I have asked our
designer for double confirm.

> (b) How do you determine X from step (5)?
> 
> Right now, your code seems to answer that X is "rxlen - 1". Correct?
> 
yes, I have used "rxlen -1", because the first of nor flash output is
located at SHREG[0], in the other words, the output data starts at
SHREG[0] and go up to SHREG[relen -1]

> If that's correct and if I put all of my understanding together
> correctly, this means that you can actually shift out (in PRGDATA) up to
> 6 bytes (that is, 1 opcode and 5 tx bytes) and shift in (in SHREG) up to
> 7 bytes, except that the first byte is received during the opcode cycle,
> and so it is discarded, and we effectively receive only 6 bytes.
> 
> Is that all correct? If so, then I think you still need to adjust the
> boundary conditions in your do_tx_rx() function. (I'll comment on the
> driver to point out the specifics.)

Yes, you are right! and I will adjust the boundary conditions in
do_tx_rx() function.

By the way, could you tell me whether I need to publish a new patch? or
you can fix them up directly?
> 
> Regards,
> Brian
> 
> [1] http://lists.infradead.org/pipermail/linux-mtd/2015-October/062951.html


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

[toc] | [prev] | [next] | [standalone]

#1267425

On Wed, Nov 11, 2015 at 10:04:14PM +0800, Bayi Cheng wrote:
> On Mon, 2015-11-09 at 18:46 -0800, Brian Norris wrote:
> > I believe you didn't completely answer all my questions from v5 though.
> > I'll repeat a bit here. Particularly, refer to [1].
> > I'll summarize; I understand that your common transmit/receive operation
> > works something like this:
> > 
> > Quoting from [1]:
> > > (1) total number of bits to send/receive goes in the COUNT register (so
> > >     far, as many as 7*8=56?)
> > > (2) opcode is written to PRGDATA5
> > > (3) other "transmit" data (like addresses), if any, are placed on PRGDATA4..0
> > > (4) command is sent (execute_cmd())
> > > (5) data is read back in SHREG{X..0}, if needed
> > 
> > My questions were:
> > 
> > (a) Why does mt8173_nor_set_read_mode() use PRGDATA3? That's not
> >     mentioned in the SoC manual, and it doesn't really match any of the
> >     steps above. Perhaps it's just a quirk of the controller's
> >     programming model?
> > 
> yes, for this question, I have done some testes, If I change the
> PRGDATA3 to PRGDATA5 for mt8173_nor_set_read_mode() like others
> functions, then the controller will be hanged, and I have asked our
> designer for double confirm.

I wasn't suggesting to change this to PRGDATA5. I just was wondering why
the difference. It's not documented. (I suppose an acceptable answer is
just "because that's how the HW works.")

> > (b) How do you determine X from step (5)?
> > 
> > Right now, your code seems to answer that X is "rxlen - 1". Correct?
> > 
> yes, I have used "rxlen -1", because the first of nor flash output is
> located at SHREG[0], in the other words, the output data starts at
> SHREG[0] and go up to SHREG[relen -1]

But, we aren't reading from SHREG[0] first; we're reading backwards from
SHREG[rxlen - 1] down to SHREG[0]. It seems that's correct, right?

> > If that's correct and if I put all of my understanding together
> > correctly, this means that you can actually shift out (in PRGDATA) up to
> > 6 bytes (that is, 1 opcode and 5 tx bytes) and shift in (in SHREG) up to
> > 7 bytes, except that the first byte is received during the opcode cycle,
> > and so it is discarded, and we effectively receive only 6 bytes.
> > 
> > Is that all correct? If so, then I think you still need to adjust the
> > boundary conditions in your do_tx_rx() function. (I'll comment on the
> > driver to point out the specifics.)
> 
> Yes, you are right! and I will adjust the boundary conditions in
> do_tx_rx() function.

OK, good. BTW, can you make sure to rewrite the appropriate MAX macro(s)
to reflect the right values? It seems like maybe you'll want separate
macros for the maximum TX and RX -- and total (?), or is this just the
same as RX? -- since they seem to have different limits.

> By the way, could you tell me whether I need to publish a new patch? or
> you can fix them up directly?

I think there are a few more adjustments to make, so please just post a
new version of the driver only. The DT binding and DTS changes look good
to go now.

Regards,
Brian

> > [1] http://lists.infradead.org/pipermail/linux-mtd/2015-October/062951.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268651

On Wed, 2015-11-11 at 12:26 -0800, Brian Norris wrote:
> On Wed, Nov 11, 2015 at 10:04:14PM +0800, Bayi Cheng wrote:
> > On Mon, 2015-11-09 at 18:46 -0800, Brian Norris wrote:
> > > I believe you didn't completely answer all my questions from v5 though.
> > > I'll repeat a bit here. Particularly, refer to [1].
> > > I'll summarize; I understand that your common transmit/receive operation
> > > works something like this:
> > > 
> > > Quoting from [1]:
> > > > (1) total number of bits to send/receive goes in the COUNT register (so
> > > >     far, as many as 7*8=56?)
> > > > (2) opcode is written to PRGDATA5
> > > > (3) other "transmit" data (like addresses), if any, are placed on PRGDATA4..0
> > > > (4) command is sent (execute_cmd())
> > > > (5) data is read back in SHREG{X..0}, if needed
> > > 
> > > My questions were:
> > > 
> > > (a) Why does mt8173_nor_set_read_mode() use PRGDATA3? That's not
> > >     mentioned in the SoC manual, and it doesn't really match any of the
> > >     steps above. Perhaps it's just a quirk of the controller's
> > >     programming model?
> > > 
> > yes, for this question, I have done some testes, If I change the
> > PRGDATA3 to PRGDATA5 for mt8173_nor_set_read_mode() like others
> > functions, then the controller will be hanged, and I have asked our
> > designer for double confirm.
> 
> I wasn't suggesting to change this to PRGDATA5. I just was wondering why
> the difference. It's not documented. (I suppose an acceptable answer is
> just "because that's how the HW works.")
> 
I have synced with our designer, and just as you said, that's how the HW
works, and the read operation is different is different from other
operation. By the way, I have double confirm our driver code, and I have
made a mistake for quad read operation, the quad command need use
PRGDATA4 Instead PRGDATA3.
PS: write PRGDATA4 0xEB(4bit I/O read mode) or 0x6B(4 bit output mode),
So I will modify mt8173_nor_set_read_mode function in next patch.


> > > (b) How do you determine X from step (5)?
> > > 
> > > Right now, your code seems to answer that X is "rxlen - 1". Correct?
> > > 
> > yes, I have used "rxlen -1", because the first of nor flash output is
> > located at SHREG[0], in the other words, the output data starts at
> > SHREG[0] and go up to SHREG[relen -1]
> 
> But, we aren't reading from SHREG[0] first; we're reading backwards from
> SHREG[rxlen - 1] down to SHREG[0]. It seems that's correct, right?

Yes, You are right!
> 
> > > If that's correct and if I put all of my understanding together
> > > correctly, this means that you can actually shift out (in PRGDATA) up to
> > > 6 bytes (that is, 1 opcode and 5 tx bytes) and shift in (in SHREG) up to
> > > 7 bytes, except that the first byte is received during the opcode cycle,
> > > and so it is discarded, and we effectively receive only 6 bytes.
> > > 
> > > Is that all correct? If so, then I think you still need to adjust the
> > > boundary conditions in your do_tx_rx() function. (I'll comment on the
> > > driver to point out the specifics.)
> > 
> > Yes, you are right! and I will adjust the boundary conditions in
> > do_tx_rx() function.
> 
> OK, good. BTW, can you make sure to rewrite the appropriate MAX macro(s)
> to reflect the right values? It seems like maybe you'll want separate
> macros for the maximum TX and RX -- and total (?), or is this just the
> same as RX? -- since they seem to have different limits.
> 

OK, I will use two MAX macros, one is for TX&RX, and the other is for 
the total.

> > By the way, could you tell me whether I need to publish a new patch? or
> > you can fix them up directly?
> 
> I think there are a few more adjustments to make, so please just post a
> new version of the driver only. The DT binding and DTS changes look good
> to go now.

Yes, I will publish a new patch next week, and Thanks again for your
grate help and support!

> 
> Regards,
> Brian
> 
> > > [1] http://lists.infradead.org/pipermail/linux-mtd/2015-October/062951.html
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek


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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web