| Message ID | 1658477520-13551-3-git-send-email-radhey.shyam.pandey@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | macb: add zynqmp SGMII dynamic configuration support | expand |
On 22.07.2022 11:12, Radhey Shyam Pandey wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Add support for the dynamic configuration which takes care of configuring > the GEM secure space configuration registers using EEMI APIs. High level > sequence is to: > - Check for the PM dynamic configuration support, if no error proceed with > GEM dynamic configurations(next steps) otherwise skip the dynamic > configuration. > - Configure GEM Fixed configurations. > - Configure GEM_CLK_CTRL (gemX_sgmii_mode). > - Trigger GEM reset. > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> > --- > drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 7eb7822cd184..97f77fa9e165 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -38,6 +38,7 @@ > #include <linux/pm_runtime.h> > #include <linux/ptp_classify.h> > #include <linux/reset.h> > +#include <linux/firmware/xlnx-zynqmp.h> > #include "macb.h" > > /* This structure is only used for MACB on SiFive FU540 devices */ > @@ -4621,6 +4622,25 @@ static int init_reset_optional(struct platform_device *pdev) > "failed to init SGMII PHY\n"); > } > > + ret = zynqmp_pm_is_function_supported(PM_IOCTL, IOCTL_SET_GEM_CONFIG); > + if (!ret) { > + u32 pm_info[2]; > + > + ret = of_property_read_u32_array(pdev->dev.of_node, "power-domains", > + pm_info, ARRAY_SIZE(pm_info)); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to read power management information\n"); You have to undo phy_init() above (not listed in this diff). > + return ret; > + } > + ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0); > + if (ret < 0) Same here. > + return ret; > + > + ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1); > + if (ret < 0) And here. > + return ret; > + } > +> /* Fully reset controller at hardware level if mapped in device tree */ > ret = device_reset_optional(&pdev->dev); > if (ret) { > -- > 2.25.1 >
On 22/07/2022 09:12, Radhey Shyam Pandey wrote: > Add support for the dynamic configuration which takes care of configuring > the GEM secure space configuration registers using EEMI APIs. High level > sequence is to: > - Check for the PM dynamic configuration support, if no error proceed with > GEM dynamic configurations(next steps) otherwise skip the dynamic > configuration. For mpfs: Tested-by: Conor Dooley <conor.dooley@microchip.com> > - Configure GEM Fixed configurations. > - Configure GEM_CLK_CTRL (gemX_sgmii_mode). > - Trigger GEM reset. > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> > --- > drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 7eb7822cd184..97f77fa9e165 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -38,6 +38,7 @@ > #include <linux/pm_runtime.h> > #include <linux/ptp_classify.h> > #include <linux/reset.h> > +#include <linux/firmware/xlnx-zynqmp.h> > #include "macb.h" > > /* This structure is only used for MACB on SiFive FU540 devices */ > @@ -4621,6 +4622,25 @@ static int init_reset_optional(struct platform_device *pdev) > "failed to init SGMII PHY\n"); > } > > + ret = zynqmp_pm_is_function_supported(PM_IOCTL, IOCTL_SET_GEM_CONFIG); > + if (!ret) { > + u32 pm_info[2]; > + > + ret = of_property_read_u32_array(pdev->dev.of_node, "power-domains", > + pm_info, ARRAY_SIZE(pm_info)); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to read power management information\n"); > + return ret; > + } > + ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0); > + if (ret < 0) > + return ret; > + > + ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1); > + if (ret < 0) > + return ret; > + } > + > /* Fully reset controller at hardware level if mapped in device tree */ > ret = device_reset_optional(&pdev->dev); > if (ret) {
> + ret = of_property_read_u32_array(pdev->dev.of_node, "power-domains", > + pm_info, ARRAY_SIZE(pm_info)); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to read power management information\n"); > + return ret; > + } > + ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0); > + if (ret < 0) > + return ret; > + Documentation/devicetree/bindings/net/cdns,macb.yaml says: power-domains: maxItems: 1 Yet you are using pm_info[1]? Andrew
> -----Original Message----- > From: Claudiu.Beznea@microchip.com <Claudiu.Beznea@microchip.com> > Sent: Friday, July 22, 2022 2:22 PM > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; > michal.simek@xilinx.com; Nicolas.Ferre@microchip.com; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; gregkh@linuxfoundation.org; ronak.jain@xilinx.com > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic > configuration support > > On 22.07.2022 11:12, Radhey Shyam Pandey wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know > > the content is safe > > > > Add support for the dynamic configuration which takes care of > > configuring the GEM secure space configuration registers using EEMI > > APIs. High level sequence is to: > > - Check for the PM dynamic configuration support, if no error proceed with > > GEM dynamic configurations(next steps) otherwise skip the dynamic > > configuration. > > - Configure GEM Fixed configurations. > > - Configure GEM_CLK_CTRL (gemX_sgmii_mode). > > - Trigger GEM reset. > > > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> > > --- > > drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c > > b/drivers/net/ethernet/cadence/macb_main.c > > index 7eb7822cd184..97f77fa9e165 100644 > > --- a/drivers/net/ethernet/cadence/macb_main.c > > +++ b/drivers/net/ethernet/cadence/macb_main.c > > @@ -38,6 +38,7 @@ > > #include <linux/pm_runtime.h> > > #include <linux/ptp_classify.h> > > #include <linux/reset.h> > > +#include <linux/firmware/xlnx-zynqmp.h> > > #include "macb.h" > > > > /* This structure is only used for MACB on SiFive FU540 devices */ @@ > > -4621,6 +4622,25 @@ static int init_reset_optional(struct platform_device > *pdev) > > "failed to init SGMII PHY\n"); > > } > > > > + ret = zynqmp_pm_is_function_supported(PM_IOCTL, > IOCTL_SET_GEM_CONFIG); > > + if (!ret) { > > + u32 pm_info[2]; > > + > > + ret = of_property_read_u32_array(pdev->dev.of_node, "power- > domains", > > + pm_info, ARRAY_SIZE(pm_info)); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "Failed to read power > > + management information\n"); > > You have to undo phy_init() above (not listed in this diff). Thanks for the review. I see , will add phy_exit() in this return path and for below error path as well. > > > + return ret; > > + } > > + ret = zynqmp_pm_set_gem_config(pm_info[1], > GEM_CONFIG_FIXED, 0); > > + if (ret < 0) > > Same here. > > > + return ret; > > + > > + ret = zynqmp_pm_set_gem_config(pm_info[1], > GEM_CONFIG_SGMII_MODE, 1); > > + if (ret < 0) > > And here. > > > + return ret; > > + } > > +> /* Fully reset controller at hardware level if mapped in > > +> device > tree */ > > ret = device_reset_optional(&pdev->dev); > > if (ret) { > > -- > > 2.25.1 > >
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Sunday, July 24, 2022 10:24 PM > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com; > claudiu.beznea@microchip.com; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic > configuration support > > > + ret = of_property_read_u32_array(pdev->dev.of_node, > "power-domains", > > + pm_info, > ARRAY_SIZE(pm_info)); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "Failed to read power > management information\n"); > > + return ret; > > + } > > + ret = zynqmp_pm_set_gem_config(pm_info[1], > GEM_CONFIG_FIXED, 0); > > + if (ret < 0) > > + return ret; > > + > > Documentation/devicetree/bindings/net/cdns,macb.yaml says: > > power-domains: > maxItems: 1 > > Yet you are using pm_info[1]? From power-domain description - It's a phandle and PM domain specifier as defined by bindings of the power controller specified by phandle. I assume the numbers of cells is specified by "#power-domain-cells": Power-domain-cell is set to 1 in this case. arch/arm64/boot/dts/xilinx/zynqmp.dtsi #power-domain-cells = <1>; power-domains = <&zynqmp_firmware PD_ETH_0>; Please let me know your thoughts. > > Andrew
On Mon, Jul 25, 2022 at 02:34:51PM +0000, Pandey, Radhey Shyam wrote: > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Sunday, July 24, 2022 10:24 PM > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com; > > claudiu.beznea@microchip.com; davem@davemloft.net; > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm- > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com> > > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic > > configuration support > > > > > + ret = of_property_read_u32_array(pdev->dev.of_node, > > "power-domains", > > > + pm_info, > > ARRAY_SIZE(pm_info)); > > > + if (ret < 0) { > > > + dev_err(&pdev->dev, "Failed to read power > > management information\n"); > > > + return ret; > > > + } > > > + ret = zynqmp_pm_set_gem_config(pm_info[1], > > GEM_CONFIG_FIXED, 0); > > > + if (ret < 0) > > > + return ret; > > > + > > > > Documentation/devicetree/bindings/net/cdns,macb.yaml says: > > > > power-domains: > > maxItems: 1 > > > > Yet you are using pm_info[1]? > > >From power-domain description - It's a phandle and PM domain > specifier as defined by bindings of the power controller specified > by phandle. > > I assume the numbers of cells is specified by "#power-domain-cells": > Power-domain-cell is set to 1 in this case. > > arch/arm64/boot/dts/xilinx/zynqmp.dtsi > #power-domain-cells = <1>; > power-domains = <&zynqmp_firmware PD_ETH_0>; > > Please let me know your thoughts. Ah, so you ignore the phandle value, and just use the PD_ETH_0? How robust is this? What if somebody specified a different power domain? Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Monday, July 25, 2022 10:41 PM > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com; > claudiu.beznea@microchip.com; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic > configuration support > > On Mon, Jul 25, 2022 at 02:34:51PM +0000, Pandey, Radhey Shyam wrote: > > > -----Original Message----- > > > From: Andrew Lunn <andrew@lunn.ch> > > > Sent: Sunday, July 24, 2022 10:24 PM > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > > > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com; > > > claudiu.beznea@microchip.com; davem@davemloft.net; > > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > > > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm- > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > > > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) > > > <git@amd.com> > > > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII > > > dynamic configuration support > > > > > > > + ret = of_property_read_u32_array(pdev->dev.of_node, > > > "power-domains", > > > > + pm_info, > > > ARRAY_SIZE(pm_info)); > > > > + if (ret < 0) { > > > > + dev_err(&pdev->dev, "Failed to read power > > > management information\n"); > > > > + return ret; > > > > + } > > > > + ret = zynqmp_pm_set_gem_config(pm_info[1], > > > GEM_CONFIG_FIXED, 0); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > > > Documentation/devicetree/bindings/net/cdns,macb.yaml says: > > > > > > power-domains: > > > maxItems: 1 > > > > > > Yet you are using pm_info[1]? > > > > >From power-domain description - It's a phandle and PM domain > > specifier as defined by bindings of the power controller specified by > > phandle. > > > > I assume the numbers of cells is specified by "#power-domain-cells": > > Power-domain-cell is set to 1 in this case. > > > > arch/arm64/boot/dts/xilinx/zynqmp.dtsi > > #power-domain-cells = <1>; > > power-domains = <&zynqmp_firmware PD_ETH_0>; > > > > Please let me know your thoughts. > > Ah, so you ignore the phandle value, and just use the PD_ETH_0? > > How robust is this? What if somebody specified a different power domain? Some background - init_reset_optional() fn is implemented for three platforms i.e., zynqmp, versal, MPFS. zynqmp_pm_set_gem_config API expect first argument as GEM node id so, power-domain DT property is passed to get node ID. However, power-domain property is read only if underlying firmware supports configuration of GEM secure space. It's only true for zynqmp SGMII case and for zynqmp power domain is fixed. In addition to it there is an error handling in power-domain property parsing. Hope this answers the question. > > Andrew
> -----Original Message----- > From: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > Sent: Wednesday, July 27, 2022 12:18 AM > To: Andrew Lunn <andrew@lunn.ch> > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com; > claudiu.beznea@microchip.com; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com> > Subject: RE: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic > configuration support > > > > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Monday, July 25, 2022 10:41 PM > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com; > > claudiu.beznea@microchip.com; davem@davemloft.net; > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm- > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com> > > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic > > configuration support > > > > On Mon, Jul 25, 2022 at 02:34:51PM +0000, Pandey, Radhey Shyam wrote: > > > > -----Original Message----- > > > > From: Andrew Lunn <andrew@lunn.ch> > > > > Sent: Sunday, July 24, 2022 10:24 PM > > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > > > > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com; > > > > claudiu.beznea@microchip.com; davem@davemloft.net; > > > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > > > > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm- > > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > > > > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) > > > > <git@amd.com> > > > > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII > > > > dynamic configuration support > > > > > > > > > + ret = of_property_read_u32_array(pdev- > >dev.of_node, > > > > "power-domains", > > > > > + pm_info, > > > > ARRAY_SIZE(pm_info)); > > > > > + if (ret < 0) { > > > > > + dev_err(&pdev->dev, "Failed to read power > > > > management information\n"); > > > > > + return ret; > > > > > + } > > > > > + ret = zynqmp_pm_set_gem_config(pm_info[1], > > > > GEM_CONFIG_FIXED, 0); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > > > > Documentation/devicetree/bindings/net/cdns,macb.yaml says: > > > > > > > > power-domains: > > > > maxItems: 1 > > > > > > > > Yet you are using pm_info[1]? > > > > > > >From power-domain description - It's a phandle and PM domain > > > specifier as defined by bindings of the power controller specified > > > by phandle. > > > > > > I assume the numbers of cells is specified by "#power-domain-cells": > > > Power-domain-cell is set to 1 in this case. > > > > > > arch/arm64/boot/dts/xilinx/zynqmp.dtsi > > > #power-domain-cells = <1>; > > > power-domains = <&zynqmp_firmware PD_ETH_0>; > > > > > > Please let me know your thoughts. > > > > Ah, so you ignore the phandle value, and just use the PD_ETH_0? > > > > How robust is this? What if somebody specified a different power domain? > > Some background - init_reset_optional() fn is implemented for three > platforms i.e., zynqmp, versal, MPFS. > > zynqmp_pm_set_gem_config API expect first argument as GEM node id so, > power-domain DT property is passed to get node ID. > > However, power-domain property is read only if underlying firmware > supports configuration of GEM secure space. It's only true for zynqmp SGMII > case and for zynqmp power domain is fixed. > In addition to it there is an error handling in power-domain property parsing. > Hope this answers the question. Please let me know the implementation looks fine or needs any modification?
> > > How robust is this? What if somebody specified a different power domain? > > > > Some background - init_reset_optional() fn is implemented for three > > platforms i.e., zynqmp, versal, MPFS. > > > > zynqmp_pm_set_gem_config API expect first argument as GEM node id so, > > power-domain DT property is passed to get node ID. > > > > However, power-domain property is read only if underlying firmware > > supports configuration of GEM secure space. It's only true for zynqmp SGMII > > case and for zynqmp power domain is fixed. > > In addition to it there is an error handling in power-domain property parsing. > > Hope this answers the question. > > Please let me know the implementation looks fine or needs any modification? Given that explanation, it looks fine. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 7eb7822cd184..97f77fa9e165 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -38,6 +38,7 @@ #include <linux/pm_runtime.h> #include <linux/ptp_classify.h> #include <linux/reset.h> +#include <linux/firmware/xlnx-zynqmp.h> #include "macb.h" /* This structure is only used for MACB on SiFive FU540 devices */ @@ -4621,6 +4622,25 @@ static int init_reset_optional(struct platform_device *pdev) "failed to init SGMII PHY\n"); } + ret = zynqmp_pm_is_function_supported(PM_IOCTL, IOCTL_SET_GEM_CONFIG); + if (!ret) { + u32 pm_info[2]; + + ret = of_property_read_u32_array(pdev->dev.of_node, "power-domains", + pm_info, ARRAY_SIZE(pm_info)); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to read power management information\n"); + return ret; + } + ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0); + if (ret < 0) + return ret; + + ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1); + if (ret < 0) + return ret; + } + /* Fully reset controller at hardware level if mapped in device tree */ ret = device_reset_optional(&pdev->dev); if (ret) {
Add support for the dynamic configuration which takes care of configuring the GEM secure space configuration registers using EEMI APIs. High level sequence is to: - Check for the PM dynamic configuration support, if no error proceed with GEM dynamic configurations(next steps) otherwise skip the dynamic configuration. - Configure GEM Fixed configurations. - Configure GEM_CLK_CTRL (gemX_sgmii_mode). - Trigger GEM reset. Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> --- drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)