Re: [PATCH v3 11/12] configs: pinetab2-rk3566: Enable video and USB keyboard

[Dang Huynh <danct12@riseup.net>]

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 <danct12@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