Message ID | 20231212115151.20016-1-quic_luoj@quicinc.com |
---|---|
Headers | show |
Series | support ipq5332 platform | expand |
Hello, On Tue, 12 Dec 2023 19:51:47 +0800 Luo Jie <quic_luoj@quicinc.com> wrote: > On the platform ipq5332, the related SoC uniphy GCC clocks need > to be enabled for making the MDIO slave devices accessible. > > These UNIPHY clocks are from the SoC platform GCC clock provider, > which are enabled for the connected PHY devices working. > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com> [...] > static int ipq4019_mdio_wait_busy(struct mii_bus *bus) > @@ -209,14 +230,43 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum, > static int ipq_mdio_reset(struct mii_bus *bus) > { > struct ipq4019_mdio_data *priv = bus->priv; > - int ret; > + int ret, index; > + unsigned long rate; Please remember to use reverse christmas-tree ordering, meaning longer declaration lines go first : struct ipq4019_mdio_data *priv = bus->priv; unsigned long rate; int ret, index; > + > + /* For the platform ipq5332, there are two SoC uniphies available > + * for connecting with ethernet PHY, the SoC uniphy gcc clock > + * should be enabled for resetting the connected device such > + * as qca8386 switch, qca8081 PHY or other PHYs effectively. > + * > + * Configure MDIO/UNIPHY clock source frequency if clock instance > + * is specified in the device tree. > + */ > + for (index = MDIO_CLK_MDIO_AHB; index < MDIO_CLK_CNT; index++) { > + switch (index) { > + case MDIO_CLK_MDIO_AHB: > + rate = IPQ_MDIO_CLK_RATE; > + break; > + case MDIO_CLK_UNIPHY0_AHB: > + case MDIO_CLK_UNIPHY1_AHB: > + rate = IPQ_UNIPHY_AHB_CLK_RATE; > + break; > + case MDIO_CLK_UNIPHY0_SYS: > + case MDIO_CLK_UNIPHY1_SYS: > + rate = IPQ_UNIPHY_SYS_CLK_RATE; > + break; > + default: > + break; > + } > > - /* Configure MDIO clock source frequency if clock is specified in the device tree */ > - ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE); > - if (ret) > - return ret; > + ret = clk_set_rate(priv->clk[index], rate); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(priv->clk[index]); > + if (ret) > + return ret; > + } > > - ret = clk_prepare_enable(priv->mdio_clk); > if (ret == 0) > mdelay(10); > > @@ -240,10 +290,6 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) > if (IS_ERR(priv->membase)) > return PTR_ERR(priv->membase); > > - priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk"); > - if (IS_ERR(priv->mdio_clk)) > - return PTR_ERR(priv->mdio_clk); > - > /* These platform resources are provided on the chipset IPQ5018 or > * IPQ5332. > */ > @@ -271,6 +317,13 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) > } > } > > + for (index = 0; index < MDIO_CLK_CNT; index++) { > + priv->clk[index] = devm_clk_get_optional(&pdev->dev, > + mdio_clk_name[index]); > + if (IS_ERR(priv->clk[index])) > + return PTR_ERR(priv->clk[index]); > + } You should be able to use devm_clk_bulk_get_optional(), to avoid that loop. Thanks, Maxime
Hello, On Tue, 12 Dec 2023 19:51:46 +0800 Luo Jie <quic_luoj@quicinc.com> wrote: > The ethernet LDO provides the clock for the ethernet PHY that > is connected with PCS, each LDO enables the clock output to > each PCS, after the clock output enablement, the PHY GPIO reset > can take effect. > > For the PHY taking the MDIO bus level GPIO reset, the ethernet > LDO should be enabled before the MDIO bus register. > > For example, the qca8084 PHY takes the MDIO bus level GPIO > reset for quad PHYs, there is another reason for qca8084 PHY > using MDIO bus level GPIO reset instead of PHY level GPIO > reset as below. > > The work sequence of qca8084: > 1. enable ethernet LDO. > 2. GPIO reset on quad PHYs. > 3. register clock provider based on MDIO device of qca8084. > 4. PHY probe function called for initializing common clocks. > 5. PHY capabilities acquirement. > > If qca8084 takes PHY level GPIO reset in the step 4, the clock > provider of qca8084 can't be registered correctly, since the > clock parent(reading the current qca8084 hardware registers in > step 3) of the registered clocks is deserted after GPIO reset. > > There are two PCS(UNIPHY) supported in SOC side on ipq5332, > and three PCS(UNIPHY) supported on ipq9574. > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com> > --- [...] > @@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) > if (IS_ERR(priv->mdio_clk)) > return PTR_ERR(priv->mdio_clk); > > - /* The platform resource is provided on the chipset IPQ5018 */ > - /* This resource is optional */ > - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > - if (res) > - priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res); > + /* These platform resources are provided on the chipset IPQ5018 or > + * IPQ5332. > + */ > + /* This resource are optional */ > + for (index = 0; index < ETH_LDO_RDY_CNT; index++) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1); > + if (res) { > + priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev, > + res->start, > + resource_size(res)); You can simplify that sequence by using devm_platform_get_and_ioremap_resource(), which will do both the platform_get_resource and the devm_ioremap at once for you. Thanks, Maxime
Hello, I have some more minor comments for yoi :) On Tue, 12 Dec 2023 19:51:48 +0800 Luo Jie <quic_luoj@quicinc.com> wrote: > The reference clock of CMN PLL block is selectable, the internal > 48MHZ is used by default. > > The output clock of CMN PLL block is for providing the clock > source of ethernet device(such as qca8084), there are 1 * 25MHZ > and 3 * 50MHZ output clocks available for the ethernet devices. > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com> > --- [...] > +/* For the CMN PLL block, the reference clock can be configured according to > + * the device tree property "cmn-reference-clock", the internal 48MHZ is used > + * by default on the ipq533 platform. > + * > + * The output clock of CMN PLL block is provided to the ethernet devices, > + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default. > + * > + * Such as the output 50M clock for the qca8084 ethernet PHY. > + */ > +static int ipq_cmn_clock_config(struct mii_bus *bus) > +{ > + int ret; > + u32 reg_val, src_sel, ref_clk; > + struct ipq4019_mdio_data *priv; Here you should also use reverse christmas-tree notation [...] > @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) > } > } > > + /* The CMN block resource is for providing clock source to ethernet, > + * which can be optionally configured on the platform ipq9574 and > + * ipq5332. > + */ > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk"); > + if (res) { > + priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->cmn_membase)) > + return PTR_ERR(priv->cmn_membase); > + } > + And here you can simplify a bit by using devm_platform_ioremap_resource_byname() Thanks, Maxime
On Tue, Dec 12, 2023 at 01:50:01PM +0100, Maxime Chevallier wrote: > Hello, > > On Tue, 12 Dec 2023 19:51:46 +0800 > Luo Jie <quic_luoj@quicinc.com> wrote: > > @@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) > > if (IS_ERR(priv->mdio_clk)) > > return PTR_ERR(priv->mdio_clk); > > > > - /* The platform resource is provided on the chipset IPQ5018 */ > > - /* This resource is optional */ > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > - if (res) > > - priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res); > > + /* These platform resources are provided on the chipset IPQ5018 or > > + * IPQ5332. > > + */ > > + /* This resource are optional */ > > + for (index = 0; index < ETH_LDO_RDY_CNT; index++) { > > + res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1); > > + if (res) { > > + priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev, > > + res->start, > > + resource_size(res)); > > You can simplify that sequence by using > devm_platform_get_and_ioremap_resource(), which will do both the > platform_get_resource and the devm_ioremap at once for you. Sadly it can't if resources are optional. __devm_ioremap_resource() which will be capped by devm_platform_get_and_ioremap_resource() will be passed a NULL 'res', which will lead to: if (!res || resource_type(res) != IORESOURCE_MEM) { dev_err(dev, "invalid resource %pR\n", res); return IOMEM_ERR_PTR(-EINVAL); } There isn't an "optional" version of devm_platform_get_and_ioremap_resource().
On Tue, Dec 12, 2023 at 01:54:17PM +0100, Maxime Chevallier wrote: > Hello, > > I have some more minor comments for yoi :) > > On Tue, 12 Dec 2023 19:51:48 +0800 > Luo Jie <quic_luoj@quicinc.com> wrote: > > + /* The CMN block resource is for providing clock source to ethernet, > > + * which can be optionally configured on the platform ipq9574 and > > + * ipq5332. > > + */ > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk"); > > + if (res) { > > + priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(priv->cmn_membase)) > > + return PTR_ERR(priv->cmn_membase); > > + } > > + > > And here you can simplify a bit by using > devm_platform_ioremap_resource_byname() Not if the resource is optional.
On 12/12/2023 8:46 PM, Maxime Chevallier wrote: > Hello, > > On Tue, 12 Dec 2023 19:51:47 +0800 > Luo Jie <quic_luoj@quicinc.com> wrote: > >> On the platform ipq5332, the related SoC uniphy GCC clocks need >> to be enabled for making the MDIO slave devices accessible. >> >> These UNIPHY clocks are from the SoC platform GCC clock provider, >> which are enabled for the connected PHY devices working. >> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com> > > [...] > >> static int ipq4019_mdio_wait_busy(struct mii_bus *bus) >> @@ -209,14 +230,43 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum, >> static int ipq_mdio_reset(struct mii_bus *bus) >> { >> struct ipq4019_mdio_data *priv = bus->priv; >> - int ret; >> + int ret, index; >> + unsigned long rate; > > Please remember to use reverse christmas-tree ordering, meaning longer > declaration lines go first : > > struct ipq4019_mdio_data *priv = bus->priv; > unsigned long rate; > int ret, index; Thanks, i will update this. > >> + >> + /* For the platform ipq5332, there are two SoC uniphies available >> + * for connecting with ethernet PHY, the SoC uniphy gcc clock >> + * should be enabled for resetting the connected device such >> + * as qca8386 switch, qca8081 PHY or other PHYs effectively. >> + * >> + * Configure MDIO/UNIPHY clock source frequency if clock instance >> + * is specified in the device tree. >> + */ >> + for (index = MDIO_CLK_MDIO_AHB; index < MDIO_CLK_CNT; index++) { >> + switch (index) { >> + case MDIO_CLK_MDIO_AHB: >> + rate = IPQ_MDIO_CLK_RATE; >> + break; >> + case MDIO_CLK_UNIPHY0_AHB: >> + case MDIO_CLK_UNIPHY1_AHB: >> + rate = IPQ_UNIPHY_AHB_CLK_RATE; >> + break; >> + case MDIO_CLK_UNIPHY0_SYS: >> + case MDIO_CLK_UNIPHY1_SYS: >> + rate = IPQ_UNIPHY_SYS_CLK_RATE; >> + break; >> + default: >> + break; >> + } >> >> - /* Configure MDIO clock source frequency if clock is specified in the device tree */ >> - ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE); >> - if (ret) >> - return ret; >> + ret = clk_set_rate(priv->clk[index], rate); >> + if (ret) >> + return ret; >> + >> + ret = clk_prepare_enable(priv->clk[index]); >> + if (ret) >> + return ret; >> + } >> >> - ret = clk_prepare_enable(priv->mdio_clk); >> if (ret == 0) >> mdelay(10); >> >> @@ -240,10 +290,6 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) >> if (IS_ERR(priv->membase)) >> return PTR_ERR(priv->membase); >> >> - priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk"); >> - if (IS_ERR(priv->mdio_clk)) >> - return PTR_ERR(priv->mdio_clk); >> - >> /* These platform resources are provided on the chipset IPQ5018 or >> * IPQ5332. >> */ >> @@ -271,6 +317,13 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) >> } >> } >> >> + for (index = 0; index < MDIO_CLK_CNT; index++) { >> + priv->clk[index] = devm_clk_get_optional(&pdev->dev, >> + mdio_clk_name[index]); >> + if (IS_ERR(priv->clk[index])) >> + return PTR_ERR(priv->clk[index]); >> + } > > You should be able to use devm_clk_bulk_get_optional(), to avoid that > loop. > > Thanks, > > Maxime Thanks Maxime for the suggestion. These clocks need to be configured the different clock rate, MDIO system clock works on 100MHZ, but UNIPHY system clock works on 24MHZ. For the clock rate set, i still need the loop to configure the different clock rate on the different clock instance. So i use the devm_clk_get_optional to acquire the exact clock ID here.
On 12/12/2023 8:54 PM, Maxime Chevallier wrote: > Hello, > > I have some more minor comments for yoi :) > > On Tue, 12 Dec 2023 19:51:48 +0800 > Luo Jie <quic_luoj@quicinc.com> wrote: > >> The reference clock of CMN PLL block is selectable, the internal >> 48MHZ is used by default. >> >> The output clock of CMN PLL block is for providing the clock >> source of ethernet device(such as qca8084), there are 1 * 25MHZ >> and 3 * 50MHZ output clocks available for the ethernet devices. >> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com> >> --- > > [...] > >> +/* For the CMN PLL block, the reference clock can be configured according to >> + * the device tree property "cmn-reference-clock", the internal 48MHZ is used >> + * by default on the ipq533 platform. >> + * >> + * The output clock of CMN PLL block is provided to the ethernet devices, >> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default. >> + * >> + * Such as the output 50M clock for the qca8084 ethernet PHY. >> + */ >> +static int ipq_cmn_clock_config(struct mii_bus *bus) >> +{ >> + int ret; >> + u32 reg_val, src_sel, ref_clk; >> + struct ipq4019_mdio_data *priv; > > Here you should also use reverse christmas-tree notation Ok, will correct this, thanks. > > [...] > >> @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) >> } >> } >> >> + /* The CMN block resource is for providing clock source to ethernet, >> + * which can be optionally configured on the platform ipq9574 and >> + * ipq5332. >> + */ >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk"); >> + if (res) { >> + priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(priv->cmn_membase)) >> + return PTR_ERR(priv->cmn_membase); >> + } >> + > > And here you can simplify a bit by using > devm_platform_ioremap_resource_byname() > > Thanks, > > Maxime > As Russell mentioned, since this resource is optional, so devm_platform_ioremap_resource_byname can't be used here.
Hello Russell, On Tue, 12 Dec 2023 19:11:15 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Tue, Dec 12, 2023 at 01:50:01PM +0100, Maxime Chevallier wrote: > > Hello, > > > > On Tue, 12 Dec 2023 19:51:46 +0800 > > Luo Jie <quic_luoj@quicinc.com> wrote: > > > @@ -252,11 +244,32 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) > > > if (IS_ERR(priv->mdio_clk)) > > > return PTR_ERR(priv->mdio_clk); > > > > > > - /* The platform resource is provided on the chipset IPQ5018 */ > > > - /* This resource is optional */ > > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > > - if (res) > > > - priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res); > > > + /* These platform resources are provided on the chipset IPQ5018 or > > > + * IPQ5332. > > > + */ > > > + /* This resource are optional */ > > > + for (index = 0; index < ETH_LDO_RDY_CNT; index++) { > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1); > > > + if (res) { > > > + priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev, > > > + res->start, > > > + resource_size(res)); > > > > You can simplify that sequence by using > > devm_platform_get_and_ioremap_resource(), which will do both the > > platform_get_resource and the devm_ioremap at once for you. > > Sadly it can't if resources are optional. __devm_ioremap_resource() > which will be capped by devm_platform_get_and_ioremap_resource() will > be passed a NULL 'res', which will lead to: > > if (!res || resource_type(res) != IORESOURCE_MEM) { > dev_err(dev, "invalid resource %pR\n", res); > return IOMEM_ERR_PTR(-EINVAL); > } > > There isn't an "optional" version of > devm_platform_get_and_ioremap_resource(). > Ah right, I missed that part indeed. Sorry for the noise then, and thanks for double-checking :) Best regards, Maxime
On Wed, 13 Dec 2023 16:09:53 +0800 Jie Luo <quic_luoj@quicinc.com> wrote: > On 12/12/2023 8:54 PM, Maxime Chevallier wrote: > > Hello, > > > > I have some more minor comments for yoi :) > > > > On Tue, 12 Dec 2023 19:51:48 +0800 > > Luo Jie <quic_luoj@quicinc.com> wrote: > > > >> The reference clock of CMN PLL block is selectable, the internal > >> 48MHZ is used by default. > >> > >> The output clock of CMN PLL block is for providing the clock > >> source of ethernet device(such as qca8084), there are 1 * 25MHZ > >> and 3 * 50MHZ output clocks available for the ethernet devices. > >> > >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com> > >> --- > > > > [...] > > > >> +/* For the CMN PLL block, the reference clock can be configured according to > >> + * the device tree property "cmn-reference-clock", the internal 48MHZ is used > >> + * by default on the ipq533 platform. > >> + * > >> + * The output clock of CMN PLL block is provided to the ethernet devices, > >> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default. > >> + * > >> + * Such as the output 50M clock for the qca8084 ethernet PHY. > >> + */ > >> +static int ipq_cmn_clock_config(struct mii_bus *bus) > >> +{ > >> + int ret; > >> + u32 reg_val, src_sel, ref_clk; > >> + struct ipq4019_mdio_data *priv; > > > > Here you should also use reverse christmas-tree notation > > Ok, will correct this, thanks. > > > > > [...] > > > >> @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) > >> } > >> } > >> > >> + /* The CMN block resource is for providing clock source to ethernet, > >> + * which can be optionally configured on the platform ipq9574 and > >> + * ipq5332. > >> + */ > >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk"); > >> + if (res) { > >> + priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res); > >> + if (IS_ERR(priv->cmn_membase)) > >> + return PTR_ERR(priv->cmn_membase); > >> + } > >> + > > > > And here you can simplify a bit by using > > devm_platform_ioremap_resource_byname() > > > > Thanks, > > > > Maxime > > > As Russell mentioned, since this resource is optional, > so devm_platform_ioremap_resource_byname can't be used here. > Indeed, my bad I missed that point. Sorry for the noise :/ Thanks, Maxime