[PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[Phil Elwell]

Restrict clock sources for the PCM peripheral to the oscillator and
PLLD_PER because other source may have varying rates or be switched off.
Prevent other sources from being selected by replacing their names in
the list of potential parents with dummy entries (entry index is
significant).

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/clk/bcm/clk-bcm2835.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 0258538..facc346 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1511,6 +1511,16 @@ struct bcm2835_clk_desc {
 	"pllh_aux",
 };
 
+static const char *const bcm2835_pcm_per_parents[] = {
+	"-",
+	"xosc",
+	"-",
+	"-",
+	"-",
+	"-",
+	"plld_per",
+};
+
 #define REGISTER_PER_CLK(...)	REGISTER_CLK(				\
 	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),	\
 	.parents = bcm2835_clock_per_parents,				\
@@ -2000,6 +2010,7 @@ struct bcm2835_clk_desc {
 		.int_bits = 12,
 		.frac_bits = 12,
 		.is_mash_clock = true,
+		.parents = bcm2835_pcm_per_parents,
 		.tcnt_mux = 23),
 	[BCM2835_CLOCK_PWM]	= REGISTER_PER_CLK(
 		.name = "pwm",
-- 
1.9.1

Re: [PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[Stefan Wahren]

Hi Phil,

> Phil Elwell <phil@raspberrypi.org> hat am 30. Mai 2017 um 18:28 geschrieben:
> 
> 
> Restrict clock sources for the PCM peripheral to the oscillator and
> PLLD_PER because other source may have varying rates or be switched off.

> Prevent other sources from being selected by replacing their names in
> the list of potential parents with dummy entries (entry index is
> significant).

i like to have this as a comment above the definition of bcm2835_pcm_per_parents.

> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
>  drivers/clk/bcm/clk-bcm2835.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 0258538..facc346 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1511,6 +1511,16 @@ struct bcm2835_clk_desc {
>  	"pllh_aux",
>  };
>
> +static const char *const bcm2835_pcm_per_parents[] = {

As mentioned above, there should be a comment like all the others. 

> +	"-",
> +	"xosc",
> +	"-",
> +	"-",
> +	"-",
> +	"-",
> +	"plld_per",
> +};

Is there a dummy entry for "pllh_aux" missing?

> +
>  #define REGISTER_PER_CLK(...)	REGISTER_CLK(				\
>  	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),	\
>  	.parents = bcm2835_clock_per_parents,				\
> @@ -2000,6 +2010,7 @@ struct bcm2835_clk_desc {
>  		.int_bits = 12,
>  		.frac_bits = 12,
>  		.is_mash_clock = true,
> +		.parents = bcm2835_pcm_per_parents,

This looks a little bit hacky to me. Not sure, but can we do something like this?

#define REGISTER_PCM_CLK(...)	REGISTER_CLK(				\
	.num_mux_parents = ARRAY_SIZE(bcm2835_pcm_per_parents),	\
	.parents = bcm2835_pcm_per_parents,				\
	__VA_ARGS__)

Regards
Stefan

>  		.tcnt_mux = 23),
>  	[BCM2835_CLOCK_PWM]	= REGISTER_PER_CLK(
>  		.name = "pwm",
> -- 
> 1.9.1
>

Re: [PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[Phil Elwell]

Hi Stefan,

On 30/05/2017 19:41, Stefan Wahren wrote:
> Hi Phil,
> 
>> Phil Elwell <phil@raspberrypi.org> hat am 30. Mai 2017 um 18:28 geschrieben:
>>
>>
>> Restrict clock sources for the PCM peripheral to the oscillator and
>> PLLD_PER because other source may have varying rates or be switched off.
> 
>> Prevent other sources from being selected by replacing their names in
>> the list of potential parents with dummy entries (entry index is
>> significant).
> 
> i like to have this as a comment above the definition of bcm2835_pcm_per_parents.

Sure - good idea.

>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>> ---
>>  drivers/clk/bcm/clk-bcm2835.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> index 0258538..facc346 100644
>> --- a/drivers/clk/bcm/clk-bcm2835.c
>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>> @@ -1511,6 +1511,16 @@ struct bcm2835_clk_desc {
>>  	"pllh_aux",
>>  };
>>
>> +static const char *const bcm2835_pcm_per_parents[] = {
> 
> As mentioned above, there should be a comment like all the others.

Yes, will do.

>> +	"-",
>> +	"xosc",
>> +	"-",
>> +	"-",
>> +	"-",
>> +	"-",
>> +	"plld_per",
>> +};
> 
> Is there a dummy entry for "pllh_aux" missing?

Yes and no - adding it will cause an extra iteration around the loop, but 
perhaps it's less confusing. I'll add one.

>> +
>>  #define REGISTER_PER_CLK(...)	REGISTER_CLK(				\
>>  	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),	\
>>  	.parents = bcm2835_clock_per_parents,				\
>> @@ -2000,6 +2010,7 @@ struct bcm2835_clk_desc {
>>  		.int_bits = 12,
>>  		.frac_bits = 12,
>>  		.is_mash_clock = true,
>> +		.parents = bcm2835_pcm_per_parents,
> 
> This looks a little bit hacky to me. Not sure, but can we do something like this?
> 
> #define REGISTER_PCM_CLK(...)	REGISTER_CLK(				\
> 	.num_mux_parents = ARRAY_SIZE(bcm2835_pcm_per_parents),	\
> 	.parents = bcm2835_pcm_per_parents,				\
> 	__VA_ARGS__)

Of course - no problem.

Thanks for the feedback - it will be incorporated into V2.

Phil

Re: [PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[Stefan Wahren]

Am 31.05.2017 um 10:28 schrieb Phil Elwell:
> Hi Stefan,
>
> On 30/05/2017 19:41, Stefan Wahren wrote:
>> Hi Phil,
>>
>>> Phil Elwell <phil@raspberrypi.org> hat am 30. Mai 2017 um 18:28 geschrieben:
>>>
>>>
>>> Restrict clock sources for the PCM peripheral to the oscillator and
>>> PLLD_PER because other source may have varying rates or be switched off.
>>> Prevent other sources from being selected by replacing their names in
>>> the list of potential parents with dummy entries (entry index is
>>> significant).
>> i like to have this as a comment above the definition of bcm2835_pcm_per_parents.
> Sure - good idea.
>
>>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>>> ---
>>>  drivers/clk/bcm/clk-bcm2835.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>>> index 0258538..facc346 100644
>>> --- a/drivers/clk/bcm/clk-bcm2835.c
>>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>>> @@ -1511,6 +1511,16 @@ struct bcm2835_clk_desc {
>>>  	"pllh_aux",
>>>  };
>>>
>>> +static const char *const bcm2835_pcm_per_parents[] = {
>> As mentioned above, there should be a comment like all the others.
> Yes, will do.
>
>>> +	"-",
>>> +	"xosc",
>>> +	"-",
>>> +	"-",
>>> +	"-",
>>> +	"-",
>>> +	"plld_per",
>>> +};
>> Is there a dummy entry for "pllh_aux" missing?
> Yes and no - adding it will cause an extra iteration around the loop, but 
> perhaps it's less confusing. I'll add one.

In case you want to save an iteration, you could add short comment
instead of a dummy entry.

>
>>> +
>>>  #define REGISTER_PER_CLK(...)	REGISTER_CLK(				\
>>>  	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),	\
>>>  	.parents = bcm2835_clock_per_parents,				\
>>> @@ -2000,6 +2010,7 @@ struct bcm2835_clk_desc {
>>>  		.int_bits = 12,
>>>  		.frac_bits = 12,
>>>  		.is_mash_clock = true,
>>> +		.parents = bcm2835_pcm_per_parents,
>> This looks a little bit hacky to me. Not sure, but can we do something like this?
>>
>> #define REGISTER_PCM_CLK(...)	REGISTER_CLK(				\
>> 	.num_mux_parents = ARRAY_SIZE(bcm2835_pcm_per_parents),	\
>> 	.parents = bcm2835_pcm_per_parents,				\
>> 	__VA_ARGS__)
> Of course - no problem.
>
> Thanks for the feedback - it will be incorporated into V2.
>
> Phil

[PATCH v2 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[Phil Elwell]

Restrict clock sources for the PCM peripheral to the oscillator and
PLLD_PER because other source may have varying rates or be switched off.
Prevent other sources from being selected by replacing their names in
the list of potential parents with dummy entries (entry index is
significant).

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/clk/bcm/clk-bcm2835.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 0258538..81ecd4c 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1516,6 +1516,31 @@ struct bcm2835_clk_desc {
 	.parents = bcm2835_clock_per_parents,				\
 	__VA_ARGS__)
 
+/*
+ * Restrict clock sources for the PCM peripheral to the oscillator and
+ * PLLD_PER because other source may have varying rates or be switched
+ * off.
+ *
+ * Prevent other sources from being selected by replacing their names in
+ * the list of potential parents with dummy entries (entry index is
+ * significant).
+ */
+static const char *const bcm2835_pcm_per_parents[] = {
+	"-",
+	"xosc",
+	"-",
+	"-",
+	"-",
+	"-",
+	"plld_per",
+	"-",
+};
+
+#define REGISTER_PCM_CLK(...)	REGISTER_CLK(				\
+	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_pcm_per_parents),	\
+	.parents = bcm2835_clock_pcm_per_parents,			\
+	__VA_ARGS__)
+
 /* main vpu parent mux */
 static const char *const bcm2835_clock_vpu_parents[] = {
 	"gnd",
@@ -1993,7 +2018,7 @@ struct bcm2835_clk_desc {
 		.int_bits = 4,
 		.frac_bits = 8,
 		.tcnt_mux = 22),
-	[BCM2835_CLOCK_PCM]	= REGISTER_PER_CLK(
+	[BCM2835_CLOCK_PCM]	= REGISTER_PCM_CLK(
 		.name = "pcm",
 		.ctl_reg = CM_PCMCTL,
 		.div_reg = CM_PCMDIV,
-- 
1.9.1

Re: [PATCH v2 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5950 bytes --]

Hi Phil,

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v4.12-rc3 next-20170601]
[cannot apply to linux-rpi/for-rpi-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Phil-Elwell/clk-bcm2835-Limit-PCM-clock-to-OSC-and-PLLD_PER/20170601-185211
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All error/warnings (new ones prefixed by >>):

>> drivers/clk//bcm/clk-bcm2835.c:1540:32: error: 'bcm2835_clock_pcm_per_parents' undeclared here (not in a function)
     .num_mux_parents = ARRAY_SIZE(bcm2835_clock_pcm_per_parents), \
                                   ^
   drivers/clk//bcm/clk-bcm2835.c:1473:16: note: in definition of macro '_REGISTER'
           .data = __VA_ARGS__ }
                   ^~~~~~~~~~~
>> drivers/clk//bcm/clk-bcm2835.c:1539:31: note: in expansion of macro 'REGISTER_CLK'
    #define REGISTER_PCM_CLK(...) REGISTER_CLK(    \
                                  ^~~~~~~~~~~~
>> drivers/clk//bcm/clk-bcm2835.c:1540:21: note: in expansion of macro 'ARRAY_SIZE'
     .num_mux_parents = ARRAY_SIZE(bcm2835_clock_pcm_per_parents), \
                        ^~~~~~~~~~
>> drivers/clk//bcm/clk-bcm2835.c:2021:24: note: in expansion of macro 'REGISTER_PCM_CLK'
     [BCM2835_CLOCK_PCM] = REGISTER_PCM_CLK(
                           ^~~~~~~~~~~~~~~~
   include/linux/bug.h:37:45: error: bit-field '<anonymous>' width not an integer constant
    #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
                                                ^
   drivers/clk//bcm/clk-bcm2835.c:1473:16: note: in definition of macro '_REGISTER'
           .data = __VA_ARGS__ }
                   ^~~~~~~~~~~
>> drivers/clk//bcm/clk-bcm2835.c:1539:31: note: in expansion of macro 'REGISTER_CLK'
    #define REGISTER_PCM_CLK(...) REGISTER_CLK(    \
                                  ^~~~~~~~~~~~
   include/linux/compiler-gcc.h:64:28: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
    #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
                               ^~~~~~~~~~~~~~~~~
   include/linux/kernel.h:59:59: note: in expansion of macro '__must_be_array'
    #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
                                                              ^~~~~~~~~~~~~~~
>> drivers/clk//bcm/clk-bcm2835.c:1540:21: note: in expansion of macro 'ARRAY_SIZE'
     .num_mux_parents = ARRAY_SIZE(bcm2835_clock_pcm_per_parents), \
                        ^~~~~~~~~~
>> drivers/clk//bcm/clk-bcm2835.c:2021:24: note: in expansion of macro 'REGISTER_PCM_CLK'
     [BCM2835_CLOCK_PCM] = REGISTER_PCM_CLK(
                           ^~~~~~~~~~~~~~~~

vim +/bcm2835_clock_pcm_per_parents +1540 drivers/clk//bcm/clk-bcm2835.c

  1467		bcm2835_clk_register clk_register;
  1468		const void *data;
  1469	};
  1470	
  1471	/* assignment helper macros for different clock types */
  1472	#define _REGISTER(f, ...) { .clk_register = (bcm2835_clk_register)f, \
> 1473				    .data = __VA_ARGS__ }
  1474	#define REGISTER_PLL(...)	_REGISTER(&bcm2835_register_pll,	\
  1475						  &(struct bcm2835_pll_data)	\
  1476						  {__VA_ARGS__})
  1477	#define REGISTER_PLL_DIV(...)	_REGISTER(&bcm2835_register_pll_divider, \
  1478						  &(struct bcm2835_pll_divider_data) \
  1479						  {__VA_ARGS__})
  1480	#define REGISTER_CLK(...)	_REGISTER(&bcm2835_register_clock,	\
  1481						  &(struct bcm2835_clock_data)	\
  1482						  {__VA_ARGS__})
  1483	#define REGISTER_GATE(...)	_REGISTER(&bcm2835_register_gate,	\
  1484						  &(struct bcm2835_gate_data)	\
  1485						  {__VA_ARGS__})
  1486	
  1487	/* parent mux arrays plus helper macros */
  1488	
  1489	/* main oscillator parent mux */
  1490	static const char *const bcm2835_clock_osc_parents[] = {
  1491		"gnd",
  1492		"xosc",
  1493		"testdebug0",
  1494		"testdebug1"
  1495	};
  1496	
  1497	#define REGISTER_OSC_CLK(...)	REGISTER_CLK(				\
  1498		.num_mux_parents = ARRAY_SIZE(bcm2835_clock_osc_parents),	\
  1499		.parents = bcm2835_clock_osc_parents,				\
  1500		__VA_ARGS__)
  1501	
  1502	/* main peripherial parent mux */
  1503	static const char *const bcm2835_clock_per_parents[] = {
  1504		"gnd",
  1505		"xosc",
  1506		"testdebug0",
  1507		"testdebug1",
  1508		"plla_per",
  1509		"pllc_per",
  1510		"plld_per",
  1511		"pllh_aux",
  1512	};
  1513	
  1514	#define REGISTER_PER_CLK(...)	REGISTER_CLK(				\
  1515		.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),	\
  1516		.parents = bcm2835_clock_per_parents,				\
  1517		__VA_ARGS__)
  1518	
  1519	/*
  1520	 * Restrict clock sources for the PCM peripheral to the oscillator and
  1521	 * PLLD_PER because other source may have varying rates or be switched
  1522	 * off.
  1523	 *
  1524	 * Prevent other sources from being selected by replacing their names in
  1525	 * the list of potential parents with dummy entries (entry index is
  1526	 * significant).
  1527	 */
  1528	static const char *const bcm2835_pcm_per_parents[] = {
  1529		"-",
  1530		"xosc",
  1531		"-",
  1532		"-",
  1533		"-",
  1534		"-",
  1535		"plld_per",
  1536		"-",
  1537	};
  1538	
> 1539	#define REGISTER_PCM_CLK(...)	REGISTER_CLK(				\
> 1540		.num_mux_parents = ARRAY_SIZE(bcm2835_clock_pcm_per_parents),	\
  1541		.parents = bcm2835_clock_pcm_per_parents,			\
  1542		__VA_ARGS__)
  1543	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35149 bytes --]

Re: [PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Phil Elwell <phil@raspberrypi.org> writes:

> Restrict clock sources for the PCM peripheral to the oscillator and
> PLLD_PER because other source may have varying rates or be switched off.
> Prevent other sources from being selected by replacing their names in
> the list of potential parents with dummy entries (entry index is
> significant).

I might be up for giving my r-b on this, but first I'd like to check if
we can simplify even more.  Looking through this list:

static const char *const bcm2835_clock_per_parents[] = {
	"gnd",
	"xosc",
	"testdebug0",
	"testdebug1",
	"plla_per",
	"pllc_per",
	"plld_per",
	"pllh_aux",
};

PLLA is off and unused and we don't want any peripheral to turn it on
(unless we wanted PCM to do so, but we aren't doing that here).

PLLC's rate gets changed by the VPU and so it isn't reliable.
67615c588a059b731df9d019edc3c561d8006ec9 made it so that nobody uses it
that isn't using it by firmware setup, and EMMC is the only one that the
firmware is having use PLLC.  Would we be better off just having EMMC
always be on PLLD?  Or, we could special-case EMMC to be the only one to
use PLLC.

PLLD is stable.

PLLH should only be used as a parent by VEC (or HSM, assuming that
drm/vc4 rate-sets PLLH_PIX first, but I don't know of a reason for HSM
to not just be fractionally divided off of PLLD).  If you've got
firmware display in use, it may change rate or be disabled behind
Linux's back, so we don't want anything but Linux-controlled VEC to use
it.  We could special-case VEC to be the only one that had PLLH_AUX as
parent.

So, my proposal would be to basically make everything but VEC and maybe
EMMC use your new list, and drop the code in
67615c588a059b731df9d019edc3c561d8006ec9.  That said, if you want to do
this first for PCM and then extend it to the rest of the clock
consumers, I'm fine with that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

[Phil Elwell]

On 31/05/2017 22:24, Eric Anholt wrote:
> Phil Elwell <phil@raspberrypi.org> writes:
> 
>> Restrict clock sources for the PCM peripheral to the oscillator and
>> PLLD_PER because other source may have varying rates or be switched off.
>> Prevent other sources from being selected by replacing their names in
>> the list of potential parents with dummy entries (entry index is
>> significant).
> 
> I might be up for giving my r-b on this, but first I'd like to check if
> we can simplify even more.  Looking through this list:
> 
> static const char *const bcm2835_clock_per_parents[] = {
> 	"gnd",
> 	"xosc",
> 	"testdebug0",
> 	"testdebug1",
> 	"plla_per",
> 	"pllc_per",
> 	"plld_per",
> 	"pllh_aux",
> };
> 
> PLLA is off and unused and we don't want any peripheral to turn it on
> (unless we wanted PCM to do so, but we aren't doing that here).
> 
> PLLC's rate gets changed by the VPU and so it isn't reliable.
> 67615c588a059b731df9d019edc3c561d8006ec9 made it so that nobody uses it
> that isn't using it by firmware setup, and EMMC is the only one that the
> firmware is having use PLLC.  Would we be better off just having EMMC
> always be on PLLD?  Or, we could special-case EMMC to be the only one to
> use PLLC.
> 
> PLLD is stable.
> 
> PLLH should only be used as a parent by VEC (or HSM, assuming that
> drm/vc4 rate-sets PLLH_PIX first, but I don't know of a reason for HSM
> to not just be fractionally divided off of PLLD).  If you've got
> firmware display in use, it may change rate or be disabled behind
> Linux's back, so we don't want anything but Linux-controlled VEC to use
> it.  We could special-case VEC to be the only one that had PLLH_AUX as
> parent.
> 
> So, my proposal would be to basically make everything but VEC and maybe
> EMMC use your new list, and drop the code in
> 67615c588a059b731df9d019edc3c561d8006ec9.  That said, if you want to do
> this first for PCM and then extend it to the rest of the clock
> consumers, I'm fine with that.

Thank, Eric. In the Linux spirit of doing things in small, incremental steps
(that and cowardice) I'd like to just restrict PCM for now. I'll add
your Reviewed-by to v3.

Phil