| Message ID | 1e02585381b1fb93a8fd5b826ac33d355eaedaa5.1331119143.git.vipin.kumar@st.com |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2012-03-07 at 17:00 +0530, Vipin Kumar wrote: > +static struct mtd_partition partition_info_64KB_blk[] = { > + { > + .name = "X-loader", > + .offset = 0, > + .size = 4*0x10000, > + }, > + { > + .name = "U-Boot", > + .offset = 4*0x10000, > + .size = 8*0x40000, > + }, > + { > + .name = "Kernel", > + .offset = (4+8)*0x10000, > + .size = 64*0x40000, > + }, > + { > + .name = "Root File System", > + .offset = (4+8+64)*0x10000, > + .size = MTDPART_SIZ_FULL, > + }, > +}; Shouldn't this kind of data come from DT/platform data or cmdline instead? Does it make sense to have it hard-coded in the driver?
On 03/09/2012 02:07 PM, Artem Bityutskiy wrote: > On Wed, 2012-03-07 at 17:00 +0530, Vipin Kumar wrote: >> +static struct mtd_partition partition_info_64KB_blk[] = { >> + { >> + .name = "X-loader", >> + .offset = 0, >> + .size = 4*0x10000, >> + }, >> + { >> + .name = "U-Boot", >> + .offset = 4*0x10000, >> + .size = 8*0x40000, >> + }, >> + { >> + .name = "Kernel", >> + .offset = (4+8)*0x10000, >> + .size = 64*0x40000, >> + }, >> + { >> + .name = "Root File System", >> + .offset = (4+8+64)*0x10000, >> + .size = MTDPART_SIZ_FULL, >> + }, >> +}; > > Shouldn't this kind of data come from DT/platform data or cmdline > instead? Does it make sense to have it hard-coded in the driver? > These are just the default partitions. We are passing them also from platform, and it is possible also from cmdline. But in case we don't, this is the default. Shouldn't we treat such a case? Ciao, Arm
On Fri, 2012-03-09 at 15:47 +0100, Armando Visconti wrote: > On 03/09/2012 02:07 PM, Artem Bityutskiy wrote: > > On Wed, 2012-03-07 at 17:00 +0530, Vipin Kumar wrote: > >> +static struct mtd_partition partition_info_64KB_blk[] = { > >> + { > >> + .name = "X-loader", > >> + .offset = 0, > >> + .size = 4*0x10000, > >> + }, > >> + { > >> + .name = "U-Boot", > >> + .offset = 4*0x10000, > >> + .size = 8*0x40000, > >> + }, > >> + { > >> + .name = "Kernel", > >> + .offset = (4+8)*0x10000, > >> + .size = 64*0x40000, > >> + }, > >> + { > >> + .name = "Root File System", > >> + .offset = (4+8+64)*0x10000, > >> + .size = MTDPART_SIZ_FULL, > >> + }, > >> +}; > > > > Shouldn't this kind of data come from DT/platform data or cmdline > > instead? Does it make sense to have it hard-coded in the driver? > > > > These are just the default partitions. I thing the default should be "no partitions" instead of a hard-coded list of partitions tailored to a specific system. > We are passing them also from platform, and it is possible also > from cmdline. Fine, then deleting the defaults should not hurt :-)
On 03/09/2012 04:11 PM, Artem Bityutskiy wrote: > On Fri, 2012-03-09 at 15:47 +0100, Armando Visconti wrote: >> On 03/09/2012 02:07 PM, Artem Bityutskiy wrote: >>> On Wed, 2012-03-07 at 17:00 +0530, Vipin Kumar wrote: >>>> +static struct mtd_partition partition_info_64KB_blk[] = { >>>> + { >>>> + .name = "X-loader", >>>> + .offset = 0, >>>> + .size = 4*0x10000, >>>> + }, >>>> + { >>>> + .name = "U-Boot", >>>> + .offset = 4*0x10000, >>>> + .size = 8*0x40000, >>>> + }, >>>> + { >>>> + .name = "Kernel", >>>> + .offset = (4+8)*0x10000, >>>> + .size = 64*0x40000, >>>> + }, >>>> + { >>>> + .name = "Root File System", >>>> + .offset = (4+8+64)*0x10000, >>>> + .size = MTDPART_SIZ_FULL, >>>> + }, >>>> +}; >>> >>> Shouldn't this kind of data come from DT/platform data or cmdline >>> instead? Does it make sense to have it hard-coded in the driver? >>> >> >> These are just the default partitions. > > I thing the default should be "no partitions" instead of a hard-coded > list of partitions tailored to a specific system. > >> We are passing them also from platform, and it is possible also >> from cmdline. > > Fine, then deleting the defaults should not hurt :-) > Yes, Artem, I think you are right. Actually I checked better and I can say that: 1. We do have the provision to pass the partitions thru pdata and cmdline 2. Nevertheless, we are not passing partitions thru pdata in none of our platforms. I think we need to change this and pass partitions thru pdata. Vipin, what's your opinion? Thx, Arm
On 3/13/2012 3:23 PM, Armando VISCONTI wrote: > On 03/09/2012 04:11 PM, Artem Bityutskiy wrote: >> On Fri, 2012-03-09 at 15:47 +0100, Armando Visconti wrote: >>> On 03/09/2012 02:07 PM, Artem Bityutskiy wrote: >>>> On Wed, 2012-03-07 at 17:00 +0530, Vipin Kumar wrote: >>>>> +static struct mtd_partition partition_info_64KB_blk[] = { >>>>> + { >>>>> + .name = "X-loader", >>>>> + .offset = 0, >>>>> + .size = 4*0x10000, >>>>> + }, >>>>> + { >>>>> + .name = "U-Boot", >>>>> + .offset = 4*0x10000, >>>>> + .size = 8*0x40000, >>>>> + }, >>>>> + { >>>>> + .name = "Kernel", >>>>> + .offset = (4+8)*0x10000, >>>>> + .size = 64*0x40000, >>>>> + }, >>>>> + { >>>>> + .name = "Root File System", >>>>> + .offset = (4+8+64)*0x10000, >>>>> + .size = MTDPART_SIZ_FULL, >>>>> + }, >>>>> +}; >>>> >>>> Shouldn't this kind of data come from DT/platform data or cmdline >>>> instead? Does it make sense to have it hard-coded in the driver? >>>> >>> >>> These are just the default partitions. >> >> I thing the default should be "no partitions" instead of a hard-coded >> list of partitions tailored to a specific system. >> >>> We are passing them also from platform, and it is possible also >>> from cmdline. >> >> Fine, then deleting the defaults should not hurt :-) >> > > Yes, Artem, I think you are right. > Actually I checked better and I can say that: > > 1. We do have the provision to pass the partitions thru pdata and > cmdline > 2. Nevertheless, we are not passing partitions thru pdata in none > of our platforms. > > I think we need to change this and pass partitions thru pdata. > Vipin, what's your opinion? > Yes, I was thinking on the same lines. Infact I have prepared the patches and only going through a basic testing before sending the patches to mainline Regards Vipin > Thx, > Arm >
On Tue, Mar 13, 2012 at 10:53 AM, Armando Visconti <armando.visconti@st.com> wrote: > Actually I checked better and I can say that: > > 1. We do have the provision to pass the partitions thru pdata and > cmdline > 2. Nevertheless, we are not passing partitions thru pdata in none > of our platforms. > > I think we need to change this and pass partitions thru pdata. This would be cleaner, actually I am using the same driver on the U300 (also for Nomadik the day I find some time to adopt it) and I get the default partitions from some SPEAr platform currently, I think I should rather get some warning message from the driver or MTD core like "you haven't defined any partitions". Linus Walleij
On Tue, Mar 13, 2012 at 12:56 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Mar 13, 2012 at 10:53 AM, Armando Visconti > >> Actually I checked better and I can say that: >> >> 1. We do have the provision to pass the partitions thru pdata and >> cmdline Oh. no, wait. The driver is broken, I just realized: commit 0d04eda1430e9a796214bee644b7e05d99cfe613 "mtd: fsmc_nand.c: use mtd_device_parse_register" broke the ability to pass in partitions from platform data and I did not realize until now. :-/ > (...) and I get the default partitions from some SPEAr platform > currently, This is just because of the above bug. I'll fix! Yours, Linus Walleij
On Tue, 2012-03-13 at 13:39 +0100, Linus Walleij wrote: > On Tue, Mar 13, 2012 at 12:56 PM, Linus Walleij > <linus.walleij@linaro.org> wrote: > > On Tue, Mar 13, 2012 at 10:53 AM, Armando Visconti > > > >> Actually I checked better and I can say that: > >> > >> 1. We do have the provision to pass the partitions thru pdata and > >> cmdline > > Oh. no, wait. The driver is broken, I just realized: > commit 0d04eda1430e9a796214bee644b7e05d99cfe613 > "mtd: fsmc_nand.c: use mtd_device_parse_register" broke > the ability to pass in partitions from platform data and I did > not realize until now. :-/ > > > (...) and I get the default partitions from some SPEAr platform > > currently, > > This is just because of the above bug. Yeah, there were many bug-fixes like for this stuff. Please, base your patch on top of the l2-mtd.git tree: git://git.infradead.org/users/dedekind/l2-mtd.git and add Cc: stable@kernel.org [3.2+] Thanks!
On Tuesday 13 March 2012 12:56:47 Linus Walleij wrote: > On Tue, Mar 13, 2012 at 10:53 AM, Armando Visconti > > <armando.visconti@st.com> wrote: > > Actually I checked better and I can say that: > > > > 1. We do have the provision to pass the partitions thru pdata and > > cmdline > > 2. Nevertheless, we are not passing partitions thru pdata in none > > of our platforms. > > > > I think we need to change this and pass partitions thru pdata. > > This would be cleaner, actually I am using the same driver on the > U300 (also for Nomadik the day I find some time to adopt it) > and I get the default partitions from some SPEAr platform > currently, I think I should rather get some warning message from > the driver or MTD core like "you haven't defined any partitions". I have a patch in my queue to add device-tree support to the FSMC driver. This will also add parsing partition via DT. As SPEAr currently doesn't support this driver in mainline, I suggest not to add pdata support to those SPEAr platforms, but move to DT directly. Thanks, Stefan
On Tuesday 13 March 2012 13:39:32 Linus Walleij wrote: > > (...) and I get the default partitions from some SPEAr platform > > currently, > > This is just because of the above bug. > > I'll fix! I already sent a fix for this: mtd: fsmc_nand.c: Partition info from pdata overrides driver defaults Thanks, Stefan
On Tue, Mar 13, 2012 at 1:53 PM, Stefan Roese <sr@denx.de> wrote: > On Tuesday 13 March 2012 13:39:32 Linus Walleij wrote: >> > (...) and I get the default partitions from some SPEAr platform >> > currently, >> >> This is just because of the above bug. >> >> I'll fix! > > I already sent a fix for this: > > mtd: fsmc_nand.c: Partition info from pdata overrides driver defaults Oh sorry I was coding too quickly. Missed it previously. Thanks for fixing this. Artem, forget my patch if you like, if you already have Stefan's patch applied, please add Cc: stable@kernel.org on it because I'm using this driver... Yours, Linus Walleij
On Tue, 2012-03-13 at 14:01 +0100, Linus Walleij wrote: > On Tue, Mar 13, 2012 at 1:53 PM, Stefan Roese <sr@denx.de> wrote: > > On Tuesday 13 March 2012 13:39:32 Linus Walleij wrote: > >> > (...) and I get the default partitions from some SPEAr platform > >> > currently, > >> > >> This is just because of the above bug. > >> > >> I'll fix! > > > > I already sent a fix for this: > > > > mtd: fsmc_nand.c: Partition info from pdata overrides driver defaults > > Oh sorry I was coding too quickly. Missed it previously. > Thanks for fixing this. > > Artem, forget my patch if you like, if you already have Stefan's patch > applied, please add Cc: stable@kernel.org on it because I'm using > this driver... I somehow missed Stefan's patch, and was not pinged, unfortunately. I see it in my mailbox, but it does not apply anymore because it conflicts with patches from st.com which I applied recently. Stefan, would you re-send a new version and add: Cc: stable@kernel.org [3.2+] Sorry for missing your patch. Additional notes about -stable. When Greg will try to push this patch to -stable, the patch won't apply, and he traditionally sends an e-mail in such cases with something like "if you care, send me an applicable version, or I'll drop this patch". He usually adds people who signed-off the patch to CC. So to make sure Linus W. gets this patch in -stable, we need to add his Signed-off by as well, and then he'll need to respond to Greg.
Artem, On Tuesday 13 March 2012 14:19:40 Artem Bityutskiy wrote: > > Oh sorry I was coding too quickly. Missed it previously. > > Thanks for fixing this. > > > > Artem, forget my patch if you like, if you already have Stefan's patch > > applied, please add Cc: stable@kernel.org on it because I'm using > > this driver... > > I somehow missed Stefan's patch, and was not pinged, unfortunately. I > see it in my mailbox, but it does not apply anymore because it conflicts > with patches from st.com which I applied recently. > > Stefan, would you re-send a new version and add: > > Cc: stable@kernel.org [3.2+] > > Sorry for missing your patch. No problem. But the patch still applies clean onto kernel.org. I suspect that the conflicts are with your l2-mtd.git tree, correct? Will you push the l2 tree still into v3.3? If not, then this patch could be applied as is into mainline. I'm confused. ;) Please advise. Thanks, Stefan
On Tue, 2012-03-13 at 14:36 +0100, Stefan Roese wrote: > No problem. > > But the patch still applies clean onto kernel.org. OK. > I suspect that the > conflicts are with your l2-mtd.git tree, correct? Right. > Will you push the l2 tree > still into v3.3? No. > If not, then this patch could be applied as is into mainline. Well, if you can make this happen, fine, but I cannot help here.
On 3/13/2012 7:37 PM, Artem Bityutskiy wrote: > On Tue, 2012-03-13 at 14:36 +0100, Stefan Roese wrote: >> No problem. >> >> But the patch still applies clean onto kernel.org. > > OK. > I am sorry for pitching in too late. It seems that Stefan, Linus and I have sent the same patch (in the same order) achieving same result. It was a mistake on my part to not have rebased on either of these patches. Artem, please suggest how to proceed as there would be conflicts if you try to apply Stefan's patch before my patch-set >> I suspect that the >> conflicts are with your l2-mtd.git tree, correct? > > Right. > >> Will you push the l2 tree >> still into v3.3? > > No. > So, these changes would be merged in next release Am I right Regards Vipin
On Wednesday 14 March 2012 07:33:36 Vipin Kumar wrote: > On 3/13/2012 7:37 PM, Artem Bityutskiy wrote: > > On Tue, 2012-03-13 at 14:36 +0100, Stefan Roese wrote: > >> No problem. > >> > >> But the patch still applies clean onto kernel.org. > > > > OK. > > I am sorry for pitching in too late. It seems that Stefan, Linus and I > have sent the same patch (in the same order) achieving same result. It > was a mistake on my part to not have rebased on either of these patches. > > Artem, please suggest how to proceed as there would be conflicts if you > try to apply Stefan's patch before my patch-set I suggest to just drop my patch then, since I still have to rebase it on-top of the l2 tree. So easiest is most likely, to apply Vipins's patchset now. Okay? Thanks, Stefan
On 3/14/2012 12:40 PM, Stefan Roese wrote: > On Wednesday 14 March 2012 07:33:36 Vipin Kumar wrote: >> On 3/13/2012 7:37 PM, Artem Bityutskiy wrote: >>> On Tue, 2012-03-13 at 14:36 +0100, Stefan Roese wrote: >>>> No problem. >>>> >>>> But the patch still applies clean onto kernel.org. >>> >>> OK. >> >> I am sorry for pitching in too late. It seems that Stefan, Linus and I >> have sent the same patch (in the same order) achieving same result. It >> was a mistake on my part to not have rebased on either of these patches. >> >> Artem, please suggest how to proceed as there would be conflicts if you >> try to apply Stefan's patch before my patch-set > > I suggest to just drop my patch then, since I still have to rebase it on-top > of the l2 tree. So easiest is most likely, to apply Vipins's patchset now. > > Okay? > Thanks Stefan > Thanks, > Stefan
On Wed, Mar 14, 2012 at 8:10 AM, Stefan Roese <sr@denx.de> wrote: > On Wednesday 14 March 2012 07:33:36 Vipin Kumar wrote: >> Artem, please suggest how to proceed as there would be conflicts if you >> try to apply Stefan's patch before my patch-set > > I suggest to just drop my patch then, since I still have to rebase it on-top > of the l2 tree. So easiest is most likely, to apply Vipins's patchset now. I agree, use Vipin's stuff! Yours, Linus Walleij
On Wed, 2012-03-14 at 09:57 +0100, Linus Walleij wrote: > On Wed, Mar 14, 2012 at 8:10 AM, Stefan Roese <sr@denx.de> wrote: > > On Wednesday 14 March 2012 07:33:36 Vipin Kumar wrote: > >> Artem, please suggest how to proceed as there would be conflicts if you > >> try to apply Stefan's patch before my patch-set > > > > I suggest to just drop my patch then, since I still have to rebase it on-top > > of the l2 tree. So easiest is most likely, to apply Vipins's patchset now. > > I agree, use Vipin's stuff! Could you guys signe-off that patch to make sure you get Greg's complaint later on and send him the version for -stable?
On Wed, Mar 14, 2012 at 12:02 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Wed, 2012-03-14 at 09:57 +0100, Linus Walleij wrote: >> >> I agree, use Vipin's stuff! > > Could you guys signe-off that patch to make sure you get Greg's > complaint later on and send him the version for -stable? OK I think I understand what you want... I'll reply to Vipin's patch with SoB? Linus
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c index 4a018d0..e9b2265 100644 --- a/drivers/mtd/nand/fsmc_nand.c +++ b/drivers/mtd/nand/fsmc_nand.c @@ -284,17 +284,17 @@ static struct mtd_partition partition_info_16KB_blk[] = { }, { .name = "U-Boot", - .offset = 0x10000, + .offset = 4*0x4000, .size = 20*0x4000, }, { .name = "Kernel", - .offset = 0x60000, + .offset = (4+20)*0x4000, .size = 256*0x4000, }, { .name = "Root File System", - .offset = 0x460000, + .offset = (4+20+256)*0x4000, .size = MTDPART_SIZ_FULL, }, }; @@ -303,6 +303,29 @@ static struct mtd_partition partition_info_16KB_blk[] = { * Default partition layout for large page(> 512 bytes) devices * Size for "Root file system" is updated in driver based on actual device size */ +static struct mtd_partition partition_info_64KB_blk[] = { + { + .name = "X-loader", + .offset = 0, + .size = 4*0x10000, + }, + { + .name = "U-Boot", + .offset = 4*0x10000, + .size = 8*0x40000, + }, + { + .name = "Kernel", + .offset = (4+8)*0x10000, + .size = 64*0x40000, + }, + { + .name = "Root File System", + .offset = (4+8+64)*0x10000, + .size = MTDPART_SIZ_FULL, + }, +}; + static struct mtd_partition partition_info_128KB_blk[] = { { .name = "X-loader", @@ -311,21 +334,112 @@ static struct mtd_partition partition_info_128KB_blk[] = { }, { .name = "U-Boot", - .offset = 0x80000, + .offset = 4*0x20000, .size = 12*0x20000, }, { .name = "Kernel", - .offset = 0x200000, + .offset = (4+12)*0x20000, .size = 48*0x20000, }, { .name = "Root File System", - .offset = 0x800000, + .offset = (4+12+48)*0x20000, .size = MTDPART_SIZ_FULL, }, }; +static struct mtd_partition partition_info_256KB_blk[] = { + { + .name = "X-loader", + .offset = 0, + .size = 4*0x40000, + }, + { + .name = "U-Boot", + .offset = 4*0x40000, + .size = 6*0x40000, + }, + { + .name = "Kernel", + .offset = (4+6)*0x40000, + .size = 24*0x40000, + }, + { + .name = "Root File System", + .offset = (4+6+24)*0x40000, + .size = MTDPART_SIZ_FULL, + }, +}; + +static struct mtd_partition partition_info_512KB_blk[] = { + { + .name = "X-loader", + .offset = 0, + .size = 4*0x80000, + }, + { + .name = "U-Boot", + .offset = 4*0x80000, + .size = 6*0x80000, + }, + { + .name = "Kernel", + .offset = (4+6)*0x80000, + .size = 24*0x80000, + }, + { + .name = "Root File System", + .offset = (4+6+24)*0x80000, + .size = MTDPART_SIZ_FULL, + }, +}; + +static struct mtd_partition partition_info_1024KB_blk[] = { + { + .name = "X-loader", + .offset = 0, + .size = 4*0x100000, + }, + { + .name = "U-Boot", + .offset = 4*0x100000, + .size = 4*0x100000, + }, + { + .name = "Kernel", + .offset = (4+4)*0x100000, + .size = 12*0x100000, + }, + { + .name = "Root File System", + .offset = (4+4+12)*0x100000, + .size = MTDPART_SIZ_FULL, + }, +}; + +static struct mtd_partition partition_info_2048KB_blk[] = { + { + .name = "X-loader", + .offset = 0, + .size = 4*0x200000, + }, + { + .name = "U-Boot", + .offset = 4*0x200000, + .size = 4*0x200000, + }, + { + .name = "Kernel", + .offset = (4+4)*0x200000, + .size = 6*0x200000, + }, + { + .name = "Root File System", + .offset = (4+4+6)*0x200000, + .size = MTDPART_SIZ_FULL, + }, +}; /** * struct fsmc_nand_data - structure for FSMC NAND device state @@ -726,6 +840,8 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) struct nand_chip *nand; struct fsmc_regs *regs; struct resource *res; + struct mtd_partition *parts; + int nr_parts; int ret = 0; u32 pid; int i; @@ -940,13 +1056,41 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) * Check for partition info passed */ host->mtd.name = "nand"; - ret = mtd_device_parse_register(&host->mtd, NULL, 0, - host->mtd.size <= 0x04000000 ? - partition_info_16KB_blk : - partition_info_128KB_blk, - host->mtd.size <= 0x04000000 ? - ARRAY_SIZE(partition_info_16KB_blk) : - ARRAY_SIZE(partition_info_128KB_blk)); + + if (host->mtd.erasesize == 0x200000) { + parts = partition_info_2048KB_blk; + nr_parts = ARRAY_SIZE(partition_info_2048KB_blk); + + } else if (host->mtd.erasesize == 0x100000) { + parts = partition_info_1024KB_blk; + nr_parts = ARRAY_SIZE(partition_info_1024KB_blk); + + } else if (host->mtd.erasesize == 0x80000) { + parts = partition_info_512KB_blk; + nr_parts = ARRAY_SIZE(partition_info_512KB_blk); + + } else if (host->mtd.erasesize == 0x40000) { + parts = partition_info_256KB_blk; + nr_parts = ARRAY_SIZE(partition_info_256KB_blk); + + } else if (host->mtd.erasesize == 0x20000) { + parts = partition_info_128KB_blk; + nr_parts = ARRAY_SIZE(partition_info_128KB_blk); + + } else if (host->mtd.erasesize == 0x10000) { + parts = partition_info_64KB_blk; + nr_parts = ARRAY_SIZE(partition_info_64KB_blk); + + } else if (host->mtd.erasesize == 0x4000) { + parts = partition_info_16KB_blk; + nr_parts = ARRAY_SIZE(partition_info_16KB_blk); + + } else { + dev_err(&pdev->dev, "Unrecognized erase size\n"); + goto err_probe; + } + + ret = mtd_device_parse_register(&host->mtd, NULL, 0, parts, nr_parts); if (ret) goto err_probe;