Re: Re: [PATCH v2 3/3] DVBSky V3 PCIe card: add some changes to M88DS3103forsupportingthe demod of M88RS6000

["Nibble Max" <nibble.max@gmail.com>]

Hello Antti,
On 2014-10-31 11:13:59, Antti Palosaari wrote:
>
>
>On 10/31/2014 04:55 AM, Antti Palosaari wrote:
>>
>>
>> On 10/31/2014 04:34 AM, Nibble Max wrote:
>>> Hello Antti,
>>>
>>> On 2014-10-31 01:36:14, Antti Palosaari wrote:
>>>>
>>>>
>>>> On 10/30/2014 06:38 AM, Nibble Max wrote:
>>>>
>>>>>>> -    if (tab_len > 83) {
>>>>>>> +    if (tab_len > 86) {
>>>>>>
>>>>>> That is not nice, but I could try find better solution and fix it
>>>>>> later.
>>>>>
>>>>> What is the reason to check this parameter?
>>>>> How about remove this check?
>>>>
>>>> It is just to check you will not overwrite buffer accidentally. Buffer
>>>> is 83 bytes long, which should be also increased...
>>>> The correct solution is somehow calculate max possible tab size on
>>>> compile time. It should be possible as init tabs are static const
>>>> tables. Use some macro to calculate max value and use it - not plain
>>>> numbers.
>>>>
>>>> Something like that
>>>> #define BUF_SIZE   MAX(m88ds3103_tab_dvbs, m88ds3103_tab_dvbs2,
>>>> m88rs6000_tab_dvbs, m88rs6000_tab_dvbs2)
>>>>
>>>>
>>>>>> Clock selection. What this does:
>>>>>> * select mclk_khz
>>>>>> * select target_mclk
>>>>>> * calls set_config() in order to pass target_mclk to tuner driver
>>>>>> * + some strange looking sleep, which is not likely needed
>>>>>
>>>>> The clock of M88RS6000's demod comes from tuner dies.
>>>>> So the first thing is turning on the demod main clock from tuner die
>>>>> after the demod reset.
>>>>> Without this clock, the following register's content will fail to
>>>>> update.
>>>>> Before changing the demod main clock, it should close clock path.
>>>>> After changing the demod main clock, it open clock path and wait the
>>>>> clock to stable.
>>>>>
>>>>>>
>>>>>> One thing what I don't like this is that you have implemented
>>>>>> M88RS6000
>>>>>> things here and M88DS3103 elsewhere. Generally speaking, code must
>>>>>> have
>>>>>> some logic where same things are done in same place. So I expect to
>>>>>> see
>>>>>> both M88DS3103 and M88RS6000 target_mclk and mclk_khz selection
>>>>>> implemented here or these moved to place where M88DS3103
>>>>>> implementation is.
>>>>>>
>>>>>
>>>>> I will move M88DS3103 implementation to here.
>>>>>
>>>>>> Also, even set_config() is somehow logically correctly used here, I
>>>>>> prefer to duplicate that 4 line long target_mclk selection to tuner
>>>>>> driver and get rid of whole set_config(). Even better solution
>>>>>> could be
>>>>>> to register M88RS6000 as a clock provider using clock framework, but
>>>>>> imho it is not worth  that simple case.
>>>>>
>>>>> Do you suggest to set demod main clock and ts clock in tuner's
>>>>> set_params call back?
>>>>
>>>> Yes, and you did it already on that latest patch, thanks. It is not
>>>> logically correct, but a bit hackish solution, but I think it is best in
>>>> that special case in order to keep things simple here.
>>>>
>>>>
>>>>
>>>> One thing with that new patch I would like to check is this 10us delay
>>>> after clock path is enabled. You enable clock just before mcu is stopped
>>>> and demod is configured during mcu is on freeze. 10us is almost nothing
>>>> and it sounds like having no need in a situation you stop even mcu. It
>>>> is about one I2C command which will took longer than 10us. Hard to see
>>>> why you need wait 10us to settle clock in such case. What happens if you
>>>> don't wait? I assume nothing, it works just as it should as stable
>>>> clocks are needed a long after that, when mcu is take out of reset.
>>>>
>>>
>>> usleep_range(10000, 20000);
>>> This delay time at least is 10ms, not 10us.
>>
>> ah, yes, you were correct. 10ms is indeed a much larger, it is about
>> century on a digital logic signal path where frequency is around 100MHz.
>> 100MHz clock means clock cycle is 10ns, 10ms is 10000000ns => 1,000,000
>> - one million clock cycles. Whilst I don't know how chip designed in a
>> logic level, I have still done some digital logic designing myself and
>> this sounds long.
>> If you don't enable clock path, what is next command which will fail?
>> Probably it will not even fail, but never lock to signal as demod core
>> is not clocked at all.
>
>But if you want keep that delay then keep it. For my eyes it sounds 
>weird to wait that long after clock path is enabled. I have feeling it 
>is clock which is needed much later, but I may be wrong and I cannot 
>even make any tests without hardware. It is also possible that master 
>clock is needed in order to perform all the next commands.
>

If don't enable clock path, the next I2C commands looks ok, no error message.
But the demod can not lock to the signal any more.
I am not the chip designer so I do not know the exact detail information of this internal logic.

>regards
>Antti
>
>-- 
>http://palosaari.fi/
BR,
Max