Re: [PATCH 1/1] pinephone_defconfig: lower DRAM clock to 528

[André Przywara]

On 22/02/2021 22:11, Bob wrote:

Hi Bob,

> Lower DRAM clock speed to the official speed for the pinephone design: 528

Can you elaborate why this is necessary? Are there reports with the
existing data rate causing problems?

Please keep in mind that the whole DRAM timings we use do not confirm to
any JEDEC standards, mostly the parameters are derived from some
Allwinner provided data. Since the clock frequency is connected to the
timing parameters (many values are defined in terms of number of
cycles), just lowering the frequency can have any kind of effects.

> Signed-off-by: Bobby The Builder <b...@najdan.com>

Since this is a legal statement, I am afraid it has to carry your real name.

Cheers,
Andre

> ---
>  configs/pinephone_defconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig
> index d8ee930a69..702e2bdc14 100644
> --- a/configs/pinephone_defconfig
> +++ b/configs/pinephone_defconfig
> @@ -4,7 +4,7 @@ CONFIG_SPL=y
>  CONFIG_PINEPHONE_LEDS=y
>  CONFIG_MACH_SUN50I=y
>  CONFIG_SUNXI_DRAM_LPDDR3_STOCK=y
> -CONFIG_DRAM_CLK=552
> +CONFIG_DRAM_CLK=528
>  CONFIG_DRAM_ZQ=3881949
>  CONFIG_MMC_SUNXI_SLOT_EXTRA=2
>  CONFIG_PINEPHONE_DT_SELECTION=y

[Andre Przywara]

On Mon, 1 Mar 2021 18:39:44 -0500
Bob <b...@najdan.com> wrote:

Hi Bob,

(adding Icenowy and Ondrej)

> On 2/28/21 8:25 AM, André Przywara wrote:
> > On 22/02/2021 22:11, Bob wrote:
> >
> > Hi Bob,
>

> Hello André,
>
> Thanks for reaching out.

>
> >> Lower DRAM clock speed to the official speed for the pinephone design: 528
> > Can you elaborate why this is necessary? Are there reports with the
> > existing data rate causing problems?
> >
> > Please keep in mind that the whole DRAM timings we use do not confirm to
> > any JEDEC standards, mostly the parameters are derived from some
> > Allwinner provided data. Since the clock frequency is connected to the
> > timing parameters (many values are defined in terms of number of
> > cycles), just lowering the frequency can have any kind of effects.
>

> Sure, here is a quick resume of the issue logged at postmarketOS
> (https://gitlab.com/postmarketOS/pmaports/-/issues/981)

Where do these "WARNING: clock skew detected" messages come from? And
someone should fix those initrd errors ;-)

> I own many pinephone and one of them, in the recent KDE batch, had
> random crashes.
>
> After reaching out to the pmOS team, Oliver Smith and Martijn Braam
> asked me to test with the clock set at 528.
>
> This solved the problem right away. Martijn also confirmed the official
> speed for the pinephone design is 528.
>
> Let me know if you need addition info.

Ok, thanks for the explanation. Indeed lowering the DRAM frequency
is a common way of improving stability, although it mostly just papers
over incorrect timing data. But since we don't know that and no-one
came up with either proper timing data or a way to find that, I am fine
with that approach.

Ondrej, Icenowy, Samuel: Can one of you confirm that this makes sense?
Have you seen stability issues with the current frequency/timings?

> >> Signed-off-by: Bobby The Builder <b...@najdan.com>
> > Since this is a legal statement, I am afraid it has to carry your real name.
>

> Would you mind sharing a link related to this legal requirement ? I'd
> like to understand my legal obligations related to this patch proposal.

U-Boot doesn't seem to have a document explaining this explicitly, but
in those cases it tends to fall back to Linux:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n361

Basically you say that you have the right to publish this patch under
the respective license (GPL in this case).
In a nutshell this tries to ensure two things:
- The origin of the patch must be clean, e.g. you didn't take it from
some source code with an incompatible license (proprietary or not).
- You have the right to publish this patch, e.g. your employer (if
applicable) allows this.
It does not necessarily mean that you wrote it, you can take patches
from other people, provided that the above conditions are met, and you
keep the authorship.

Doesn't really matter in this particular case, but it's still required
even for trivial patches.

> Once I understand it better, what's the suggested way to update this
> patch request ?

You should send it again, with a:
[PATCH v2] sunxi: a64: lower DRAM clock for Pinephone
subject line. You could also wait a few days if someone gives you a
Tested-by:, then add this to the patch.

Cheers,
Andre

P.S. Can someone please update the Pinephone wiki page? In particular
the DRAM chips used would be interesting to know.
http://linux-sunxi.org/PinePhone