[PATCH 2/2] usb: musb-new: sunxi: clarify the purpose of SRAM initialization

[Andre Przywara]

On Fri, 9 Jun 2023 13:16:00 -0600
Sam Edwards <cfsworks at gmail.com> wrote:

Hi Sam,

> I've applied most of this feedback (most of which comes as a relief; I 
> dislike inventing names for mystery bits) in preparation to send a v2, 
> but had two questions:
> 
> On 6/9/23 04:13, Andre Przywara wrote:
> >> The new comments and function name are not from any official source,
> >> but are updated to mirror how the Linux kernel sources treat this
> >> mystery magic bit. If we wanted to confirm that this speculation is
> >> correct, we could verify that SRAM-D is inaccessible whenever the
> >> bit is set, and then try clearing it again while the MUSB is in use
> >> to see what debris gets left behind in SRAM-D.
> >>
> >> This cleanup also adds a TODO comment about runtime discovery
> >> of the SYSCON base, per discussion with Andre.  
> > 
> > thanks for sending this, looks good. Some stylistic comments below.  
> 
> I take it that this (in combination with your review on 1/2) means you 
> concur with my speculated purpose of the mystery bit. If so, I'd like to 

Oh, but why mystery bit? The A20 manual [1] spells that out:
Offset: 0x4 | SRAM_CTRL_REG1
...
Bit 0: R/W, default 0x0: SRAMD_MAP: SRAM D Area Config:
			 0: map to CPU/DMA
			 1: map to USB0

So I am afraid you need to rephrase the commit message again ;-)

> rephrase the above paragraph in the commit message to:
> 
> """
> The new comments and function name are not from any official source,
> but are updated to mirror how the Linux kernel sources treat this
> mystery magic bit. This also reflects what's been observed on actual
> hardware: SRAM-D is inaccessible by the CPU when the bit is set, and
> the MUSB unit crashes when this bit is cleared while USB is in use,
> leaving behind debris in SRAM-D from its use as a "scratch space."
> """
> 
> Does this accurately reflect what you've seen, particularly (especially) 
> that last line, or should I end the commentary at "is in use."?

We mostly gave up on finding 100% proof for everything, especially in
U-Boot there is more pragmatic approach (check the DRAM controller init
code): it if works and the BSP code does it like this, it's good to go.

I confirmed that setting this bit before calling "ums" makes the
difference between the gadget appearing on the other end or not. I did
not turn it off while the gadget was in literal use - which is tricky
to pull off anyway, since you don't have a prompt while a gadget is
running.

So while it's honourable to make sure that this is correct, there is no
real need to provide mathematical proof for those bits, especially if
they have been working over years now ;-) 

> > I think we should use "non-net" commenting style, with the "/*" on a line
> > on its own.  
> 
> This seems to be an obscure term of art, and searching "non-net comment" 
> and "non-net style" on Google isn't finding me any style guide or set of 
> rules to check against. (Amusingly: if I search for the "net" style, I 
> get a lot of .NET suggestions.)

Ah, sorry, I somewhat made this term up, apparently in a non-Google
compatible way ;-) The Linux kernel recommends this "/* on a line of its
own" commenting style, but inside the net/ folder the rules are slightly
different:

https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting

Hope that helps!

Cheers,
Andre


> The main thing I'm trying to figure out is if it also demands */ on its 
> own line, which I would intuitively think makes sense (it does look 
> better to me that way), but I'm unsure if your lack of critique at the 
> closing side of my multiline comments means you would prefer:
> /*
>   * A block of text that's long enough to become a
>   * multiline comment and ends up looking like this */
> 
> Thanks for your quick and thorough feedback,
> Sam