From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <4BFBA4BA.9020502@compulab.co.il> Date: Tue, 25 May 2010 13:21:46 +0300 From: Mike Rapoport MIME-Version: 1.0 To: Lei Wen Subject: Re: [PATCH 01/20] mtd: pxa3xx_nand: refuse the flash definition get from platform References: <4BFA2B5B.4080105@compulab.co.il> <4BFA5C58.7050109@compulab.co.il> <4BFA7D60.6020703@compulab.co.il> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Eric Miao , David Woodhouse , Haojian Zhuang , Marek Vasut , linux-mtd@lists.infradead.org, Marc Kleine-Budde , David Woodhouse , linux-arm-kernel List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Lei Wen wrote: > On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport wrote: >> Lei Wen wrote: >>> Hi Mike, >>> >>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport >>> wrote: >>>> Hi Lei, >>>> >>>> Lei Wen wrote: >>>>> Hi Mike, >>>>> >>>> 2) I don't like hadrcoding of NAND parameters into the driver. You remove >>>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and >>>> instead >>>> you enforce use of built-in definitions. The driver in its current state >>>> is >>>> robust enough to allow platforms to define optimized NAND timings either >>>> in >>>> the bootloader or in the kernel. If you don't like that multiple >>>> platforms >>>> define the same flash chip create an enumeration of built-in types and >>>> let >>>> platforms to use this enumeration to select the NAND chip. But, anyway, >>>> there should be a fallback mode that will support NAND chips that are not >>>> defined in the driver, probably with suboptimal timings. >>> Original driver also use hardcoding. And in bootloader, this timing >>> parameter is also hard coding. >>> We cannot deduce a parameter set only from the nand id, that is why we >>> need a table to preset it. >>> If one nand chip is not listed in that table, the chip id would still >>> be printed out, so that we can do something for that. >>> If we encourage people to continue on this, we would not able to >>> really "driver" that nand. >> Currently pxa3xx-nand has three operational modes: >> - use NAND parameters supplied by the platform >> - use presets configured by the bootloader chain >> - use built-in NAND parameters, marked as deprecated in favor of the first >> two >> You remove the first two modes completely and require that each and every >> NAND chip used on pxa3xx based platform will be added to the driver. This >> way you make the driver less robust and harder to use for platform >> developers, not mentioning you're breaking the existing platforms. >> In my opinion, the driver *may* support built-in definitions for certain >> NAND flashes and *must* support configuration of the NAND parameters by the >> platform code and bootloader. >> > > Hi Mike, > > Well... I would submit another patch set which would reserve a way > that platform could pass its parameter setting. > Like specify the certain type of nand chip parameter for each chip > select. Is that ok for you? > > For bootloader pass parameter method, I think this way should be > dropped... For there is attributed which we could not tell from > registers... > What do you think of this? Can you please elaborate what are the attributes that cannot be detected? > Thanks, > Lei -- Sincerely yours, Mike. From mboxrd@z Thu Jan 1 00:00:00 1970 From: mike@compulab.co.il (Mike Rapoport) Date: Tue, 25 May 2010 13:21:46 +0300 Subject: [PATCH 01/20] mtd: pxa3xx_nand: refuse the flash definition get from platform In-Reply-To: References: <4BFA2B5B.4080105@compulab.co.il> <4BFA5C58.7050109@compulab.co.il> <4BFA7D60.6020703@compulab.co.il> Message-ID: <4BFBA4BA.9020502@compulab.co.il> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Lei Wen wrote: > On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport wrote: >> Lei Wen wrote: >>> Hi Mike, >>> >>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport >>> wrote: >>>> Hi Lei, >>>> >>>> Lei Wen wrote: >>>>> Hi Mike, >>>>> >>>> 2) I don't like hadrcoding of NAND parameters into the driver. You remove >>>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and >>>> instead >>>> you enforce use of built-in definitions. The driver in its current state >>>> is >>>> robust enough to allow platforms to define optimized NAND timings either >>>> in >>>> the bootloader or in the kernel. If you don't like that multiple >>>> platforms >>>> define the same flash chip create an enumeration of built-in types and >>>> let >>>> platforms to use this enumeration to select the NAND chip. But, anyway, >>>> there should be a fallback mode that will support NAND chips that are not >>>> defined in the driver, probably with suboptimal timings. >>> Original driver also use hardcoding. And in bootloader, this timing >>> parameter is also hard coding. >>> We cannot deduce a parameter set only from the nand id, that is why we >>> need a table to preset it. >>> If one nand chip is not listed in that table, the chip id would still >>> be printed out, so that we can do something for that. >>> If we encourage people to continue on this, we would not able to >>> really "driver" that nand. >> Currently pxa3xx-nand has three operational modes: >> - use NAND parameters supplied by the platform >> - use presets configured by the bootloader chain >> - use built-in NAND parameters, marked as deprecated in favor of the first >> two >> You remove the first two modes completely and require that each and every >> NAND chip used on pxa3xx based platform will be added to the driver. This >> way you make the driver less robust and harder to use for platform >> developers, not mentioning you're breaking the existing platforms. >> In my opinion, the driver *may* support built-in definitions for certain >> NAND flashes and *must* support configuration of the NAND parameters by the >> platform code and bootloader. >> > > Hi Mike, > > Well... I would submit another patch set which would reserve a way > that platform could pass its parameter setting. > Like specify the certain type of nand chip parameter for each chip > select. Is that ok for you? > > For bootloader pass parameter method, I think this way should be > dropped... For there is attributed which we could not tell from > registers... > What do you think of this? Can you please elaborate what are the attributes that cannot be detected? > Thanks, > Lei -- Sincerely yours, Mike.