Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series

[<Manikandan.M@microchip.com>]

Hi Krzysztof,

On 04/07/24 4:27 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 04/07/2024 10:48, Manikandan Muralidharan wrote:
>> Add the Microchip's DSI controller wrapper driver that uses
>> the Synopsys DesignWare MIPI DSI host controller bridge.
>>
>> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
>> ---
> 
> 
> ...
> 
>> +
>> +#define HSTT(_maxfreq, _c_lp2hs, _c_hs2lp, _d_lp2hs, _d_hs2lp)       \
>> +{                                    \
>> +     .maxfreq = _maxfreq,            \
>> +     .timing = {                     \
>> +             .clk_lp2hs = _c_lp2hs,  \
>> +             .clk_hs2lp = _c_hs2lp,  \
>> +             .data_lp2hs = _d_lp2hs, \
>> +             .data_hs2lp = _d_hs2lp, \
>> +     }                               \
>> +}
>> +
>> +struct hstt hstt_table[] = {
> 
> So more globals? No.

In the sam9x7 datasheet, the high speed transition time for data and
clock lane at different freq for the DSI controller ranges are tabulated
with constant values.
I referred other similar platforms for the functionality and found 
similar way of implementation,  only a few had equations to derive the 
low power and high speed timings.I am not able to come up with a more 
efficient method. If there is something I am missing, please suggest.
TIA
> 
>> +     HSTT(90,  32, 20,  26, 13),
>> +     HSTT(100,  35, 23,  28, 14),
>> +     HSTT(110,  32, 22,  26, 13),
>> +     HSTT(130,  31, 20,  27, 13),
>> +     HSTT(140,  33, 22,  26, 14),
>> +     HSTT(150,  33, 21,  26, 14),
>> +     HSTT(170,  32, 20,  27, 13),
>> +     HSTT(180,  36, 23,  30, 15),
>> +     HSTT(200,  40, 22,  33, 15),
>> +     HSTT(220,  40, 22,  33, 15),
>> +     HSTT(240,  44, 24,  36, 16),
>> +     HSTT(250,  48, 24,  38, 17),
>> +     HSTT(270,  48, 24,  38, 17),
>> +     HSTT(300,  50, 27,  41, 18),
>> +     HSTT(330,  56, 28,  45, 18),
>> +     HSTT(360,  59, 28,  48, 19),
>> +     HSTT(400,  61, 30,  50, 20),
>> +     HSTT(450,  67, 31,  55, 21),
>> +     HSTT(500,  73, 31,  59, 22),
>> +     HSTT(550,  79, 36,  63, 24),
>> +     HSTT(600,  83, 37,  68, 25),
>> +     HSTT(650,  90, 38,  73, 27),
>> +     HSTT(700,  95, 40,  77, 28),
>> +     HSTT(750, 102, 40,  84, 28),
>> +     HSTT(800, 106, 42,  87, 30),
>> +     HSTT(850, 113, 44,  93, 31),
>> +     HSTT(900, 118, 47,  98, 32),
>> +     HSTT(950, 124, 47, 102, 34),
>> +     HSTT(1000, 130, 49, 107, 35),
>> +};
>> +
> 
> ...
> 
>> +
>> +static void dw_mipi_dsi_mchp_power_on(void *priv_data)
>> +{
>> +     struct dw_mipi_dsi_mchp *dsi = priv_data;
>> +
>> +     /* Enable the DSI wrapper */
>> +     dsi_write(dsi, DSI_PWR_UP, HOST_PWRUP);
>> +}
>> +
>> +static void dw_mipi_dsi_mchp_power_off(void *priv_data)
>> +{
>> +     struct dw_mipi_dsi_mchp *dsi = priv_data;
>> +
>> +     /* Disable the DSI wrapper */
>> +     dsi_write(dsi, DSI_PWR_UP, HOST_RESET);
>> +}
>> +
>> +struct dw_mipi_dsi_phy_ops dw_mipi_dsi_mchp_phy_ops = {
> 
> Why this is not static?
> 
> Why this is not const?
> 
>> +     .init = dw_mipi_dsi_mchp_init,
>> +     .power_on = dw_mipi_dsi_mchp_power_on,
>> +     .power_off = dw_mipi_dsi_mchp_power_off,
>> +     .get_lane_mbps = dw_mipi_dsi_mchp_get_lane_mbps,
>> +     .get_timing = dw_mipi_dsi_mchp_get_timing,
>> +};
>> +
>> +static int dw_mipi_dsi_mchp_probe(struct platform_device *pdev)
>> +{
>> +     struct dw_mipi_dsi_mchp *dsi;
>> +     struct resource *res;
>> +     struct regmap *sfr;
>> +     const struct dw_mipi_dsi_mchp_chip_data *cdata;
>> +     int ret;
>> +
>> +     dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
>> +     if (!dsi)
>> +             return -ENOMEM;
>> +
>> +     dsi->dev = &pdev->dev;
>> +     cdata = of_device_get_match_data(dsi->dev);
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     dsi->base = devm_ioremap_resource(&pdev->dev, res);
> 
> There is a helper for these two.
> 
>> +     if (IS_ERR(dsi->base)) {
>> +             ret = PTR_ERR(dsi->base);
>> +             dev_err(dsi->dev, "Unable to get DSI Base address: %d\n", ret);
> 
> return dev_err_probe
> 
>> +             return ret;
>> +     }
>> +
>> +     dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
>> +     if (IS_ERR(dsi->pclk)) {
>> +             ret = PTR_ERR(dsi->pclk);
>> +             dev_err(dsi->dev, "Unable to get pclk: %d\n", ret);
> 
> return dev_err_probe
> 
> You are upstreaming some old code, aren't you?
> 
>> +             return ret;
>> +     }
>> +
>> +     dsi->pllref_clk = devm_clk_get(&pdev->dev, "refclk");
>> +     if (IS_ERR(dsi->pllref_clk)) {
>> +             ret = PTR_ERR(dsi->pllref_clk);
>> +             dev_err(dsi->dev, "Unable to get DSI PHY PLL reference clock: %d\n",
> 
> return dev_err_probe
> 
> 
>> +                     ret);
>> +             return ret;
>> +     }
>> +
>> +     clk_set_rate(dsi->pllref_clk, DSI_PLL_REF_CLK);
>> +     if (clk_get_rate(dsi->pllref_clk) != DSI_PLL_REF_CLK) {
>> +             dev_err(dsi->dev, "Failed to set DSI PHY PLL reference clock\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     ret = clk_prepare_enable(dsi->pllref_clk);
> 
> Enable clock later, so your error paths will be simpler.
> 
>> +     if (ret) {
>> +             dev_err(dsi->dev, "Failed to enable DSI PHY PLL reference clock: %d\n",
>> +                     ret);
>> +             return ret;
>> +     }
>> +
>> +     sfr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "microchip,sfr");
>> +     if (IS_ERR_OR_NULL(sfr)) {
> 
> NULL? Can it be NULL?
> 
>> +             ret = PTR_ERR(sfr);
>> +             dev_err(dsi->dev, "Failed to get handle on Special Function Register: %d\n",
>> +                     ret);
> 
> ret = dev_err_probe
> 
>> +             goto err_dsi_probe;
>> +     }
>> +     /* Select DSI in SFR's ISS Configuration Register */
>> +     ret = regmap_write(sfr, SFR_ISS_CFG, ISS_CFG_DSI_MODE);
>> +     if (ret) {
>> +             dev_err(dsi->dev, "Failed to enable DSI in SFR ISS configuration register: %d\n",
>> +                     ret);
>> +             goto err_dsi_probe;
>> +     }
> 
> 
> 
> Best regards,
> Krzysztof
> 

-- 
Thanks and Regards,
Manikandan M.