New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Realtek DHC target rtd1xxx and its subtargets #12520
Conversation
@phinexhung Thank you for this! Out of curiosity (before I dive into it), how related is it to the rtl83xx and rtl93xx targets? Can they share code at all? If so, we probably want to integrate this into those targets instead. |
subtargets within realtek folder, such as rtl83xx and rtl93xx targets are router SoC. What I have committed are media SoC series. They are developed by different groups within Realtek. Media SoCs (rtd129x, rtd1319, and rtd1619b) have rich interfaces, such as PCIe, USB 3, and SATA ports, but without embedded Wifi support. These media SoCs have been used by several NAS vendors, such as synology and QNAP. I don't think they can be integrated with router SoCs (rtl83xx and rtl93xx), since the HW might be quite different. |
Add cortex-a55 support for toolchain option. Create subtargets, which are rtd129x, rtd1319 and rtd1619b. These subtargets have been used in several NAS products, either using SPI or eMMC as booting media. Signed-off-by: Phinex Hung <phinex@realtek.com>
support the following chips: rtd129x series: rtd1293, rtd1295, rtd1296 rtd1319 rtd1619b Signed-off-by: James Tai <james.tai@realtek.com> Signed-off-by: Phinex Hung <phinex@realtek.com>
enable chip node for rtd129x DT node Signed-off-by: Phinex Hung <phinex@realtek.com>
support rtd129x, rtd1319 and rtd1619b common clock driver Signed-off-by: James Tai <james.tai@realtek.com> Signed-off-by: Phinex Hung <phinex@realtek.com>
add two irqmux DT nodes and update irq settings for uarts Signed-off-by: Phinex Hung <phinex@realtek.com>
add SB2 based HW spinlock driver for DHC SoCs Signed-off-by: James Tai <james.tai@realtek.com> Signed-off-by: Phinex Hung <phinex@realtek.com>
add hwspinlock DT nodes for rtd129x Signed-off-by: Phinex Hung <phinex@realtek.com>
add Realtek common clock driver for DHC SoCs Signed-off-by: James Tai <james.tai@realtek.com Signed-off-by: Phinex Hung <phinex@realtek.com>
Also modify the reserved memory region for rtd19x. Signed-off-by: Phinex Hung <phinex@realtek.com>
Remove simple reset and rely on realtek's reset driver. Update uart clock and reset control accordingly. Signed-off-by: Phinex Hung <phinex@realtek.com>
add Realtek pinctrl driver for rtd129x, rtd1319 and rtd1619b Signed-off-by: James Tai <james.tai@realtek.com> Signed-off-by: Phinex Hung <phinex@realtek.com>
add rtd129x-pinctrl.dtsi for the detail setting for each pin Signed-off-by: Phinex Hung <phinex@realtek.com>
This is required for rtd129x SoC to ensure data consistency Signed-off-by: Phinex Hung <phinex@realtek.com>
rtd129x boot code would check if there is default bootargs Signed-off-by: Phinex Hung <phinex@realtek.com>
Also use SoC's code name to set hostname. Signed-off-by: Phinex Hung <phinex@realtek.com>
add dts files for the following two boards: 1.rtd1295 giraffe eMMC 2GB board 2.rtd1296 saola eMMC 2GB board Signed-off-by: Phinex Hung <phinex@realtek.com>
add dtsi files for rtd1319 boards, including basic settings for rtd1319 pymparticles boards Signed-off-by: James Tai <james.tai@realtek.com> Signed-off-by: Phinex Hung <phinex@realtek.com>
add rtd1319 pymparticles eMMC 2GB board DT file Signed-off-by: Phinex Hung <phinex@realtek.com>
add dtsi files for rtd1619b boards, including basic settings for rtd1619b bleedingedge boards Signed-off-by: James Tai <james.tai@realtek.com> Signed-off-by: Phinex Hung <phinex@realtek.com>
add rtd1619b bleedingedge eMMC 2GB board DT file Signed-off-by: Phinex Hung <phinex@realtek.com>
Nice to see Realtek contributing. What is the upstream situation with these SoC-s, as I am seeing downstream drivers being added? |
not sure if that'll be a huge deal to be honest. The major thing we'd have to work out, is dealing with arm and mips in the same target :) but I think they can at least live in the same place :) btw, what's the difference between RTK, RTD and RTL? RTK is RealTeK i take it; your official abbreviation? RTD is probably RealTek Digital home center? what about RTL? Just curious :) |
basic architecture has been upstreamed since kernel 5.8, this means that with a simple dts, you can get it boot to console easily. We have been continuing to get all of the drivers to be accepted by kernel community, but it take time to get this done. As you can see my patches, some are just modifying the upstream code, especially for board dts files. |
If MIPS and ARM can not share the same driver, what is the benefit to use the same target folder for this? For our rtd1xxx target, most SoCs share the same driver, this means that you can use different compatible string to leverage the same driver with different data set. In my opinion, these make sense to use the same target folder.
Yes, RTD means Realtek Digital Home Center. For RTL, this has been widely used in communication group, probably at the beginning it means LAN, but it extend to most of the networking related chip or SoC. |
but it really depends on your peripherials, that's where the shared stuff would be. In the end, you'll have some drivers for some peripherials, that are either very much related, and thus use can use the same driver. A simple example is the RTL chip info driver, the structure looks similar. A nother example would/could be an SPI or I2C peripherial. But it really depends on what you guys shared internally :)
Exactly, but from an openwrt point of view, a user would select 'realtek' as a target, and then the subtarget would be rtl83xx or rtd1xx.
I was guesstimating that as well :) but thanks for sharing. |
Exactly, but even within Realtek, we have no idea how other SoCs are designed by different groups. If the peripherals use the same IP, ex Synopsys uart, basically we even don't need our own driver. The similar example applied to I2C as well. This is how my patches are based, trying to leverage the upstream code as much as possible.
Beside drivers, another thing is firmware layout. Not sure whether we can share the same firmware layout for both RTD and RTL chips. In any case, I can try to redo these patches into realtek folder, if this means sense. Or any other suggestions for this ?
|
In the end, this is all makefile magic; rtl83xx and rtl93xx are quite different as well. Also realize, that ultimatly, everything will have to be upstream as you said :)
Yes please, thank you.
To move things? start with a rt1?xx directory, and then include, like we do for the other images. If you mean other suggestions for this MR? a few; for one, we really need to drop the magic values, proper descriptive names for variables, functions and content. I saw some where 'use a9 something'. Secondly, I think you guys miss-understood how to use regmaps, especially Secondly, any hardware you recommend/could sponsor? Without hardware, it will be hard to verify what is going on. Finally, have any datasheets you can share :) documentation is probably the biggest issue. Really well defined variable names, define names and regmap fields helps a lot though. |
Sure. Then should I abandon this pull request and restart a new one, probably each commit for a single pull request?
Do you have any good reference for us to refactor the source?
I can check internally to see how many we can sponsor, what we have are our EVM boards, is that enough? The other options are using some existing NAS products that uses these SoCs which are available in the market. These board dts files are already upstreamed in kernel, such as Synology DS418.
We have planned to share what we can share since several years ago. Hope we can get this done in the near future. |
Hi @oliv3r Another possibility is to use Banana PI BPI-W2 |
I also adjust some structure according to what have been done in realtek folder. And also submit as less as possible for basic function to make review process more easily, |
Yes, making it smaller is certainly better :) but even then, we could have re-used this MR :p but lets put our focus on the new MR. |
Thanks for the hint. I will use force push in the latter modification for the new MR. |
|
||
if COMMON_CLK_REALTEK | ||
|
||
config RTK_CLK_COMMON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should figure out when to abbreviate,a nd when not. the two notitions in this line and the line above look a bit weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COMMON_CLK_REALTEK is more like a global switch which controls other config options.
RTK_CLK_COMMON is bind to clk-rtk.o this module.
Probably we can remove RTK_CLK_COMMON and enable this clk-rtk.o by default in the Makefile ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, What triggered me, is that we have something in the pipeline for the switch chips; https://gitlab.com/olliver/openwrt/openwrt/-/tree/dev/realtek-wip/target/linux/realtek/files-5.15/drivers/clk/realtek, so i'll be good if we can combine these things somehow/somewhat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review this again once those main issues have been address.
But regardless, thank you so far!
bool | ||
|
||
config COMMON_CLK_RTD1295 | ||
tristate "RTD1295 Clock Controller" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have a name for the clock controller? Who's the original IP from? was it all done in-house? Is it unique to the RTD1295 or shared with other RTD/RTL designs? Just asking whether we need to have a lot of menu entries here in the future.
Maybe more importantly, can we share code for the different drivers. Look how sunxi is doing it; they support a ton of hardware chips, from a single driver.
@@ -0,0 +1,308 @@ | |||
// SPDX-License-Identifier: GPL-2.0-only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, any reason you prefer 'gpl-2.0-only'? I always like to give flexibility to upstream, so do 'gpl-2.0-or-later' personally. Either is fine, it does not matter much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we just copy this template from other kernel source and believe that Linux would be GPL 2.0 only, as this was mentioned by Torvalds ?
*/ | ||
|
||
#include <linux/clk-provider.h> | ||
#include <linux/module.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Author: Cheng-Yu Lee <cylee12@realtek.com> | ||
*/ | ||
|
||
#include <linux/clk-provider.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you certain these are all includes? I bet there's a ton missing here :)
#include "common.h" | ||
#include "clk-pll.h" | ||
|
||
static inline uint32_t get_phrt0(struct clk_pll_mmc *clkm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upstream 'forbids' uint32_t
sadly in that they prefer u32.
More importantly though; it's okay to be more verbose in your naming. What are you getting, and what is a 'phrt'?
|
||
static inline uint32_t get_phrt0(struct clk_pll_mmc *clkm) | ||
{ | ||
return (clk_regmap_read(&clkm->clkr, clkm->pll_ofs) >> 1) & 0x1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look into regmap fields.
also, this is very magical. 'clkr' is clk register? (which is solved with regmap_fields) and 'pll_ofs' is pll_offset?
And as said before, do not use magic values :)
static inline void set_phrt0(struct clk_pll_mmc *clkm, uint32_t val) | ||
{ | ||
clk_regmap_update(&clkm->clkr, clkm->pll_ofs, 0x00000002, val << 1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lots of magic values here too :( they all should be able to have meaning no?
static inline void set_phsel(struct clk_pll_mmc *clkm, int id, uint32_t val) | ||
{ | ||
uint32_t sft = id ? 8 : 3; | ||
//printk(KERN_ERR "%s: sft=%d, id=%d, val=0x%x\n", __func__, sft, id, val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can put this under dev_dbg (or pr_dbg if you can't derive the dev somehow).
otherwise, if it's 'testing' code, don't commit it.
speaking of commented code, this driver is very light on comments, a few lines to explain how things work would be nice (no need to write comments 'set id to 8, or 3' of course.
.set_phase = clk_pll_mmc_phase_set_phase, | ||
.get_phase = clk_pll_mmc_phase_get_phase, | ||
}; | ||
EXPORT_SYMBOL_GPL(clk_pll_mmc_phase_ops); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this being exported to be used with the entire kernel? hopefully no need :)
the sunxi drivers are my favorite reference drivers, as they are generally quite well done (albeit a little bit old now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be used by eMMC driver to tune phase in order to get better performance.
any better idea to avoid exporting the symbol but can be used by other driver ?
|
||
if COMMON_CLK_REALTEK | ||
|
||
config RTK_CLK_COMMON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, What triggered me, is that we have something in the pipeline for the switch chips; https://gitlab.com/olliver/openwrt/openwrt/-/tree/dev/realtek-wip/target/linux/realtek/files-5.15/drivers/clk/realtek, so i'll be good if we can combine these things somehow/somewhat.
Hi @oliv3r For common clock driver, let us review this later after my 1st MR has been merged. I probably would submit a series of MR, breaking down the whole drivers into several parts:
So let us forget about this MR, and focus on the new MR instead. |
Hah, sorry, I just had this open in my browser and had started reviewing things already! I just hit submit; but at least you are aware :) Of course we should focus on what the open MR; sorry for abusing this one. |
It's okay, since we are internally reviewing this, probably a new clock driver would be submitted later. |
Hi @oliv3r Is below the latest source your are currently WIP ? We can try to combine ours into this in the future. Just let me know your opinion. |
Add Realtek DHC target rtd1xxx and its subtargets, which have been used in some of the NAS markets.
This pull request contains several commits with basic functionality supports, such as irqmux, pinctrl, smp...etc.
With these patches, you can boot into console with SMP enabled.
Either using tftp or fatload to load the generated kernel image and dtb under uboot can easily verify these patches.