| Message ID | 20190211094705.2845-42-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | omapdrm: drm_bridge and drm_panel support | expand |
Hi Laurent, On 11/02/2019 11:46, Laurent Pinchart wrote: > + /* Get the sampling edge from the endpoint. */ > + of_property_read_u32(ep, "pclk-sample", &pclk_sample); > + of_node_put(ep); > + > + timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH; > + > + switch (pclk_sample) { > + case 0: > + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE > + | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE; > + break; > + case 1: > + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE > + | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE; > + break; > + default: > + return -EINVAL; > + } The default for pclk_sample is just the opposite of what omapdrm's tfp410 used to do. The dts doc file also says that pclk-sample is required, but the driver works fine without it, defaulting to 0. This means that none of the omap dts files with tfp410 work correctly, instead they silently use the wrong settings which may work but easily also won't... As the bus flags are added in this patch for the first time, maybe we can assume that no one is using them, and the default could be made to be the same as was on omapdrm's tfp410? Tomi
On 28/02/2019 12:27, Tomi Valkeinen wrote: > Hi Laurent, > > On 11/02/2019 11:46, Laurent Pinchart wrote: > >> + /* Get the sampling edge from the endpoint. */ >> + of_property_read_u32(ep, "pclk-sample", &pclk_sample); >> + of_node_put(ep); >> + >> + timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH; >> + >> + switch (pclk_sample) { >> + case 0: >> + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE >> + | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE; >> + break; >> + case 1: >> + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE >> + | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE; >> + break; >> + default: >> + return -EINVAL; >> + } > > The default for pclk_sample is just the opposite of what omapdrm's > tfp410 used to do. The dts doc file also says that pclk-sample is > required, but the driver works fine without it, defaulting to 0. > > This means that none of the omap dts files with tfp410 work correctly, > instead they silently use the wrong settings which may work but easily > also won't... > > As the bus flags are added in this patch for the first time, maybe we > can assume that no one is using them, and the default could be made to > be the same as was on omapdrm's tfp410? Aaaand never mind. In omapdrm's driver we were using DRM_BUS_FLAG_SYNC_DRIVE_* variant, here we have SAMPLE variant. So it's fine =). Sorry for the noise. Tomi
On 28/02/2019 12.31, Tomi Valkeinen wrote: > On 28/02/2019 12:27, Tomi Valkeinen wrote: >> Hi Laurent, >> >> On 11/02/2019 11:46, Laurent Pinchart wrote: >> >>> + /* Get the sampling edge from the endpoint. */ >>> + of_property_read_u32(ep, "pclk-sample", &pclk_sample); >>> + of_node_put(ep); >>> + >>> + timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH; >>> + >>> + switch (pclk_sample) { >>> + case 0: >>> + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE >>> + | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE; >>> + break; >>> + case 1: >>> + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE >>> + | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE; >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >> >> The default for pclk_sample is just the opposite of what omapdrm's >> tfp410 used to do. The dts doc file also says that pclk-sample is >> required, but the driver works fine without it, defaulting to 0. >> >> This means that none of the omap dts files with tfp410 work correctly, >> instead they silently use the wrong settings which may work but easily >> also won't... >> >> As the bus flags are added in this patch for the first time, maybe we >> can assume that no one is using them, and the default could be made to >> be the same as was on omapdrm's tfp410? > > Aaaand never mind. In omapdrm's driver we were using > DRM_BUS_FLAG_SYNC_DRIVE_* variant, here we have SAMPLE variant. So it's > fine =). If the pclk-sample is not defined in DT, it will default to 0 which selects SAMPLE_NEGEDGE (== DRIVE_POSEDGE), right? But all the boards where I can find schematics with tfp410 have their EDGE/HTPLG pin pulled up and according to the documentation when EDGE=1 then tfp410 will sample on the rising edge. imho the pclk_sample should be initialized to 1 to avoid regression for most of the boards using tfp410. > > Sorry for the noise. > > Tomi > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 15/03/2019 13:30, Peter Ujfalusi wrote: > > > On 28/02/2019 12.31, Tomi Valkeinen wrote: >> On 28/02/2019 12:27, Tomi Valkeinen wrote: >>> Hi Laurent, >>> >>> On 11/02/2019 11:46, Laurent Pinchart wrote: >>> >>>> + /* Get the sampling edge from the endpoint. */ >>>> + of_property_read_u32(ep, "pclk-sample", &pclk_sample); >>>> + of_node_put(ep); >>>> + >>>> + timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH; >>>> + >>>> + switch (pclk_sample) { >>>> + case 0: >>>> + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE >>>> + | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE; >>>> + break; >>>> + case 1: >>>> + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE >>>> + | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE; >>>> + break; >>>> + default: >>>> + return -EINVAL; >>>> + } >>> >>> The default for pclk_sample is just the opposite of what omapdrm's >>> tfp410 used to do. The dts doc file also says that pclk-sample is >>> required, but the driver works fine without it, defaulting to 0. >>> >>> This means that none of the omap dts files with tfp410 work correctly, >>> instead they silently use the wrong settings which may work but easily >>> also won't... >>> >>> As the bus flags are added in this patch for the first time, maybe we >>> can assume that no one is using them, and the default could be made to >>> be the same as was on omapdrm's tfp410? >> >> Aaaand never mind. In omapdrm's driver we were using >> DRM_BUS_FLAG_SYNC_DRIVE_* variant, here we have SAMPLE variant. So it's >> fine =). > > If the pclk-sample is not defined in DT, it will default to 0 which > selects SAMPLE_NEGEDGE (== DRIVE_POSEDGE), right? > > But all the boards where I can find schematics with tfp410 have their > EDGE/HTPLG pin pulled up and according to the documentation when EDGE=1 > then tfp410 will sample on the rising edge. > > imho the pclk_sample should be initialized to 1 to avoid regression for > most of the boards using tfp410. Define "regression" =). If the omapdrm driver was always using DRIVE_POSEDGE, this driver should also be using DRIVE_POSEDGE, no? If it does the same as the old driver, it can't be a regression. This is, of course, only considering omapdrm based boards. That said, it sounds odd that this would be wrong in the old driver, but then again, it might well be, as code related to these sync signals has changed sooo many times, and the related DSS registers are somewhat confusing. That gave me an idea, I need to write an rwmem script to print the DSS sync flags in plain english... Tomi
On 15/03/2019 14.07, Tomi Valkeinen wrote: >> If the pclk-sample is not defined in DT, it will default to 0 which >> selects SAMPLE_NEGEDGE (== DRIVE_POSEDGE), right? >> >> But all the boards where I can find schematics with tfp410 have their >> EDGE/HTPLG pin pulled up and according to the documentation when EDGE=1 >> then tfp410 will sample on the rising edge. >> >> imho the pclk_sample should be initialized to 1 to avoid regression for >> most of the boards using tfp410. > > Define "regression" =). If the omapdrm driver was always using > DRIVE_POSEDGE, this driver should also be using DRIVE_POSEDGE, no? If it > does the same as the old driver, it can't be a regression. This is, of > course, only considering omapdrm based boards. > > That said, it sounds odd that this would be wrong in the old driver, but > then again, it might well be, as code related to these sync signals has > changed sooo many times, and the related DSS registers are somewhat > confusing. On the boards data skew is enabled as well with maximum delay selected with DK1=DK2=DK3=1, so sampling of data is delayed by 3 * 350 ps, so about 1 ns, not much, but might be enough for the signal to transition on the bus so even if the HW drivers on POSEDGE and tfp410 samples on POSEDGE (+1ns delay from the edge) we don't see corruption? > That gave me an idea, I need to write an rwmem script to print the DSS > sync flags in plain english... > > Tomi > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 15/03/2019 14:28, Peter Ujfalusi wrote: > On 15/03/2019 14.07, Tomi Valkeinen wrote: >>> If the pclk-sample is not defined in DT, it will default to 0 which >>> selects SAMPLE_NEGEDGE (== DRIVE_POSEDGE), right? >>> >>> But all the boards where I can find schematics with tfp410 have their >>> EDGE/HTPLG pin pulled up and according to the documentation when EDGE=1 >>> then tfp410 will sample on the rising edge. >>> >>> imho the pclk_sample should be initialized to 1 to avoid regression for >>> most of the boards using tfp410. >> >> Define "regression" =). If the omapdrm driver was always using >> DRIVE_POSEDGE, this driver should also be using DRIVE_POSEDGE, no? If it >> does the same as the old driver, it can't be a regression. This is, of >> course, only considering omapdrm based boards. >> >> That said, it sounds odd that this would be wrong in the old driver, but >> then again, it might well be, as code related to these sync signals has >> changed sooo many times, and the related DSS registers are somewhat >> confusing. > > On the boards data skew is enabled as well with maximum delay selected > with DK1=DK2=DK3=1, so sampling of data is delayed by 3 * 350 ps, so > about 1 ns, not much, but might be enough for the signal to transition > on the bus so even if the HW drivers on POSEDGE and tfp410 samples on > POSEDGE (+1ns delay from the edge) we don't see corruption? Ok. I don't think anyone has looked at it that closely. So, the syncs could well be wrong. Still, not a regression if it's the same way as it was. If the fixed default works fine (or better) than the wrong ones, I think that can be the default. But I'd be careful about changing what the default is, if we've had it the old way for a long time. Tomi
On 15/03/2019 15.30, Tomi Valkeinen wrote: > On 15/03/2019 14:28, Peter Ujfalusi wrote: >> On 15/03/2019 14.07, Tomi Valkeinen wrote: >>>> If the pclk-sample is not defined in DT, it will default to 0 which >>>> selects SAMPLE_NEGEDGE (== DRIVE_POSEDGE), right? >>>> >>>> But all the boards where I can find schematics with tfp410 have their >>>> EDGE/HTPLG pin pulled up and according to the documentation when EDGE=1 >>>> then tfp410 will sample on the rising edge. >>>> >>>> imho the pclk_sample should be initialized to 1 to avoid regression for >>>> most of the boards using tfp410. >>> >>> Define "regression" =). If the omapdrm driver was always using >>> DRIVE_POSEDGE, this driver should also be using DRIVE_POSEDGE, no? If it >>> does the same as the old driver, it can't be a regression. This is, of >>> course, only considering omapdrm based boards. >>> >>> That said, it sounds odd that this would be wrong in the old driver, but >>> then again, it might well be, as code related to these sync signals has >>> changed sooo many times, and the related DSS registers are somewhat >>> confusing. >> >> On the boards data skew is enabled as well with maximum delay selected >> with DK1=DK2=DK3=1, so sampling of data is delayed by 3 * 350 ps, so >> about 1 ns, not much, but might be enough for the signal to transition >> on the bus so even if the HW drivers on POSEDGE and tfp410 samples on >> POSEDGE (+1ns delay from the edge) we don't see corruption? > > Ok. I don't think anyone has looked at it that closely. So, the syncs > could well be wrong. Still, not a regression if it's the same way as it was. True. > If the fixed default works fine (or better) than the wrong ones, I think > that can be the default. But I'd be careful about changing what the > default is, if we've had it the old way for a long time. The HW default in I2C mode is the EDGE=1 so SAMPLE_POSEDGE and it looks to me that the same default was implemented via hw control on the boards I can access the schematics. If it was the other way in the past, then it is safer to not change the default in the driver, but probably put a comment somewhere that the HW default is the opposite? In any case if the i2c control is implemented we will read and/or configure the tfp410 anyways. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tomi, On Fri, Mar 15, 2019 at 03:30:11PM +0200, Tomi Valkeinen wrote: > On 15/03/2019 14:28, Peter Ujfalusi wrote: > > On 15/03/2019 14.07, Tomi Valkeinen wrote: > >>> If the pclk-sample is not defined in DT, it will default to 0 which > >>> selects SAMPLE_NEGEDGE (== DRIVE_POSEDGE), right? > >>> > >>> But all the boards where I can find schematics with tfp410 have their > >>> EDGE/HTPLG pin pulled up and according to the documentation when EDGE=1 > >>> then tfp410 will sample on the rising edge. > >>> > >>> imho the pclk_sample should be initialized to 1 to avoid regression for > >>> most of the boards using tfp410. > >> > >> Define "regression" =). If the omapdrm driver was always using > >> DRIVE_POSEDGE, this driver should also be using DRIVE_POSEDGE, no? If it > >> does the same as the old driver, it can't be a regression. This is, of > >> course, only considering omapdrm based boards. > >> > >> That said, it sounds odd that this would be wrong in the old driver, but > >> then again, it might well be, as code related to these sync signals has > >> changed sooo many times, and the related DSS registers are somewhat > >> confusing. > > > > On the boards data skew is enabled as well with maximum delay selected > > with DK1=DK2=DK3=1, so sampling of data is delayed by 3 * 350 ps, so > > about 1 ns, not much, but might be enough for the signal to transition > > on the bus so even if the HW drivers on POSEDGE and tfp410 samples on > > POSEDGE (+1ns delay from the edge) we don't see corruption? > > Ok. I don't think anyone has looked at it that closely. So, the syncs > could well be wrong. Still, not a regression if it's the same way as it was. > > If the fixed default works fine (or better) than the wrong ones, I think > that can be the default. But I'd be careful about changing what the > default is, if we've had it the old way for a long time. I don't object changing the default (given proper testing), but that should be done on top of this series then.
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c index b0213d573434..285be4a0f4bd 100644 --- a/drivers/gpu/drm/bridge/ti-tfp410.c +++ b/drivers/gpu/drm/bridge/ti-tfp410.c @@ -34,6 +34,8 @@ struct tfp410 { struct delayed_work hpd_work; struct gpio_desc *powerdown; + struct drm_bridge_timings timings; + struct device *dev; }; @@ -180,6 +182,70 @@ static irqreturn_t tfp410_hpd_irq_thread(int irq, void *arg) return IRQ_HANDLED; } +static const struct drm_bridge_timings tfp410_default_timings = { + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE + | DRM_BUS_FLAG_DE_HIGH, + .setup_time_ps = 1200, + .hold_time_ps = 1300, +}; + +static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c) +{ + struct drm_bridge_timings *timings = &dvi->timings; + struct device_node *ep; + u32 pclk_sample = 0; + s32 deskew = 0; + + /* Start with defaults. */ + *timings = tfp410_default_timings; + + if (i2c) + /* + * In I2C mode timings are configured through the I2C interface. + * As the driver doesn't support I2C configuration yet, we just + * go with the defaults (BSEL=1, DSEL=1, DKEN=0, EDGE=1). + */ + return 0; + + /* + * In non-I2C mode, timings are configured through the BSEL, DSEL, DKEN + * and EDGE pins. They are specified in DT through endpoint properties + * and vendor-specific properties. + */ + ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0); + if (!ep) + return -EINVAL; + + /* Get the sampling edge from the endpoint. */ + of_property_read_u32(ep, "pclk-sample", &pclk_sample); + of_node_put(ep); + + timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH; + + switch (pclk_sample) { + case 0: + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE + | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE; + break; + case 1: + timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE + | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE; + break; + default: + return -EINVAL; + } + + /* Get the setup and hold time from vendor-specific properties. */ + of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew); + if (deskew < -4 || deskew > 3) + return -EINVAL; + + timings->setup_time_ps = min(0, 1200 - 350 * deskew); + timings->hold_time_ps = min(0, 1300 + 350 * deskew); + + return 0; +} + static int tfp410_get_connector_properties(struct tfp410 *dvi) { struct device_node *connector_node, *ddc_phandle; @@ -223,7 +289,7 @@ static int tfp410_get_connector_properties(struct tfp410 *dvi) return ret; } -static int tfp410_init(struct device *dev) +static int tfp410_init(struct device *dev, bool i2c) { struct tfp410 *dvi; int ret; @@ -240,8 +306,13 @@ static int tfp410_init(struct device *dev) dvi->bridge.funcs = &tfp410_bridge_funcs; dvi->bridge.of_node = dev->of_node; + dvi->bridge.timings = &dvi->timings; dvi->dev = dev; + ret = tfp410_parse_timings(dvi, i2c); + if (ret) + goto fail; + ret = tfp410_get_connector_properties(dvi); if (ret) goto fail; @@ -294,7 +365,7 @@ static int tfp410_fini(struct device *dev) static int tfp410_probe(struct platform_device *pdev) { - return tfp410_init(&pdev->dev); + return tfp410_init(&pdev->dev, false); } static int tfp410_remove(struct platform_device *pdev) @@ -331,7 +402,7 @@ static int tfp410_i2c_probe(struct i2c_client *client, return -ENXIO; } - return tfp410_init(&client->dev); + return tfp410_init(&client->dev, true); } static int tfp410_i2c_remove(struct i2c_client *client)