From owner-freebsd-embedded@FreeBSD.ORG Mon Nov 4 11:06:47 2013 Return-Path: Delivered-To: freebsd-embedded@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 44CBF618 for ; Mon, 4 Nov 2013 11:06:47 +0000 (UTC) (envelope-from owner-bugmaster@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 3181B2C2B for ; Mon, 4 Nov 2013 11:06:47 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.7/8.14.7) with ESMTP id rA4B6lUv048342 for ; Mon, 4 Nov 2013 11:06:47 GMT (envelope-from owner-bugmaster@FreeBSD.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.7/8.14.7/Submit) id rA4B6k7L048340 for freebsd-embedded@FreeBSD.org; Mon, 4 Nov 2013 11:06:46 GMT (envelope-from owner-bugmaster@FreeBSD.org) Date: Mon, 4 Nov 2013 11:06:46 GMT Message-Id: <201311041106.rA4B6k7L048340@freefall.freebsd.org> X-Authentication-Warning: freefall.freebsd.org: gnats set sender to owner-bugmaster@FreeBSD.org using -f From: FreeBSD bugmaster To: freebsd-embedded@FreeBSD.org Subject: Current problem reports assigned to freebsd-embedded@FreeBSD.org X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Nov 2013 11:06:47 -0000 Note: to view an individual PR, use: http://www.freebsd.org/cgi/query-pr.cgi?pr=(number). The following is a listing of current problems submitted by FreeBSD users. These represent problem reports covering all versions including experimental development code and obsolete releases. S Tracker Resp. Description -------------------------------------------------------------------------------- o misc/52256 embedded [picobsd] picobsd build script does not read in user/s o kern/42728 embedded [picobsd] many problems in src/usr.sbin/ppp/* after c 2 problems total. From owner-freebsd-embedded@FreeBSD.ORG Wed Nov 6 21:28:58 2013 Return-Path: Delivered-To: freebsd-embedded@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 02432D07; Wed, 6 Nov 2013 21:28:58 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id BAA312FC2; Wed, 6 Nov 2013 21:28:57 +0000 (UTC) Received: from vega.codepro.be (unknown [172.16.1.3]) by venus.codepro.be (Postfix) with ESMTP id B82B594B5; Wed, 6 Nov 2013 22:28:55 +0100 (CET) Received: by vega.codepro.be (Postfix, from userid 1001) id B5381DC23; Wed, 6 Nov 2013 22:28:55 +0100 (CET) Date: Wed, 6 Nov 2013 22:28:55 +0100 From: Kristof Provost To: freebsd-embedded@freebsd.org Subject: Incorrect struct onfi_params definition Message-ID: <20131106212855.GF58987@vega.codepro.be> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-PGP-Fingerprint: E114 D9EA 909E D469 8F57 17A5 7D15 91C6 9EFA F286 User-Agent: Mutt/1.5.22 (2013-10-16) Cc: Grzegorz Bernacki X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Nov 2013 21:28:58 -0000 Hi, The definition of struct onfi_params in sys/dev/nand/nand.h is incorrect. The total structure size should be 256 bytes, but it's only 176 bytes. That's because the vendor_spec array was declared as being 8 bytes, rather than the 88 bytes it should be. Clearly a typo. This patch should fix it: diff --git a/sys/dev/nand/nand.h b/sys/dev/nand/nand.h index 0d6d7b4..46b6993 100644 --- a/sys/dev/nand/nand.h +++ b/sys/dev/nand/nand.h @@ -217,7 +217,7 @@ struct onfi_params { uint8_t driver_strength_support; uint8_t res4[12]; uint16_t vendor_rev; - uint8_t vendor_spec[8]; + uint8_t vendor_spec[88]; uint16_t crc; }__attribute__((packed)); Regards, Kristof From owner-freebsd-embedded@FreeBSD.ORG Wed Nov 6 21:56:55 2013 Return-Path: Delivered-To: freebsd-embedded@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id CBB594BB for ; Wed, 6 Nov 2013 21:56:55 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from mail-ie0-f181.google.com (mail-ie0-f181.google.com [209.85.223.181]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 9B09F21C8 for ; Wed, 6 Nov 2013 21:56:55 +0000 (UTC) Received: by mail-ie0-f181.google.com with SMTP id ar20so231701iec.12 for ; Wed, 06 Nov 2013 13:56:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:subject:mime-version:content-type:from :in-reply-to:date:cc:content-transfer-encoding:message-id:references :to; bh=6aVBfmz26GQWktt49He+Si+xeQLQR6wopKMjp+VSmoY=; b=L/jNfn40Me8xD4e7vRpmAZj7QanlyM5SavKfgiXmGL6RMe3IUqgseQnKV1P61O7Ay1 AQsq/T+9dX24ELgjjrk7ct3HQsYUvz1WHrOyuTJoUXrKk4trLTXjY3H07RZ61xnY7fT8 eLbR2416XGOEuC+1ocplDp+QomyvAUYrPnG1VDcAYy+eSCHUUvaZoufmoPPzon82mgQn bXIh5D9yYpLBMmCme1iV45v3fIZ2cq5j6C7kcUwbgad9+uqylKggyV22yEl7Z7BwsmjL iozys3GSh3i2A8yewMfUsoaW5JLykfTVtjgIoZgu61bqWG3fI/IhDP6rlABeVq2AvhLK O5RA== X-Gm-Message-State: ALoCoQme/d0d+xpd3JbzsT1Yv6C9iamXP48dsDXGTBE88iklS+xYFv/jG6PfgxD/g53mVjs92Mjv X-Received: by 10.50.45.73 with SMTP id k9mr22003636igm.38.1383775009383; Wed, 06 Nov 2013 13:56:49 -0800 (PST) Received: from fusionlt2834a.int.fusionio.com ([209.119.30.70]) by mx.google.com with ESMTPSA id x6sm16287698igb.3.2013.11.06.13.56.47 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 06 Nov 2013 13:56:48 -0800 (PST) Sender: Warner Losh Subject: Re: Incorrect struct onfi_params definition Mime-Version: 1.0 (Apple Message framework v1085) Content-Type: text/plain; charset=us-ascii From: Warner Losh In-Reply-To: <20131106212855.GF58987@vega.codepro.be> Date: Wed, 6 Nov 2013 13:56:46 -0800 Content-Transfer-Encoding: 7bit Message-Id: References: <20131106212855.GF58987@vega.codepro.be> To: Kristof Provost X-Mailer: Apple Mail (2.1085) Cc: Grzegorz Bernacki , freebsd-embedded@freebsd.org X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Nov 2013 21:56:55 -0000 On Nov 6, 2013, at 1:28 PM, Kristof Provost wrote: > Hi, > > The definition of struct onfi_params in sys/dev/nand/nand.h is > incorrect. The total structure size should be 256 bytes, but it's only > 176 bytes. > > That's because the vendor_spec array was declared as being 8 bytes, > rather than the 88 bytes it should be. Clearly a typo. > > This patch should fix it: > > diff --git a/sys/dev/nand/nand.h b/sys/dev/nand/nand.h > index 0d6d7b4..46b6993 100644 > --- a/sys/dev/nand/nand.h > +++ b/sys/dev/nand/nand.h > @@ -217,7 +217,7 @@ struct onfi_params { > uint8_t driver_strength_support; > uint8_t res4[12]; > uint16_t vendor_rev; > - uint8_t vendor_spec[8]; > + uint8_t vendor_spec[88]; > uint16_t crc; > }__attribute__((packed)); > I'd add a CT_ASSERT sizeof(onfi_params) == 256 too. Warner > Regards, > Kristof > _______________________________________________ > freebsd-embedded@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-embedded > To unsubscribe, send any mail to "freebsd-embedded-unsubscribe@freebsd.org" From owner-freebsd-embedded@FreeBSD.ORG Wed Nov 6 23:00:38 2013 Return-Path: Delivered-To: freebsd-embedded@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id BA428E66; Wed, 6 Nov 2013 23:00:38 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 7E12D25E7; Wed, 6 Nov 2013 23:00:38 +0000 (UTC) Received: from vega.codepro.be (unknown [172.16.1.3]) by venus.codepro.be (Postfix) with ESMTP id 4347F9831; Thu, 7 Nov 2013 00:00:36 +0100 (CET) Received: by vega.codepro.be (Postfix, from userid 1001) id 3A97BDD22; Thu, 7 Nov 2013 00:00:35 +0100 (CET) Date: Thu, 7 Nov 2013 00:00:35 +0100 From: Kristof Provost To: Warner Losh Subject: Re: Incorrect struct onfi_params definition Message-ID: <20131106230035.GG58987@vega.codepro.be> References: <20131106212855.GF58987@vega.codepro.be> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-PGP-Fingerprint: E114 D9EA 909E D469 8F57 17A5 7D15 91C6 9EFA F286 User-Agent: Mutt/1.5.22 (2013-10-16) Cc: Grzegorz Bernacki , freebsd-embedded@freebsd.org X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Nov 2013 23:00:38 -0000 On 2013-11-06 13:56:46 (-0800), Warner Losh wrote: > On Nov 6, 2013, at 1:28 PM, Kristof Provost wrote: > > The definition of struct onfi_params in sys/dev/nand/nand.h is > > incorrect. The total structure size should be 256 bytes, but it's only > > 176 bytes. > > > > That's because the vendor_spec array was declared as being 8 bytes, > > rather than the 88 bytes it should be. Clearly a typo. > > > > This patch should fix it: > > I'd add a CT_ASSERT sizeof(onfi_params) == 256 too. > Ok, here's an updated patch: diff --git a/sys/dev/nand/nand.h b/sys/dev/nand/nand.h index 0d6d7b4..6e3fc04 100644 --- a/sys/dev/nand/nand.h +++ b/sys/dev/nand/nand.h @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -217,9 +218,10 @@ struct onfi_params { uint8_t driver_strength_support; uint8_t res4[12]; uint16_t vendor_rev; - uint8_t vendor_spec[8]; + uint8_t vendor_spec[88]; uint16_t crc; }__attribute__((packed)); +CTASSERT(sizeof(struct onfi_params) == 256); struct nand_ecc_data { int eccsize; /* Number of data bytes per ECC step */ Regards, Kristof From owner-freebsd-embedded@FreeBSD.ORG Wed Nov 6 23:59:23 2013 Return-Path: Delivered-To: freebsd-embedded@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 59B09BFF; Wed, 6 Nov 2013 23:59:23 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from mho-02-ewr.mailhop.org (mho-02-ewr.mailhop.org [204.13.248.72]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id EC69D29B1; Wed, 6 Nov 2013 23:59:22 +0000 (UTC) Received: from c-24-8-230-52.hsd1.co.comcast.net ([24.8.230.52] helo=damnhippie.dyndns.org) by mho-02-ewr.mailhop.org with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.72) (envelope-from ) id 1VeD0G-0002gb-Eo; Wed, 06 Nov 2013 23:59:16 +0000 Received: from [172.22.42.240] (revolution.hippie.lan [172.22.42.240]) by damnhippie.dyndns.org (8.14.3/8.14.3) with ESMTP id rA6NxDW5064046; Wed, 6 Nov 2013 16:59:13 -0700 (MST) (envelope-from ian@FreeBSD.org) X-Mail-Handler: Dyn Standard SMTP by Dyn X-Originating-IP: 24.8.230.52 X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information) X-MHO-User: U2FsdGVkX1/5TgYflIXJ3TxCCYuP+5P6 Subject: Re: Incorrect struct onfi_params definition From: Ian Lepore To: Kristof Provost In-Reply-To: <20131106212855.GF58987@vega.codepro.be> References: <20131106212855.GF58987@vega.codepro.be> Content-Type: multipart/mixed; boundary="=-lCegRwvccTO5hvg1FXvI" Date: Wed, 06 Nov 2013 16:59:13 -0700 Message-ID: <1383782353.31172.183.camel@revolution.hippie.lan> Mime-Version: 1.0 X-Mailer: Evolution 2.32.1 FreeBSD GNOME Team Port Cc: Grzegorz Bernacki , freebsd-embedded@FreeBSD.org X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Nov 2013 23:59:23 -0000 --=-lCegRwvccTO5hvg1FXvI Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Wed, 2013-11-06 at 22:28 +0100, Kristof Provost wrote: > Hi, > > The definition of struct onfi_params in sys/dev/nand/nand.h is > incorrect. The total structure size should be 256 bytes, but it's only > 176 bytes. > > That's because the vendor_spec array was declared as being 8 bytes, > rather than the 88 bytes it should be. Clearly a typo. > > This patch should fix it: > > diff --git a/sys/dev/nand/nand.h b/sys/dev/nand/nand.h > index 0d6d7b4..46b6993 100644 > --- a/sys/dev/nand/nand.h > +++ b/sys/dev/nand/nand.h > @@ -217,7 +217,7 @@ struct onfi_params { > uint8_t driver_strength_support; > uint8_t res4[12]; > uint16_t vendor_rev; > - uint8_t vendor_spec[8]; > + uint8_t vendor_spec[88]; > uint16_t crc; > }__attribute__((packed)); > > There's more wrong with the onfi support than just that. The biggie is that it doesn't handle packed/unaligned accesses and the endianess of the data in the struct. I've got patches from like a year ago that I've been meaning to dust off and check in. They address more than just the onfi, and they contain a mix of unrelated stuff and style changes, so there's some work involved in making them committable. I'll attach them here so you can at least get an idea what I'm talking about and see if there's anything useful for you in them. -- Ian --=-lCegRwvccTO5hvg1FXvI Content-Disposition: attachment; filename="nand1.diff" Content-Type: text/x-patch; name="nand1.diff"; charset="us-ascii" Content-Transfer-Encoding: 7bit # HG changeset patch # Date 1351015343 21600 # Branch FBSD-TSC-10-241610 # Node ID 0ae2a52fc778fff97f9c4e577d141eb6d7485061 # Parent 1d9f84bdc8b6c7fd2ab9b4ca9e9dd5ff24dad879 Checkpoint WIP on nand mid-layer code. Changes include: - Rework the routine that returns a pointer to the table of software ECC byte positions within the OOB area to support chips with unusual OOB sizes such as 218 or 224 bytes (the table for 128 byte OOB works for these but it assumes 3 bytes of ECC per 256 byte block, and in the case of an ONFI chip the params page may ask for something different). - If a usable software ECC position table isn't found, don't try to dereference the resulting NULL point, instead just skip the ECC calcs. (There should probably be some whining about this to dmesg.) - Update the fields in the onfi_params struct to reflect standards updates. - Expand the function that reads the ONFI params to include the required CRC check, signature check, and loop to find a good copy of the table if the first copy is corrupted. - Unpack (alignm) and decode (endian) the values in the onfi_params struct. - Replace a couple hard-coded DELAY()s with calls to nandbus_wait_ready(). There is probably more to do along these lines, plus there should be some way to use the read_rnb() instead of the low-level driver provides it. diff -r 1d9f84bdc8b6 -r 0ae2a52fc778 sys/dev/nand/nand.c --- a/sys/dev/nand/nand.c Tue Oct 23 11:09:33 2012 -0600 +++ b/sys/dev/nand/nand.c Tue Oct 23 12:02:23 2012 -0600 @@ -309,24 +309,25 @@ nand_get_chip_param(struct nand_chip *ch static uint16_t * default_software_ecc_positions(struct nand_chip *chip) { - struct nand_ecc_data *eccd; - eccd = &chip->nand->ecc; + /* If positions have been set already, use them. */ + if (chip->nand->ecc.eccpositions) + return (chip->nand->ecc.eccpositions); - if (eccd->eccpositions) - return (eccd->eccpositions); + /* + * XXX Note that the following logic isn't really sufficient, especially + * in the ONFI case where the number of ECC bytes can be dictated by + * values in the parameters page, and that could lead to needing more + * byte positions than exist within the tables of software-ecc defaults. + */ + if (chip->chip_geom.oob_size >= 128) + return (default_software_ecc_positions_128); + else if (chip->chip_geom.oob_size >= 64) + return (default_software_ecc_positions_64); + else if (chip->chip_geom.oob_size >= 16) + return (default_software_ecc_positions_16); - switch (chip->chip_geom.oob_size) { - case 16: - return ((uint16_t *)&default_software_ecc_positions_16); - case 64: - return ((uint16_t *)&default_software_ecc_positions_64); - case 128: - return ((uint16_t *)&default_software_ecc_positions_128); - default: - return (NULL); /* No ecc bytes positions defs available */ - } - + /* No ecc bytes positions defs available */ return (NULL); } @@ -477,7 +478,7 @@ nand_read_pages(struct nand_chip *chip, uint32_t page, num, steps = 0; int i, retval = 0, needwrite; - nand_debug(NDBG_NAND,"%p read page %x[%x]", chip, offset, len); + nand_debug(NDBG_NAND,"%p read page 0x%x len 0x%x", chip, offset, len); cg = &chip->chip_geom; eccd = &chip->nand->ecc; page = offset_to_page(cg, offset); @@ -502,7 +503,7 @@ nand_read_pages(struct nand_chip *chip, break; } - if (eccd->eccmode != NAND_ECC_NONE) { + if (eccd->eccmode != NAND_ECC_NONE && eccpos != NULL) { if (NAND_GET_ECC(chip->dev, ptr, eccd->ecccalculated, &needwrite)) { retval = ENXIO; @@ -653,7 +654,7 @@ nand_prog_pages(struct nand_chip *chip, break; } - if (eccd->eccmode != NAND_ECC_NONE) { + if (eccd->eccmode != NAND_ECC_NONE && eccpos != NULL) { if (NAND_GET_ECC(chip->dev, buf, &eccd->ecccalculated, &needwrite)) { err = ENXIO; diff -r 1d9f84bdc8b6 -r 0ae2a52fc778 sys/dev/nand/nand.h --- a/sys/dev/nand/nand.h Tue Oct 23 11:09:33 2012 -0600 +++ b/sys/dev/nand/nand.h Tue Oct 23 12:02:23 2012 -0600 @@ -176,12 +176,16 @@ struct onfi_params { uint16_t rev; uint16_t features; uint16_t optional_commands; - uint8_t res1[22]; + uint8_t res1[2]; + uint16_t extended_parameter_page_length; + uint8_t parameter_page_count; + uint8_t res2[17]; char manufacturer_name[12]; char device_model[20]; uint8_t manufacturer_id; - uint16_t date; - uint8_t res2[13]; + uint8_t manufacture_date_yy; + uint8_t manufacture_date_ww; + uint8_t res3[13]; uint32_t bytes_per_page; uint16_t spare_bytes_per_page; uint32_t bytes_per_partial_page; @@ -200,7 +204,7 @@ struct onfi_params { uint8_t bits_of_ecc; uint8_t interleaved_addr_bits; uint8_t interleaved_oper_attr; - uint8_t res3[13]; + uint8_t res4[13]; uint8_t pin_capacitance; uint16_t asynch_timing_mode_support; uint16_t asynch_prog_cache_timing_mode_support; @@ -215,9 +219,11 @@ struct onfi_params { uint16_t input_capacitance; uint8_t input_capacitance_max; uint8_t driver_strength_support; - uint8_t res4[12]; + uint16_t t_r_interleaved; + uint16_t t_adl; + uint8_t res5[8]; uint16_t vendor_rev; - uint8_t vendor_spec[8]; + uint8_t vendor_spec[88]; uint16_t crc; }; diff -r 1d9f84bdc8b6 -r 0ae2a52fc778 sys/dev/nand/nand_generic.c --- a/sys/dev/nand/nand_generic.c Tue Oct 23 11:09:33 2012 -0600 +++ b/sys/dev/nand/nand_generic.c Tue Oct 23 12:02:23 2012 -0600 @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -319,10 +320,32 @@ check_fail(device_t nandbus) return (0); } +static uint16_t +onfi_crc(const void *buf, size_t buflen) +{ + int i, j; + uint16_t crc; + const uint8_t *bufptr; + + bufptr = buf; + crc = 0x4f4e; + for (j = 0; j < buflen; j++) { + crc ^= *bufptr++ << 8; + for (i = 0; i < 8; i++) + if (crc & 0x8000) + crc = (crc << 1) ^ 0x8005; + else + crc <<= 1; + } + return crc; +} + static int onfi_read_parameter(struct nand_chip *chip, struct onfi_params *params) { device_t nandbus; + int found, sigcount, trycopy; + uint8_t buf[256]; nand_debug(NDBG_GEN,"read parameter"); @@ -339,12 +362,96 @@ onfi_read_parameter(struct nand_chip *ch if (NANDBUS_START_COMMAND(nandbus)) return (ENXIO); - NANDBUS_READ_BUFFER(nandbus, params, sizeof(struct onfi_params)); + /* + * XXX Bogus DELAY, we really need a nandbus_wait_ready() here, but it's + * not accessible from here (static to nandbus). + */ + DELAY(1000); - /* TODO */ - /* Check for signature */ - /* Check CRC */ - /* Use redundant page if necessary */ + /* + * The ONFI spec mandates a minimum of three copies of the parameter + * data, so loop up to 3 times trying to find good data. Each copy is + * validated by a signature of "ONFI" and a crc. There is a very strange + * rule that the signature is valid if any 2 of the 4 bytes are correct. + */ + for (found= 0, trycopy = 0; !found && trycopy < 3; trycopy++) { + NANDBUS_READ_BUFFER(nandbus, &buf, sizeof(buf)); + sigcount = buf[0] == 'O'; + sigcount += buf[1] == 'N'; + sigcount += buf[2] == 'F'; + sigcount += buf[3] == 'I'; + if (sigcount < 2) + continue; + if (onfi_crc(buf, 254) != le16dec(&buf[254])) + continue; + found = 1; + } + if (!found) + return (ENXIO); + + /* + * XXX Is unpacking/decoding the entire parameters page to a struct the + * right thing to do here? The only current caller allocates the + * struct, calls this function, then plucks a few values from the struct + * and frees it. A couple alternatives exist... + * - Declare the struct as __packed and let the caller use leXXdec() to + * pull values it wants out of it. + * - Declare a new struct that contains only the values of interest to + * the caller and unpack just those here. + */ + memcpy(¶ms->signature, &buf[0], sizeof(params->signature)); + params->rev = le16dec(&buf[4]); + params->features = le16dec(&buf[6]); + params->optional_commands = le16dec(&buf[8]); + memcpy(¶ms->res1, &buf[10], sizeof(params->res1)); + params->extended_parameter_page_length = le16dec(&buf[12]); + params->parameter_page_count = buf[14]; + memcpy(¶ms->res2, &buf[15], sizeof(params->res2)); + memcpy(¶ms->manufacturer_name, &buf[32], sizeof(params->manufacturer_name)); + memcpy(¶ms->device_model, &buf[44], sizeof(params->device_model)); + params->manufacturer_id = buf[64]; + params->manufacture_date_yy = buf[65]; + params->manufacture_date_ww = buf[66]; + memcpy(¶ms->res3, &buf[67], sizeof(params->res3)); + params->bytes_per_page = le32dec(&buf[80]); + params->spare_bytes_per_page = le16dec(&buf[84]); + params->bytes_per_partial_page = le32dec(&buf[86]); + params->spare_bytes_per_partial_page = le16dec(&buf[90]); + params->pages_per_block = le32dec(&buf[92]); + params->blocks_per_lun = le32dec(&buf[96]); + params->luns = buf[100]; + params->address_cycles = buf[101]; + params->bits_per_cell = buf[102]; + params->max_bad_block_per_lun = le16dec(&buf[103]); + params->block_endurance = le16dec(&buf[105]); + params->guaranteed_valid_blocks = buf[107]; + params->valid_block_endurance = le16dec(&buf[108]); + params->programs_per_page = buf[110]; + params->partial_prog_attr = buf[111]; + params->bits_of_ecc = buf[112]; + params->interleaved_addr_bits = buf[113]; + params->interleaved_oper_attr = buf[114]; + memcpy(¶ms->res4, &buf[115], sizeof(params->res4)); + params->pin_capacitance = buf[128]; + params->asynch_timing_mode_support = le16dec(&buf[129]); + params->asynch_prog_cache_timing_mode_support = le16dec(&buf[131]); + params->t_prog = le16dec(&buf[133]); + params->t_bers = le16dec(&buf[135]); + params->t_r = le16dec(&buf[137]); + params->t_ccs = le16dec(&buf[139]); + params->source_synch_timing_mode_support = le16dec(&buf[141]); + params->source_synch_feat = buf[143]; + params->clk_input_capacitance = le16dec(&buf[144]); + params->io_capacitance = le16dec(&buf[146]); + params->input_capacitance = le16dec(&buf[148]); + params->input_capacitance_max = buf[150]; + params->driver_strength_support = buf[151]; + params->t_r_interleaved = le16dec(&buf[152]); + params->t_adl = le16dec(&buf[154]); + memcpy(¶ms->res5, &buf[156], sizeof(params->res5)); + params->vendor_rev = le16dec(&buf[164]); + memcpy(¶ms->vendor_spec, &buf[166], sizeof(params->vendor_spec)); + params->crc = le16dec(&buf[254]); return (0); } diff -r 1d9f84bdc8b6 -r 0ae2a52fc778 sys/dev/nand/nandbus.c --- a/sys/dev/nand/nandbus.c Tue Oct 23 11:09:33 2012 -0600 +++ b/sys/dev/nand/nandbus.c Tue Oct 23 12:02:23 2012 -0600 @@ -331,7 +331,7 @@ static int nand_probe_onfi(device_t bus, uint8_t *onfi_compliant) { device_t nfc; - char onfi_id[] = {'o', 'n', 'f', 'i', '\0'}; + char onfi_id[] = {'O', 'N', 'F', 'I', '\0'}; int i; nand_debug(NDBG_BUS,"probing ONFI"); @@ -352,6 +352,7 @@ nand_probe_onfi(device_t bus, uint8_t *o nand_debug(NDBG_BUS,"Error : could not start command"); return (ENXIO); } + for (i = 0; onfi_id[i] != '\0'; i++) if (NFC_READ_BYTE(nfc) != onfi_id[i]) { nand_debug(NDBG_BUS,"ONFI non-compliant"); @@ -369,6 +370,8 @@ static int nand_reset(device_t bus) { device_t nfc; + uint8_t status; + nand_debug(NDBG_BUS,"resetting..."); nfc = device_get_parent(bus); @@ -383,7 +386,7 @@ nand_reset(device_t bus) return (ENXIO); } - DELAY(1000); + nandbus_wait_ready(bus, &status); return (0); } --=-lCegRwvccTO5hvg1FXvI Content-Disposition: attachment; filename="nand2.diff" Content-Type: text/x-patch; name="nand2.diff"; charset="us-ascii" Content-Transfer-Encoding: 7bit # HG changeset patch # Date 1354805229 25200 # Branch FBSD-TSC-10-241610 # Node ID 86ddfcf6723778494cd0cfcf5bda78986d127bed # Parent c909fc491c2199c5dd5609be9b42059b64e27ebe Remove all use of DELAY() in normal page and oob reads and writes. Instead use new routines wait_read_ready_status() and wait_write_done_status(). Each of these polls the chip for done status rather than assuming that a DELAY for some amount of time will be good enough. In the write paths, consitantly check for write protect and return EROFS. There are still some DELAY() calls in the "non-normal" IO functions that are wrapped in a big #if 0 block. These are the cache read/write, copyback read/write, etc. They probably need the same treatment as normal routines. diff -r c909fc491c21 -r 86ddfcf67237 sys/dev/nand/nand_generic.c --- a/sys/dev/nand/nand_generic.c Mon Nov 26 09:23:38 2012 -0700 +++ b/sys/dev/nand/nand_generic.c Thu Dec 06 07:47:09 2012 -0700 @@ -306,14 +306,44 @@ can_write(device_t nandbus) return (1); } +/* + * Wait for read-ready status. + * After sending a read command and the address for it, wait for ready status + * (which issues "read status" commands), then send the "continue read" command + * (a zero) to stop reading status and resume the original read command. + */ static int -check_fail(device_t nandbus) +wait_read_ready_status(device_t nandbus) +{ + uint8_t status; + + if (NANDBUS_WAIT_READY(nandbus, &status)) { + nand_debug(NDBG_GEN,"Waiting for read-ready timed out %#x", status); + return (ENXIO); + } + + if (NANDBUS_SEND_COMMAND(nandbus, 0)) { + nand_debug(NDBG_GEN,"Sending continue cmd after wait-ready failed"); + return (ENXIO); + } + + if (status & NAND_STATUS_FAIL) { + nand_debug(NDBG_GEN,"Waiting for read-ready failed %#x", status); + return (ENXIO); + } + + return (0); +} + +static int +wait_write_done_status(device_t nandbus) { uint8_t status; NANDBUS_WAIT_READY(nandbus, &status); + if (status & NAND_STATUS_FAIL) { - nand_debug(NDBG_GEN,"Status failed %x", status); + nand_debug(NDBG_GEN,"Waiting for write-complete failed %#x", status); return (ENXIO); } @@ -323,21 +353,21 @@ check_fail(device_t nandbus) static uint16_t onfi_crc(const void *buf, size_t buflen) { - int i, j; - uint16_t crc; - const uint8_t *bufptr; - - bufptr = buf; - crc = 0x4f4e; - for (j = 0; j < buflen; j++) { - crc ^= *bufptr++ << 8; - for (i = 0; i < 8; i++) - if (crc & 0x8000) - crc = (crc << 1) ^ 0x8005; - else - crc <<= 1; - } - return crc; + int i, j; + uint16_t crc; + const uint8_t *bufptr; + + bufptr = buf; + crc = 0x4f4e; + for (j = 0; j < buflen; j++) { + crc ^= *bufptr++ << 8; + for (i = 0; i < 8; i++) + if (crc & 0x8000) + crc = (crc << 1) ^ 0x8005; + else + crc <<= 1; + } + return crc; } static int @@ -359,16 +389,10 @@ onfi_read_parameter(struct nand_chip *ch if (nand_send_address(chip->dev, -1, -1, PAGE_PARAMETER_DEF)) return (ENXIO); - if (NANDBUS_START_COMMAND(nandbus)) + if (wait_read_ready_status(nandbus) != 0) return (ENXIO); /* - * XXX Bogus DELAY, we really need a nandbus_wait_ready() here, but it's - * not accessible from here (static to nandbus). - */ - DELAY(1000); - - /* * The ONFI spec mandates a minimum of three copies of the parameter * data, so loop up to 3 times trying to find good data. Each copy is * validated by a signature of "ONFI" and a crc. There is a very strange @@ -499,13 +523,11 @@ generic_read_page(device_t nand, uint32_ offset)) return (ENXIO); - DELAY(chip->t_r); + if (wait_read_ready_status(nandbus) != 0) + return (ENXIO); NANDBUS_READ_BUFFER(nandbus, buf, len); - if (check_fail(nandbus)) - return (ENXIO); - pg_stat = &(chip->pg_stat[page]); pg_stat->page_raw_read++; @@ -525,7 +547,7 @@ generic_read_oob(device_t nand, uint32_t nandbus = device_get_parent(nand); if (nand_check_page_boundary(chip, page)) { - nand_debug(NDBG_GEN,"page boundary check failed: %08x\n", page); + nand_debug(NDBG_GEN,"page boundary check failed: %08x", page); return (ENXIO); } @@ -537,13 +559,11 @@ generic_read_oob(device_t nand, uint32_t offset)) return (ENXIO); - DELAY(chip->t_r); + if (wait_read_ready_status(nandbus) != 0) + return (ENXIO); NANDBUS_READ_BUFFER(nandbus, buf, len); - if (check_fail(nandbus)) - return (ENXIO); - return (0); } @@ -604,9 +624,7 @@ generic_program_page(device_t nand, uint if (send_end_program_page(nandbus, NAND_CMD_PROG_END)) return (ENXIO); - DELAY(chip->t_prog); - - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); pg_stat = &(chip->pg_stat[page]); @@ -628,13 +646,16 @@ generic_program_page_intlv(device_t nand chip = device_get_softc(nand); nandbus = device_get_parent(nand); + if (!can_write(nandbus)) + return (EROFS); + if (nand_check_page_boundary(chip, page)) return (ENXIO); page_to_row(&chip->chip_geom, page, &row); if (!can_write(nandbus)) - return (ENXIO); + return (EROFS); if (send_start_program_page(nand, row, offset)) return (ENXIO); @@ -644,9 +665,7 @@ generic_program_page_intlv(device_t nand if (send_end_program_page(nandbus, NAND_CMD_PROG_INTLV)) return (ENXIO); - DELAY(chip->t_prog); - - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); pg_stat = &(chip->pg_stat[page]); @@ -675,7 +694,7 @@ generic_program_oob(device_t nand, uint3 offset += chip->chip_geom.page_size; if (!can_write(nandbus)) - return (ENXIO); + return (EROFS); if (send_start_program_page(nand, row, offset)) return (ENXIO); @@ -685,9 +704,7 @@ generic_program_oob(device_t nand, uint3 if (send_end_program_page(nandbus, NAND_CMD_PROG_END)) return (ENXIO); - DELAY(chip->t_prog); - - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); return (0); @@ -734,13 +751,11 @@ generic_erase_block(device_t nand, uint3 nand_debug(NDBG_GEN,"%p erase block row %x", nand, row); if (!can_write(nandbus)) - return (ENXIO); + return (EROFS); send_erase_block(nand, row, NAND_CMD_ERASE_END); - DELAY(chip->t_bers); - - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); blk_stat = &(chip->blk_stat[block]); @@ -768,13 +783,11 @@ generic_erase_block_intlv(device_t nand, chip->chip_geom.blk_mask; if (!can_write(nandbus)) - return (ENXIO); + return (EROFS); send_erase_block(nand, row, NAND_CMD_ERASE_INTLV); - DELAY(chip->t_bers); - - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); blk_stat = &(chip->blk_stat[block]); @@ -872,13 +885,11 @@ small_read_page(device_t nand, uint32_t return (ENXIO); } - DELAY(chip->t_r); + if (wait_read_ready_status(nandbus) != 0) + return (ENXIO); NANDBUS_READ_BUFFER(nandbus, buf, len); - if (check_fail(nandbus)) - return (ENXIO); - pg_stat = &(chip->pg_stat[page]); pg_stat->page_raw_read++; @@ -906,13 +917,11 @@ small_read_oob(device_t nand, uint32_t p if (send_small_read_page(nand, NAND_CMD_SMALLOOB, row, 0)) return (ENXIO); - DELAY(chip->t_r); + if (wait_read_ready_status(nandbus) != 0) + return (ENXIO); NANDBUS_READ_BUFFER(nandbus, buf, len); - if (check_fail(nandbus)) - return (ENXIO); - pg_stat = &(chip->pg_stat[page]); pg_stat->page_raw_read++; @@ -937,7 +946,7 @@ small_program_page(device_t nand, uint32 page_to_row(&chip->chip_geom, page, &row); if (!can_write(nandbus)) - return (ENXIO); + return (EROFS); if (offset < 256) { if (NANDBUS_SEND_COMMAND(nandbus, NAND_CMD_SMALLA)) @@ -955,9 +964,7 @@ small_program_page(device_t nand, uint32 if (send_end_program_page(nandbus, NAND_CMD_PROG_END)) return (ENXIO); - DELAY(chip->t_prog); - - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); return (0); @@ -981,7 +988,7 @@ small_program_oob(device_t nand, uint32_ page_to_row(&chip->chip_geom, page, &row); if (!can_write(nandbus)) - return (ENXIO); + return (EROFS); if (NANDBUS_SEND_COMMAND(nandbus, NAND_CMD_SMALLOOB)) return (ENXIO); @@ -994,9 +1001,7 @@ small_program_oob(device_t nand, uint32_ if (send_end_program_page(nandbus, NAND_CMD_PROG_END)) return (ENXIO); - DELAY(chip->t_prog); - - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); return (0); @@ -1184,7 +1189,7 @@ nand_copyback_read(device_t dev, uint32_ return (ENXIO); DELAY(chip->t_r); - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); if (buf != NULL && len > 0) @@ -1231,7 +1236,7 @@ nand_copyback_prog(device_t dev, uint32_ DELAY(chip->t_prog); - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); pg_stat = &(chip->pg_stat[page]); @@ -1268,7 +1273,7 @@ nand_copyback_prog_intlv(device_t dev, u DELAY(chip->t_prog); - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); pg_stat = &(chip->pg_stat[page]); @@ -1314,7 +1319,7 @@ nand_prog_cache(device_t dev, uint32_t p DELAY(chip->t_prog); - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); pg_stat = &(chip->pg_stat[page]); @@ -1362,7 +1367,7 @@ nand_read_cache(device_t dev, uint32_t p return (ENXIO); DELAY(chip->t_r); - if (check_fail(nandbus)) + if (wait_write_done_status(nandbus) != 0) return (ENXIO); if (buf != NULL && len > 0) --=-lCegRwvccTO5hvg1FXvI-- From owner-freebsd-embedded@FreeBSD.ORG Thu Nov 7 08:33:11 2013 Return-Path: Delivered-To: freebsd-embedded@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id F0EFAC2C; Thu, 7 Nov 2013 08:33:11 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id B46B92546; Thu, 7 Nov 2013 08:33:11 +0000 (UTC) Received: from vega.codepro.be (unknown [172.16.1.3]) by venus.codepro.be (Postfix) with ESMTP id 11DFD9B14; Thu, 7 Nov 2013 09:33:09 +0100 (CET) Received: by vega.codepro.be (Postfix, from userid 1001) id 0BDDCDF3E; Thu, 7 Nov 2013 09:33:09 +0100 (CET) Date: Thu, 7 Nov 2013 09:33:09 +0100 From: Kristof Provost To: Ian Lepore Subject: Re: Incorrect struct onfi_params definition Message-ID: <20131107083308.GH58987@vega.codepro.be> References: <20131106212855.GF58987@vega.codepro.be> <1383782353.31172.183.camel@revolution.hippie.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1383782353.31172.183.camel@revolution.hippie.lan> X-PGP-Fingerprint: E114 D9EA 909E D469 8F57 17A5 7D15 91C6 9EFA F286 User-Agent: Mutt/1.5.22 (2013-10-16) Cc: Grzegorz Bernacki , freebsd-embedded@FreeBSD.org X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Nov 2013 08:33:12 -0000 On 2013-11-06 16:59:13 (-0700), Ian Lepore wrote: > On Wed, 2013-11-06 at 22:28 +0100, Kristof Provost wrote: > > The definition of struct onfi_params in sys/dev/nand/nand.h is > > incorrect. The total structure size should be 256 bytes, but it's only > > 176 bytes. > > > There's more wrong with the onfi support than just that. The biggie is > that it doesn't handle packed/unaligned accesses and the endianess of > the data in the struct. I've got patches from like a year ago that I've > been meaning to dust off and check in. They address more than just the > onfi, and they contain a mix of unrelated stuff and style changes, so > there's some work involved in making them committable. > > I'll attach them here so you can at least get an idea what I'm talking > about and see if there's anything useful for you in them. > Thanks. I see these patches also fix the original problem I was chasing. I have an OpenRD board with a flash chip which claims to be ONFI but isn't. It replies 'ONFI' when I read the ID, but gives me empty parameters. The CRC check should at least detect that. I won't be able to test the ONFI case, but I'll try to pick the patches apart and submit them. Regards, Kristof