Re: [PATCH v2 3/8] media: platform: amd: Add helpers to configure isp4 mipi phy

Du, Bin
2025-08-06T09:56:47Z

Thanks Laurent Pinchart for the review.

On 8/5/2025 6:53 PM, Laurent Pinchart wrote:
> On Tue, Aug 05, 2025 at 05:53:30PM +0800, Du, Bin wrote:
>> On 7/28/2025 2:33 PM, Sakari Ailus wrote:
>>> On Wed, Jun 18, 2025 at 05:19:54PM +0800, Bin Du wrote:
>>>> The helper functions is for configuring, starting and stop the MIPI PHY.
>>>> All configurations related to MIPI PHY configuration and calibration
>>>> parameters are encapsulated in two helper functions: start and stop
>>>> mipi phy.
>>>>
>>>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>>> ---
>>>> drivers/media/platform/amd/isp4/Makefile | 1 +
>>>> drivers/media/platform/amd/isp4/isp4_phy.c | 1547 ++++++++++++++++++++
>>>> drivers/media/platform/amd/isp4/isp4_phy.h | 14 +
>>>> 3 files changed, 1562 insertions(+)
>>>> create mode 100644 drivers/media/platform/amd/isp4/isp4_phy.c
>>>> create mode 100644 drivers/media/platform/amd/isp4/isp4_phy.h
>>>>
>>>> diff --git a/drivers/media/platform/amd/isp4/Makefile b/drivers/media/platform/amd/isp4/Makefile
>>>> index 8ca1c4dfe246..0e36201fbb30 100644
>>>> --- a/drivers/media/platform/amd/isp4/Makefile
>>>> +++ b/drivers/media/platform/amd/isp4/Makefile
>>>> @@ -4,6 +4,7 @@
>>>>
>>>> obj-$(CONFIG_AMD_ISP4) += amd_capture.o
>>>> amd_capture-objs := isp4.o \
>>>> + isp4_phy.o \
>>>> isp4_hw.o \
>>>>
>>>> ccflags-y += -I$(srctree)/drivers/media/platform/amd/isp4
>>>> diff --git a/drivers/media/platform/amd/isp4/isp4_phy.c b/drivers/media/platform/amd/isp4/isp4_phy.c
>>>> new file mode 100644
>>>> index 000000000000..8d31a21074bb
>>>> --- /dev/null

[snip]

>>>> +
>>>> +static const struct isp4phy_mipi_reg *isp_mipi_phy_reg[ISP_MIPI_PHY_ID_MAX] = {
>>>> + (struct isp4phy_mipi_reg *)ISP_MIPI_PHY0_REG0,
>>>> + (struct isp4phy_mipi_reg *)ISP_MIPI_PHY1_REG0,
>>>> + (struct isp4phy_mipi_reg *)ISP_MIPI_PHY2_REG0,
>>>
>>> That's an interesting way to prefill structs. I don't think these macros
>>> expand to valid pointers.
>>
>> Yes, these are valid pointers, but they are not pointing to system
>> memory, they are phy registers address, can be accessed by adding the
>> mmio base.
>
> Please don't do that. Store register addresses in macros, and use them
> through the code as appropriate. Stop passing the base mmio address
> around through functions, store it in a PHY object, and pass that object
> to the isp4phy_start() instead of a dev pointer, base pointer and phy
> id.
>

Sure, will do that and update in the next patch

>>>> +};
>>>> +
>>>> +static const struct isp4phy_mipi_reg_seq startup_seq_general_common_config[] = {
>>>> + { PPI_STARTUP_RW_COMMON_DPHY_10, PPI_STARTUP_RW_COMMON_DPHY_10_PHY_READY_ADDR_MASK, 0x30 },
>>>> + {
>>>> + CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_2,
>>>> + CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_2_GLOBAL_ULPS_OVR_VAL_MASK, 0x0
>>>> + },
>>>> + {
>>>> + CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_2,
>>>> + CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_2_GLOBAL_ULPS_OVR_EN_MASK, 0x1
>>>> + },
>>>> + {
>>>> + CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_0,
>>>> + CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_0_CB_LP_DCO_EN_DLY_MASK, 0x3F
>>>> + },
>>>> + {
>>>> + PPI_STARTUP_RW_COMMON_STARTUP_1_1,
>>>> + PPI_STARTUP_RW_COMMON_STARTUP_1_1_PHY_READY_DLY_MASK, 0x233
>>>> + },
>>>> + { PPI_STARTUP_RW_COMMON_DPHY_6, PPI_STARTUP_RW_COMMON_DPHY_6_LP_DCO_CAL_ADDR_MASK, 0x27 },
>>>> + { PPI_CALIBCTRL_RW_COMMON_BG_0, PPI_CALIBCTRL_RW_COMMON_BG_0_BG_MAX_COUNTER_MASK, 0x1F4 },
>>>> + { PPI_RW_LPDCOCAL_NREF, PPI_RW_LPDCOCAL_NREF_LPDCOCAL_NREF_MASK, 0x320 },
>>>> + { PPI_RW_LPDCOCAL_NREF_RANGE, PPI_RW_LPDCOCAL_NREF_RANGE_LPDCOCAL_NREF_RANGE_MASK, 0x1B },
>>>> + { PPI_RW_LPDCOCAL_TWAIT_CONFIG, PPI_RW_LPDCOCAL_TWAIT_CONFIG_LPDCOCAL_TWAIT_PON_MASK, 0x7F},
>>>> + { PPI_RW_LPDCOCAL_VT_CONFIG, PPI_RW_LPDCOCAL_VT_CONFIG_LPDCOCAL_VT_NREF_RANGE_MASK, 0x1B },
>>>> + { PPI_RW_LPDCOCAL_VT_CONFIG, PPI_RW_LPDCOCAL_VT_CONFIG_LPDCOCAL_USE_IDEAL_NREF_MASK, 0x1 },
>>>> + { PPI_RW_LPDCOCAL_VT_CONFIG, PPI_RW_LPDCOCAL_VT_CONFIG_LPDCOCAL_VT_TRACKING_EN_MASK, 0x0 },
>>>> + { PPI_RW_LPDCOCAL_COARSE_CFG, PPI_RW_LPDCOCAL_COARSE_CFG_NCOARSE_START_MASK, 0x1 },
>>>> + { PPI_RW_COMMON_CFG, PPI_RW_COMMON_CFG_CFG_CLK_DIV_FACTOR_MASK, 0x3 },
>>>> +};
>>>> +
>>>> +static const struct isp4phy_mipi_reg_seq startup_seq_common[] = {
>>>> + { PPI_STARTUP_RW_COMMON_DPHY_2, PPI_STARTUP_RW_COMMON_DPHY_2_RCAL_ADDR_MASK, 0x5 },
>>>> + { PPI_RW_TERMCAL_CFG_0, PPI_RW_TERMCAL_CFG_0_TERMCAL_TIMER_MASK, 0x17 },
>>>> + { PPI_RW_OFFSETCAL_CFG_0, PPI_RW_OFFSETCAL_CFG_0_OFFSETCAL_WAIT_THRESH_MASK, 0x4 },
>>>> + { PPI_RW_LPDCOCAL_TIMEBASE, PPI_RW_LPDCOCAL_TIMEBASE_LPDCOCAL_TIMEBASE_MASK, 0x5F },
>>>> + {
>>>> + PPI_RW_LPDCOCAL_TWAIT_CONFIG,
>>>> + PPI_RW_LPDCOCAL_TWAIT_CONFIG_LPDCOCAL_TWAIT_COARSE_MASK, 0x1D
>>>> + },

[snip]

--
Regards,
Bin