[PATCH 2/6] clk: actions: Add SD/MMC clocks

[Amit Tomer]

Hi,

Thanks for having the detailed look and providing comments:

> According to the datasheet the clock source could also be NAND_PLL,
> depending on bit 9.
> Both PLLs use the same rate calculation, so it's just matter of the PLL
> address offset to use for covering both.

Ok, should I change the comment as Clock output of DEV/NAND_PLL ?

> > +
> > +     /* Clock output of DEV_PLL
> > +      * Range: 48M ~ 756M
> > +      * Frequency= DEVPLLCLK * 6
> > +      */
> > +     parent_rate = readl(priv->base + CMU_DEVPLL) & 0x7f;
> > +     parent_rate *= 6000000;
> > +
> > +     rate *= 2;
>
> Where does this come from?

While experimenting about it, I found that doubling input rate doubles the
speed , and BSP code does the same.

Not sure if it is a good enough explanation or maybe its side effect of not
programming divider value well when its >=128. Would check it again.

> > +     div = (parent_rate / rate) - 1;
>
> Please check rate being not zero before you dividing by it. And I would
> keep the real divider value for now, so drop the -1 here.

Ok. I would make the check.

> > +
> > +     if (div >= 128)
> > +             div |= 0x100;
>
> This does not seem right. You would need to divide the value by 128,
> once you decided to use the "divide-by-128" bit (which is bit 8).
> What about moving this below, after you know the register value?
> Then you can divide by 128 and set bit 8.

Sure, Thanks for explaining it in detail. It helped understanding
ACTIONS not so usual
clock design.

> > +     val = readl(priv->base + (CMU_SD0CLK + sd_index*0x4));
> > +     /* Bits 4..0 is used to program div value */
> > +     val &= ~0x1f;
> > +     val |= div;
>
> Please mask div before applying.

But I am masking the first 4.0 bit above , ~0x1f;

> > +     /* As per Manuals Bits 31..10 are reserved but Bits 11 and
> > +      * 10 needed to be set for proper operation
> > +      */
> > +     val |= 0xc00;
>
> Where does this come from? I don't see the Linux driver doing that?

Actually if I don't set 11th and 10th bit , I see SD speed of 2KiB/s
and unstable behaviour
while loading files from the card.
BSP U-boot driver chooses a value 0xfffffce0 for it.


Thanks
Amit