Page MenuHomeFreeBSD

Support for rockchip serial flash controller
Needs ReviewPublic

Authored by titus_edc.ro on Oct 20 2023, 7:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 1, 7:28 AM
Unknown Object (File)
Tue, Apr 30, 7:16 AM
Unknown Object (File)
Tue, Apr 30, 7:16 AM
Unknown Object (File)
Tue, Apr 30, 7:16 AM
Unknown Object (File)
Tue, Apr 30, 12:30 AM
Unknown Object (File)
Feb 23 2024, 7:41 PM
Unknown Object (File)
Jan 27 2024, 5:48 AM
Unknown Object (File)
Jan 21 2024, 12:38 PM

Details

Reviewers
andrew
manu
Group Reviewers
arm64
Summary

Its mostly a clone of the linux driver excluding dma support.
Freebsd's SPI implementation does not have provision for SPI transfer direction and because SFC is half duplex you have to guess the direction from the SPI command code.
Also the cmd code, address and length are not sent via the bus but via mmio.

I added to flash devices to sys/dev/flash/mx25l.c (the ones i have on my boards)

Test Plan

i tested with dd in and out and it made an identical copy (not the first time :))

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

If "[i]ts mostly a clone of the linux driver" means copying of GPL licensed source code, FreeBSD avoids adding GPL licensed source code, so that the resultant source code in FreeBSD is not GPL licensed. Relicensing GPL source code to enable its addition into FreeBSD needs to be explicit with the original owners of the original source code license(s)/copyrights.

From the wording I cannot tell if there is a licensing issue here or not.

manu requested changes to this revision.Oct 21 2023, 8:26 AM

If "[i]ts mostly a clone of the linux driver" means copying of GPL licensed source code, FreeBSD avoids adding GPL licensed source code, so that the resultant source code in FreeBSD is not GPL licensed. Relicensing GPL source code to enable its addition into FreeBSD needs to be explicit with the original owners of the original source code license(s)/copyrights.

From the wording I cannot tell if there is a licensing issue here or not.

Agreed, I do not really understand too if it's "copied from Linux" or "Took Linux code as an inspiration".
Can someone (not me) look at both this code and Linux so compare ? I'd like to avoid tainting myself in case I really need to review this code and make comments/suggestions.

This revision now requires changes to proceed.Oct 21 2023, 8:26 AM

I'll take a look... but the concept of being tainted merely by reading gpl code has no basis in law and is utter BS and a scare campaign by some. It didn't work with AT&T who had actual contracts in place and there is no legal theory that's ever been successfully put forward that comes anywhere close....

However, given the FUD in this area, I don't blame anybody for erring on the side of caution.

I've taken a look at spi-rockport-sfc.c and compared it to what's under review. I've not looked at every single line, only a sample at the present time. I've eyeballed the rations below.

This is not a verbatim copy.
The structure of the driver is very similar (which isn't necessarily a problem: controlling hardware can be done in a limited number of ways).
Some routines are very similar, but they talk to hardware so a more careful analysis is needed.
The driver uses the same bit/register/cmd names. This isn't necessarily a problem (they likely originated in the datasheet, so aren't original in the above driver). I've not chased down this detail though.
The newbus parts of the driver are original.
The interfacing it to FreeBSD-specific elements is also original.
There's a number of places where code is inlined that should be macros that replace linux-specific function calls. Were these to be replaced with a macro, the original code would be more than the similar code.
About half of the driver falls into the original camp, the other half is copied with transformative changes. Those are confined to talking to the hardware, and there's a limited number of ways to express that code.
There's printf and comments that are copied from the original driver. These are a problem and need to be rewritten.

So with the verbatim comments and printf statements fixed; the repeated code turned into macros; and possibly some rewriting here and there, the risk would be low. The current submission is a mix of heavily inspired and thinly rewritten code with a fair amount of completely original code. My recommendation would be for me to go through this and make specific recommendations to change, have the author change them and see where we wind up. There's a number of other drivers in this, and other BSDs, that lift the core bits of talking to the hardware differ this much or less from the linux drivers. With some changes, I believe that the differences between this and the linux driver will be safely on the 'transformative changes such that it creates a new work' side of the blurry line. Without these changes, I'd be hesitant to commit the driver.

Comments?

I checked the RK3328 TRM, and the register names appear to match (there are several that are the same, but I didn't check them all).

In D42318#965728, @imp wrote:

I'll take a look... but the concept of being tainted merely by reading gpl code has no basis in law and is utter BS and a scare campaign by some. It didn't work with AT&T who had actual contracts in place and there is no legal theory that's ever been successfully put forward that comes anywhere close....

However, given the FUD in this area, I don't blame anybody for erring on the side of caution.

I know that being tainted by reading GPL is mostly BS but for "small" drivers like that it's easy to read the Linux code and just copy it by "accident", I real GPL code without problems but I usually do it when I'm having problems implementing a driver (usually because datasheet is incomplete or wrong).

In D42318#965747, @imp wrote:

I've taken a look at spi-rockport-sfc.c and compared it to what's under review. I've not looked at every single line, only a sample at the present time. I've eyeballed the rations below.

This is not a verbatim copy.

Good news ;)

The structure of the driver is very similar (which isn't necessarily a problem: controlling hardware can be done in a limited number of ways).
Some routines are very similar, but they talk to hardware so a more careful analysis is needed.

Yeah writing to registers for an operation will always be the same.

The driver uses the same bit/register/cmd names. This isn't necessarily a problem (they likely originated in the datasheet, so aren't original in the above driver). I've not chased down this detail though.

Likely not a problem.

The newbus parts of the driver are original.

Of course.

The interfacing it to FreeBSD-specific elements is also original.

Obvisously.

There's a number of places where code is inlined that should be macros that replace linux-specific function calls. Were these to be replaced with a macro, the original code would be more than the similar code.
About half of the driver falls into the original camp, the other half is copied with transformative changes. Those are confined to talking to the hardware, and there's a limited number of ways to express that code.
There's printf and comments that are copied from the original driver. These are a problem and need to be rewritten.

Ok, thanks for this.

So with the verbatim comments and printf statements fixed; the repeated code turned into macros; and possibly some rewriting here and there, the risk would be low. The current submission is a mix of heavily inspired and thinly rewritten code with a fair amount of completely original code. My recommendation would be for me to go through this and make specific recommendations to change, have the author change them and see where we wind up. There's a number of other drivers in this, and other BSDs, that lift the core bits of talking to the hardware differ this much or less from the linux drivers. With some changes, I believe that the differences between this and the linux driver will be safely on the 'transformative changes such that it creates a new work' side of the blurry line. Without these changes, I'd be hesitant to commit the driver.

That sounds good for me.

Comments?

So, it looks like the Linux driver used its own names, not the names that are in the TRM (at least not the ones in the RK3568 that I found).
As such, I've made a boatload of notes as to what the right names are.
IF you can document them in a different trm/datasheet as matching, then we'll point people there.

I know it's a pain. Using the names from the TRM makes this 'just using the interface names, as defined by the spec' which is not copywriteable.
Linux CHOSE different names. That choice is a creative choice. We're likely OK just using them, but if someone wanted to make trouble they could since the names originated with Linux.

So I'd use the correct names to create a sed script. I'd also do s/SFC_/FSPI_/ in the register names and bit/field names since the registers are named FSPI in the TRM.

That's enough for the first round of changes. I'll write more for places that common code can be reduced to a macro/inline function.

sys/arm64/rockchip/rk_sfc.c
29 ↗(On Diff #129174)

The TRM uses FSPI_ instead of SFC_, at least for the RK3568. For all the registers.
I'd use that throughout, and add a comment at the start or register definitions that the Linux driver uses SFC_ instead of FSPI_. I'd also note that it uses different names than are in the TRMs, (though maybe the reflect some internal spec that I've not been able to find).
I'll comment on the names of the registers and bits more briefly from here on out.
The names are kinda short, so I'd be tempted to add a longer comment inspired by the linux name.
http://opensource.rock-chips.com/images/2/26/Rockchip_RK3568_TRM_Part1_V1.3-20220930P.PDF

30 ↗(On Diff #129174)

SHIFTPHASE is the bitname in the TRM

31 ↗(On Diff #129174)

CMDB

32 ↗(On Diff #129174)

ADDRB

33 ↗(On Diff #129174)

DATAB

37 ↗(On Diff #129174)

RXFM

38 ↗(On Diff #129174)

RXUM

39 ↗(On Diff #129174)

TXOM

40 ↗(On Diff #129174)

TXEM

41 ↗(On Diff #129174)

TRANSM

42 ↗(On Diff #129174)

AHBM

43 ↗(On Diff #129174)

NSPIM

44 ↗(On Diff #129174)

DMAM

48 ↗(On Diff #129174)

Same as IMR except s/M/C/ for the bit names.

57 ↗(On Diff #129174)

I'd be tempted to just delete this register since it's not used.
If retained, I'd change to match the spec.

59 ↗(On Diff #129174)

TXFTLR is the field name.

61 ↗(On Diff #129174)

RXFTLR

66 ↗(On Diff #129174)

RCVR

75 ↗(On Diff #129174)

These are the same as IMR s/M/S/ in the final character.

87 ↗(On Diff #129174)

TXFS

88 ↗(On Diff #129174)

TXES

89 ↗(On Diff #129174)

RXES

90 ↗(On Diff #129174)

RXFS

92 ↗(On Diff #129174)

TXWLVL

94 ↗(On Diff #129174)

RXWLVL

99 ↗(On Diff #129174)

I'd ditch the idle and use just SR here for the bit name.

102 ↗(On Diff #129174)

Same as ISR register for bit names.

120 ↗(On Diff #129174)

SCLK_SMP_SEL

126 ↗(On Diff #129174)

DMATR for both register name and bit name

129 ↗(On Diff #129174)

DMAADDR in the data sheet, though the extra _ is fine.

134 ↗(On Diff #129174)

This is a new register, not a value for the LEN_CTRL register.

138 ↗(On Diff #129174)

CMD

139 ↗(On Diff #129174)

DUMM

140 ↗(On Diff #129174)

WR

143 ↗(On Diff #129174)

ADDRB

148 ↗(On Diff #129174)

TRB is the field name.

194 ↗(On Diff #129174)

None of these values are defined in the datasheet, though the names are fine and likely from a SPI FLASH datasheet. I seem to recall that the values were the same between vendors, but the names they used in their datasheets/TRMs differed. Though that's a memory from 20 years ago...

In D42318#965864, @imp wrote:

So, it looks like the Linux driver used its own names, not the names that are in the TRM (at least not the ones in the RK3568 that I found).
As such, I've made a boatload of notes as to what the right names are.
IF you can document them in a different trm/datasheet as matching, then we'll point people there.

I know it's a pain. Using the names from the TRM makes this 'just using the interface names, as defined by the spec' which is not copywriteable.
Linux CHOSE different names. That choice is a creative choice. We're likely OK just using them, but if someone wanted to make trouble they could since the names originated with Linux.

So I'd use the correct names to create a sed script. I'd also do s/SFC_/FSPI_/ in the register names and bit/field names since the registers are named FSPI in the TRM.

I've looked at the PX30 TRM and the registers are named like in this code.
So I guess that this was the SoC for which this driver was first written and then just changed the name in the TRM of later SoCs.

That's enough for the first round of changes. I'll write more for places that common code can be reduced to a macro/inline function.

If the PX30 uses these names, then ignore my name change request. I could only find the datasheetcwhich lacks the register info.

I'll do round 2 tomorrow.

fixed register names, bit names after rk3568 trn manual
added dma support

there is a problem with the maximum transfer size which for gen 3 is rather small (15K)
the most obvious fspi client will be probably a spi-nor mx25l device which will advertise a 64k transfer size to upper layers
there should be a way for the flash device to find out that the spi controller has a limitation and advertise a lower size to upper layers

the easiest way would probably be to add a function to spibus_if which default to returning 0 (no maximum transfer size) and override it in rk_fspi and then use it in mx25l.c to advertise it to upper layer

sys/arm64/rockchip/rk_fspi.c
2

Do we need to have a copyright ? @imp you might know more than me on this.

Needs the copyright. My first pass is that this is different enough to not pose problems.

sys/arm64/rockchip/rk_fspi.c
2

Yea. I don't see one otherwise.