Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1213307 > unrolled thread

Re: [PATCH v10 1/5] mtd: nand: vf610_nfc: Freescale NFC for VF610, MPC5125 and others

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v10 1/5] mtd: nand: vf610_nfc: Freescale NFC for VF610,  MPC5125 and others Brian Norris <computersforpeace@gmail.com> - 2015-08-25 22:20 +0200
    Re: [PATCH v10 1/5] mtd: nand: vf610_nfc: Freescale NFC for VF610,  MPC5125 and others Stefan Agner <stefan@agner.ch> - 2015-08-27 03:10 +0200
      Re: [PATCH v10 1/5] mtd: nand: vf610_nfc: Freescale NFC for VF610,  MPC5125 and others Brian Norris <computersforpeace@gmail.com> - 2015-08-27 18:40 +0200
        Re: [PATCH v10 1/5] mtd: nand: vf610_nfc: Freescale NFC for VF610,  MPC5125 and others Stefan Agner <stefan@agner.ch> - 2015-08-27 19:30 +0200

#1213307 — Re: [PATCH v10 1/5] mtd: nand: vf610_nfc: Freescale NFC for VF610, MPC5125 and others

A few more comments.

On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> new file mode 100644
> index 0000000..5c8dfe8
> --- /dev/null
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -0,0 +1,645 @@

...

> +/*
> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
> + */
> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
> +{
> +#ifdef CONFIG_SOC_VF610

Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.

If this is trying to handle the comment above ("This function supports
Vybrid only (MPC5125 would have full RB and four CS)") then that's the
wrong way of doing it, as you need to support multiplatform kernels.
You'll need to have a way to differentiate the different platform
support at runtime, not compile time.

> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> +	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
> +
> +	tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
> +	tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
> +
> +	if (chip == 0)
> +		tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
> +	else if (chip == 1)
> +		tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;

	else ... ?

Maybe you can write this as a formulaic pattern (e.g.:

	tmp |= (chip + 1) << ROW_ADDR_CHIP_SEL_SHIFT;

) and just do the "max # of chips" checks on a per-platform basis in the
probe(). Then I'm guessing this same function can apply to both
platforms. (I'm not looking at HW datasheets for this, BTW, just
guessing based on the context here.)

But wait...I see that you call nand_scan_ident() with a max of 1 chip.
So you won't ever see the chip > 0 case, right?

So does this driver support multiple flash attached or not? Looks like
you're assuming you'll only be using chip-select 0. (This is fine for
now, but at least your code should acknowledge this. Perhaps a comment
at the top under "limitations.")

> +
> +	vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
> +#endif
> +}

...

> +static int vf610_nfc_probe(struct platform_device *pdev)
> +{

...

> +	/* first scan to find the device and get the page size */
> +	if (nand_scan_ident(mtd, 1, NULL)) {
> +		err = -ENXIO;
> +		goto error;
> +	}

...

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1214261

On 2015-08-25 13:16, Brian Norris wrote:
> A few more comments.
> 
> On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
>> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> new file mode 100644
>> index 0000000..5c8dfe8
>> --- /dev/null
>> +++ b/drivers/mtd/nand/vf610_nfc.c
>> @@ -0,0 +1,645 @@
> 
> ...
> 
>> +/*
>> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>> + */
>> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
>> +{
>> +#ifdef CONFIG_SOC_VF610
> 
> Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.
> 
> If this is trying to handle the comment above ("This function supports
> Vybrid only (MPC5125 would have full RB and four CS)") then that's the
> wrong way of doing it, as you need to support multiplatform kernels.
> You'll need to have a way to differentiate the different platform
> support at runtime, not compile time.

Yes it is trying to handle the comment above. Well, the other two
platforms I am aware of are also different architectures... (PowerPC and
ColdFire). I think we won't have a multi-architecture kernel anytime
soon, hence I think removing the code at compile time is the right thing
todo.

However, probably CONFIG_SOC_VF610 is the wrong symbol then, I could
just use CONFIG_ARM and add a comment that this might be different on
another other ARM SoC than VF610.

Just checked CodingStyle, and I see that IS_ENABLED is the preferred way
for conditional compiling.

So my suggestion:

static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
{
	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);

	if (!IS_ENABLED(CONFIG_ARM))
		return;

	/*
	 * This code is only tested on the ARM platform VF610
	 * PowerPC based MPC5125 would have full RB and four CS
	 */
....

With that the compiler should be able to remove this (currently) ARM
VF610 specific code on the other supported architectures...

What do you think?


> 
>> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
>> +
>> +	tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
>> +	tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
>> +
>> +	if (chip == 0)
>> +		tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
>> +	else if (chip == 1)
>> +		tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;
> 
> 	else ... ?
> 
> Maybe you can write this as a formulaic pattern (e.g.:
> 
> 	tmp |= (chip + 1) << ROW_ADDR_CHIP_SEL_SHIFT;
> 
> ) and just do the "max # of chips" checks on a per-platform basis in the
> probe(). Then I'm guessing this same function can apply to both
> platforms. (I'm not looking at HW datasheets for this, BTW, just
> guessing based on the context here.)

It seems that MCP5125 is different than VF610. MCP5125 has 4 chip
selects and 4 R/B signals, whereas VF610 has only 2 chip selects and
just 1 R/B signals...

> But wait...I see that you call nand_scan_ident() with a max of 1 chip.
> So you won't ever see the chip > 0 case, right?
> 
> So does this driver support multiple flash attached or not? Looks like
> you're assuming you'll only be using chip-select 0. (This is fine for
> now, but at least your code should acknowledge this. Perhaps a comment
> at the top under "limitations.")
> 

Ok, will add that information under limitations.


>> +
>> +	vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
>> +#endif
>> +}
> 
> ...
> 
>> +static int vf610_nfc_probe(struct platform_device *pdev)
>> +{
> 
> ...
> 
>> +	/* first scan to find the device and get the page size */
>> +	if (nand_scan_ident(mtd, 1, NULL)) {
>> +		err = -ENXIO;
>> +		goto error;
>> +	}

--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214691

On Wed, Aug 26, 2015 at 06:02:31PM -0700, Stefan Agner wrote:
> On 2015-08-25 13:16, Brian Norris wrote:
> > On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
> >> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> >> new file mode 100644
> >> index 0000000..5c8dfe8
> >> --- /dev/null
> >> +++ b/drivers/mtd/nand/vf610_nfc.c
> >> @@ -0,0 +1,645 @@
> > 
> > ...
> > 
> >> +/*
> >> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
> >> + */
> >> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
> >> +{
> >> +#ifdef CONFIG_SOC_VF610
> > 
> > Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.
> > 
> > If this is trying to handle the comment above ("This function supports
> > Vybrid only (MPC5125 would have full RB and four CS)") then that's the
> > wrong way of doing it, as you need to support multiplatform kernels.
> > You'll need to have a way to differentiate the different platform
> > support at runtime, not compile time.
> 
> Yes it is trying to handle the comment above. Well, the other two
> platforms I am aware of are also different architectures... (PowerPC and
> ColdFire). I think we won't have a multi-architecture kernel anytime
> soon,

Ha, right. Sorry, I don't really know this particular IP.

> hence I think removing the code at compile time is the right thing
> todo.

I don't believe that conclusion follows though.

> However, probably CONFIG_SOC_VF610 is the wrong symbol then, I could
> just use CONFIG_ARM and add a comment that this might be different on
> another other ARM SoC than VF610.
> 
> Just checked CodingStyle, and I see that IS_ENABLED is the preferred way
> for conditional compiling.
> 
> So my suggestion:
> 
> static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
> {
> 	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> 	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
> 
> 	if (!IS_ENABLED(CONFIG_ARM))
> 		return;
> 
> 	/*
> 	 * This code is only tested on the ARM platform VF610
> 	 * PowerPC based MPC5125 would have full RB and four CS
> 	 */
> ....
> 
> With that the compiler should be able to remove this (currently) ARM
> VF610 specific code on the other supported architectures...
> 
> What do you think?

The code structure isn't bad, and yes, IS_ENABLED() would be preferable,
as it removes some of the problems with #ifdef, but I still don't think
the processor architecture has much to do with the version of the IP.
The canonical way of distiguishing per-IP revisions is to key on the
compatible property. So you'd have some kind of enum, which would
currently only have an entry for VF610. i.e.:

	/* MPC5125 not yet supported */
	if (nfc->revision != NAND_VFC610)
		return;

> >> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> >> +	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
> >> +
> >> +	tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
> >> +	tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
> >> +
> >> +	if (chip == 0)
> >> +		tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
> >> +	else if (chip == 1)
> >> +		tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;
> > 
> > 	else ... ?
> > 
> > Maybe you can write this as a formulaic pattern (e.g.:
> > 
> > 	tmp |= (chip + 1) << ROW_ADDR_CHIP_SEL_SHIFT;
> > 
> > ) and just do the "max # of chips" checks on a per-platform basis in the
> > probe(). Then I'm guessing this same function can apply to both
> > platforms. (I'm not looking at HW datasheets for this, BTW, just
> > guessing based on the context here.)
> 
> It seems that MCP5125 is different than VF610. MCP5125 has 4 chip
> selects and 4 R/B signals, whereas VF610 has only 2 chip selects and
> just 1 R/B signals...

OK I don't presume to know what the different IP versions look like. And
if you just note they are unsupported/untested, you're fine.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214734

On 2015-08-27 09:34, Brian Norris wrote:
> On Wed, Aug 26, 2015 at 06:02:31PM -0700, Stefan Agner wrote:
>> On 2015-08-25 13:16, Brian Norris wrote:
>> > On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
>> >> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> >> new file mode 100644
>> >> index 0000000..5c8dfe8
>> >> --- /dev/null
>> >> +++ b/drivers/mtd/nand/vf610_nfc.c
>> >> @@ -0,0 +1,645 @@
>> >
>> > ...
>> >
>> >> +/*
>> >> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>> >> + */
>> >> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
>> >> +{
>> >> +#ifdef CONFIG_SOC_VF610
>> >
>> > Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.
>> >
>> > If this is trying to handle the comment above ("This function supports
>> > Vybrid only (MPC5125 would have full RB and four CS)") then that's the
>> > wrong way of doing it, as you need to support multiplatform kernels.
>> > You'll need to have a way to differentiate the different platform
>> > support at runtime, not compile time.
>>
>> Yes it is trying to handle the comment above. Well, the other two
>> platforms I am aware of are also different architectures... (PowerPC and
>> ColdFire). I think we won't have a multi-architecture kernel anytime
>> soon,
> 
> Ha, right. Sorry, I don't really know this particular IP.
> 
>> hence I think removing the code at compile time is the right thing
>> todo.
> 
> I don't believe that conclusion follows though.
> 
>> However, probably CONFIG_SOC_VF610 is the wrong symbol then, I could
>> just use CONFIG_ARM and add a comment that this might be different on
>> another other ARM SoC than VF610.
>>
>> Just checked CodingStyle, and I see that IS_ENABLED is the preferred way
>> for conditional compiling.
>>
>> So my suggestion:
>>
>> static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
>> {
>> 	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> 	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
>>
>> 	if (!IS_ENABLED(CONFIG_ARM))
>> 		return;
>>
>> 	/*
>> 	 * This code is only tested on the ARM platform VF610
>> 	 * PowerPC based MPC5125 would have full RB and four CS
>> 	 */
>> ....
>>
>> With that the compiler should be able to remove this (currently) ARM
>> VF610 specific code on the other supported architectures...
>>
>> What do you think?
> 
> The code structure isn't bad, and yes, IS_ENABLED() would be preferable,
> as it removes some of the problems with #ifdef, but I still don't think
> the processor architecture has much to do with the version of the IP.

Well yes, the processor architecture has probably not much to do with
the IP version.

However, that particular problem, the wiring up of the CS/RB signals, is
probably more SoC (as a whole) specific, and how that is done might have
some relation which architecture is in use...

I do not have a strong opinion on this, so we might as well go with the
run-time distinction using compatible. If there are different IP
variants within one architecture, we anyway would need to do that.

> The canonical way of distiguishing per-IP revisions is to key on the
> compatible property. So you'd have some kind of enum, which would
> currently only have an entry for VF610. i.e.:
> 
> 	/* MPC5125 not yet supported */
> 	if (nfc->revision != NAND_VFC610)
> 		return;
> 

Ok, just checked, I can use the data field of the of table to assign
specific data to a compatible string, similar to how pxa3xx_nand.c does
it.

--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web