| Message ID | 1445577373-21252-3-git-send-email-anup.patel@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote: > The NAND controller on NS2 SoC is compatible with existing > BRCM IPROC NAND driver so let's enable it in NS2 DT and > NS2 SVK DT. > > This patch also fixes use of node labels in ns2-svk.dts. > > Signed-off-by: Anup Patel <anup.patel@broadcom.com> > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > --- > arch/arm64/boot/dts/broadcom/ns2-svk.dts | 30 ++++++++++++++++++++---------- > arch/arm64/boot/dts/broadcom/ns2.dtsi | 14 ++++++++++++++ > 2 files changed, 34 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts > index e5950d5..6bb3d4d 100644 > --- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts > +++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts > @@ -50,18 +50,28 @@ > device_type = "memory"; > reg = <0x000000000 0x80000000 0x00000000 0x40000000>; > }; > +}; > > - soc: soc { > - i2c0: i2c@66080000 { > - status = "ok"; > - }; > +&i2c0 { > + status = "ok"; > +}; > > - i2c1: i2c@660b0000 { > - status = "ok"; > - }; > +&i2c1 { > + status = "ok"; > +}; > + > +&uart3 { > + status = "ok"; > +}; > > - uart3: serial@66130000 { > - status = "ok"; > - }; > +&nand { > + nandcs@0 { > + compatible = "brcm,nandcs"; > + reg = <0>; > + nand-ecc-mode = "hw"; > + nand-ecc-strength = <8>; > + nand-ecc-step-size = <512>; > + #address-cells = <1>; > + #size-cells = <1>; > }; > }; > diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi > index f603277..9610822 100644 > --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi > +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi > @@ -212,5 +212,19 @@ > compatible = "brcm,iproc-rng200"; > reg = <0x66220000 0x28>; > }; > + > + nand: nand@66460000 { > + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1"; Technically, the binding says you should also have "brcm,brcmnand" as a last resort. Otherwise (for the NAND parts): Reviewed-by: Brian Norris <computersforpeace@gmail.com> > + reg = <0x66460000 0x600>, > + <0x67015408 0x600>, > + <0x66460f00 0x20>; > + reg-names = "nand", "iproc-idm", "iproc-ext"; > + interrupts = <GIC_SPI 420 IRQ_TYPE_LEVEL_HIGH>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + brcm,nand-has-wp; > + }; > }; > }; > -- > 1.9.1 >
On 10/27/2015 5:19 PM, Brian Norris wrote: > On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote: >> The NAND controller on NS2 SoC is compatible with existing >> BRCM IPROC NAND driver so let's enable it in NS2 DT and >> NS2 SVK DT. >> >> This patch also fixes use of node labels in ns2-svk.dts. >> >> Signed-off-by: Anup Patel <anup.patel@broadcom.com> >> Reviewed-by: Ray Jui <rjui@broadcom.com> >> Reviewed-by: Scott Branden <sbranden@broadcom.com> >> --- >> arch/arm64/boot/dts/broadcom/ns2-svk.dts | 30 ++++++++++++++++++++---------- >> arch/arm64/boot/dts/broadcom/ns2.dtsi | 14 ++++++++++++++ >> 2 files changed, 34 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts >> index e5950d5..6bb3d4d 100644 >> --- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts >> +++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts >> @@ -50,18 +50,28 @@ >> device_type = "memory"; >> reg = <0x000000000 0x80000000 0x00000000 0x40000000>; >> }; >> +}; >> >> - soc: soc { >> - i2c0: i2c@66080000 { >> - status = "ok"; >> - }; >> +&i2c0 { >> + status = "ok"; >> +}; >> >> - i2c1: i2c@660b0000 { >> - status = "ok"; >> - }; >> +&i2c1 { >> + status = "ok"; >> +}; >> + >> +&uart3 { >> + status = "ok"; >> +}; >> >> - uart3: serial@66130000 { >> - status = "ok"; >> - }; >> +&nand { >> + nandcs@0 { >> + compatible = "brcm,nandcs"; >> + reg = <0>; >> + nand-ecc-mode = "hw"; >> + nand-ecc-strength = <8>; >> + nand-ecc-step-size = <512>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> }; >> }; >> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi >> index f603277..9610822 100644 >> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi >> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi >> @@ -212,5 +212,19 @@ >> compatible = "brcm,iproc-rng200"; >> reg = <0x66220000 0x28>; >> }; >> + >> + nand: nand@66460000 { >> + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1"; > > Technically, the binding says you should also have "brcm,brcmnand" as a > last resort. Otherwise (for the NAND parts): > I believe Anup was seeing issues when both "brcm,nand-iproc" and "brcm,brcmnand" are present. Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls 'brcmnand_probe' in the end. "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls 'brcmstb_probe', but without all the prep configuration required for "brcm,nand-iproc". > Reviewed-by: Brian Norris <computersforpeace@gmail.com>
On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote: > On 10/27/2015 5:19 PM, Brian Norris wrote: > >On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote: > >>diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi > >>index f603277..9610822 100644 > >>--- a/arch/arm64/boot/dts/broadcom/ns2.dtsi > >>+++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi > >>@@ -212,5 +212,19 @@ > >> compatible = "brcm,iproc-rng200"; > >> reg = <0x66220000 0x28>; > >> }; > >>+ > >>+ nand: nand@66460000 { > >>+ compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1"; > > > >Technically, the binding says you should also have "brcm,brcmnand" as a > >last resort. Otherwise (for the NAND parts): > > > > I believe Anup was seeing issues when both "brcm,nand-iproc" and > "brcm,brcmnand" are present. > > Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls > 'brcmnand_probe' in the end. > > "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls > 'brcmstb_probe', but without all the prep configuration required for > "brcm,nand-iproc". Ah, I forgot about that problem. That seems like an OF infrastructure issue that could be fixed. We could lump these drivers back together, and make sure that "brcm,nand-iproc" gets the priority in the of_device_id list. Or we could just relax the DT binding. But wait, wouldn't cygnus already have that problem? You're using the binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi. Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile: # link order matters; don't link the more generic brcmstb_nand.o before the # more specific iproc_nand.o, for instance Brian
On 10/27/2015 5:39 PM, Brian Norris wrote: > On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote: >> On 10/27/2015 5:19 PM, Brian Norris wrote: >>> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote: >>>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>> index f603277..9610822 100644 >>>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>> @@ -212,5 +212,19 @@ >>>> compatible = "brcm,iproc-rng200"; >>>> reg = <0x66220000 0x28>; >>>> }; >>>> + >>>> + nand: nand@66460000 { >>>> + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1"; >>> >>> Technically, the binding says you should also have "brcm,brcmnand" as a >>> last resort. Otherwise (for the NAND parts): >>> >> >> I believe Anup was seeing issues when both "brcm,nand-iproc" and >> "brcm,brcmnand" are present. >> >> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls >> 'brcmnand_probe' in the end. >> >> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls >> 'brcmstb_probe', but without all the prep configuration required for >> "brcm,nand-iproc". > > Ah, I forgot about that problem. That seems like an OF infrastructure > issue that could be fixed. We could lump these drivers back together, > and make sure that "brcm,nand-iproc" gets the priority in the > of_device_id list. > > Or we could just relax the DT binding. > > But wait, wouldn't cygnus already have that problem? You're using the > binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi. Interestingly, we do not see this problem with Cygnus or NSP, but only on NS2 (arm64 based). There may be a difference between how OF devices are instantiated between arm and arm64? > > Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile: > > # link order matters; don't link the more generic brcmstb_nand.o before the > # more specific iproc_nand.o, for instance Yes, I see that too (after sending out my previous email, :)). Maybe Anup can help to elaborate on the problem. I'm now getting a bit confused on how the problem can surface on NS2. But in general, I think it's a good idea to relax the requirement in the DT binding document to not require "brcm,brcmnand", in the case when "brcm,nand-iproc" and "brcm,nand-bcm63138" are present. > > Brian > Thanks, Ray
> -----Original Message----- > From: Ray Jui [mailto:rjui@broadcom.com] > Sent: 28 October 2015 06:17 > To: Brian Norris > Cc: Anup Patel; David Woodhouse; Linux MTD; Rob Herring; Pawel Moll; Mark > Rutland; Catalin Marinas; Will Deacon; Sudeep Holla; Ian Campbell; Kumar Gala; > Scott Branden; Florian Fainelli; Pramod Kumar; Vikram Prakash; Sandeep > Tripathy; Linux ARM Kernel; Device Tree; Linux Kernel; bcm-kernel-feedback-list > Subject: Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for > NS2 > > > > On 10/27/2015 5:39 PM, Brian Norris wrote: > > On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote: > >> On 10/27/2015 5:19 PM, Brian Norris wrote: > >>> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote: > >>>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi > >>>> b/arch/arm64/boot/dts/broadcom/ns2.dtsi > >>>> index f603277..9610822 100644 > >>>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi > >>>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi > >>>> @@ -212,5 +212,19 @@ > >>>> compatible = "brcm,iproc-rng200"; > >>>> reg = <0x66220000 0x28>; > >>>> }; > >>>> + > >>>> + nand: nand@66460000 { > >>>> + compatible = "brcm,nand-iproc", "brcm,brcmnand- > v6.1"; > >>> > >>> Technically, the binding says you should also have "brcm,brcmnand" > >>> as a last resort. Otherwise (for the NAND parts): > >>> > >> > >> I believe Anup was seeing issues when both "brcm,nand-iproc" and > >> "brcm,brcmnand" are present. > >> > >> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls > >> 'brcmnand_probe' in the end. > >> > >> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls > >> 'brcmstb_probe', but without all the prep configuration required for > >> "brcm,nand-iproc". > > > > Ah, I forgot about that problem. That seems like an OF infrastructure > > issue that could be fixed. We could lump these drivers back together, > > and make sure that "brcm,nand-iproc" gets the priority in the > > of_device_id list. > > > > Or we could just relax the DT binding. > > > > But wait, wouldn't cygnus already have that problem? You're using the > > binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi. > > Interestingly, we do not see this problem with Cygnus or NSP, but only on NS2 > (arm64 based). There may be a difference between how OF devices are > instantiated between arm and arm64? Alternately, it could be also about order in-which platform drivers are matched for newly created OF device. > > > > > Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile: > > > > # link order matters; don't link the more generic brcmstb_nand.o before the > > # more specific iproc_nand.o, for instance > > Yes, I see that too (after sending out my previous email, :)). Maybe > Anup can help to elaborate on the problem. I'm now getting a bit > confused on how the problem can surface on NS2. I think for a newly created OF devices the Linux device driver framework will match the platform drivers in the order in which they are registered by module init functions. Now the order of module init function calls will be based how the .initcall section is formed by linker and order in which objects are linked. IMHO, if multiple platform drivers match given OF device then platform driver with longest matching compatible string should only be probed. I don't know how big change this would be for OF framework. > > But in general, I think it's a good idea to relax the requirement in the > DT binding document to not require "brcm,brcmnand", in the case when > "brcm,nand-iproc" and "brcm,nand-bcm63138" are present. Even I think, it will be good to relax the DT bindings requirement for BRCM NAND driver. Regards, Anup
On 10/28/2015 2:06 AM, Anup Patel wrote: > > >> -----Original Message----- >> From: Ray Jui [mailto:rjui@broadcom.com] >> Sent: 28 October 2015 06:17 >> To: Brian Norris >> Cc: Anup Patel; David Woodhouse; Linux MTD; Rob Herring; Pawel Moll; Mark >> Rutland; Catalin Marinas; Will Deacon; Sudeep Holla; Ian Campbell; Kumar Gala; >> Scott Branden; Florian Fainelli; Pramod Kumar; Vikram Prakash; Sandeep >> Tripathy; Linux ARM Kernel; Device Tree; Linux Kernel; bcm-kernel-feedback-list >> Subject: Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for >> NS2 >> >> >> >> On 10/27/2015 5:39 PM, Brian Norris wrote: >>> On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote: >>>> On 10/27/2015 5:19 PM, Brian Norris wrote: >>>>> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote: >>>>>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>>>> b/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>>>> index f603277..9610822 100644 >>>>>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>>>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>>>> @@ -212,5 +212,19 @@ >>>>>> compatible = "brcm,iproc-rng200"; >>>>>> reg = <0x66220000 0x28>; >>>>>> }; >>>>>> + >>>>>> + nand: nand@66460000 { >>>>>> + compatible = "brcm,nand-iproc", "brcm,brcmnand- >> v6.1"; >>>>> >>>>> Technically, the binding says you should also have "brcm,brcmnand" >>>>> as a last resort. Otherwise (for the NAND parts): >>>>> >>>> >>>> I believe Anup was seeing issues when both "brcm,nand-iproc" and >>>> "brcm,brcmnand" are present. >>>> >>>> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls >>>> 'brcmnand_probe' in the end. >>>> >>>> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls >>>> 'brcmstb_probe', but without all the prep configuration required for >>>> "brcm,nand-iproc". >>> >>> Ah, I forgot about that problem. That seems like an OF infrastructure >>> issue that could be fixed. We could lump these drivers back together, >>> and make sure that "brcm,nand-iproc" gets the priority in the >>> of_device_id list. >>> >>> Or we could just relax the DT binding. >>> >>> But wait, wouldn't cygnus already have that problem? You're using the >>> binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi. >> >> Interestingly, we do not see this problem with Cygnus or NSP, but only on NS2 >> (arm64 based). There may be a difference between how OF devices are >> instantiated between arm and arm64? > > Alternately, it could be also about order in-which platform drivers are matched > for newly created OF device. > >> >>> >>> Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile: >>> >>> # link order matters; don't link the more generic brcmstb_nand.o before the >>> # more specific iproc_nand.o, for instance >> >> Yes, I see that too (after sending out my previous email, :)). Maybe >> Anup can help to elaborate on the problem. I'm now getting a bit >> confused on how the problem can surface on NS2. > > I think for a newly created OF devices the Linux device driver framework will > match the platform drivers in the order in which they are registered by module > init functions. Now the order of module init function calls will be based how > the .initcall section is formed by linker and order in which objects are linked. > Yes, what you said is my understanding as well, but then here is the mystery. This is the link order in brcmnand/Makefile: 1 # link order matters; don't link the more generic brcmstb_nand.o before the 2 # more specific iproc_nand.o, for instance 3 obj-$(CONFIG_MTD_NAND_BRCMNAND) += iproc_nand.o 4 obj-$(CONFIG_MTD_NAND_BRCMNAND) += bcm63138_nand.o 5 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o 6 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o Based on the order above, probe from iproc_nand should always be called first if a matching compatible string is found. If so, then why having both compatible strings "brcm,brcmnand" and "brcm,nand-iproc" causes issues for NS2 (I remember it broke smoketest in the past when you submitted the change)? I'm not saying we should have "brcm,brcmnand" for iProc devices, but I don't get why it would cause any issue. Does the order of the compatible string matter when they are assigned to the same 'compatible' property like this? compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; > IMHO, if multiple platform drivers match given OF device then platform driver > with longest matching compatible string should only be probed. I don't know > how big change this would be for OF framework. > >> >> But in general, I think it's a good idea to relax the requirement in the >> DT binding document to not require "brcm,brcmnand", in the case when >> "brcm,nand-iproc" and "brcm,nand-bcm63138" are present. > > Even I think, it will be good to relax the DT bindings requirement for > BRCM NAND driver. > > Regards, > Anup >
On 28/10/15 09:08, Ray Jui wrote: > > > On 10/28/2015 2:06 AM, Anup Patel wrote: >> >> >>> -----Original Message----- >>> From: Ray Jui [mailto:rjui@broadcom.com] >>> Sent: 28 October 2015 06:17 >>> To: Brian Norris >>> Cc: Anup Patel; David Woodhouse; Linux MTD; Rob Herring; Pawel Moll; >>> Mark >>> Rutland; Catalin Marinas; Will Deacon; Sudeep Holla; Ian Campbell; >>> Kumar Gala; >>> Scott Branden; Florian Fainelli; Pramod Kumar; Vikram Prakash; Sandeep >>> Tripathy; Linux ARM Kernel; Device Tree; Linux Kernel; >>> bcm-kernel-feedback-list >>> Subject: Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for >>> NS2 >>> >>> >>> >>> On 10/27/2015 5:39 PM, Brian Norris wrote: >>>> On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote: >>>>> On 10/27/2015 5:19 PM, Brian Norris wrote: >>>>>> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote: >>>>>>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>>>>> b/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>>>>> index f603277..9610822 100644 >>>>>>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>>>>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi >>>>>>> @@ -212,5 +212,19 @@ >>>>>>> compatible = "brcm,iproc-rng200"; >>>>>>> reg = <0x66220000 0x28>; >>>>>>> }; >>>>>>> + >>>>>>> + nand: nand@66460000 { >>>>>>> + compatible = "brcm,nand-iproc", "brcm,brcmnand- >>> v6.1"; >>>>>> >>>>>> Technically, the binding says you should also have "brcm,brcmnand" >>>>>> as a last resort. Otherwise (for the NAND parts): >>>>>> >>>>> >>>>> I believe Anup was seeing issues when both "brcm,nand-iproc" and >>>>> "brcm,brcmnand" are present. >>>>> >>>>> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls >>>>> 'brcmnand_probe' in the end. >>>>> >>>>> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls >>>>> 'brcmstb_probe', but without all the prep configuration required for >>>>> "brcm,nand-iproc". >>>> >>>> Ah, I forgot about that problem. That seems like an OF infrastructure >>>> issue that could be fixed. We could lump these drivers back together, >>>> and make sure that "brcm,nand-iproc" gets the priority in the >>>> of_device_id list. >>>> >>>> Or we could just relax the DT binding. >>>> >>>> But wait, wouldn't cygnus already have that problem? You're using the >>>> binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi. >>> >>> Interestingly, we do not see this problem with Cygnus or NSP, but >>> only on NS2 >>> (arm64 based). There may be a difference between how OF devices are >>> instantiated between arm and arm64? >> >> Alternately, it could be also about order in-which platform drivers >> are matched >> for newly created OF device. >> >>> >>>> >>>> Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile: >>>> >>>> # link order matters; don't link the more generic brcmstb_nand.o >>>> before the >>>> # more specific iproc_nand.o, for instance >>> >>> Yes, I see that too (after sending out my previous email, :)). Maybe >>> Anup can help to elaborate on the problem. I'm now getting a bit >>> confused on how the problem can surface on NS2. >> >> I think for a newly created OF devices the Linux device driver >> framework will >> match the platform drivers in the order in which they are registered >> by module >> init functions. Now the order of module init function calls will be >> based how >> the .initcall section is formed by linker and order in which objects >> are linked. >> > > Yes, what you said is my understanding as well, but then here is the > mystery. This is the link order in brcmnand/Makefile: > > 1 # link order matters; don't link the more generic brcmstb_nand.o > before the > 2 # more specific iproc_nand.o, for instance > 3 obj-$(CONFIG_MTD_NAND_BRCMNAND) += iproc_nand.o > 4 obj-$(CONFIG_MTD_NAND_BRCMNAND) += bcm63138_nand.o > 5 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o > 6 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o > > Based on the order above, probe from iproc_nand should always be called > first if a matching compatible string is found. If so, then why having > both compatible strings "brcm,brcmnand" and "brcm,nand-iproc" causes > issues for NS2 (I remember it broke smoketest in the past when you > submitted the change)? I'm not saying we should have "brcm,brcmnand" for > iProc devices, but I don't get why it would cause any issue. > > Does the order of the compatible string matter when they are assigned to > the same 'compatible' property like this? > > compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; It is possible that by the time this was designed and tested, and the baseline you are now using, there have been changes in how probing is done and resolved. I have not been able to pin point which commit would be responsible for that, but it would not surprise me that something changed in that area, especially with all the on-going on-demand probing business happening.
On Wed, Oct 28, 2015 at 09:08:02AM -0700, Ray Jui wrote: > On 10/28/2015 2:06 AM, Anup Patel wrote: > > > >I think for a newly created OF devices the Linux device driver framework will > >match the platform drivers in the order in which they are registered by module > >init functions. Now the order of module init function calls will be based how > >the .initcall section is formed by linker and order in which objects are linked. > > > > Yes, what you said is my understanding as well, but then here is the > mystery. This is the link order in brcmnand/Makefile: > > 1 # link order matters; don't link the more generic brcmstb_nand.o > before the > 2 # more specific iproc_nand.o, for instance > 3 obj-$(CONFIG_MTD_NAND_BRCMNAND) += iproc_nand.o > 4 obj-$(CONFIG_MTD_NAND_BRCMNAND) += bcm63138_nand.o > 5 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o > 6 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o > > Based on the order above, probe from iproc_nand should always be > called first if a matching compatible string is found. If so, then > why having both compatible strings "brcm,brcmnand" and > "brcm,nand-iproc" causes issues for NS2 (I remember it broke > smoketest in the past when you submitted the change)? I'm not saying > we should have "brcm,brcmnand" for iProc devices, but I don't get > why it would cause any issue. FWIW, the above hack doesn't do anything if these are built as modules, AFAICT. So I guess udev's (or similar) module rules would be another point of failure here? Not that any of us probably care too much about this driver as a module, but just throwing it out there... I have a feeling we'll have to solve this locally, by avoiding having "independent" drivers handling similar compatible properties, as I expect (despite our expectation that compatible ordering should matter) this problem will not be solved any time soon in the generic infrastructure. Or we can just use a hack (as Anup is doing) to leave off the "brcm,brcmnand" compatibility property. Unless someone has brilliant ideas, I guess we go with Anup's hack for now. Brian
On 10/30/2015 11:49 AM, Brian Norris wrote: > On Wed, Oct 28, 2015 at 09:08:02AM -0700, Ray Jui wrote: >> On 10/28/2015 2:06 AM, Anup Patel wrote: >>> >>> I think for a newly created OF devices the Linux device driver framework will >>> match the platform drivers in the order in which they are registered by module >>> init functions. Now the order of module init function calls will be based how >>> the .initcall section is formed by linker and order in which objects are linked. >>> >> >> Yes, what you said is my understanding as well, but then here is the >> mystery. This is the link order in brcmnand/Makefile: >> >> 1 # link order matters; don't link the more generic brcmstb_nand.o >> before the >> 2 # more specific iproc_nand.o, for instance >> 3 obj-$(CONFIG_MTD_NAND_BRCMNAND) += iproc_nand.o >> 4 obj-$(CONFIG_MTD_NAND_BRCMNAND) += bcm63138_nand.o >> 5 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o >> 6 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o >> >> Based on the order above, probe from iproc_nand should always be >> called first if a matching compatible string is found. If so, then >> why having both compatible strings "brcm,brcmnand" and >> "brcm,nand-iproc" causes issues for NS2 (I remember it broke >> smoketest in the past when you submitted the change)? I'm not saying >> we should have "brcm,brcmnand" for iProc devices, but I don't get >> why it would cause any issue. > > FWIW, the above hack doesn't do anything if these are built as modules, > AFAICT. So I guess udev's (or similar) module rules would be another > point of failure here? Not that any of us probably care too much about > this driver as a module, but just throwing it out there... > > I have a feeling we'll have to solve this locally, by avoiding having > "independent" drivers handling similar compatible properties, as I > expect (despite our expectation that compatible ordering should matter) > this problem will not be solved any time soon in the generic > infrastructure. > > Or we can just use a hack (as Anup is doing) to leave off the > "brcm,brcmnand" compatibility property. Unless someone has brilliant > ideas, I guess we go with Anup's hack for now. I'm fine with that, :) Ray > > Brian >
diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts index e5950d5..6bb3d4d 100644 --- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts +++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts @@ -50,18 +50,28 @@ device_type = "memory"; reg = <0x000000000 0x80000000 0x00000000 0x40000000>; }; +}; - soc: soc { - i2c0: i2c@66080000 { - status = "ok"; - }; +&i2c0 { + status = "ok"; +}; - i2c1: i2c@660b0000 { - status = "ok"; - }; +&i2c1 { + status = "ok"; +}; + +&uart3 { + status = "ok"; +}; - uart3: serial@66130000 { - status = "ok"; - }; +&nand { + nandcs@0 { + compatible = "brcm,nandcs"; + reg = <0>; + nand-ecc-mode = "hw"; + nand-ecc-strength = <8>; + nand-ecc-step-size = <512>; + #address-cells = <1>; + #size-cells = <1>; }; }; diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi index f603277..9610822 100644 --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi @@ -212,5 +212,19 @@ compatible = "brcm,iproc-rng200"; reg = <0x66220000 0x28>; }; + + nand: nand@66460000 { + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1"; + reg = <0x66460000 0x600>, + <0x67015408 0x600>, + <0x66460f00 0x20>; + reg-names = "nand", "iproc-idm", "iproc-ext"; + interrupts = <GIC_SPI 420 IRQ_TYPE_LEVEL_HIGH>; + + #address-cells = <1>; + #size-cells = <0>; + + brcm,nand-has-wp; + }; }; };