diff mbox series

Re: patch for allowing the use of the SOCKET target of the SF600

Message ID 70356638.IQTIvWWjPW@varadin
State New
Headers show
Series Re: patch for allowing the use of the SOCKET target of the SF600 | expand

Commit Message

Florian Kaiser Jan. 14, 2019, 12:20 p.m. UTC 
Hello Nico,

thanks for your feedback! I attached an improved patch that hopefully solves 
these issues.

Thanks a lot!

Florian

Am Sonntag, 23. Dezember 2018, 14:34:16 schrieb Nico Huber:
> Hello Florian,
> 
> thanks for your patch, I've commented on it inline.
> 
> On 10.12.18 11:05, Florian Kaiser wrote:
> > I am using this patch for a while now and I did not encounter any
> > problems. I am not sure if this was/is not supported. Is there any reason
> > why this is not supported?
> 
> No reason beside that nobody cared about it yet, I guess.
> 
> Nico
> 
> > From 5df0b1ff7bb95f4384c98727a56a03660dde6aaa Mon Sep 17 00:00:00 2001
> > From: Florian Kaiser <fkaiser@genua.de>
> > Date: Tue, 31 Jul 2018 13:42:22 +0200
> > Subject: [PATCH] dediprog: allow the use of the programming target
> > "socket".
> > 
> > Change-Id: I8c5120ce2151138093be0f27951916ec7f725574
> > Signed-off-by: Florian Kaiser <fkaiser@genua.de>
> > ---
> > 
> >  dediprog.c      | 6 +++++-
> >  flashrom.8.tmpl | 5 +++--
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/dediprog.c b/dediprog.c
> > index 72818ea..0eb84ec 100644
> > --- a/dediprog.c
> > +++ b/dediprog.c
> > @@ -1028,7 +1028,7 @@ int dediprog_init(void)
> > 
> >  			free(target_str);
> >  			return 1;
> >  		
> >  		}
> > 
> > -		if (target < 1 || target > 2) {
> > +		if (target < 1 || target > 3) {
> > 
> >  			msg_perr("Error: Value for 'target' is out of range.\n");
> >  			free(target_str);
> >  			return 1;
> > 
> > @@ -1047,6 +1047,10 @@ int dediprog_init(void)
> > 
> >  			msg_pinfo("Using target %s.\n", 
"FLASH_TYPE_APPLICATION_FLASH_2");
> >  			target = FLASH_TYPE_APPLICATION_FLASH_2;
> >  			break;
> > 
> > +		case 3:
> > +			msg_pinfo("Using target %s.\n", "FLASH_TYPE_SOCKET");
> > +			target = FLASH_TYPE_SOCKET;
> > +			break;
> 
> This makes a flaw in set_target_flash() obvious now. Not all programmers
> have a socket. I just checked set_target_flash() silently fails with an
> SF 100. So we should check for compatibility there or at the calling
> site.
> 
> >  		default:
> >  			break;
> >  		
> >  		}
> > 
> > diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
> > index c557af7..04fd1c8 100644
> > --- a/flashrom.8.tmpl
> > +++ b/flashrom.8.tmpl
> > @@ -946,8 +946,9 @@ parameter specifies which target chip should be used.
> > Syntax is> 
> >  where
> >  .B value
> >  can be
> > 
> > -.BR 1 " or " 2
> > -to select target chip 1 or 2 respectively. The default is target chip 1.
> > +.BR 1 ", " 2 " or " 3
> > +to select target chip 1 or 2 respectively. The default is target chip 1.
> > To use the programming socket
> This reads confusing now (set 1, 2 or 3 to choose between 1 and 2?).
> 
> > +of the SF600 you need to select target 3.
> > 
> >  .SS

Comments

David Hendricks Jan. 15, 2019, 7:13 a.m. UTC | #1 
Looks good to me. Thanks!

FWIW, I actually wrote a patch that is very similar, but never quite
completed it: https://review.coreboot.org/c/flashrom/+/19673

So at least we know your patch is already tested ;-)

On Mon, Jan 14, 2019 at 5:02 AM Florian Kaiser <florian_kaiser@genua.de>
wrote:

> Hello Nico,
>
> thanks for your feedback! I attached an improved patch that hopefully
> solves
> these issues.
>
> Thanks a lot!
>
> Florian
>
> Am Sonntag, 23. Dezember 2018, 14:34:16 schrieb Nico Huber:
> > Hello Florian,
> >
> > thanks for your patch, I've commented on it inline.
> >
> > On 10.12.18 11:05, Florian Kaiser wrote:
> > > I am using this patch for a while now and I did not encounter any
> > > problems. I am not sure if this was/is not supported. Is there any
> reason
> > > why this is not supported?
> >
> > No reason beside that nobody cared about it yet, I guess.
> >
> > Nico
> >
> > > From 5df0b1ff7bb95f4384c98727a56a03660dde6aaa Mon Sep 17 00:00:00 2001
> > > From: Florian Kaiser <fkaiser@genua.de>
> > > Date: Tue, 31 Jul 2018 13:42:22 +0200
> > > Subject: [PATCH] dediprog: allow the use of the programming target
> > > "socket".
> > >
> > > Change-Id: I8c5120ce2151138093be0f27951916ec7f725574
> > > Signed-off-by: Florian Kaiser <fkaiser@genua.de>
> > > ---
> > >
> > >  dediprog.c      | 6 +++++-
> > >  flashrom.8.tmpl | 5 +++--
> > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/dediprog.c b/dediprog.c
> > > index 72818ea..0eb84ec 100644
> > > --- a/dediprog.c
> > > +++ b/dediprog.c
> > > @@ -1028,7 +1028,7 @@ int dediprog_init(void)
> > >
> > >                     free(target_str);
> > >                     return 1;
> > >
> > >             }
> > >
> > > -           if (target < 1 || target > 2) {
> > > +           if (target < 1 || target > 3) {
> > >
> > >                     msg_perr("Error: Value for 'target' is out of
> range.\n");
> > >                     free(target_str);
> > >                     return 1;
> > >
> > > @@ -1047,6 +1047,10 @@ int dediprog_init(void)
> > >
> > >                     msg_pinfo("Using target %s.\n",
> "FLASH_TYPE_APPLICATION_FLASH_2");
> > >                     target = FLASH_TYPE_APPLICATION_FLASH_2;
> > >                     break;
> > >
> > > +           case 3:
> > > +                   msg_pinfo("Using target %s.\n",
> "FLASH_TYPE_SOCKET");
> > > +                   target = FLASH_TYPE_SOCKET;
> > > +                   break;
> >
> > This makes a flaw in set_target_flash() obvious now. Not all programmers
> > have a socket. I just checked set_target_flash() silently fails with an
> > SF 100. So we should check for compatibility there or at the calling
> > site.
> >
> > >             default:
> > >                     break;
> > >
> > >             }
> > >
> > > diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
> > > index c557af7..04fd1c8 100644
> > > --- a/flashrom.8.tmpl
> > > +++ b/flashrom.8.tmpl
> > > @@ -946,8 +946,9 @@ parameter specifies which target chip should be
> used.
> > > Syntax is>
> > >  where
> > >  .B value
> > >  can be
> > >
> > > -.BR 1 " or " 2
> > > -to select target chip 1 or 2 respectively. The default is target chip
> 1.
> > > +.BR 1 ", " 2 " or " 3
> > > +to select target chip 1 or 2 respectively. The default is target chip
> 1.
> > > To use the programming socket
> > This reads confusing now (set 1, 2 or 3 to choose between 1 and 2?).
> >
> > > +of the SF600 you need to select target 3.
> > >
> > >  .SS
>
> --
> Mit freundlichen Grüssen
>
> Florian Kaiser
> Microkernel Systems Development
> tel +49 89 991950 - 320
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> genua
> Gesellschaft für Netzwerk - und Unix-Administration mbH
> Domagkstr. 7, D-85551 Kirchheim. http://www.genua.de
> Tel: (089) 99 19 50-0, Fax: (089) 99 19 50 - 999
>
> Geschaeftsfuehrer: Matthias Ochs, Marc Tesch
> Amtsgericht Muenchen HRB 98238
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> --
> Mit freundlichen Grüssen
>
> Florian Kaiser
> Microkernel Systems Development
> tel +49 89 991950 - 320
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> genua
> Gesellschaft für Netzwerk - und Unix-Administration mbH
> Domagkstr. 7, D-85551 Kirchheim. http://www.genua.de
> Tel: (089) 99 19 50-0, Fax: (089) 99 19 50 - 999
>
> Geschaeftsfuehrer: Matthias Ochs, Marc Tesch
> Amtsgericht Muenchen HRB 98238
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> _______________________________________________
> flashrom mailing list -- flashrom@flashrom.org
> To unsubscribe send an email to flashrom-leave@flashrom.org
>
diff mbox series

Patch

From f452463c4592f5ef2f7c52929eac885b8c153948 Mon Sep 17 00:00:00 2001
From: Florian Kaiser <fkaiser@genua.de>
Date: Tue, 31 Jul 2018 13:42:22 +0200
Subject: [PATCH] dediprog: allow the use of the programming target "socket".

Change-Id: I8c5120ce2151138093be0f27951916ec7f725574
Signed-off-by: Florian Kaiser <fkaiser@genua.de>
---
 dediprog.c      | 15 +++++++++++++--
 flashrom.8.tmpl |  5 +++--
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/dediprog.c b/dediprog.c
index 72818ea..0f7da9f 100644
--- a/dediprog.c
+++ b/dediprog.c
@@ -860,7 +860,14 @@  static int dediprog_command_b(void)
 
 static int set_target_flash(enum dediprog_target target)
 {
-	int ret = dediprog_write(CMD_SET_TARGET, target, 0, NULL, 0);
+	int ret;
+
+	if (dediprog_devicetype != DEV_SF600 && target == FLASH_TYPE_SOCKET) {
+		msg_perr("Target type 'FLASH_TYPE_SOCKET' can only be used with a SF600 Programmer.\n");
+		return 1;
+	}
+
+	ret = dediprog_write(CMD_SET_TARGET, target, 0, NULL, 0);
 	if (ret != 0) {
 		msg_perr("set_target_flash failed (%s)!\n", libusb_error_name(ret));
 		return 1;
@@ -1028,7 +1035,7 @@  int dediprog_init(void)
 			free(target_str);
 			return 1;
 		}
-		if (target < 1 || target > 2) {
+		if (target < 1 || target > 3) {
 			msg_perr("Error: Value for 'target' is out of range.\n");
 			free(target_str);
 			return 1;
@@ -1047,6 +1054,10 @@  int dediprog_init(void)
 			msg_pinfo("Using target %s.\n", "FLASH_TYPE_APPLICATION_FLASH_2");
 			target = FLASH_TYPE_APPLICATION_FLASH_2;
 			break;
+		case 3:
+			msg_pinfo("Using target %s.\n", "FLASH_TYPE_SOCKET");
+			target = FLASH_TYPE_SOCKET;
+			break;
 		default:
 			break;
 		}
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
index c557af7..c9e6ea2 100644
--- a/flashrom.8.tmpl
+++ b/flashrom.8.tmpl
@@ -946,8 +946,9 @@  parameter specifies which target chip should be used. Syntax is
 where
 .B value
 can be
-.BR 1 " or " 2
-to select target chip 1 or 2 respectively. The default is target chip 1.
+.BR 1 ", " 2 " or " 3
+to select target chip 1, 2 or 3 respectively. The default is target chip 1. To use the programming socket
+of the SF600 you need to select target 3.
 .SS
 .BR "rayer_spi " programmer
 .IP
-- 
2.20.1