Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series
On 04.07.2024 11: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 > --- > drivers/gpu/drm/bridge/Kconfig| 8 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c | 544 ++ > 3 files changed, 553 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index c621be1a99a8..459ad9768234 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -196,6 +196,14 @@ config DRM_MICROCHIP_LVDS_SERIALIZER > help > Support for Microchip's LVDS serializer. > > +config DRM_MICROCHIP_DW_MIPI_DSI > + tristate "Microchip specific extensions for Synopsys DW MIPI DSI" > + depends on DRM_ATMEL_HLCDC > + select DRM_DW_MIPI_DSI > + help > + This selects support for Microchip's SoC specific extensions > + for the Synopsys DesignWare dsi driver. > + > config DRM_NWL_MIPI_DSI > tristate "Northwest Logic MIPI DSI Host controller" > depends on DRM > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 7df87b582dca..aff5052100b9 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -14,6 +14,7 @@ obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o > obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o > obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += > megachips-stdp-ge-b850v3-fw.o > obj-$(CONFIG_DRM_MICROCHIP_LVDS_SERIALIZER) += microchip-lvds.o > +obj-$(CONFIG_DRM_MICROCHIP_DW_MIPI_DSI) += dw-mipi-dsi-mchp.o > obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o > obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > obj-$(CONFIG_DRM_PARADE_PS8640) += parade-ps8640.o > diff --git a/drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c > b/drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c > new file mode 100644 > index ..d2c4525677ab > --- /dev/null > +++ b/drivers/gpu/drm/bridge/dw-mipi-dsi-mchp.c > @@ -0,0 +1,544 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2024 Microchip Technology Inc. and its subsidiaries > + * > + * Author: Manikandan Muralidharan > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DSI_PLL_REF_CLK 2400 > + > +#define DSI_PWR_UP 0x04 > +#define HOST_RESET BIT(0) > +#define HOST_PWRUP 0 > + > +#define DSI_PHY_RSTZ 0xa0 > +#define PHY_SHUTDOWNZ0 > + > +#define DSI_PHY_TST_CTRL00xb4 > +#define PHY_TESTCLK BIT(1) > +#define PHY_UNTESTCLK0 > +#define PHY_TESTCLR BIT(0) > +#define PHY_UNTESTCLR0 > + > +#define DSI_PHY_TST_CTRL10xb8 > +#define PHY_TESTEN BIT(16) > +#define PHY_UNTESTEN 0 > +#define PHY_TESTDOUT(n) (((n) & 0xff) << 8) > +#define PHY_TESTDIN(n) (((n) & 0xff) << 0) These 2 looks like FIELD_PREP() candidates. > + > +#define BYPASS_VCO_RANGE BIT(7) > +#define VCO_RANGE_CON_SEL(val) (((val) & 0x7) << 3) This, too. > +#define VCO_IN_CAP_CON_LOW BIT(1) > + > +#define CP_CURRENT_0 0x2 > +#define CP_CURRENT_1 0x4 > +#define CP_CURRENT_2 0x5 > +#define CP_CURRENT_3 0x6 > +#define CP_CURRENT_4 0x7 > +#define CP_CURRENT_5 0x8 > +#define CP_CURRENT_6 0xc > +#define CP_CURRENT_SEL(val) ((val) & 0xf) This, too. > +#define CP_PROGRAM_ENBIT(7) > + > +#define LPF_RESISTORS_18KOHM 0x0 > +#define LPF_RESISTORS_15_6KOHM 0x1 > +#define LPF_RESISTORS_15KOHM 0x2 > +#define LPF_RESISTORS_14_4KOHM 0x4 > +#define LPF_RESISTORS_12_8KOHM 0x8 > +#define LPF_RESISTORS_11_4KOHM 0x10 > +#define LPF_RESISTORS_10_5KOHM 0x20 Some of these are unsused. > +#define LPF_PROGRAM_EN BIT(6) > +#define LPF_RESISTORS_SEL(val) ((val) & 0x3f) This, too > + > +#define HSFREQRANGE_SEL(val) (((val) & 0x3f) << 1) Unused > + > +#define INPUT_DIVIDER(val) (((val) - 1) & 0x7f) > +#define LOW_PROGRAM_EN 0 > +#define HIGH_PROGRAM_EN BIT(7) > +#define LOOP_DIV_LOW_SEL(val)(((val) - 1) & 0x1f) > +#define LOOP_DIV_HIGH_SEL(val) val) - 1) >> 5) & 0xf) > +#define PLL_LOOP_DIV_EN BIT(5) >
Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series
On 11/07/24 3:28 pm, Conor Dooley wrote: > On Thu, Jul 11, 2024 at 11:05:37AM +0200, Krzysztof Kozlowski wrote: >> On 11/07/2024 10:30,[email protected] wrote: >>> 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 > --- ... > + > +#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 >> Yeah, this should not be a global. Nothing above suggests it. > I think what Krzysztof is suggesting here is use of the static > keyword... Yes, after looking at the implementation in similar platforms, I did find out. Thank you Conor. > >> BTW, no W=1 clang warnings? Are you sure? >> >> Best regards, >> Krzysztof -- Thanks and Regards, Manikandan M.
Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series
On Thu, Jul 11, 2024 at 11:05:37AM +0200, Krzysztof Kozlowski wrote: > On 11/07/2024 10:30, [email protected] wrote: > > 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 > >>> --- > >> > >> > >> ... > >> > >>> + > >>> +#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 > > Yeah, this should not be a global. Nothing above suggests it. I think what Krzysztof is suggesting here is use of the static keyword... > > BTW, no W=1 clang warnings? Are you sure? > > Best regards, > Krzysztof > signature.asc Description: PGP signature
Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series
On 11/07/2024 10:30, [email protected] wrote: > 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 >>> --- >> >> >> ... >> >>> + >>> +#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 Yeah, this should not be a global. Nothing above suggests it. BTW, no W=1 clang warnings? Are you sure? Best regards, Krzysztof
Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series
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
>> ---
>
>
> ...
>
>> +
>> +#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->d
Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series
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
> ---
...
> +
> +#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.
> + 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
