Re: [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845

[Doug Anderson]

Hi,

On Thu, Jul 19, 2018 at 12:02 PM, Taniya Das <tdas@xxxxxxxxxxxxxx> wrote:
>
>
> On 7/19/2018 11:25 PM, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Thu, Jul 19, 2018 at 4:04 AM, Taniya Das <tdas@xxxxxxxxxxxxxx> wrote:
>>>
>>> Hi Doug,
>>>
>>> Please find my comments inline.
>>>
>>> On 7/18/2018 11:34 PM, Douglas Anderson wrote:
>>>>
>>>>
>>>> Add both the interface and core clock.
>>>>
>>>> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>>>> ---
>>>>
>>>>    drivers/clk/qcom/gcc-sdm845.c | 73
>>>> +++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 73 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/qcom/gcc-sdm845.c
>>>> b/drivers/clk/qcom/gcc-sdm845.c
>>>> index 0f694ed4238a..2ee96f9bc217 100644
>>>> --- a/drivers/clk/qcom/gcc-sdm845.c
>>>> +++ b/drivers/clk/qcom/gcc-sdm845.c
>>>> @@ -162,6 +162,20 @@ static const char * const gcc_parent_names_10[] = {
>>>>          "core_bi_pll_test_se",
>>>>    };
>>>>    +static const struct parent_map gcc_parent_map_9[] = {
>>>> +       { P_BI_TCXO, 0 },
>>>> +       { P_GPLL0_OUT_MAIN, 1 },
>>>> +       { P_GPLL0_OUT_EVEN, 6 },
>>>> +       { P_SLEEP_CLK, 7 },
>>>
>>>
>>>
>>> SRC 7 has 'core_bi_pll_test_se' and not 'sleep_clk'.
>>>
>>> Please use the 'gcc_parent_map_0'
>>
>>
>> Are you sure?  I'm looking at a doc showing the bitfields of
>> GCC_QSPI_CORE_CFG_RCGR.  It says:
>>
>> 0x0: bi_tcxo.
>> 0x1: gpll0_out_main.
>> 0x6: gpll0_out_even.
>> 0x7: sleep_clk.
>>
>> This contrasts with other clocks using 'gcc_parent_map_0' (for
>> instance "gcc_qupv3_wrap0_s0_clk_src") where 0x7 is simply not listed
>> in my doc.
>>
>> ...so either my doc is wrong or yours is.  Any way to resolve that?
>>
>
> I am not sure of the document you are referring, but the connectivity
> details I have shared are from the design side and they are ones which we
> have to follow.

I'll start up a separate thread about this w/ you and the people I
received the doc from.  It's concerning if the doc we received is
wrong.


>>> F(150000000, P_GPLL0_OUT_MAIN, 4, 0, 0),
>>
>>
>> Sure.  For my edification, is there a reason to use main vs. even?
>>
>
> The frequencies to be generated are also as per design data. These source
> usage depends on timing closure for certain max frequencies.

Sounds a bit like a non-answer, but I guess it's not critical to
understand the mysteries here.



>>>> +static struct clk_rcg2 gcc_qspi_core_clk_src = {
>>>> +       .cmd_rcgr = 0x4b008,
>>>> +       .mnd_width = 0,
>>>> +       .hid_width = 5,
>>>> +       .parent_map = gcc_parent_map_9,
>>>> +       .freq_tbl = ftbl_gcc_qspi_core_clk_src,
>>>> +       .clkr.hw.init = &(struct clk_init_data){
>>>> +               .name = "gcc_qspi_core_clk_src",
>>>> +               .parent_names = gcc_parent_names_9,
>>>> +               .num_parents = 4,
>>>> +               .ops = &clk_rcg2_floor_ops,
>>>
>>>
>>>
>>> Could we use the rcg2_ops instead?
>>
>>
>> I'd rather not.  Any reason why you think that'd be a good idea?
>>
>> Specifically imagine that we have a SPI flash chip that's rated to run
>> at a max of 20 MHz.  In the device tree we'd ideally want to specify:
>>
>>    spi-max-frequency = <20000000>;
>>
>> It appears that we need to run the SPI core as 4 times the rate of the
>> SPI bus, so we'd try to set this clock to 80 MHz.  If we round up
>> we'll end up at 100 MHz or 150 MHz for the SPI core and have a SPI bus
>> rate of 25 MHz or 37.5 MHz.  That would violate the whole idea of
>> "spi-max-frequency".  It's much better to round down to 75 MHz.
>>
>>
>> In general I've always seen that for safety it's always better the
>> round clocks down and round voltage up, so I was actually confused by
>> the fact that most of the clocks in this file used rcg2_ops instead of
>> clk_rcg2_floor_ops...  I'd be curious if we should we change more of
>> them to clk_rcg2_floor_ops.  As a random example I'll take
>> "gcc_sdcc2_apps_clk_src".  If someone happened to have a full sized SD
>> slot and put an MMC card in then you'd be in trouble.  Why?
>>
>> For MMC a valid rate to request is 52000000.  When the SD card core
>> requests this you'll round up to 100 MHz.  Oops.  That makes the card
>> not work.
>>
>> I just happen to have a micro to full size adapter at my desk and a
>> 2GB MMC card and I can confirm that's a true bug that prevents this
>> card from enumerating.  Changing this to a clk_rcg2_floor_ops fixes
>> it.  True that it's unlikely anyone will really plug a MMC card into
>> this slot, but I fail to see the advantage of rounding up when
>> rounding down is safer.
>>
>>
>
> These ops were mostly for SDCC/MMC usage only as this was a requirement of
> rounding the frequency, as the clock framework didn't provide any such API.
> The QSPI should be safe to use the normal rcg ops as the frequency requests
> should be from the table itself. Thus request is to move to use the
> rcg2_ops.

I'm not totally sure I understand what you say here.  I'll try to
break my confusion down:

1. I think you're saying that "clk_rcg2_floor_ops" was introduced
originally for use by SD/MMC.  Is that right?  ...but my example above
shows specifically that SD/MMC is _not_ using "clk_rcg2_floor_ops".


2. I'm confused about you say that the QSPI driver will only be
requesting rates from the table itself.  Does that mean we need to
duplicate the table here in the QSPI driver so the QSPI driver will
only ask for exact rates that it knows the clock driver will be able
to provide?  ...or are you saying that we should always specify SPI
transfer rates in the device tree that we know will be achievable by
the clock driver?  Both of these ideas seem fragile.


I'm still confused about what benefit you think we get for rounding up
for this clock.


-Doug