[PATCH 4/7] mtd: nand: automate NAND timings selection

On Tue, 6 Sep 2016 17:04:58 +0200
Sascha Hauer [off-list ref] wrote:

quoted

quoted

quoted

quoted

+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	int modes, mode, ret;
+	const struct nand_data_interface *conf;
+
+	/*
+	 * First try to identify the best timings from ONFI parameters and
+	 * if the NAND does not support ONFI, fallback to the default ONFI
+	 * timing mode.
+	 */
+	modes = onfi_get_async_timing_mode(chip);
+	if (modes == ONFI_TIMING_MODE_UNKNOWN)
+		modes = GENMASK(chip->onfi_timing_mode_default, 0);
+
+	ret = -EINVAL;
+	for (mode = fls(modes) - 1; mode >= 0; mode--) {
+		conf = onfi_async_timing_mode_to_data_interface(mode);    

I'd still prefer to have conf allocated at the beginning of the
function and timings copied from
onfi_async_timing_mode_to_sdr_timings(mode), but maybe you can convince
me otherwise.    

Let me ask the other way round: If we need struct nand_data_interface to
fully describe a timing, why don't we keep an array of these in the
kernel? Having an array of struct nand_sdr_timings() means we always
have to copy it to a bigger struct to make it usable.  

Actually, the plan is to let vendor specific code tweak the timings if
needed.
Some NANDs that do not support ONFI have to pick timing mode 0 because
one of their timing is not matching the ONFI spec. I'd like to let
the door to fined-grained timing tweaking open, and this is only
possible if the chip has its own nand_data_interface object (not the
const one defined in nand_timings.c).

Also note that some timings are not statically defined (like tPROG),
and are extracted from another ONFI field, and I'd like to add them to
the nand_sdr_timings struct, which again, is only possible if the
nand_chip has its own nand_data_interface instance.  

Hm, in the current series the nand_chip has it's own nand_data_interface
instance, it's allocated in nand_find_data_interface().

Yes, but I thought you were suggesting to drop the allocation and
directly point to the static declaration returned by
onfi_async_timing_mode_to_data_interface().

quoted

quoted

quoted

quoted

@@ -759,6 +759,10 @@ struct nand_chip {
 	int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
 			int feature_addr, uint8_t *subfeature_para);
 	int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode);
+	int (*setup_data_interface)(struct mtd_info *mtd,
+				    const struct nand_data_interface *conf,
+				    bool check_only);
+
 
 	int chip_delay;
 	unsigned int options;

@@ -788,6 +792,8 @@ struct nand_chip {
 		struct nand_jedec_params jedec_params;
 	};
 
+	const struct nand_data_interface *data_iface;
+    

How about making this field non-const so that you only allocate it once
and modify it when you switch from one mode to another.    

As said above, I need two different timings. If we modify this
nand_data_interface instance twice during reset there's not much point
in storing it in struct nand_chip at all. That was one variant I tried:
Always calculcate the timing from the supported ONFI modes when we need
it in nand_reset(). I stepped away from this variant because of the
overhead.  

Yes, your device will be configured twice (first mode 0, then the
highest supported timing mode), but that does not mean you need to have
2 instances of nand_data_interface.
  
->data_iface should always be assigned to the current data interface  
config. If you reset the chip and go back to timing 0, then
chip->data_iface should be set to sdr mode timing zero, and once a
new timing mode is applied, it should be updated.

And yes, there's a small overhead (copying the nand_sdr_timings data
twice), but I'm pretty sure it's negligible compared to the whole NAND
chip init overhead.
And it's not like nand_reset() is called so regularly that it's useful
to optimize this kind of things.  

I haven't really thought about overhead in terms of burnt CPU cycles but
more about how easy it is to follow the code.

Ok.

Anyway, I do as you wish, expect a new series tomorrow ;)

Let's see how it looks like.

Thanks,

Boris