Hi Jonas,
On Tue, Apr 15, 2025 at 05:52:55PM +0200, Jonas Karlman wrote:
> On 2025-04-15 15:29, Dang Huynh wrote:
> > On Sun, Apr 13, 2025 at 05:26:03PM +0200, Jonas Karlman wrote:
> >> Hi,
> >>
> >> On 2025-04-12 16:27, Dang Huynh wrote:
> >>> Now that we have VOP2 support, let's enable it and support the
> >>> built in USB keyboard.
> >>>
> >>> Signed-off-by: Dang Huynh <danc...@riseup.net>
> >>> ---
> >>> configs/pinetab2-rk3566_defconfig | 11 +++++++++++
> >>> 1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/configs/pinetab2-rk3566_defconfig
> >>> b/configs/pinetab2-rk3566_defconfig
> >>> index
> >>> 45e63b42d192dc172f24e071b09c8203675965d8..792ec0ec119437fd7a920558a149363c886c87de
> >>> 100644
> >>> --- a/configs/pinetab2-rk3566_defconfig
> >>> +++ b/configs/pinetab2-rk3566_defconfig
> >>> @@ -57,6 +57,7 @@ CONFIG_BUTTON=y
> >>> CONFIG_BUTTON_ADC=y
> >>> CONFIG_BUTTON_GPIO=y
> >>> CONFIG_SPL_CLK=y
> >>> +CONFIG_CLK_CCF=y
> >>
> >> Rockchip clk drivers does not use this, and I do not see any new clock
> >> driver being added in this series.
> >>
> >> Why is this needed?
> > I initially added this to get around clk_enable error, but it seems like
> > you don't need to enable VOP clocks.
> >
> > However, I noticed that enabling CCF will give us clock rate that are
> > pretty close to how it's configured on Linux.
>
> As mentioned on IRC with CCF enabled the clk enable() ops does not need
> to be enabled however without CFF the enable() ops must be enable or the
> set_parent() ops is just ignored.
>
> So I expect the issue with different rates is that some clock is never
> changing parent as requested by DT or driver code.
>
> Please try following diff with CCF disabled:
>
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 4b3d812f9c65..93dd32ebe74d 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -624,7 +624,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
> return -ENOSYS;
>
> ret = clk_enable(parent);
> - if (ret) {
> + if (ret && ret != -ENOSYS) {
> printf("Cannot enable parent %s\n", parent->dev->name);
> return ret;
> }
>
>
> Not sure this is a good solution or if it will be better to let the
> clk_rk3568.c driver just implement a dummy enable() ops.
>
Applying the diff does appear to make the clock rate show up correctly
and the mentioned "Cannot enable parent" error disappeared.
Before:
rk_display_init(vop@fe040000): dclk_vp1 clkrate 73000000
After:
rk_display_init(vop@fe040000): dclk_vp1 clkrate 73428571
> Regards,
> Jonas
>
> >
> >>
> >> Regards,
> >> Jonas
> >>
> >>> # CONFIG_USB_FUNCTION_FASTBOOT is not set
> >>> CONFIG_ROCKCHIP_GPIO=y
> >>> CONFIG_SYS_I2C_ROCKCHIP=y
> >>> @@ -69,6 +70,7 @@ CONFIG_MMC_SDHCI_SDMA=y
> >>> CONFIG_MMC_SDHCI_ROCKCHIP=y
> >>> CONFIG_SPI_FLASH_SFDP_SUPPORT=y
> >>> CONFIG_SPI_FLASH_SILICONKAISER=y
> >>> +CONFIG_PHY_ROCKCHIP_INNO_DSIDPHY=y
> >>> CONFIG_PHY_ROCKCHIP_INNO_USB2=y
> >>> CONFIG_PHY_ROCKCHIP_NANENG_COMBOPHY=y
> >>> CONFIG_SPL_PINCTRL=y
> >>> @@ -94,6 +96,7 @@ CONFIG_USB_OHCI_HCD=y
> >>> CONFIG_USB_OHCI_GENERIC=y
> >>> CONFIG_USB_DWC3=y
> >>> CONFIG_USB_DWC3_GENERIC=y
> >>> +CONFIG_USB_KEYBOARD=y
> >>> CONFIG_USB_HOST_ETHER=y
> >>> CONFIG_USB_ETHER_ASIX=y
> >>> CONFIG_USB_ETHER_ASIX88179=y
> >>> @@ -101,4 +104,12 @@ CONFIG_USB_ETHER_RTL8152=y
> >>> CONFIG_USB_GADGET=y
> >>> CONFIG_USB_GADGET_DOWNLOAD=y
> >>> CONFIG_USB_FUNCTION_ROCKUSB=y
> >>> +CONFIG_VIDEO=y
> >>> +CONFIG_VIDEO_LCD_BOE_TH101MB31IG002_28A=y
> >>> +CONFIG_DISPLAY=y
> >>> +CONFIG_VIDEO_ROCKCHIP=y
> >>> +CONFIG_DISPLAY_ROCKCHIP_HDMI=y
> >>> +CONFIG_DISPLAY_ROCKCHIP_DW_MIPI=y
> >>> +CONFIG_VIDEO_BRIDGE=y
> >>> +CONFIG_VIDEO_REMOVE=y
> >>> CONFIG_ERRNO_STR=y
> >>>
> >>
>
Best regards,
Dang
