| Message ID | 20220402052451.2517469-3-victor.liu@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: phy-fsl-imx8-mipi-dphy: Add i.MX8qxp LVDS PHY mode support | expand |
On 02-04-22, 13:24, Liu Ying wrote: > This patch allows LVDS PHYs to be configured through > the generic functions and through a custom structure > added to the generic union. > > The parameters added here are based on common LVDS PHY > implementation practices. The set of parameters > should cover all potential users. > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > Cc: Vinod Koul <vkoul@kernel.org> > Cc: NXP Linux Team <linux-imx@nxp.com> > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v5->v6: > * Rebase upon v5.17-rc1. > > v4->v5: > * Align kernel-doc style to include/linux/phy/phy.h. (Vinod) > * Trivial tweaks. > * Drop Robert's R-b tag. > > v3->v4: > * Add Robert's R-b tag. > > v2->v3: > * No change. > > v1->v2: > * No change. > > include/linux/phy/phy-lvds.h | 32 ++++++++++++++++++++++++++++++++ > include/linux/phy/phy.h | 4 ++++ > 2 files changed, 36 insertions(+) > create mode 100644 include/linux/phy/phy-lvds.h > > diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-lvds.h > new file mode 100644 > index 000000000000..7a2f4747f624 > --- /dev/null > +++ b/include/linux/phy/phy-lvds.h > @@ -0,0 +1,32 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright 2020 NXP 2022 now > + */ > + > +#ifndef __PHY_LVDS_H_ > +#define __PHY_LVDS_H_ > + > +/** > + * struct phy_configure_opts_lvds - LVDS configuration set > + * @bits_per_lane_and_dclk_cycle: Number of bits per data lane and > + * differential clock cycle. What does it mean by bits per data lane and differential clock cycle? > + * @differential_clk_rate: Clock rate, in Hertz, of the LVDS > + * differential clock. > + * @lanes: Number of active, consecutive, > + * data lanes, starting from lane 0, > + * used for the transmissions. > + * @is_slave: Boolean, true if the phy is a slave > + * which works together with a master > + * phy to support dual link transmission, > + * otherwise a regular phy or a master phy. > + * > + * This structure is used to represent the configuration state of a LVDS phy. > + */ > +struct phy_configure_opts_lvds { > + unsigned int bits_per_lane_and_dclk_cycle; > + unsigned long differential_clk_rate; > + unsigned int lanes; > + bool is_slave; > +}; Where is the user of this new configuration? Can you post that patch for reference as well please
Hi Vinod, On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > On 02-04-22, 13:24, Liu Ying wrote: > > This patch allows LVDS PHYs to be configured through > > the generic functions and through a custom structure > > added to the generic union. > > > > The parameters added here are based on common LVDS PHY > > implementation practices. The set of parameters > > should cover all potential users. > > > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > > Cc: Vinod Koul <vkoul@kernel.org> > > Cc: NXP Linux Team <linux-imx@nxp.com> > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > v5->v6: > > * Rebase upon v5.17-rc1. > > > > v4->v5: > > * Align kernel-doc style to include/linux/phy/phy.h. (Vinod) > > * Trivial tweaks. > > * Drop Robert's R-b tag. > > > > v3->v4: > > * Add Robert's R-b tag. > > > > v2->v3: > > * No change. > > > > v1->v2: > > * No change. > > > > include/linux/phy/phy-lvds.h | 32 ++++++++++++++++++++++++++++++++ > > include/linux/phy/phy.h | 4 ++++ > > 2 files changed, 36 insertions(+) > > create mode 100644 include/linux/phy/phy-lvds.h > > > > diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy- > > lvds.h > > new file mode 100644 > > index 000000000000..7a2f4747f624 > > --- /dev/null > > +++ b/include/linux/phy/phy-lvds.h > > @@ -0,0 +1,32 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright 2020 NXP > > 2022 now I may change it to 'Copyright 2020,2022 NXP'. > > > + */ > > + > > +#ifndef __PHY_LVDS_H_ > > +#define __PHY_LVDS_H_ > > + > > +/** > > + * struct phy_configure_opts_lvds - LVDS configuration set > > + * @bits_per_lane_and_dclk_cycle: Number of bits per data lane > > and > > + * differential clock cycle. > > What does it mean by bits per data lane and differential clock cycle? Please check Documentation/devicetree/bindings/display/panel/lvds.yaml. lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the number of slots. But, I don't find the word 'slot' in my lvds relevant specs which mentioned in lvds.yaml, so 'slots' is probably not a generic name(lvds.yaml is for display panel). So, I use 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > > + * @differential_clk_rate: Clock rate, in Hertz, of the > > LVDS > > + * differential clock. > > + * @lanes: Number of active, consecutive, > > + * data lanes, starting from lane > > 0, > > + * used for the transmissions. > > + * @is_slave: Boolean, true if the > > phy is a slave > > + * which works together with a > > master > > + * phy to support dual link > > transmission, > > + * otherwise a regular phy or a > > master phy. > > + * > > + * This structure is used to represent the configuration state of > > a LVDS phy. > > + */ > > +struct phy_configure_opts_lvds { > > + unsigned int bits_per_lane_and_dclk_cycle; > > + unsigned long differential_clk_rate; > > + unsigned int lanes; > > + bool is_slave; > > +}; > > Where is the user of this new configuration? Can you post that patch > for > reference as well please Patch 5/5 uses it. Also, I posted two drm bridge drivers[1][2] which use it. [1] https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-13-git-send-email-victor.liu@nxp.com/ [2] https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-14-git-send-email-victor.liu@nxp.com/ Regards, Liu Ying
On 13-04-22, 18:04, Liu Ying wrote: > Hi Vinod, > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > On 02-04-22, 13:24, Liu Ying wrote: > > > This patch allows LVDS PHYs to be configured through > > > the generic functions and through a custom structure > > > added to the generic union. > > > > > > The parameters added here are based on common LVDS PHY > > > implementation practices. The set of parameters > > > should cover all potential users. > > > > > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > > > Cc: Vinod Koul <vkoul@kernel.org> > > > Cc: NXP Linux Team <linux-imx@nxp.com> > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > --- > > > v5->v6: > > > * Rebase upon v5.17-rc1. > > > > > > v4->v5: > > > * Align kernel-doc style to include/linux/phy/phy.h. (Vinod) > > > * Trivial tweaks. > > > * Drop Robert's R-b tag. > > > > > > v3->v4: > > > * Add Robert's R-b tag. > > > > > > v2->v3: > > > * No change. > > > > > > v1->v2: > > > * No change. > > > > > > include/linux/phy/phy-lvds.h | 32 ++++++++++++++++++++++++++++++++ > > > include/linux/phy/phy.h | 4 ++++ > > > 2 files changed, 36 insertions(+) > > > create mode 100644 include/linux/phy/phy-lvds.h > > > > > > diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy- > > > lvds.h > > > new file mode 100644 > > > index 000000000000..7a2f4747f624 > > > --- /dev/null > > > +++ b/include/linux/phy/phy-lvds.h > > > @@ -0,0 +1,32 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +/* > > > + * Copyright 2020 NXP > > > > 2022 now > > I may change it to 'Copyright 2020,2022 NXP'. > > > > > > + */ > > > + > > > +#ifndef __PHY_LVDS_H_ > > > +#define __PHY_LVDS_H_ > > > + > > > +/** > > > + * struct phy_configure_opts_lvds - LVDS configuration set > > > + * @bits_per_lane_and_dclk_cycle: Number of bits per data lane > > > and > > > + * differential clock cycle. > > > > What does it mean by bits per data lane and differential clock cycle? > > Please check Documentation/devicetree/bindings/display/panel/lvds.yaml. > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the > number of slots. But, I don't find the word 'slot' in my lvds relevant > specs which mentioned in lvds.yaml, so 'slots' is probably not a > generic name(lvds.yaml is for display panel). So, I use > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. variable name is fine, explanation for bit per lane and differential clock cycle didnt help, maybe add better explanation of what this variable means > > > > > > + * @differential_clk_rate: Clock rate, in Hertz, of the > > > LVDS > > > + * differential clock. > > > + * @lanes: Number of active, consecutive, > > > + * data lanes, starting from lane > > > 0, > > > + * used for the transmissions. > > > + * @is_slave: Boolean, true if the > > > phy is a slave > > > + * which works together with a > > > master > > > + * phy to support dual link > > > transmission, > > > + * otherwise a regular phy or a > > > master phy. > > > + * > > > + * This structure is used to represent the configuration state of > > > a LVDS phy. > > > + */ > > > +struct phy_configure_opts_lvds { > > > + unsigned int bits_per_lane_and_dclk_cycle; > > > + unsigned long differential_clk_rate; > > > + unsigned int lanes; > > > + bool is_slave; > > > +}; > > > > Where is the user of this new configuration? Can you post that patch > > for > > reference as well please > > Patch 5/5 uses it. Also, I posted two drm bridge drivers[1][2] which > use it. That is phy driver not the user > > [1] > https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-13-git-send-email-victor.liu@nxp.com/ > [2] > https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-14-git-send-email-victor.liu@nxp.com/ this is helpful, thanks
On Wed, 2022-04-13 at 16:19 +0530, Vinod Koul wrote: > On 13-04-22, 18:04, Liu Ying wrote: > > Hi Vinod, > > > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > > On 02-04-22, 13:24, Liu Ying wrote: > > > > This patch allows LVDS PHYs to be configured through > > > > the generic functions and through a custom structure > > > > added to the generic union. > > > > > > > > The parameters added here are based on common LVDS PHY > > > > implementation practices. The set of parameters > > > > should cover all potential users. > > > > > > > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > > > > Cc: Vinod Koul <vkoul@kernel.org> > > > > Cc: NXP Linux Team <linux-imx@nxp.com> > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > --- [...] > > > > + */ > > > > + > > > > +#ifndef __PHY_LVDS_H_ > > > > +#define __PHY_LVDS_H_ > > > > + > > > > +/** > > > > + * struct phy_configure_opts_lvds - LVDS configuration set > > > > + * @bits_per_lane_and_dclk_cycle: Number of bits per data > > > > lane > > > > and > > > > + * differential clock > > > > cycle. > > > > > > What does it mean by bits per data lane and differential clock > > > cycle? > > > > Please check > > Documentation/devicetree/bindings/display/panel/lvds.yaml. > > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the > > number of slots. But, I don't find the word 'slot' in my lvds > > relevant > > specs which mentioned in lvds.yaml, so 'slots' is probably not a > > generic name(lvds.yaml is for display panel). So, I use > > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > variable name is fine, explanation for bit per lane and differential > clock cycle didnt help, maybe add better explanation of what this > variable means I may add an example diagram as below... > > > > > > > > > > + * @differential_clk_rate: Clock rate, in Hertz, > > > > of the > > > > LVDS > > > > + * differential clock. > > > > + * @lanes: Number of active, > > > > consecutive, > > > > + * data lanes, starting > > > > from lane > > > > 0, > > > > + * used for the > > > > transmissions. > > > > + * @is_slave: Boolean, true if the > > > > phy is a slave > > > > + * which works together > > > > with a > > > > master > > > > + * phy to support dual > > > > link > > > > transmission, > > > > + * otherwise a regular phy > > > > or a > > > > master phy. ---------------------------------8<------------------------------------ + * This is an example with 4 lanes and 7 bits per data lane and differential + * clock cycle: + * + * :: + * |<------------- one differential clock cycle -------- ----->| + * ________________ ____________
On 13-04-22, 20:39, Liu Ying wrote: > On Wed, 2022-04-13 at 16:19 +0530, Vinod Koul wrote: > > On 13-04-22, 18:04, Liu Ying wrote: > > > Hi Vinod, > > > > > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > > > On 02-04-22, 13:24, Liu Ying wrote: > > > > > This patch allows LVDS PHYs to be configured through > > > > > the generic functions and through a custom structure > > > > > added to the generic union. > > > > > > > > > > The parameters added here are based on common LVDS PHY > > > > > implementation practices. The set of parameters > > > > > should cover all potential users. > > > > > > > > > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > > > > > Cc: Vinod Koul <vkoul@kernel.org> > > > > > Cc: NXP Linux Team <linux-imx@nxp.com> > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > > --- > > [...] > > > > > > + */ > > > > > + > > > > > +#ifndef __PHY_LVDS_H_ > > > > > +#define __PHY_LVDS_H_ > > > > > + > > > > > +/** > > > > > + * struct phy_configure_opts_lvds - LVDS configuration set > > > > > + * @bits_per_lane_and_dclk_cycle: Number of bits per data > > > > > lane > > > > > and > > > > > + * differential clock > > > > > cycle. > > > > > > > > What does it mean by bits per data lane and differential clock > > > > cycle? > > > > > > Please check > > > Documentation/devicetree/bindings/display/panel/lvds.yaml. > > > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the > > > number of slots. But, I don't find the word 'slot' in my lvds > > > relevant > > > specs which mentioned in lvds.yaml, so 'slots' is probably not a > > > generic name(lvds.yaml is for display panel). So, I use > > > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > > > variable name is fine, explanation for bit per lane and differential > > clock cycle didnt help, maybe add better explanation of what this > > variable means > > I may add an example diagram as below... Not really a diagram, you can add if you like.. But something which explains in a sentence or few about the variable. bits per lane per differential clock cycle ?
On Thu, 2022-04-14 at 11:07 +0530, Vinod Koul wrote: > On 13-04-22, 20:39, Liu Ying wrote: > > On Wed, 2022-04-13 at 16:19 +0530, Vinod Koul wrote: > > > On 13-04-22, 18:04, Liu Ying wrote: > > > > Hi Vinod, > > > > > > > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > > > > On 02-04-22, 13:24, Liu Ying wrote: > > > > > > This patch allows LVDS PHYs to be configured through > > > > > > the generic functions and through a custom structure > > > > > > added to the generic union. > > > > > > > > > > > > The parameters added here are based on common LVDS PHY > > > > > > implementation practices. The set of parameters > > > > > > should cover all potential users. > > > > > > > > > > > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > > > > > > Cc: Vinod Koul <vkoul@kernel.org> > > > > > > Cc: NXP Linux Team <linux-imx@nxp.com> > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > > > > > --- > > > > [...] > > > > > > > > + */ > > > > > > + > > > > > > +#ifndef __PHY_LVDS_H_ > > > > > > +#define __PHY_LVDS_H_ > > > > > > + > > > > > > +/** > > > > > > + * struct phy_configure_opts_lvds - LVDS configuration set > > > > > > + * @bits_per_lane_and_dclk_cycle: Number of bits per data > > > > > > lane > > > > > > and > > > > > > + * differential clock > > > > > > cycle. > > > > > > > > > > What does it mean by bits per data lane and differential > > > > > clock > > > > > cycle? > > > > > > > > Please check > > > > Documentation/devicetree/bindings/display/panel/lvds.yaml. > > > > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means > > > > the > > > > number of slots. But, I don't find the word 'slot' in my lvds > > > > relevant > > > > specs which mentioned in lvds.yaml, so 'slots' is probably not > > > > a > > > > generic name(lvds.yaml is for display panel). So, I use > > > > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > > > > > variable name is fine, explanation for bit per lane and > > > differential > > > clock cycle didnt help, maybe add better explanation of what this > > > variable means > > > > I may add an example diagram as below... > > Not really a diagram, you can add if you like.. But something which > explains in a sentence or few about the variable. Ok, no diagram. > > bits per lane per differential clock cycle ? Ok, will use this explaination. Regards, Liu Ying
diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-lvds.h new file mode 100644 index 000000000000..7a2f4747f624 --- /dev/null +++ b/include/linux/phy/phy-lvds.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright 2020 NXP + */ + +#ifndef __PHY_LVDS_H_ +#define __PHY_LVDS_H_ + +/** + * struct phy_configure_opts_lvds - LVDS configuration set + * @bits_per_lane_and_dclk_cycle: Number of bits per data lane and + * differential clock cycle. + * @differential_clk_rate: Clock rate, in Hertz, of the LVDS + * differential clock. + * @lanes: Number of active, consecutive, + * data lanes, starting from lane 0, + * used for the transmissions. + * @is_slave: Boolean, true if the phy is a slave + * which works together with a master + * phy to support dual link transmission, + * otherwise a regular phy or a master phy. + * + * This structure is used to represent the configuration state of a LVDS phy. + */ +struct phy_configure_opts_lvds { + unsigned int bits_per_lane_and_dclk_cycle; + unsigned long differential_clk_rate; + unsigned int lanes; + bool is_slave; +}; + +#endif /* __PHY_LVDS_H_ */ diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index f3286f4cd306..b1413757fcc3 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -17,6 +17,7 @@ #include <linux/regulator/consumer.h> #include <linux/phy/phy-dp.h> +#include <linux/phy/phy-lvds.h> #include <linux/phy/phy-mipi-dphy.h> struct phy; @@ -57,10 +58,13 @@ enum phy_media { * the MIPI_DPHY phy mode. * @dp: Configuration set applicable for phys supporting * the DisplayPort protocol. + * @lvds: Configuration set applicable for phys supporting + * the LVDS phy mode. */ union phy_configure_opts { struct phy_configure_opts_mipi_dphy mipi_dphy; struct phy_configure_opts_dp dp; + struct phy_configure_opts_lvds lvds; }; /**
This patch allows LVDS PHYs to be configured through the generic functions and through a custom structure added to the generic union. The parameters added here are based on common LVDS PHY implementation practices. The set of parameters should cover all potential users. Cc: Kishon Vijay Abraham I <kishon@ti.com> Cc: Vinod Koul <vkoul@kernel.org> Cc: NXP Linux Team <linux-imx@nxp.com> Signed-off-by: Liu Ying <victor.liu@nxp.com> --- v5->v6: * Rebase upon v5.17-rc1. v4->v5: * Align kernel-doc style to include/linux/phy/phy.h. (Vinod) * Trivial tweaks. * Drop Robert's R-b tag. v3->v4: * Add Robert's R-b tag. v2->v3: * No change. v1->v2: * No change. include/linux/phy/phy-lvds.h | 32 ++++++++++++++++++++++++++++++++ include/linux/phy/phy.h | 4 ++++ 2 files changed, 36 insertions(+) create mode 100644 include/linux/phy/phy-lvds.h