-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Rk322x #1666
Rk322x #1666
Conversation
core/arch/arm/plat-rockchip/cru.h
Outdated
#define CRU_SNDRST_VAL 0xeca8 | ||
#define PLLS_SLOW_MODE 0x11030000 | ||
|
||
#define CORE_SOFT_RESET(core) (0x100010 << (core)) |
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.
Please use SHIFT_U32()
(from <util.h.>
, like SHIFT_U32(0x100010, (core))
instead
Same for the other defines below.
read_actlr r0 | ||
orr r0, r0, #ACTLR_SMP | ||
write_actlr r0 | ||
|
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.
bx lr
missing?
|
||
#define SGRF_SOC_CON(n) ((n) * 4) | ||
#define DDR_SGRF_DDR_CON(n) ((n) * 4) | ||
#define DDR_RGN0_NS ((1 << 30) | (0 << 14)) |
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.
Please use the BIT32()
macro from <util.h>
instead, like (BIT32(30) | BIT32(14))
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.
Hi, there is (0 << 14), not (1 << 14).
I would like to set bit14 as 0. For obvious, so I use (0 << 14). Should I use "#define DDR_RGN0_NS BIT(30)" ?
/* Make stacks aligned to data cache line length */ | ||
#define STACK_ALIGNMENT 64 | ||
|
||
#ifdef ARM64 |
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.
Since this is a ARMv7 platform, ARM64
will never be defined so this isn't needed.
#include <tee/entry_std.h> | ||
#include <tee/entry_fast.h> | ||
|
||
#define CORE_WFE_MSK(core) (0x02 << (core)) |
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.
Use SHIFT_U32()
instead
} | ||
|
||
int psci_cpu_on(uint32_t core_idx, uint32_t entry, | ||
uint32_t context_id __attribute__((unused))) |
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.
Please use the __unused
define from <compiler.h>
write32(LOCK_TAG, isram_base + LOCK_ADDR_OFFSET); | ||
dsb(); | ||
|
||
__asm("sev"); |
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.
Please define a sev()
function in <arm32.h>
and use that instead.
|
||
/* in order to sync with secondary up cpu */ | ||
cache_op_inner(DCACHE_CLEAN_INV, NULL, 0); | ||
dsb(); |
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.
The function doing the clean invalidate ends with:
dsb
isb
just before returning so these two should not be needed.
isb(); | ||
|
||
while (1) | ||
__asm("wfi"); |
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.
Please define a wfi()
function in <arm32.h>
and use that instead.
core/drivers/serial8250_uart.c
Outdated
@@ -86,7 +86,7 @@ static int serial8250_uart_getchar(struct serial_chip *chip) | |||
/* Transmit FIFO is empty, waiting again */ | |||
; | |||
} | |||
return read8(base + UART_RHR); | |||
return read32(base + UART_RHR); |
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 wonder if we should mask the result with 0xff
just to make sure that this function doesn't return unexpected bits.
|
Agreed, ideally I'd like some Tested-by's on the driver patch before merging it. |
vaddr_t va_base = (vaddr_t)phys_to_virt_io(GRF_BASE); | ||
|
||
wfei_mask = CORE_WFE_I_MSK(core); | ||
while (!(read32(va_base + GRF_CPU_STATUS1) & wfei_mask) && loop < 500) { |
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.
What happens if loop
reaches 500 and the condition is still not met? Should you be returning an error for the caller to deal with the situation? Or assert()
?
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 discussed with my team mate, we thought it is better to print error message and return an error code, then give up cpu up this time.
core/arch/arm/plat-rockchip/delay.h
Outdated
#ifndef DELAY_H | ||
#define DELAY_H | ||
|
||
#include <arm32.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.
Replace with #include <arm.h>
and move this file to core/arch/arm/include/kernel/
. Other platforms will likely need this sooner or later.
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.
Sorry for my clerical error:"while (read_cntfrq() - start <= target)" , it should be read_cntpct(). I will fix this.
} | ||
} | ||
|
||
static int core_held_in_reset(uint32_t core) |
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.
s/int/bool/
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.
pardon? I can't get what you mean.
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 mean this function should return a bool type? like this: static bool core_held_in_reset(uint32_t core) ?
@jenswi-linaro: Zynqmp uses a Cadence UART (drivers/cdns_uart.c). I don't think we have any dependency on the 8250. |
We need T-b for platforms |
@jforissier right,
I can test on RPi3 and TI-AM57xx today |
@jforissier @jenswi-linaro
|
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.
Just 2 minor comments, in any case:
Reviewed-by: Jerome Forissier <[email protected]>
|
||
val = read32(va_base + CRU_SOFTRST_CON(0)); | ||
|
||
return !!(val & CORE_HELD_IN_RESET(core)); |
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.
!!
is not needed since you're returning bool
now
loop++; | ||
} | ||
|
||
return !!(read32(va_base + GRF_CPU_STATUS1) & wfei_mask); |
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.
Here also !!
is useless
@sorenb-xlnx thanks, my mistake. |
The same for TI-AM57xx (checked only SW output over UART):
|
Hi, Jens, do you approve above update ? |
core/arch/arm/include/kernel/delay.h
Outdated
|
||
#include <arm.h> | ||
|
||
static inline void udelay(uint32_t us) |
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.
This function looks a bit large to be an inline function.
core/arch/arm/include/kernel/delay.h
Outdated
* POSSIBILITY OF SUCH DAMAGE. | ||
*/ | ||
|
||
#ifndef DELAY_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.
Use __KERNEL_DELAY_H
instead
core/arch/arm/include/kernel/delay.h
Outdated
uint64_t start, target; | ||
|
||
start = read_cntpct(); | ||
target = read_cntfrq() / 1000000 * us; |
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.
Can this overflow?
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.
Agreed, I think use 1000000ULL can fix this.
if (!core_held_in_reset(core_idx)) { | ||
wfei = wait_core_wfe_i(core_idx); | ||
if (!wfei) { | ||
EMSG("Can't wait cpu%d wfei before softrst", core_idx); |
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.
core_idx
is an uint32_t
so replace %d
with %" PRIu32 "
Please fix the EMSG()
below also.
I think it's better to only add fixes to this pull request. Once we've merged this create a new pull request with the new stuff. |
Please fixup all the commits. You can apply my tag to all the commits, but I think we should give @jforissier another chance to approve it. |
Hi, jens, do you mean I should fixup 3th~11th commits into 1st, 2nd commits, only 2 commit left finally and use git push -f ?(Sorry that I am not fimiliar with pull request work flow) |
Yes, I guess it will be around 3 commits in the end. |
Hey, Jens, this weekend, I found MrVan's patches for plat-imx PSCI was merged a few days ago, I read it and there are something good for me to follow, so I want to have a update before fixup my commits. I will do a update later. |
Hi, jens, some opinions ? Should I fixup commits ? |
Please fixup and I'll review it again. |
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.
Looks good to me. For all 3 commits:
Acked-by: Jerome Forissier <[email protected]>
@TonyXie06 you can also apply @igoropaniuk's Tested-by's to the UART commit. Could someone test on mtk-mt8173
?
#define PLAT_ROCKCHIP_COMMON_H | ||
|
||
/* For SMP cpu bootup, they are common for rockchip platforms */ | ||
#define LOCK_TAG 0xDEADBEAF |
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.
Is it really 0xDEADBEAF
and not 0xDEADBEEF
?
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.
Yes, really.
Jforissier, thank you for your approve and remind, I will apply tags after Jens review and approve. |
|
Due to hardware design, some platforms can't access the peripheral IO registers once a byte(8-bit) but once a word(32-bit). Obviously, using 32-bit accesses to the registers is more flexible for other plaforms to use serial8250 uart. Signed-off-by: Joseph Chen <[email protected]> Acked-by: Jerome Forissier <[email protected]> Acked-by: Jens Wiklander <[email protected]> Tested-by: Igor Opaniuk <[email protected]> (serial8250_uart, TI-AM57xx)
Using ARM Generic Timer to support time delay, make sure CNTFRQ register has been initialized before use. Signed-off-by: Joseph Chen <[email protected]> Acked-by: Jerome Forissier <[email protected]> Acked-by: Jens Wiklander <[email protected]>
Initial version support for rockchip SoCs.(RK322X and next SoCs). This patch adds to support the RK322X. It is one of the Rockchip family SoCs, which is a 4*A7 multi-cores ARM SoCs. plat-rockchip support features: 1.Support SMP cpu boot up and power down; 2.Support system reset; 3.Support GIC driver initialization. make PLATFORM=rockchip-rk322x Signed-off-by: Joseph Chen <[email protected]> Acked-by: Jerome Forissier <[email protected]> Acked-by: Jens Wiklander <[email protected]>
tags applied. |
@TonyXie06 thanks for your contribution! One thing I notice only now: would you mind updating |
…nux/kernel/git/mmind/linux-rockchip into next/dt Pull "Rockchip dts32 changes for 4.14" from Heiko Stübner: Removal of the deprectated num-slots property from all Rockchip dw-mmc nodes. The rv1108 gains support for sd-cards on the evaluation board and the general nodes get a bit of cosmetic. On rk3288 the evb gains support saradc and the adc-key connected to it while some more boards also get their mali gpu enabled (fennec, evb, tinker). The biggest set of changes can be found on the rk3228/rk3229 combo this time. It gets core support for efuse, sdmmc, sdio, io-domans and spdif as well as a separate rk3229.dtsi that will keep the slight differences between the two brothers rk3228/rk3229. The evaluation board also gets some attention and abled nodes (regulators, io-domains, emmc, tsadc keys) But I think the most interesting change is the cpu enable-method for it. Instead of using the older in-kernel method, we're now also moving to handling this in firmware via the psci interface on 32bit Rockchip socs. In a recently merged pull request [0] support for the rk3228/rk3229 was added to OP-TEE including the psci support and it seems supporting other 32bit Rockchip socs that way is also planned for the future. [0] OP-TEE/optee_os#1666 * tag 'v4.14-rockchip-dts32-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip: (23 commits) ARM: dts: rockchip: fix property-ordering in rv1108 mmc nodes ARM: dts: rockchip: enable sdmmc for rv1108 evb ARM: dts: rockchip: add efuse device node for rk3228 ARM: dts: rockchip: add gpio power-key for rk3229-evb ARM: dts: rockchip: enable tsadc for rk3229-evb ARM: dts: rockchip: enable eMMC for rk3229-evb ARM: dts: rockchip: enable io-domain for rk3229-evb ARM: dts: rockchip: add cpu-supply property for cpu node of rk3229-evb ARM: dts: rockchip: add regulator nodes for rk3229-evb ARM: dts: rockchip: add sdmmc and sdio nodes for rk3228 SoC ARM: dts: rockchip: fix compatible string for eMMC node of rk3228 SoC ARM: dts: rockchip: add cpu enable method for rk3228 SoC ARM: dts: rockchip: remove num-slots from all platforms ARM: dts: rockchip: Add io-domain node for rk3228 ARM: dts: rockchip: add basic dtsi file for RK3229 SoC ARM: dts: rockchip: enable adc key for rk3288-evb ARM: dts: rockchip: enable saradc for rk3288-evb ARM: dts: rockchip: enable ARM Mali GPU on rk3288-fennec ARM: dts: rockchip: enable ARM Mali GPU on rk3288-evb ARM: dts: rockchip: enable ARM Mali GPU on rk3288-tinker ...
…nux/kernel/git/mmind/linux-rockchip into next/dt Pull "Rockchip dts32 changes for 4.14" from Heiko Stübner: Removal of the deprectated num-slots property from all Rockchip dw-mmc nodes. The rv1108 gains support for sd-cards on the evaluation board and the general nodes get a bit of cosmetic. On rk3288 the evb gains support saradc and the adc-key connected to it while some more boards also get their mali gpu enabled (fennec, evb, tinker). The biggest set of changes can be found on the rk3228/rk3229 combo this time. It gets core support for efuse, sdmmc, sdio, io-domans and spdif as well as a separate rk3229.dtsi that will keep the slight differences between the two brothers rk3228/rk3229. The evaluation board also gets some attention and abled nodes (regulators, io-domains, emmc, tsadc keys) But I think the most interesting change is the cpu enable-method for it. Instead of using the older in-kernel method, we're now also moving to handling this in firmware via the psci interface on 32bit Rockchip socs. In a recently merged pull request [0] support for the rk3228/rk3229 was added to OP-TEE including the psci support and it seems supporting other 32bit Rockchip socs that way is also planned for the future. [0] OP-TEE/optee_os#1666 * tag 'v4.14-rockchip-dts32-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip: (23 commits) ARM: dts: rockchip: fix property-ordering in rv1108 mmc nodes ARM: dts: rockchip: enable sdmmc for rv1108 evb ARM: dts: rockchip: add efuse device node for rk3228 ARM: dts: rockchip: add gpio power-key for rk3229-evb ARM: dts: rockchip: enable tsadc for rk3229-evb ARM: dts: rockchip: enable eMMC for rk3229-evb ARM: dts: rockchip: enable io-domain for rk3229-evb ARM: dts: rockchip: add cpu-supply property for cpu node of rk3229-evb ARM: dts: rockchip: add regulator nodes for rk3229-evb ARM: dts: rockchip: add sdmmc and sdio nodes for rk3228 SoC ARM: dts: rockchip: fix compatible string for eMMC node of rk3228 SoC ARM: dts: rockchip: add cpu enable method for rk3228 SoC ARM: dts: rockchip: remove num-slots from all platforms ARM: dts: rockchip: Add io-domain node for rk3228 ARM: dts: rockchip: add basic dtsi file for RK3229 SoC ARM: dts: rockchip: enable adc key for rk3288-evb ARM: dts: rockchip: enable saradc for rk3288-evb ARM: dts: rockchip: enable ARM Mali GPU on rk3288-fennec ARM: dts: rockchip: enable ARM Mali GPU on rk3288-evb ARM: dts: rockchip: enable ARM Mali GPU on rk3288-tinker ... [ upstream commit: fc244e3 ]
add plat-rockchip support into optee_os, the SoC is RK322X.