Re: [PATCH 1/1] net: dsa: rtl8366rb: standardize init jam tables

On Tue, Jan 26, 2021 at 11:15:33PM +0100, Lorenzo Carletti wrote:

quoted

And did you manage to find out what these tables actually do?

I was unable to do so. I was looking for Intel 8051 instructions in them:
I created a small piece of code that generates an hypotetical
registers space in which the tables are then jammed, but I didn't
find anything.
It's clear that some of the values of the tables are configuration
parameters for stuff like the bandwidth, but that's the extent of what
I was able to understand... So not that much.

quoted

Why? What difference does it make?

So, allow me to explain. The kernel jams every "i + 1" value in the array
tables into the registers at " i", and then increments "i" by 2.
These can be seen as [n][2] matrixes, just like the ethernet one.
Having the arrays converted to matrixes can help visualize which
value is jammed where, or at least that's how I feel like it is.
I know it's not a big change...

Got it, thanks. It is better, in fact, once you get over that whole
0xBE00 thing...

quoted

On which RTL8366RB chip revisions did you test for regressions?

I don't have any of the chips to test this. What I agreed on with
Linus Walleji was to send the patch after making sure everything
compiled properly and checkpatch was happy with what I produced.
Once the patch was sent, he said he'd test it.
I ran some simulations, but that's pretty much it. I know those
are not enough, so I'm waiting as well.

It is probably a safe change as it is, if you ran some simulations and
the same values at the same register addresses are jammed before and
after, it should be fine. The code looks okay.

On Wed, Jan 27, 2021 at 12:28:05AM +0200, Vladimir Oltean wrote:

quoted

So, allow me to explain. The kernel jams every "i + 1" value in the array
tables into the registers at " i", and then increments "i" by 2.
These can be seen as [n][2] matrixes, just like the ethernet one.
Having the arrays converted to matrixes can help visualize which
value is jammed where, or at least that's how I feel like it is.
I know it's not a big change...

Got it, thanks. It is better, in fact, once you get over that whole
0xBE00 thing...

If you really want beautiful code, I guess you could create a structure
with two fields:

struct rtl8366rb_jam_table_entry {
	u16 addr;
	u16 val;
};

and then convert those ugly looking matrix definitions:
u16 (*jam_table)[2]
with:
struct rtl8366rb_jam_table_entry *jam_table

and this:
		ret = regmap_write(smi->map,
				   jam_table[i][0],
				   jam_table[i][1]);
with this:
		ret = regmap_write(smi->map,
				   jam_table[i].addr,
				   jam_table[i].val);

The memory footprint would be exactly the same, and the struct
initializers would look exactly the same as your current array
declarations.