From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965013AbbBDKYR (ORCPT ); Wed, 4 Feb 2015 05:24:17 -0500 Received: from eusmtp01.atmel.com ([212.144.249.243]:33341 "EHLO eusmtp01.atmel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751292AbbBDKYN (ORCPT ); Wed, 4 Feb 2015 05:24:13 -0500 Message-ID: <54D1F338.4080005@atmel.com> Date: Wed, 4 Feb 2015 18:23:52 +0800 From: Josh Wu User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Boris Brezillon CC: Brian Norris , David Woodhouse , , Nicolas Ferre , Jean-Christophe Plagniol-Villard , Alexandre Belloni , Andrew Victor , Rob Herring , Pawel Moll , "Mark Rutland" , Ian Campbell , Kumar Gala , , , Subject: Re: [PATCH 2/4] mtd: nand: atmel: Update DT documentation after splitting NFC and NAND References: <1417732214-3292-1-git-send-email-boris.brezillon@free-electrons.com> <1417732214-3292-3-git-send-email-boris.brezillon@free-electrons.com> <20150202075737.GC3523@norris-Latitude-E6410> <20150202104236.649f99ad@bbrezillon> <54D08AD7.3050209@atmel.com> <20150203103736.3bcc222e@bbrezillon> In-Reply-To: <20150203103736.3bcc222e@bbrezillon> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.168.5.13] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Boris Thanks a lot for your explanation, check my reply for more description for my suggestion. On 2/3/2015 5:37 PM, Boris Brezillon wrote: > On Tue, 3 Feb 2015 16:46:15 +0800 > Josh Wu wrote: > >> Hi, Boris, Brian >> >> On 2/2/2015 5:42 PM, Boris Brezillon wrote: >>> Hi Brian, >>> >>> On Sun, 1 Feb 2015 23:57:37 -0800 >>> Brian Norris wrote: >>> >>>> Hi Boris, >>>> >>>> BTW, this series has a few conflicts with other things I have queued, so >>>> you'll need to refresh. >>> Yes, that's not a problem, but I'd like to be sure this is the way we >>> want to go before rebasing this series. >>> >>>> On Thu, Dec 04, 2014 at 11:30:12PM +0100, Boris Brezillon wrote: >>>>> The NAND and NFC (NAND Flash Controller) were linked together with a >>>>> parent <-> child relationship. >>>>> >>>>> This model has several drawbacks: >>>>> - it does not allow for multiple NAND chip handling while the controller >>>>> support multi-chip (even though the driver is not ready yet) >>>>> - it mixes NAND partitions and NFC nodes at the same level (which is a bit >>>>> disturbing) >>>> I agree that this is disturbing. (FWIW, it also seems a bit disturbing >>>> that atmel_nand.c actually registers two different drivers and the tries >>>> to synchronize them; this seems like it could be handled better, but I'm >>>> not sure how at the moment.) >>> Yep, that's my feeling too, but I'm not sure how this could/should be >>> done. >>> My problem here is that the pinmux should be requested by the EBI >>> device because the EBI manages several type of devices and the data and >>> address signals are shared by all the devices, hence the idea of >>> defining the nand chip node under the EBI node. >>> In the other hand, the NFC is not part of the EBI bus, and thus should >>> not be defined under the EBI node. >>> >>> This might lead to the NFC device being probed before the NAND chip, >>> hence the need for this synchronization. >> OMHO, there is another way, which is change the NFC node to many NFC >> properties, just like PMECC. >> As NFC, PMECC or hamming ecc HW could be part of current NAND node (in >> sama5, HSMC maybe a better name for this node. ) >> >> And this change can avoid the sync problem and avoid two drivers in >> atmel_nand.c. > Sorry I don't get it... > You gave a pseudo DT example in your following answers but I still > don't understand how you'll link the NFC and its associated NAND chips. > >>>>> - the introduction of the EBI bus implies defining NAND chips under the >>>>> EBI node, and the ranges available under the EBI node should be >>>>> restricted to EBI address space, while the NFC references several >>>>> registers outside of these EBI ranges. >>>> That's an interesting bit. I've actually run across this sort of problem >>>> on other SoCs, where we have a relationship between two pieces of >>>> hardware--the NAND chip and the NAND controller--where the former might >>>> be on one bus (like your EBI bus, with chip selects), and the latter is >>>> part of the top-level MMIO register space. >>>> >>>> But can you elaborate here a bit more? Does the NAND chip actually need >>>> to be represented under your EBI bus? >>> Yes, as said above this is all about pinmux conflicts, the NAND >>> controller has to request the appropriate pinmux for its NAND chips but >>> it will conflict with the pinmux requested by the EBI bus (data and >>> address signals are shared by all the devices connected on the EBI). >>> >>>>> Move the NFC node outside of the NAND node, to get a more future-proof >>>>> model. >>>> I'm curious if an alternative solution might work, maybe one like the >>>> Allwiner NAND (sunxi-nand) DT, which just reverses the roles; the 'NFC' >>>> is the parent of the NAND chip(s). We've seen this pattern in other >>>> contexts too. >> I also prefer this. Then the dt node should looks like finally: >> >> nand (SMC may be more correct) node { > This nand node contains nand chip nodes, so 'nand' is definitely not > the appropriate name for this node. > We could name it SMC, but I'd like to keep EBI (External Bus > Interface), because the only thing that can register child devices in > linux are busses (or MFD devices :-)). > The SMC (Static Memory Controller) is just a additional control logic > acting on top of the EBI. After further thought, It seems the SMC should be correct name for nand chips' parent. Before SAMA5 chips, the PMECC/ECC registers address is out of SMC address. In SAMA5 chips, the PMECC/NFC-hw registers address is in SMC address. take sama5d3 for example: NFC regs: 0xffffc000 0x00000070 PMECC regs: 0xffffc070 0x00000490 PMECC error regs: 0xffffc500 0x00000100 And the HSMC regs is: 0xffffc000 0x00000700 which include PMECC/NFC-hw registers. >> PMECC properties >> NFC properties --> we can make the NFC not a node, just many NFC >> properties. > But all NAND chips will have to point to the same nfc struct, and I'd > rather represent the NFC IP in the DT than hide it into the driver's > logic. > Moreover, the NFC IP is not part of the EBI memory range, so I'd prefer > to keep it outside of the EBI node (though I'm not sure you're trying to > represent the EBI bus here). > >> pinctrl-nand0 >> nand chip 0: { >> partitions... >> } >> >> pinctrl-nand1 >> nand chip 1: { >> partitions... >> } >> } > > Could you give a real DT example instead of a pseudo DT representation, > maybe I'll understand what you're suggesting then. > >>> I would have preferred this solution too, but the EBI/pinmux constraint >>> explained above prevents this approach. >> I am not very clear about the pinmux constraint. >> Maybe we just leave one DT node (either EBI or current nand node) to >> take care the pins. >> >>> What I can do though, is reverse the referencing: reference nand chips >>> from the nand controller node. >> I guess the dt looks like: (correct me if I am wrong) >> >> EBI node { >> pinctrl-nand0 >> nand chip 0: { >> partitions... >> } >> >> pinctrl-nand1 >> nand chip 1: { >> partitions... >> } >> } > Well, that's more someting like: > > ebi@xxxx { > pinctrl-0 = <&ebi_data_bus_pins &ebi_addr_bus_pins > &ebi_nand_cs0_pin &ebi_nand_rb0_pin ...>; > > nand@0,xxxx { > /* ../ */ > }; > > nand@1,xxxx { > /* ../ */ > } > } well, so nand driver should be probed after this ebi node probed, since ebi will configure the nand pins. There should be a sync issue to solve. or maybe I miss something? >> nand (SMC/HSMC may be more correct) node { >> PMECC properties >> NFC properties --> we can make the NFC not a node, just many NFC >> properties. >> >> &nand chip0 >> &nand chip1 >> } > Okay, I guess I understand what you were talking about in your previous > suggestion, and I'm not a big fan of this representation. > > The SMC IP provides a set of registers to configure external devices > timings (and other related stuff). > Here you're representing NAND chip devices under the SMC node, which is > not exactly how I would represent them. > The IP controlling the available NAND chips is actually the NFC (NAND > Flash Controller). > How about this representation instead ? > > nfc@xxxxx { > nand-chips = <&nand0 &nand1>; > } This should be ok, but this nfc@xxxxx should be a logic block. As the real NFC hardware only appeared since SAMA5 chips. And we can disabled it. Without the real hardware NFC the nand should also works well. > Josh, could you rework your proposal with a real DT representation so > that I'll be sure to understand what you're suggesting ? Okay, first I prefer to remove the atmel_nand_nfc driver, the work that be done in atmel_nand_nfc_probe() function will move to atmel_nand_probe(). The dt should looks like: nand0: nand@80000000 { compatible = "atmel,sama5d4-nand", "atmel,at91rm9200-nand"; #address-cells = <1>; #size-cells = <1>; ranges; reg = < 0x80000000 0x08000000 /* EBI CS3 */ 0xfc05c070 0x00000490 /* SMC PMECC regs */ 0xfc05c500 0x00000100 /* SMC PMECC Error Location regs */ 0x90000000 0x08000000 /* NFC Command Registers */ 0xfc05c000 0x00000070 /* NFC HSMC regs */ 0x00100000 0x00100000 /* NFC SRAM banks */ >; interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>; atmel,nand-addr-offset = <21>; atmel,nand-cmd-offset = <22>; atmel,nand-has-dma; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_nand>; status = "disabled"; clocks = <&hsmc_clk>; atmel,write-by-sram; }; The &hsmc_clk & atmel,write-by-sram will move to uplayer. And the hardware NFC can be disabled in menuconfig some options. or add some dt properties like atmel,enable-nfc. Then we can make use of EBI/SMC node, nfc@xxxxx { compatible = "atmel,sama5d4-nand", "atmel,at91rm9200-nand"; #address-cells = <1>; #size-cells = <1>; ranges; reg = < 0xfc05c070 0x00000490 /* SMC PMECC regs */ 0xfc05c500 0x00000100 /* SMC PMECC Error Location regs */ 0xfc05c000 0x00000070 /* NFC HSMC regs */ /* all above address will be overlay with smc regs, maybe we can use it from smc? */ 0x00100000 0x00100000 /* NFC SRAM banks */ 0x90000000 0x08000000 /* NFC Command Registers */ >; interrupts = <22 IRQ_TYPE_LEVEL_HIGH 6>; atmel,nand-addr-offset = <21>; atmel,nand-cmd-offset = <22>; atmel,nand-has-dma; clocks = <&hsmc_clk>; /* needed for all smc components, like pmecc, nfc hardware */ atmel,nfc-disabled; /* disabled hw NFC */ atmel,nfc-write-by-sram; status = "disabled"; nand-chips = <&nand0 &nand1>; } I am not familiar with the EBI/SMC dt node, so above should have errors, but it's just a draft for us to discuss. > > Thanks. > > Best Regards, > > Boris > > > Best Regards, Josh Wu