[PATCH 1/3] si2157: change command for sleep

[PATCH 1/3] si2157: change command for sleep

[Olli Salonen]

Instead of sending command 13 to the tuner, send command 16 when sleeping. This 
behaviour is observed when using manufacturer provided binary-only Linux driver 
for TechnoTrend CT2-4400 (Windows driver does not do power management).

The issue with command 13 is that firmware loading is necessary after that. 
This is not an issue with tuners that do not require firmware, but resuming 
from sleep on an Si2158 takes noticeable time as firmware is loaded on resume.

Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
---
 drivers/media/tuners/si2157.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index efb5cce..c84f7b8 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -197,9 +197,10 @@ static int si2157_sleep(struct dvb_frontend *fe)
 
 	s->active = false;
 
-	memcpy(cmd.args, "\x13", 1);
-	cmd.wlen = 1;
-	cmd.rlen = 0;
+	/* standby */
+	memcpy(cmd.args, "\x16\x00", 2);
+	cmd.wlen = 2;
+	cmd.rlen = 1;
 	ret = si2157_cmd_execute(s, &cmd);
 	if (ret)
 		goto err;
-- 
1.9.1

[PATCH 2/3] si2157: avoid firmware loading if it has been loaded previously

[Olli Salonen]

Add a variable into state to keep track if firmware has been loaded or not. 
Skip firmware loading in case it is already loaded (resume from sleep).

Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
---
 drivers/media/tuners/si2157.c      | 11 +++++++++--
 drivers/media/tuners/si2157_priv.h |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index c84f7b8..5901484 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -89,7 +89,10 @@ static int si2157_init(struct dvb_frontend *fe)
 
 	dev_dbg(&s->client->dev, "\n");
 
-	/* configure? */
+	if (s->fw_loaded)
+		goto warm;
+
+	/* power up */
 	memcpy(cmd.args, "\xc0\x00\x0c\x00\x00\x01\x01\x01\x01\x01\x01\x02\x00\x00\x01", 15);
 	cmd.wlen = 15;
 	cmd.rlen = 1;
@@ -176,9 +179,12 @@ skip_fw_download:
 	if (ret)
 		goto err;
 
-	s->active = true;
+	s->fw_loaded = true;
 
+warm:
+	s->active = true;
 	return 0;
+
 err:
 	if (fw)
 		release_firmware(fw);
@@ -320,6 +326,7 @@ static int si2157_probe(struct i2c_client *client,
 	s->client = client;
 	s->fe = cfg->fe;
 	s->inversion = cfg->inversion;
+	s->fw_loaded = false;
 	mutex_init(&s->i2c_mutex);
 
 	/* check if the tuner is there */
diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h
index 3ddab5e..4080a57 100644
--- a/drivers/media/tuners/si2157_priv.h
+++ b/drivers/media/tuners/si2157_priv.h
@@ -26,6 +26,7 @@ struct si2157 {
 	struct i2c_client *client;
 	struct dvb_frontend *fe;
 	bool active;
+	bool fw_loaded;
 	bool inversion;
 };
 
-- 
1.9.1

[PATCH 3/3] si2168: avoid firmware loading if it has been loaded previously

[Olli Salonen]

Add a variable to keep track if firmware is loaded or not and skip parts of the 
initialization if fw is already loaded. Resume from sleep with a different 
command compared to initial power up and run command 85 after resume command. 
This behaviour is observed when using manufacturer provided binary-only si2168 
driver for TechnoTrend CT2-4400.

Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
---
 drivers/media/dvb-frontends/si2168.c      | 31 ++++++++++++++++++++++++++++---
 drivers/media/dvb-frontends/si2168_priv.h |  1 +
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
index 55a4212..a0797fd 100644
--- a/drivers/media/dvb-frontends/si2168.c
+++ b/drivers/media/dvb-frontends/si2168.c
@@ -363,6 +363,7 @@ static int si2168_init(struct dvb_frontend *fe)
 
 	dev_dbg(&s->client->dev, "\n");
 
+	/* initialize */
 	memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
 	cmd.wlen = 13;
 	cmd.rlen = 0;
@@ -370,6 +371,26 @@ static int si2168_init(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
+	if (s->fw_loaded) {
+		/* resume */
+		memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8);
+		cmd.wlen = 8;
+		cmd.rlen = 1;
+		ret = si2168_cmd_execute(s, &cmd);
+		if (ret)
+			goto err;
+
+		memcpy(cmd.args, "\x85", 1);
+		cmd.wlen = 1;
+		cmd.rlen = 1;
+		ret = si2168_cmd_execute(s, &cmd);
+		if (ret)
+			goto err;
+
+		goto warm;
+	}
+
+	/* power up */
 	memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
 	cmd.wlen = 8;
 	cmd.rlen = 1;
@@ -466,9 +487,6 @@ static int si2168_init(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
-	dev_info(&s->client->dev, "found a '%s' in warm state\n",
-			si2168_ops.info.name);
-
 	/* set ts mode */
 	memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
 	cmd.args[4] |= s->ts_mode;
@@ -478,6 +496,12 @@ static int si2168_init(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
+	s->fw_loaded = true;
+
+warm:
+	dev_info(&s->client->dev, "found a '%s' in warm state\n",
+			si2168_ops.info.name);
+
 	s->active = true;
 
 	return 0;
@@ -645,6 +669,7 @@ static int si2168_probe(struct i2c_client *client,
 	*config->i2c_adapter = s->adapter;
 	*config->fe = &s->fe;
 	s->ts_mode = config->ts_mode;
+	s->fw_loaded = false;
 
 	i2c_set_clientdata(client, s);
 
diff --git a/drivers/media/dvb-frontends/si2168_priv.h b/drivers/media/dvb-frontends/si2168_priv.h
index 0f83284..e13983e 100644
--- a/drivers/media/dvb-frontends/si2168_priv.h
+++ b/drivers/media/dvb-frontends/si2168_priv.h
@@ -36,6 +36,7 @@ struct si2168 {
 	fe_delivery_system_t delivery_system;
 	fe_status_t fe_status;
 	bool active;
+	bool fw_loaded;
 	u8 ts_mode;
 };
 
-- 
1.9.1

Re: [PATCH 3/3] si2168: avoid firmware loading if it has been loaded previously

[Antti Palosaari]

Moikka
Did you really need command 85 here? It will be given later in any case. 
For my Si2168 B40 there was no need for it.

regards
Antti

On 08/25/2014 09:07 PM, Olli Salonen wrote:
> Add a variable to keep track if firmware is loaded or not and skip parts of the
> initialization if fw is already loaded. Resume from sleep with a different
> command compared to initial power up and run command 85 after resume command.
> This behaviour is observed when using manufacturer provided binary-only si2168
> driver for TechnoTrend CT2-4400.
>
> Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
> ---
>   drivers/media/dvb-frontends/si2168.c      | 31 ++++++++++++++++++++++++++++---
>   drivers/media/dvb-frontends/si2168_priv.h |  1 +
>   2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
> index 55a4212..a0797fd 100644
> --- a/drivers/media/dvb-frontends/si2168.c
> +++ b/drivers/media/dvb-frontends/si2168.c
> @@ -363,6 +363,7 @@ static int si2168_init(struct dvb_frontend *fe)
>
>   	dev_dbg(&s->client->dev, "\n");
>
> +	/* initialize */
>   	memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
>   	cmd.wlen = 13;
>   	cmd.rlen = 0;
> @@ -370,6 +371,26 @@ static int si2168_init(struct dvb_frontend *fe)
>   	if (ret)
>   		goto err;
>
> +	if (s->fw_loaded) {
> +		/* resume */
> +		memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8);
> +		cmd.wlen = 8;
> +		cmd.rlen = 1;
> +		ret = si2168_cmd_execute(s, &cmd);
> +		if (ret)
> +			goto err;
> +
> +		memcpy(cmd.args, "\x85", 1);
> +		cmd.wlen = 1;
> +		cmd.rlen = 1;
> +		ret = si2168_cmd_execute(s, &cmd);
> +		if (ret)
> +			goto err;
> +
> +		goto warm;
> +	}
> +
> +	/* power up */
>   	memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
>   	cmd.wlen = 8;
>   	cmd.rlen = 1;
> @@ -466,9 +487,6 @@ static int si2168_init(struct dvb_frontend *fe)
>   	if (ret)
>   		goto err;
>
> -	dev_info(&s->client->dev, "found a '%s' in warm state\n",
> -			si2168_ops.info.name);
> -
>   	/* set ts mode */
>   	memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
>   	cmd.args[4] |= s->ts_mode;
> @@ -478,6 +496,12 @@ static int si2168_init(struct dvb_frontend *fe)
>   	if (ret)
>   		goto err;
>
> +	s->fw_loaded = true;
> +
> +warm:
> +	dev_info(&s->client->dev, "found a '%s' in warm state\n",
> +			si2168_ops.info.name);
> +
>   	s->active = true;
>
>   	return 0;
> @@ -645,6 +669,7 @@ static int si2168_probe(struct i2c_client *client,
>   	*config->i2c_adapter = s->adapter;
>   	*config->fe = &s->fe;
>   	s->ts_mode = config->ts_mode;
> +	s->fw_loaded = false;
>
>   	i2c_set_clientdata(client, s);
>
> diff --git a/drivers/media/dvb-frontends/si2168_priv.h b/drivers/media/dvb-frontends/si2168_priv.h
> index 0f83284..e13983e 100644
> --- a/drivers/media/dvb-frontends/si2168_priv.h
> +++ b/drivers/media/dvb-frontends/si2168_priv.h
> @@ -36,6 +36,7 @@ struct si2168 {
>   	fe_delivery_system_t delivery_system;
>   	fe_status_t fe_status;
>   	bool active;
> +	bool fw_loaded;
>   	u8 ts_mode;
>   };
>
>

-- 
http://palosaari.fi/

Re: [PATCH 3/3] si2168: avoid firmware loading if it has been loaded previously

[Olli Salonen]

Moro,

I'll test it once more when testing the "si2157 sleep hack" you
posted. Though I remember that the command 85 seemed to be the magic
trick that finally made it work - I agree it sounds a bit strange
considering it's run later on anyway. The proprietary driver seems to
do a command 85 after wake up, but of course that's not a guarantee of
anything.

My sniff using the proprietary driver is here:
http://trsqr.net/olli/ct2-4400/ct2-4400-wakeup-tune-sleep.txt

Cheers,
-olli

On 5 September 2014 11:36, Antti Palosaari <crope@iki.fi> wrote:
> Moikka
> Did you really need command 85 here? It will be given later in any case. For
> my Si2168 B40 there was no need for it.
>
> regards
> Antti
>
> On 08/25/2014 09:07 PM, Olli Salonen wrote:
>>
>> Add a variable to keep track if firmware is loaded or not and skip parts
>> of the
>> initialization if fw is already loaded. Resume from sleep with a different
>> command compared to initial power up and run command 85 after resume
>> command.
>> This behaviour is observed when using manufacturer provided binary-only
>> si2168
>> driver for TechnoTrend CT2-4400.
>>
>> Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
>> ---
>>   drivers/media/dvb-frontends/si2168.c      | 31
>> ++++++++++++++++++++++++++++---
>>   drivers/media/dvb-frontends/si2168_priv.h |  1 +
>>   2 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/dvb-frontends/si2168.c
>> b/drivers/media/dvb-frontends/si2168.c
>> index 55a4212..a0797fd 100644
>> --- a/drivers/media/dvb-frontends/si2168.c
>> +++ b/drivers/media/dvb-frontends/si2168.c
>> @@ -363,6 +363,7 @@ static int si2168_init(struct dvb_frontend *fe)
>>
>>         dev_dbg(&s->client->dev, "\n");
>>
>> +       /* initialize */
>>         memcpy(cmd.args,
>> "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
>>         cmd.wlen = 13;
>>         cmd.rlen = 0;
>> @@ -370,6 +371,26 @@ static int si2168_init(struct dvb_frontend *fe)
>>         if (ret)
>>                 goto err;
>>
>> +       if (s->fw_loaded) {
>> +               /* resume */
>> +               memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8);
>> +               cmd.wlen = 8;
>> +               cmd.rlen = 1;
>> +               ret = si2168_cmd_execute(s, &cmd);
>> +               if (ret)
>> +                       goto err;
>> +
>> +               memcpy(cmd.args, "\x85", 1);
>> +               cmd.wlen = 1;
>> +               cmd.rlen = 1;
>> +               ret = si2168_cmd_execute(s, &cmd);
>> +               if (ret)
>> +                       goto err;
>> +
>> +               goto warm;
>> +       }
>> +
>> +       /* power up */
>>         memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
>>         cmd.wlen = 8;
>>         cmd.rlen = 1;
>> @@ -466,9 +487,6 @@ static int si2168_init(struct dvb_frontend *fe)
>>         if (ret)
>>                 goto err;
>>
>> -       dev_info(&s->client->dev, "found a '%s' in warm state\n",
>> -                       si2168_ops.info.name);
>> -
>>         /* set ts mode */
>>         memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
>>         cmd.args[4] |= s->ts_mode;
>> @@ -478,6 +496,12 @@ static int si2168_init(struct dvb_frontend *fe)
>>         if (ret)
>>                 goto err;
>>
>> +       s->fw_loaded = true;
>> +
>> +warm:
>> +       dev_info(&s->client->dev, "found a '%s' in warm state\n",
>> +                       si2168_ops.info.name);
>> +
>>         s->active = true;
>>
>>         return 0;
>> @@ -645,6 +669,7 @@ static int si2168_probe(struct i2c_client *client,
>>         *config->i2c_adapter = s->adapter;
>>         *config->fe = &s->fe;
>>         s->ts_mode = config->ts_mode;
>> +       s->fw_loaded = false;
>>
>>         i2c_set_clientdata(client, s);
>>
>> diff --git a/drivers/media/dvb-frontends/si2168_priv.h
>> b/drivers/media/dvb-frontends/si2168_priv.h
>> index 0f83284..e13983e 100644
>> --- a/drivers/media/dvb-frontends/si2168_priv.h
>> +++ b/drivers/media/dvb-frontends/si2168_priv.h
>> @@ -36,6 +36,7 @@ struct si2168 {
>>         fe_delivery_system_t delivery_system;
>>         fe_status_t fe_status;
>>         bool active;
>> +       bool fw_loaded;
>>         u8 ts_mode;
>>   };
>>
>>
>
> --
> http://palosaari.fi/

Re: [PATCH 3/3] si2168: avoid firmware loading if it has been loaded previously

[Olli Salonen]

Moro Antti,

Tried removing the command 85 after resume, but the result is that the
demod doesn't lock after sleep. Curiously this only impacts HD or
DVB-T2 channels. DVB-T SD channels are fine even after resume.

Log of the testing here:
http://paste.ubuntu.com/8271949/

Same thing happens after applying the "si2157: sleep hack" patch: my
TT CT2-4400 does not lock on the second tune, ie. after sleep.

Log of the testing of that patch is here:
http://paste.ubuntu.com/8271869/

Cheers,
-olli

On 5 September 2014 21:54, Olli Salonen <olli.salonen@iki.fi> wrote:
> Moro,
>
> I'll test it once more when testing the "si2157 sleep hack" you
> posted. Though I remember that the command 85 seemed to be the magic
> trick that finally made it work - I agree it sounds a bit strange
> considering it's run later on anyway. The proprietary driver seems to
> do a command 85 after wake up, but of course that's not a guarantee of
> anything.
>
> My sniff using the proprietary driver is here:
> http://trsqr.net/olli/ct2-4400/ct2-4400-wakeup-tune-sleep.txt
>
> Cheers,
> -olli
>
> On 5 September 2014 11:36, Antti Palosaari <crope@iki.fi> wrote:
>> Moikka
>> Did you really need command 85 here? It will be given later in any case. For
>> my Si2168 B40 there was no need for it.
>>
>> regards
>> Antti
>>
>> On 08/25/2014 09:07 PM, Olli Salonen wrote:
>>>
>>> Add a variable to keep track if firmware is loaded or not and skip parts
>>> of the
>>> initialization if fw is already loaded. Resume from sleep with a different
>>> command compared to initial power up and run command 85 after resume
>>> command.
>>> This behaviour is observed when using manufacturer provided binary-only
>>> si2168
>>> driver for TechnoTrend CT2-4400.
>>>
>>> Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
>>> ---
>>>   drivers/media/dvb-frontends/si2168.c      | 31
>>> ++++++++++++++++++++++++++++---
>>>   drivers/media/dvb-frontends/si2168_priv.h |  1 +
>>>   2 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/dvb-frontends/si2168.c
>>> b/drivers/media/dvb-frontends/si2168.c
>>> index 55a4212..a0797fd 100644
>>> --- a/drivers/media/dvb-frontends/si2168.c
>>> +++ b/drivers/media/dvb-frontends/si2168.c
>>> @@ -363,6 +363,7 @@ static int si2168_init(struct dvb_frontend *fe)
>>>
>>>         dev_dbg(&s->client->dev, "\n");
>>>
>>> +       /* initialize */
>>>         memcpy(cmd.args,
>>> "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
>>>         cmd.wlen = 13;
>>>         cmd.rlen = 0;
>>> @@ -370,6 +371,26 @@ static int si2168_init(struct dvb_frontend *fe)
>>>         if (ret)
>>>                 goto err;
>>>
>>> +       if (s->fw_loaded) {
>>> +               /* resume */
>>> +               memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8);
>>> +               cmd.wlen = 8;
>>> +               cmd.rlen = 1;
>>> +               ret = si2168_cmd_execute(s, &cmd);
>>> +               if (ret)
>>> +                       goto err;
>>> +
>>> +               memcpy(cmd.args, "\x85", 1);
>>> +               cmd.wlen = 1;
>>> +               cmd.rlen = 1;
>>> +               ret = si2168_cmd_execute(s, &cmd);
>>> +               if (ret)
>>> +                       goto err;
>>> +
>>> +               goto warm;
>>> +       }
>>> +
>>> +       /* power up */
>>>         memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
>>>         cmd.wlen = 8;
>>>         cmd.rlen = 1;
>>> @@ -466,9 +487,6 @@ static int si2168_init(struct dvb_frontend *fe)
>>>         if (ret)
>>>                 goto err;
>>>
>>> -       dev_info(&s->client->dev, "found a '%s' in warm state\n",
>>> -                       si2168_ops.info.name);
>>> -
>>>         /* set ts mode */
>>>         memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
>>>         cmd.args[4] |= s->ts_mode;
>>> @@ -478,6 +496,12 @@ static int si2168_init(struct dvb_frontend *fe)
>>>         if (ret)
>>>                 goto err;
>>>
>>> +       s->fw_loaded = true;
>>> +
>>> +warm:
>>> +       dev_info(&s->client->dev, "found a '%s' in warm state\n",
>>> +                       si2168_ops.info.name);
>>> +
>>>         s->active = true;
>>>
>>>         return 0;
>>> @@ -645,6 +669,7 @@ static int si2168_probe(struct i2c_client *client,
>>>         *config->i2c_adapter = s->adapter;
>>>         *config->fe = &s->fe;
>>>         s->ts_mode = config->ts_mode;
>>> +       s->fw_loaded = false;
>>>
>>>         i2c_set_clientdata(client, s);
>>>
>>> diff --git a/drivers/media/dvb-frontends/si2168_priv.h
>>> b/drivers/media/dvb-frontends/si2168_priv.h
>>> index 0f83284..e13983e 100644
>>> --- a/drivers/media/dvb-frontends/si2168_priv.h
>>> +++ b/drivers/media/dvb-frontends/si2168_priv.h
>>> @@ -36,6 +36,7 @@ struct si2168 {
>>>         fe_delivery_system_t delivery_system;
>>>         fe_status_t fe_status;
>>>         bool active;
>>> +       bool fw_loaded;
>>>         u8 ts_mode;
>>>   };
>>>
>>>
>>
>> --
>> http://palosaari.fi/

Re: [PATCH 3/3] si2168: avoid firmware loading if it has been loaded previously

[Antti Palosaari]

Moro!
Removing 85 causes lose of DVB-T2 lock, but DVB-T works for me too 
(Si2168 B40). So you were correct. I will apply that patch as it is.

That tuner sleep/firmware download patch is another thing. Maybe I will 
apply it too as it prevents unnecessary firmware downloading for Si2158 
(my Si2157 does not need fw upgrade at all). It harms me a little bit to 
lose 18mA @5V idle power consumption, but 18mA is still not too much. 
Reality is that most of these Linux DVB drivers are so broken that USB 
device consumes more than 100mA on sleep. I am only person who has done 
these measurements and tried to fix all leakages, and many times done it 
even better than Windows drivers :) If Linux DVB power management is 
almost always broken, it is still not very successful on windows I have 
noticed.

It is annoying though maintain these drivers without a hardware...

regards
Antti


On 09/06/2014 08:36 PM, Olli Salonen wrote:
> Moro Antti,
>
> Tried removing the command 85 after resume, but the result is that the
> demod doesn't lock after sleep. Curiously this only impacts HD or
> DVB-T2 channels. DVB-T SD channels are fine even after resume.
>
> Log of the testing here:
> http://paste.ubuntu.com/8271949/
>
> Same thing happens after applying the "si2157: sleep hack" patch: my
> TT CT2-4400 does not lock on the second tune, ie. after sleep.
>
> Log of the testing of that patch is here:
> http://paste.ubuntu.com/8271869/
>
> Cheers,
> -olli
>
> On 5 September 2014 21:54, Olli Salonen <olli.salonen@iki.fi> wrote:
>> Moro,
>>
>> I'll test it once more when testing the "si2157 sleep hack" you
>> posted. Though I remember that the command 85 seemed to be the magic
>> trick that finally made it work - I agree it sounds a bit strange
>> considering it's run later on anyway. The proprietary driver seems to
>> do a command 85 after wake up, but of course that's not a guarantee of
>> anything.
>>
>> My sniff using the proprietary driver is here:
>> http://trsqr.net/olli/ct2-4400/ct2-4400-wakeup-tune-sleep.txt
>>
>> Cheers,
>> -olli
>>
>> On 5 September 2014 11:36, Antti Palosaari <crope@iki.fi> wrote:
>>> Moikka
>>> Did you really need command 85 here? It will be given later in any case. For
>>> my Si2168 B40 there was no need for it.
>>>
>>> regards
>>> Antti
>>>
>>> On 08/25/2014 09:07 PM, Olli Salonen wrote:
>>>>
>>>> Add a variable to keep track if firmware is loaded or not and skip parts
>>>> of the
>>>> initialization if fw is already loaded. Resume from sleep with a different
>>>> command compared to initial power up and run command 85 after resume
>>>> command.
>>>> This behaviour is observed when using manufacturer provided binary-only
>>>> si2168
>>>> driver for TechnoTrend CT2-4400.
>>>>
>>>> Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
>>>> ---
>>>>    drivers/media/dvb-frontends/si2168.c      | 31
>>>> ++++++++++++++++++++++++++++---
>>>>    drivers/media/dvb-frontends/si2168_priv.h |  1 +
>>>>    2 files changed, 29 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/dvb-frontends/si2168.c
>>>> b/drivers/media/dvb-frontends/si2168.c
>>>> index 55a4212..a0797fd 100644
>>>> --- a/drivers/media/dvb-frontends/si2168.c
>>>> +++ b/drivers/media/dvb-frontends/si2168.c
>>>> @@ -363,6 +363,7 @@ static int si2168_init(struct dvb_frontend *fe)
>>>>
>>>>          dev_dbg(&s->client->dev, "\n");
>>>>
>>>> +       /* initialize */
>>>>          memcpy(cmd.args,
>>>> "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
>>>>          cmd.wlen = 13;
>>>>          cmd.rlen = 0;
>>>> @@ -370,6 +371,26 @@ static int si2168_init(struct dvb_frontend *fe)
>>>>          if (ret)
>>>>                  goto err;
>>>>
>>>> +       if (s->fw_loaded) {
>>>> +               /* resume */
>>>> +               memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8);
>>>> +               cmd.wlen = 8;
>>>> +               cmd.rlen = 1;
>>>> +               ret = si2168_cmd_execute(s, &cmd);
>>>> +               if (ret)
>>>> +                       goto err;
>>>> +
>>>> +               memcpy(cmd.args, "\x85", 1);
>>>> +               cmd.wlen = 1;
>>>> +               cmd.rlen = 1;
>>>> +               ret = si2168_cmd_execute(s, &cmd);
>>>> +               if (ret)
>>>> +                       goto err;
>>>> +
>>>> +               goto warm;
>>>> +       }
>>>> +
>>>> +       /* power up */
>>>>          memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
>>>>          cmd.wlen = 8;
>>>>          cmd.rlen = 1;
>>>> @@ -466,9 +487,6 @@ static int si2168_init(struct dvb_frontend *fe)
>>>>          if (ret)
>>>>                  goto err;
>>>>
>>>> -       dev_info(&s->client->dev, "found a '%s' in warm state\n",
>>>> -                       si2168_ops.info.name);
>>>> -
>>>>          /* set ts mode */
>>>>          memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
>>>>          cmd.args[4] |= s->ts_mode;
>>>> @@ -478,6 +496,12 @@ static int si2168_init(struct dvb_frontend *fe)
>>>>          if (ret)
>>>>                  goto err;
>>>>
>>>> +       s->fw_loaded = true;
>>>> +
>>>> +warm:
>>>> +       dev_info(&s->client->dev, "found a '%s' in warm state\n",
>>>> +                       si2168_ops.info.name);
>>>> +
>>>>          s->active = true;
>>>>
>>>>          return 0;
>>>> @@ -645,6 +669,7 @@ static int si2168_probe(struct i2c_client *client,
>>>>          *config->i2c_adapter = s->adapter;
>>>>          *config->fe = &s->fe;
>>>>          s->ts_mode = config->ts_mode;
>>>> +       s->fw_loaded = false;
>>>>
>>>>          i2c_set_clientdata(client, s);
>>>>
>>>> diff --git a/drivers/media/dvb-frontends/si2168_priv.h
>>>> b/drivers/media/dvb-frontends/si2168_priv.h
>>>> index 0f83284..e13983e 100644
>>>> --- a/drivers/media/dvb-frontends/si2168_priv.h
>>>> +++ b/drivers/media/dvb-frontends/si2168_priv.h
>>>> @@ -36,6 +36,7 @@ struct si2168 {
>>>>          fe_delivery_system_t delivery_system;
>>>>          fe_status_t fe_status;
>>>>          bool active;
>>>> +       bool fw_loaded;
>>>>          u8 ts_mode;
>>>>    };
>>>>
>>>>
>>>
>>> --
>>> http://palosaari.fi/

-- 
http://palosaari.fi/

Re: [PATCH 1/3] si2157: change command for sleep

[Antti Palosaari]

Moikka Olli

I ran some PM tests for that patch set, using PCTV 292e, which is em28xx 
+ Si2168 B40 + Si2157 A30. Here is results:

current impementation
-------------------------------------
cold plugin     40 mA
streaming      235 mA
sleeping        42 mA

si2157: change command for sleep
-------------------------------------
cold plugin     40 mA
streaming      235 mA
sleeping        60 mA

So it increases sleep power usage surprisingly much, almost 20mA from 
the USB, nominal 5V.

It is also funny that you will not lose firmware for Si2168 when sleep 
with command 13, but that Si2157 tuner behaves differently.

I think I will still apply that, it is just firmware download time vs. 
current use in sleep.

Antti


On 08/25/2014 09:07 PM, Olli Salonen wrote:
> Instead of sending command 13 to the tuner, send command 16 when sleeping. This
> behaviour is observed when using manufacturer provided binary-only Linux driver
> for TechnoTrend CT2-4400 (Windows driver does not do power management).
>
> The issue with command 13 is that firmware loading is necessary after that.
> This is not an issue with tuners that do not require firmware, but resuming
> from sleep on an Si2158 takes noticeable time as firmware is loaded on resume.
>
> Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
> ---
>   drivers/media/tuners/si2157.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> index efb5cce..c84f7b8 100644
> --- a/drivers/media/tuners/si2157.c
> +++ b/drivers/media/tuners/si2157.c
> @@ -197,9 +197,10 @@ static int si2157_sleep(struct dvb_frontend *fe)
>
>   	s->active = false;
>
> -	memcpy(cmd.args, "\x13", 1);
> -	cmd.wlen = 1;
> -	cmd.rlen = 0;
> +	/* standby */
> +	memcpy(cmd.args, "\x16\x00", 2);
> +	cmd.wlen = 2;
> +	cmd.rlen = 1;
>   	ret = si2157_cmd_execute(s, &cmd);
>   	if (ret)
>   		goto err;
>

-- 
http://palosaari.fi/

Re: [PATCH 1/3] si2157: change command for sleep

[Mauro Carvalho Chehab]

Em Fri, 05 Sep 2014 11:34:01 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> Moikka Olli
> 
> I ran some PM tests for that patch set, using PCTV 292e, which is em28xx 
> + Si2168 B40 + Si2157 A30. Here is results:
> 
> current impementation
> -------------------------------------
> cold plugin     40 mA
> streaming      235 mA
> sleeping        42 mA
> 
> si2157: change command for sleep
> -------------------------------------
> cold plugin     40 mA
> streaming      235 mA
> sleeping        60 mA
> 
> So it increases sleep power usage surprisingly much, almost 20mA from 
> the USB, nominal 5V.
> 
> It is also funny that you will not lose firmware for Si2168 when sleep 
> with command 13, but that Si2157 tuner behaves differently.
> 
> I think I will still apply that, it is just firmware download time vs. 
> current use in sleep.

IMHO, the best is to keep it saving more power. Ok, it will take more
time to wake up but so what? If someone is putting the machine to sleep,
it is because he/she wants to save power.

So, IMHO, we should keep the default behavior as-is. Nothing prevents
that we would add a modprobe parameter or to use some other method that
would allow the user to choose between those two different ways.

Regards,
Mauro

> 
> Antti
> 
> 
> On 08/25/2014 09:07 PM, Olli Salonen wrote:
> > Instead of sending command 13 to the tuner, send command 16 when sleeping. This
> > behaviour is observed when using manufacturer provided binary-only Linux driver
> > for TechnoTrend CT2-4400 (Windows driver does not do power management).
> >
> > The issue with command 13 is that firmware loading is necessary after that.
> > This is not an issue with tuners that do not require firmware, but resuming
> > from sleep on an Si2158 takes noticeable time as firmware is loaded on resume.
> >
> > Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
> > ---
> >   drivers/media/tuners/si2157.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> > index efb5cce..c84f7b8 100644
> > --- a/drivers/media/tuners/si2157.c
> > +++ b/drivers/media/tuners/si2157.c
> > @@ -197,9 +197,10 @@ static int si2157_sleep(struct dvb_frontend *fe)
> >
> >   	s->active = false;
> >
> > -	memcpy(cmd.args, "\x13", 1);
> > -	cmd.wlen = 1;
> > -	cmd.rlen = 0;
> > +	/* standby */
> > +	memcpy(cmd.args, "\x16\x00", 2);
> > +	cmd.wlen = 2;
> > +	cmd.rlen = 1;
> >   	ret = si2157_cmd_execute(s, &cmd);
> >   	if (ret)
> >   		goto err;
> >
> 

Re: [PATCH 1/3] si2157: change command for sleep

[Mauro Carvalho Chehab]

Em Thu, 18 Sep 2014 08:22:33 -0300
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Em Fri, 05 Sep 2014 11:34:01 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
> 
> > Moikka Olli
> > 
> > I ran some PM tests for that patch set, using PCTV 292e, which is em28xx 
> > + Si2168 B40 + Si2157 A30. Here is results:
> > 
> > current impementation
> > -------------------------------------
> > cold plugin     40 mA
> > streaming      235 mA
> > sleeping        42 mA
> > 
> > si2157: change command for sleep
> > -------------------------------------
> > cold plugin     40 mA
> > streaming      235 mA
> > sleeping        60 mA
> > 
> > So it increases sleep power usage surprisingly much, almost 20mA from 
> > the USB, nominal 5V.
> > 
> > It is also funny that you will not lose firmware for Si2168 when sleep 
> > with command 13, but that Si2157 tuner behaves differently.
> > 
> > I think I will still apply that, it is just firmware download time vs. 
> > current use in sleep.
> 
> IMHO, the best is to keep it saving more power. Ok, it will take more
> time to wake up but so what? If someone is putting the machine to sleep,
> it is because he/she wants to save power.
> 
> So, IMHO, we should keep the default behavior as-is. Nothing prevents
> that we would add a modprobe parameter or to use some other method that
> would allow the user to choose between those two different ways.

Sorry, I misunderstood this patch. 

There are actually two different things, each requiring a different PM
setting:

1) to put the tuner to sleep while it is not in usage;

2) put the machine to suspend.

This patch is for (1). That's FINE. I'll apply it.

Yet, for (2), assuming a suspend to ram, the best is to save more
power. 

In the past, the DVB core didn't make any distinction between those
two, but we recently added a hook for suspend there.

So, it makes sense to keep the tuner powerdown mode for suspend.

Regards,
Mauro.


> 
> Regards,
> Mauro
> 
> > 
> > Antti
> > 
> > 
> > On 08/25/2014 09:07 PM, Olli Salonen wrote:
> > > Instead of sending command 13 to the tuner, send command 16 when sleeping. This
> > > behaviour is observed when using manufacturer provided binary-only Linux driver
> > > for TechnoTrend CT2-4400 (Windows driver does not do power management).
> > >
> > > The issue with command 13 is that firmware loading is necessary after that.
> > > This is not an issue with tuners that do not require firmware, but resuming
> > > from sleep on an Si2158 takes noticeable time as firmware is loaded on resume.
> > >
> > > Signed-off-by: Olli Salonen <olli.salonen@iki.fi>
> > > ---
> > >   drivers/media/tuners/si2157.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> > > index efb5cce..c84f7b8 100644
> > > --- a/drivers/media/tuners/si2157.c
> > > +++ b/drivers/media/tuners/si2157.c
> > > @@ -197,9 +197,10 @@ static int si2157_sleep(struct dvb_frontend *fe)
> > >
> > >   	s->active = false;
> > >
> > > -	memcpy(cmd.args, "\x13", 1);
> > > -	cmd.wlen = 1;
> > > -	cmd.rlen = 0;
> > > +	/* standby */
> > > +	memcpy(cmd.args, "\x16\x00", 2);
> > > +	cmd.wlen = 2;
> > > +	cmd.rlen = 1;
> > >   	ret = si2157_cmd_execute(s, &cmd);
> > >   	if (ret)
> > >   		goto err;
> > >
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] si2157: change command for sleep

[Antti Palosaari]

On 09/18/2014 02:50 PM, Mauro Carvalho Chehab wrote:
> Sorry, I misunderstood this patch.
>
> There are actually two different things, each requiring a different PM
> setting:
>
> 1) to put the tuner to sleep while it is not in usage;
>
> 2) put the machine to suspend.
>
> This patch is for (1). That's FINE. I'll apply it.
>
> Yet, for (2), assuming a suspend to ram, the best is to save more
> power.
>
> In the past, the DVB core didn't make any distinction between those
> two, but we recently added a hook for suspend there.
>
> So, it makes sense to keep the tuner powerdown mode for suspend.


I think old deep sleep should be used for suspend, whilst on warm state 
that new is OK.

I2C driver has suspend/resume support and it is one thing I am going to 
study in near future. Currently I have 128GB SSD and no swap at all, so 
I cannot test it now. I have already bought 256GB SSD and I am just 
waiting for Fedora 21 alpha version, which I will install with proper 
swap. Unfortunately they have delayed many times already Fedora 21 alpha...

I am not even sure if DVB frontend needs any special suspend/resume 
handling, isn't it possible to use standard kernel PM here? (I already 
added some initial PM to DVB FE ~2 years ago, but IIRC those will only 
kill threads and so which are not allowed when suspend).

So is there some reason DVB FE needs suspend/resume hooks?


regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 1/3] si2157: change command for sleep

[Mauro Carvalho Chehab]

Hi Antti,

Em Thu, 18 Sep 2014 15:00:59 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 09/18/2014 02:50 PM, Mauro Carvalho Chehab wrote:
> > Sorry, I misunderstood this patch.
> >
> > There are actually two different things, each requiring a different PM
> > setting:
> >
> > 1) to put the tuner to sleep while it is not in usage;
> >
> > 2) put the machine to suspend.
> >
> > This patch is for (1). That's FINE. I'll apply it.
> >
> > Yet, for (2), assuming a suspend to ram, the best is to save more
> > power.
> >
> > In the past, the DVB core didn't make any distinction between those
> > two, but we recently added a hook for suspend there.
> >
> > So, it makes sense to keep the tuner powerdown mode for suspend.
> 
> 
> I think old deep sleep should be used for suspend, whilst on warm state 
> that new is OK.

Agreed.

> I2C driver has suspend/resume support and it is one thing I am going to 
> study in near future.

The problem with an I2C core driven suspend/resume call is that the
suspend/resume order will be random. This will likely break support
on complex devices where resuming require a certain order to happen,
especially if the device is streaming.

> Currently I have 128GB SSD and no swap at all, so 
> I cannot test it now. I have already bought 256GB SSD and I am just 
> waiting for Fedora 21 alpha version, which I will install with proper 
> swap. Unfortunately they have delayed many times already Fedora 21 alpha...

Fedora 21 alpha will likely be too experimental for my taste ;)

The changes on Fedora next means that too much needs to be touched,
so, I'll likely wait for a few months after Fedora 21 release to
migrate most of my machines.

> I am not even sure if DVB frontend needs any special suspend/resume 
> handling, isn't it possible to use standard kernel PM here? (I already 
> added some initial PM to DVB FE ~2 years ago, but IIRC those will only 
> kill threads and so which are not allowed when suspend).

Well, dvb uses the standard Kernel PM. The way PM works on most places
is that the subsystem provides their own PM methods that are called
by the PM subsystem.

> So is there some reason DVB FE needs suspend/resume hooks?

Yes. See above.

Basically, at suspend, the tuner and demod should be stopped (including
all kthreads), and they need to be restarted and firmware may need to be
reloaded at resume time.

Regards,
Mauro