Message ID | 1418644760-18773-9-git-send-email-lee.jones@linaro.org |
---|---|
State | Superseded |
Headers | show |
On Mon, Dec 15, 2014 at 11:59:15AM +0000, Lee Jones wrote: > The previous code was based on 3-byte JEDEC IDs, with a possible 2-byte > extension. However, devices are now emerging that return 6 or more bytes of > READID data and the additional bytes are required to differentiate between > variants or generations of similar devices. > > This patch refactors the device table and JEDEC probe code to handle arbitrary > length READIDs, with the standard JEDEC definition now becoming a special case. > Functionally, there should be no change in behaviour. A subsequent patch will > update the table with extended READIDs where applicable. BTW, how's that promise going, where you work on adapting this driver to the spi-nor framework? We've already done some of this same work there. > +#define RDID(...) __VA_ARGS__ /* Dummy macro to protect array argument. */ What? What needs "protected"? ... Brian
On Mon, 12 Jan 2015, Brian Norris wrote: > On Mon, Dec 15, 2014 at 11:59:15AM +0000, Lee Jones wrote: > > The previous code was based on 3-byte JEDEC IDs, with a possible 2-byte > > extension. However, devices are now emerging that return 6 or more bytes of > > READID data and the additional bytes are required to differentiate between > > variants or generations of similar devices. > > > > This patch refactors the device table and JEDEC probe code to handle arbitrary > > length READIDs, with the standard JEDEC definition now becoming a special case. > > Functionally, there should be no change in behaviour. A subsequent patch will > > update the table with extended READIDs where applicable. > > BTW, how's that promise going, where you work on adapting this driver to > the spi-nor framework? We've already done some of this same work there. I have pushed this point within ST and someone has agreed to do the work. Last I heard it relied on these patches, but I'll ask again. > > +#define RDID(...) __VA_ARGS__ /* Dummy macro to protect array argument. */ > > What? What needs "protected"? You're asking me questions I can't answer I'm afraid and Angus has now left the building. I guess he thinks __VA_ARGS__ will prevent some kind of overflow?
On Wed, Jan 21, 2015 at 01:02:04PM +0000, Lee Jones wrote: > On Mon, 12 Jan 2015, Brian Norris wrote: > > On Mon, Dec 15, 2014 at 11:59:15AM +0000, Lee Jones wrote: > > > The previous code was based on 3-byte JEDEC IDs, with a possible 2-byte > > > extension. However, devices are now emerging that return 6 or more bytes of > > > READID data and the additional bytes are required to differentiate between > > > variants or generations of similar devices. > > > > > > This patch refactors the device table and JEDEC probe code to handle arbitrary > > > length READIDs, with the standard JEDEC definition now becoming a special case. > > > Functionally, there should be no change in behaviour. A subsequent patch will > > > update the table with extended READIDs where applicable. > > > > BTW, how's that promise going, where you work on adapting this driver to > > the spi-nor framework? We've already done some of this same work there. > > I have pushed this point within ST and someone has agreed to do the > work. Last I heard it relied on these patches, but I'll ask again. OK. > > > +#define RDID(...) __VA_ARGS__ /* Dummy macro to protect array argument. */ > > > > What? What needs "protected"? > > You're asking me questions I can't answer I'm afraid and Angus has now > left the building. I guess he thinks __VA_ARGS__ will prevent some > kind of overflow? If you don't understand your own code, how can I be expected to maintain it? This one's pretty trivial and harmless, but an accumulation of answers like this don't exactly put me in a good mood. FWIW, I expect the comment has nothing to do with the __VA_ARGS__; it's just commenting that he has placed a macro around the array just in case somebody needs/wants to rearrange formats later. That way, we don't necessarily have to rewrite the whole table, but can just change the macros. So the __VA_ARGS__ is just there to make the compiler happy (it thinks an array argument to a macro actually looks like more than one argument), and the comment is only mildly descriptive of its purpose. Brian
On Thu, 05 Feb 2015, Brian Norris wrote: > On Wed, Jan 21, 2015 at 01:02:04PM +0000, Lee Jones wrote: > > On Mon, 12 Jan 2015, Brian Norris wrote: > > > On Mon, Dec 15, 2014 at 11:59:15AM +0000, Lee Jones wrote: > > > > The previous code was based on 3-byte JEDEC IDs, with a possible 2-byte > > > > extension. However, devices are now emerging that return 6 or more bytes of > > > > READID data and the additional bytes are required to differentiate between > > > > variants or generations of similar devices. > > > > > > > > This patch refactors the device table and JEDEC probe code to handle arbitrary > > > > length READIDs, with the standard JEDEC definition now becoming a special case. > > > > Functionally, there should be no change in behaviour. A subsequent patch will > > > > update the table with extended READIDs where applicable. > > > > > > BTW, how's that promise going, where you work on adapting this driver to > > > the spi-nor framework? We've already done some of this same work there. > > > > I have pushed this point within ST and someone has agreed to do the > > work. Last I heard it relied on these patches, but I'll ask again. > > OK. > > > > > +#define RDID(...) __VA_ARGS__ /* Dummy macro to protect array argument. */ > > > > > > What? What needs "protected"? > > > > You're asking me questions I can't answer I'm afraid and Angus has now > > left the building. I guess he thinks __VA_ARGS__ will prevent some > > kind of overflow? > > If you don't understand your own code, how can I be expected to maintain > it? This one's pretty trivial and harmless, but an accumulation of > answers like this don't exactly put me in a good mood. I have never written a line of code that I didn't understand and I think you are quite aware that this has never been my 'own code'. With regards to the previous accumulation of 'answers like this'; I have never been, nor have any desire to be an expert on Memory Technology Devices. I was asked to upstream ST's NAND and NOR drivers with support of the local expert and author; however, for reasons not under neither of our controls, he has now left the company. With him went all of the historical reasoning for all for the questions you've asked. This is not my domain and have found it pretty tough to keep up with all of your demands, both on this and the NAND driver but I have been pretty subservient and keep up with them I have. I am not privy to any of the author's thoughts or reasons for any decisions taken. I can only hope that his comments might have some meaning to a like minded 'expert' such as yourself. If you don't know something, then the chances are slight that I will be able to answer your query. > FWIW, I expect the comment has nothing to do with the __VA_ARGS__; it's > just commenting that he has placed a macro around the array just in case > somebody needs/wants to rearrange formats later. That way, we don't > necessarily have to rewrite the whole table, but can just change the > macros. > > So the __VA_ARGS__ is just there to make the compiler happy (it thinks > an array argument to a macro actually looks like more than one > argument), and the comment is only mildly descriptive of its purpose. I was present Passing Variable Arguments 101, so I'm aware of what __VA_ARGS__ and "..." do at a functional level. So it looks like you had a better idea of what Angus was trying to achieve than I do. Perhaps your question should have been more specific. I guess it's just the comment that you are unhappy with. I can remove it if it makes you happy.
On Tue, Feb 10, 2015 at 05:04:33PM +0800, Lee Jones wrote: > On Thu, 05 Feb 2015, Brian Norris wrote: > > On Wed, Jan 21, 2015 at 01:02:04PM +0000, Lee Jones wrote: > > > On Mon, 12 Jan 2015, Brian Norris wrote: > > > > On Mon, Dec 15, 2014 at 11:59:15AM +0000, Lee Jones wrote: > > > > > +#define RDID(...) __VA_ARGS__ /* Dummy macro to protect array argument. */ > > > > > > > > What? What needs "protected"? > > > > > > You're asking me questions I can't answer I'm afraid and Angus has now > > > left the building. I guess he thinks __VA_ARGS__ will prevent some > > > kind of overflow? > > > > If you don't understand your own code, how can I be expected to maintain > > it? This one's pretty trivial and harmless, but an accumulation of > > answers like this don't exactly put me in a good mood. > > I have never written a line of code that I didn't understand and I > think you are quite aware that this has never been my 'own code'. Stop deflecting. I don't care that you didn't write this code. If you're asking me to apply this upstream, then you own it. Or else, hire another expert who is willing to own your drivers. > With regards to the previous accumulation of 'answers like this'; I > have never been, nor have any desire to be an expert on Memory > Technology Devices. I was asked to upstream ST's NAND and NOR drivers > with support of the local expert and author; however, for reasons not > under neither of our controls, he has now left the company. With him > went all of the historical reasoning for all for the questions you've > asked. This is not my domain and have found it pretty tough to keep > up with all of your demands, both on this and the NAND driver but I > have been pretty subservient and keep up with them I have. I am not > privy to any of the author's thoughts or reasons for any decisions > taken. I can only hope that his comments might have some meaning to a > like minded 'expert' such as yourself. If you don't know something, > then the chances are slight that I will be able to answer your query. Well, I think we have a fundamentally different view of what upstream development means, then. My job is not to worry about your company's internal problems. My job is not to worry about your company's development schedules. My job is not to be your on-call MTD expert. My job is to make sure that upstream MTD contains relatively clean, maintainable code, and to accept patches that generally improve the codebase (i.e., bug fixes, refactoring, feature improvements). Recall that at least one prominent kernel developer has suggested that it's actually not in a maintainer's interests to accept random contributors' code. The contributor's job is to give the maintainer no reason to reject his or her code. When you repeatedly claim to not understand the code you're sending me, that sets off warning bells that make it significantly harder to handle your code. Perhaps we'd all be better off if I simply removed your driver from upstream, and you can convince your company to hire an MTD expert to handle your upstreaming efforts. > > FWIW, I expect the comment has nothing to do with the __VA_ARGS__; it's > > just commenting that he has placed a macro around the array just in case > > somebody needs/wants to rearrange formats later. That way, we don't > > necessarily have to rewrite the whole table, but can just change the > > macros. > > > > So the __VA_ARGS__ is just there to make the compiler happy (it thinks > > an array argument to a macro actually looks like more than one > > argument), and the comment is only mildly descriptive of its purpose. > > I was present Passing Variable Arguments 101, so I'm aware of what > __VA_ARGS__ and "..." do at a functional level. But this is not a '101' use case, exactly. It's there because of the peculiarities of macros, it seems. And don't pretend to understand when you clearly did not when I first asked. (And to be clear: I did not understand either. That's why I asked.) > So it looks like you had a better idea of what Angus was trying to > achieve than I do. Perhaps your question should have been more > specific. I guess it's just the comment that you are unhappy with. I > can remove it if it makes you happy. No, the comment wasn't the problem, exactly. I was a bit confused, and I wrongly assumed that you understood what you're sending me. Apparently that was too much to assume. Brian
On Mon, 23 Feb 2015, Brian Norris wrote: > On Tue, Feb 10, 2015 at 05:04:33PM +0800, Lee Jones wrote: > > On Thu, 05 Feb 2015, Brian Norris wrote: > > > On Wed, Jan 21, 2015 at 01:02:04PM +0000, Lee Jones wrote: > > > > On Mon, 12 Jan 2015, Brian Norris wrote: > > > > > On Mon, Dec 15, 2014 at 11:59:15AM +0000, Lee Jones wrote: > > > > > > +#define RDID(...) __VA_ARGS__ /* Dummy macro to protect array argument. */ > > > > > > > > > > What? What needs "protected"? > > > > > > > > You're asking me questions I can't answer I'm afraid and Angus has now > > > > left the building. I guess he thinks __VA_ARGS__ will prevent some > > > > kind of overflow? > > > > > > If you don't understand your own code, how can I be expected to maintain > > > it? This one's pretty trivial and harmless, but an accumulation of > > > answers like this don't exactly put me in a good mood. > > > > I have never written a line of code that I didn't understand and I > > think you are quite aware that this has never been my 'own code'. > > Stop deflecting. I don't care that you didn't write this code. If you're > asking me to apply this upstream, then you own it. Okay, I own it. And I have a good grasp of how most of the driver operates. However, it is unreasonable to request that someone who conducts Mainlining activities who is not the author to understand every single line. It's now commonplace for an 'author' and 'submitter' to be different people. The only difference is that normally the author is still around to provide support. Or at least the submitter has access to a team who have in-depth knowledge of the hardware/driver. I have submitted drivers and fixes all over the kernel tree and have a fairly good knowledge of each of the subsystems, but to ask that I become an expert in all of them -- especially subsystems as complex as MTD is idealistic and unrealistic at best. > Or else, hire another expert who is willing to own your drivers. Silicon chip vendors do not have limitless resources to hire or train experts to conduct all their upstreaming activities? These guys are almost always focused on customer projects/requests and have little time for upstreaming. It's people like us to advocate and encourage pushing code into Mainline, as the cost of upstreaming is usually dwarfed by the cost of migrating a huge delta to a more recent code base. This activity however, has not a good example of that. > > With regards to the previous accumulation of 'answers like this'; I > > have never been, nor have any desire to be an expert on Memory > > Technology Devices. I was asked to upstream ST's NAND and NOR drivers > > with support of the local expert and author; however, for reasons not > > under neither of our controls, he has now left the company. With him > > went all of the historical reasoning for all for the questions you've > > asked. This is not my domain and have found it pretty tough to keep > > up with all of your demands, both on this and the NAND driver but I > > have been pretty subservient and keep up with them I have. I am not > > privy to any of the author's thoughts or reasons for any decisions > > taken. I can only hope that his comments might have some meaning to a > > like minded 'expert' such as yourself. If you don't know something, > > then the chances are slight that I will be able to answer your query. > > Well, I think we have a fundamentally different view of what upstream > development means, then. My job is not to worry about your company's > internal problems. My job is not to worry about your company's > development schedules. My job is not to be your on-call MTD expert. > > My job is to make sure that upstream MTD contains relatively clean, > maintainable code, and to accept patches that generally improve the > codebase (i.e., bug fixes, refactoring, feature improvements). > > Recall that at least one prominent kernel developer has suggested that > it's actually not in a maintainer's interests to accept random > contributors' code. The contributor's job is to give the maintainer no > reason to reject his or her code. When you repeatedly claim to not > understand the code you're sending me, that sets off warning bells that > make it significantly harder to handle your code. At the moment, what you're saying is; if your NOR Contreoller is not exactly the same as all the others and doesn't fit inside our mega-driver, it's not welcome. > Perhaps we'd all be better off if I simply removed your driver from > upstream, and you can convince your company to hire an MTD expert to > handle your upstreaming efforts. See my point above about the lack of limitless funds. > > > FWIW, I expect the comment has nothing to do with the __VA_ARGS__; it's > > > just commenting that he has placed a macro around the array just in case > > > somebody needs/wants to rearrange formats later. That way, we don't > > > necessarily have to rewrite the whole table, but can just change the > > > macros. > > > > > > So the __VA_ARGS__ is just there to make the compiler happy (it thinks > > > an array argument to a macro actually looks like more than one > > > argument), and the comment is only mildly descriptive of its purpose. > > > > I was present Passing Variable Arguments 101, so I'm aware of what > > __VA_ARGS__ and "..." do at a functional level. > > But this is not a '101' use case, exactly. It's there because of the > peculiarities of macros, it seems. And don't pretend to understand when > you clearly did not when I first asked. (And to be clear: I did not > understand either. That's why I asked.) Stop twisting things. You (unnecessarily) explained to me what va_args was. I was saying that I know what it is and how to declare functions with variable arguments. Not that I knew what the MACRO was doing. > > So it looks like you had a better idea of what Angus was trying to > > achieve than I do. Perhaps your question should have been more > > specific. I guess it's just the comment that you are unhappy with. I > > can remove it if it makes you happy. > > No, the comment wasn't the problem, exactly. I was a bit confused, and I > wrongly assumed that you understood what you're sending me. Apparently > that was too much to assume. Refer to my points above.
diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c index 4fac169..949745a 100644 --- a/drivers/mtd/devices/st_spi_fsm.c +++ b/drivers/mtd/devices/st_spi_fsm.c @@ -21,6 +21,7 @@ #include <linux/mtd/partitions.h> #include <linux/mtd/spi-nor.h> #include <linux/sched.h> +#include <linux/sort.h> #include <linux/delay.h> #include <linux/io.h> #include <linux/of.h> @@ -236,6 +237,11 @@ #define FLASH_STATUS_BP2 0x10 #define FLASH_STATUS_SRWP0 0x80 #define FLASH_STATUS_TIMEOUT 0xff + +/* Maximum READID length */ +#define MAX_READID_LEN 6 +#define MAX_READID_LEN_ALIGNED ALIGN(MAX_READID_LEN, 4) + /* S25FL Error Flags */ #define S25FL_STATUS_E_ERR 0x20 #define S25FL_STATUS_P_ERR 0x40 @@ -326,13 +332,9 @@ struct stfsm_boot_dev { /* SPI Flash Device Table */ struct flash_info { char *name; - /* - * JEDEC id zero means "no ID" (most older chips); otherwise it has - * a high byte of zero plus three data bytes: the manufacturer id, - * then a two byte device id. - */ - u32 jedec_id; - u16 ext_id; + /* READID data, as returned by 'SPINOR_OP_RDID' (0x9f). */ + u8 readid[MAX_READID_LEN]; + int readid_len; /* * The size listed here is what works with SPINOR_OP_SE, which isn't * necessarily called a "sector" by the vendor. @@ -366,6 +368,37 @@ static struct stfsm_boot_dev stfsm_stih416_data = { .mask = STIH416_SYSCON_BOOT_DEV_MASK, }; +/* Device with standard 3-byte JEDEC ID */ +#define JEDEC_INFO(_name, _jedec_id, _sector_size, _n_sectors, \ + _flags, _max_freq, _config) \ + { \ + .name = (_name), \ + .readid[0] = ((_jedec_id) >> 16 & 0xff), \ + .readid[1] = ((_jedec_id) >> 8 & 0xff), \ + .readid[2] = ((_jedec_id) >> 0 & 0xff), \ + .readid_len = 3, \ + .sector_size = (_sector_size), \ + .n_sectors = (_n_sectors), \ + .flags = (_flags), \ + .max_freq = (_max_freq), \ + .config = (_config) \ + } + +/* Device with arbitrary-length READID */ +#define RDID(...) __VA_ARGS__ /* Dummy macro to protect array argument. */ +#define RDID_INFO(_name, _readid, _readid_len, _sector_size, \ + _n_sectors, _flags, _max_freq, _config) \ + { \ + .name = (_name), \ + .readid = _readid, \ + .readid_len = _readid_len, \ + .sector_size = (_sector_size), \ + .n_sectors = (_n_sectors), \ + .flags = (_flags), \ + .max_freq = (_max_freq), \ + .config = (_config) \ + } + static int stfsm_n25q_config(struct stfsm *fsm); static int stfsm_mx25_config(struct stfsm *fsm); static int stfsm_s25fl_config(struct stfsm *fsm); @@ -378,19 +411,19 @@ static struct flash_info flash_types[] = { * (eg faster operating frequency) */ #define M25P_FLAG (FLASH_FLAG_READ_WRITE | FLASH_FLAG_READ_FAST) - { "m25p40", 0x202013, 0, 64 * 1024, 8, M25P_FLAG, 25, NULL }, - { "m25p80", 0x202014, 0, 64 * 1024, 16, M25P_FLAG, 25, NULL }, - { "m25p16", 0x202015, 0, 64 * 1024, 32, M25P_FLAG, 25, NULL }, - { "m25p32", 0x202016, 0, 64 * 1024, 64, M25P_FLAG, 50, NULL }, - { "m25p64", 0x202017, 0, 64 * 1024, 128, M25P_FLAG, 50, NULL }, - { "m25p128", 0x202018, 0, 256 * 1024, 64, M25P_FLAG, 50, NULL }, + JEDEC_INFO("m25p40", 0x202013, 64 * 1024, 8, M25P_FLAG, 25, NULL), + JEDEC_INFO("m25p80", 0x202014, 64 * 1024, 16, M25P_FLAG, 25, NULL), + JEDEC_INFO("m25p16", 0x202015, 64 * 1024, 32, M25P_FLAG, 25, NULL), + JEDEC_INFO("m25p32", 0x202016, 64 * 1024, 64, M25P_FLAG, 50, NULL), + JEDEC_INFO("m25p64", 0x202017, 64 * 1024, 128, M25P_FLAG, 50, NULL), + JEDEC_INFO("m25p128", 0x202018, 256 * 1024, 64, M25P_FLAG, 50, NULL), #define M25PX_FLAG (FLASH_FLAG_READ_WRITE | \ FLASH_FLAG_READ_FAST | \ FLASH_FLAG_READ_1_1_2 | \ FLASH_FLAG_WRITE_1_1_2) - { "m25px32", 0x207116, 0, 64 * 1024, 64, M25PX_FLAG, 75, NULL }, - { "m25px64", 0x207117, 0, 64 * 1024, 128, M25PX_FLAG, 75, NULL }, + JEDEC_INFO("m25px32", 0x207116, 64 * 1024, 64, M25PX_FLAG, 75, NULL), + JEDEC_INFO("m25px64", 0x207117, 64 * 1024, 128, M25PX_FLAG, 75, NULL), /* Macronix MX25xxx * - Support for 'FLASH_FLAG_WRITE_1_4_4' is omitted for devices @@ -403,15 +436,12 @@ static struct flash_info flash_types[] = { FLASH_FLAG_READ_1_1_4 | \ FLASH_FLAG_SE_4K | \ FLASH_FLAG_SE_32K) - { "mx25l3255e", 0xc29e16, 0, 64 * 1024, 64, - (MX25_FLAG | FLASH_FLAG_WRITE_1_4_4), 86, - stfsm_mx25_config}, - { "mx25l25635e", 0xc22019, 0, 64*1024, 512, - (MX25_FLAG | FLASH_FLAG_32BIT_ADDR | FLASH_FLAG_RESET), 70, - stfsm_mx25_config }, - { "mx25l25655e", 0xc22619, 0, 64*1024, 512, - (MX25_FLAG | FLASH_FLAG_32BIT_ADDR | FLASH_FLAG_RESET), 70, - stfsm_mx25_config}, + JEDEC_INFO("mx25l3255e", 0xc29e16, 64 * 1024, 64, + (MX25_FLAG | FLASH_FLAG_WRITE_1_4_4), 86, stfsm_mx25_config), + JEDEC_INFO("mx25l25635e", 0xc22019, 64 * 1024, 512, + (MX25_FLAG | FLASH_FLAG_RESET), 70, stfsm_mx25_config), + JEDEC_INFO("mx25l25655e", 0xc22619, 64 * 1024, 512, + (MX25_FLAG | FLASH_FLAG_RESET), 70, stfsm_mx25_config), /* Micron N25Qxxx */ #define N25Q_FLAG (FLASH_FLAG_READ_WRITE | \ @@ -424,8 +454,8 @@ static struct flash_info flash_types[] = { FLASH_FLAG_WRITE_1_2_2 | \ FLASH_FLAG_WRITE_1_1_4 | \ FLASH_FLAG_WRITE_1_4_4) - { "n25q128", 0x20ba18, 0, 64 * 1024, 256, N25Q_FLAG, 108, - stfsm_n25q_config }, + JEDEC_INFO("n25q128", 0x20ba18, 64 * 1024, 256, + N25Q_FLAG, 108, stfsm_n25q_config), /* Micron N25Q256/N25Q512/N25Q00A (32-bit ADDR devices) * @@ -443,12 +473,12 @@ static struct flash_info flash_types[] = { FLASH_FLAG_32BIT_ADDR | \ FLASH_FLAG_RESET) & \ ~FLASH_FLAG_WRITE_1_4_4) - { "n25q256", 0x20ba19, 0, 64 * 1024, 512, - N25Q_32BIT_ADDR_FLAG, 108, stfsm_n25q_config}, - { "n25q512", 0x20ba20, 0x1000, 64 * 1024, 1024, - N25Q_32BIT_ADDR_FLAG, 108, stfsm_n25q_config}, - { "n25q00a", 0x20ba21, 0x1000, 64 * 1024, 2048, - N25Q_32BIT_ADDR_FLAG, 108, stfsm_n25q_config}, + JEDEC_INFO("n25q256", 0x20ba19, 64 * 1024, 512, + N25Q_32BIT_ADDR_FLAG, 108, stfsm_n25q_config), + RDID_INFO("n25q512", RDID({0x20, 0xba, 0x20, 0x10, 0x00}), 5, 64 * 1024, + 1024, N25Q_32BIT_ADDR_FLAG, 108, stfsm_n25q_config), + RDID_INFO("n25q00a", RDID({0x20, 0xba, 0x21, 0x10, 0x00}), 5, 64 * 1024, + 2048, N25Q_32BIT_ADDR_FLAG, 108, stfsm_n25q_config), /* * Spansion S25FLxxxP @@ -461,12 +491,12 @@ static struct flash_info flash_types[] = { FLASH_FLAG_READ_1_4_4 | \ FLASH_FLAG_WRITE_1_1_4 | \ FLASH_FLAG_READ_FAST) - { "s25fl032p", 0x010215, 0x4d00, 64 * 1024, 64, S25FLXXXP_FLAG, 80, - stfsm_s25fl_config}, - { "s25fl129p0", 0x012018, 0x4d00, 256 * 1024, 64, S25FLXXXP_FLAG, 80, - stfsm_s25fl_config }, - { "s25fl129p1", 0x012018, 0x4d01, 64 * 1024, 256, S25FLXXXP_FLAG, 80, - stfsm_s25fl_config }, + RDID_INFO("s25fl032p", RDID({0x01, 0x02, 0x15, 0x4d, 0x00}), 5, + 64 * 1024, 64, S25FLXXXP_FLAG, 80, stfsm_s25fl_config), + RDID_INFO("s25fl129p0", RDID({0x01, 0x20, 0x18, 0x4d, 0x00}), 5, + 256 * 1024, 64, S25FLXXXP_FLAG, 80, stfsm_s25fl_config), + RDID_INFO("s25fl129p1", RDID({0x01, 0x20, 0x18, 0x4d, 0x01}), 5, + 64 * 1024, 256, S25FLXXXP_FLAG, 80, stfsm_s25fl_config), /* * Spansion S25FLxxxS @@ -480,25 +510,24 @@ static struct flash_info flash_types[] = { #define S25FLXXXS_FLAG (S25FLXXXP_FLAG | \ FLASH_FLAG_RESET | \ FLASH_FLAG_DYB_LOCKING) - { "s25fl128s0", 0x012018, 0x0300, 256 * 1024, 64, S25FLXXXS_FLAG, 80, - stfsm_s25fl_config }, - { "s25fl128s1", 0x012018, 0x0301, 64 * 1024, 256, S25FLXXXS_FLAG, 80, - stfsm_s25fl_config }, - { "s25fl256s0", 0x010219, 0x4d00, 256 * 1024, 128, - S25FLXXXS_FLAG | FLASH_FLAG_32BIT_ADDR, 80, stfsm_s25fl_config }, - { "s25fl256s1", 0x010219, 0x4d01, 64 * 1024, 512, - S25FLXXXS_FLAG | FLASH_FLAG_32BIT_ADDR, 80, stfsm_s25fl_config }, - - /* Winbond -- w25x "blocks" are 64K, "sectors" are 4KiB */ + RDID_INFO("s25fl128s0", RDID({0x01, 0x20, 0x18, 0x03, 0x00}), 5, + 256 * 1024, 64, S25FLXXXS_FLAG, 80, stfsm_s25fl_config), + RDID_INFO("s25fl128s1", RDID({0x01, 0x20, 0x18, 0x03, 0x01}), 5, + 64 * 1024, 256, S25FLXXXS_FLAG, 80, stfsm_s25fl_config), + RDID_INFO("s25fl256s0", RDID({0x01, 0x02, 0x19, 0x4d, 0x00}), 5, + 256 * 1024, 128, S25FLXXXS_FLAG, 80, stfsm_s25fl_config), + RDID_INFO("s25fl256s1", RDID({0x01, 0x02, 0x19, 0x4d, 0x01}), 5, + 64 * 1024, 512, S25FLXXXS_FLAG, 80, stfsm_s25fl_config), + #define W25X_FLAG (FLASH_FLAG_READ_WRITE | \ FLASH_FLAG_READ_FAST | \ FLASH_FLAG_READ_1_1_2 | \ FLASH_FLAG_WRITE_1_1_2) - { "w25x40", 0xef3013, 0, 64 * 1024, 8, W25X_FLAG, 75, NULL }, - { "w25x80", 0xef3014, 0, 64 * 1024, 16, W25X_FLAG, 75, NULL }, - { "w25x16", 0xef3015, 0, 64 * 1024, 32, W25X_FLAG, 75, NULL }, - { "w25x32", 0xef3016, 0, 64 * 1024, 64, W25X_FLAG, 75, NULL }, - { "w25x64", 0xef3017, 0, 64 * 1024, 128, W25X_FLAG, 75, NULL }, + JEDEC_INFO("w25x40", 0xef3013, 64 * 1024, 8, W25X_FLAG, 75, NULL), + JEDEC_INFO("w25x80", 0xef3014, 64 * 1024, 16, W25X_FLAG, 75, NULL), + JEDEC_INFO("w25x16", 0xef3015, 64 * 1024, 32, W25X_FLAG, 75, NULL), + JEDEC_INFO("w25x32", 0xef3016, 64 * 1024, 64, W25X_FLAG, 75, NULL), + JEDEC_INFO("w25x64", 0xef3017, 64 * 1024, 128, W25X_FLAG, 75, NULL), /* Winbond -- w25q "blocks" are 64K, "sectors" are 4KiB */ #define W25Q_FLAG (FLASH_FLAG_READ_WRITE | \ @@ -508,17 +537,16 @@ static struct flash_info flash_types[] = { FLASH_FLAG_READ_1_1_4 | \ FLASH_FLAG_READ_1_4_4 | \ FLASH_FLAG_WRITE_1_1_4) - { "w25q80", 0xef4014, 0, 64 * 1024, 16, W25Q_FLAG, 80, - stfsm_w25q_config }, - { "w25q16", 0xef4015, 0, 64 * 1024, 32, W25Q_FLAG, 80, - stfsm_w25q_config }, - { "w25q32", 0xef4016, 0, 64 * 1024, 64, W25Q_FLAG, 80, - stfsm_w25q_config }, - { "w25q64", 0xef4017, 0, 64 * 1024, 128, W25Q_FLAG, 80, - stfsm_w25q_config }, - - /* Sentinel */ - { NULL, 0x000000, 0, 0, 0, 0, 0, NULL }, + JEDEC_INFO("w25q80", 0xef4014, 64 * 1024, 16, + W25Q_FLAG, 80, stfsm_w25q_config), + JEDEC_INFO("w25q16", 0xef4015, 64 * 1024, 32, + W25Q_FLAG, 80, stfsm_w25q_config), + JEDEC_INFO("w25q32", 0xef4016, 64 * 1024, 64, + W25Q_FLAG, 80, stfsm_w25q_config), + JEDEC_INFO("w25q64", 0xef4017, 64 * 1024, 128, + W25Q_FLAG, 80, stfsm_w25q_config), + + { }, }; /* @@ -649,7 +677,7 @@ static struct seq_rw_config stfsm_s25fl_write4_configs[] = { #define W25Q_STATUS_QE (0x1 << 1) static struct stfsm_seq stfsm_seq_read_jedec = { - .data_size = TRANSFER_SIZE(8), + .data_size = TRANSFER_SIZE(MAX_READID_LEN_ALIGNED), .seq_opc[0] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) | SEQ_OPC_OPCODE(SPINOR_OP_RDID)), @@ -1967,45 +1995,49 @@ out1: static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *jedec) { const struct stfsm_seq *seq = &stfsm_seq_read_jedec; - uint32_t tmp[2]; + uint32_t tmp[MAX_READID_LEN_ALIGNED / 4]; stfsm_load_seq(fsm, seq); - stfsm_read_fifo(fsm, tmp, 8); + stfsm_read_fifo(fsm, tmp, MAX_READID_LEN_ALIGNED); - memcpy(jedec, tmp, 5); + memcpy(jedec, tmp, MAX_READID_LEN); stfsm_wait_seq(fsm); } +static int stfsm_cmp_flash_info_readid_len(const void *a, const void *b) +{ + return ((struct flash_info *)b)->readid_len - + ((struct flash_info *)a)->readid_len; +} + static struct flash_info *stfsm_jedec_probe(struct stfsm *fsm) { - struct flash_info *info; - u16 ext_jedec; - u32 jedec; - u8 id[5]; + uint8_t readid[MAX_READID_LEN]; + char readid_str[MAX_READID_LEN * 3 + 1]; + struct flash_info *info; - stfsm_read_jedec(fsm, id); + stfsm_read_jedec(fsm, readid); - jedec = id[0] << 16 | id[1] << 8 | id[2]; - /* - * JEDEC also defines an optional "extended device information" - * string for after vendor-specific data, after the three bytes - * we use here. Supporting some chips might require using it. - */ - ext_jedec = id[3] << 8 | id[4]; + hex_dump_to_buffer(readid, MAX_READID_LEN, 16, 1, + readid_str, sizeof(readid_str), 0); - dev_dbg(fsm->dev, "JEDEC = 0x%08x [%02x %02x %02x %02x %02x]\n", - jedec, id[0], id[1], id[2], id[3], id[4]); + dev_dbg(fsm->dev, "READID = %s\n", readid_str); + + /* The 'readid' may match multiple entries in the table. To ensure we + * retrieve the most specific match, the table is sorted in order of + * 'readid_len'. + */ + sort(flash_types, ARRAY_SIZE(flash_types) - 1, + sizeof(struct flash_info), stfsm_cmp_flash_info_readid_len, NULL); for (info = flash_types; info->name; info++) { - if (info->jedec_id == jedec) { - if (info->ext_id && info->ext_id != ext_jedec) - continue; + if (memcmp(info->readid, readid, info->readid_len) == 0) return info; - } } - dev_err(fsm->dev, "Unrecognized JEDEC id %06x\n", jedec); + + dev_err(fsm->dev, "Unrecognized READID [%s]\n", readid_str); return NULL; }