Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Linux>
  3. Kernel
[PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks
seanga2 at gmail
Mar 13, 2023, 6:15 PM
Post #1 of 4 (117 views)
Permalink
Most of the second half of the PCI/SBUS probe functions are the same.
Consolidate them into a common function.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

drivers/net/ethernet/sun/sunhme.c | 183 ++++++++++++------------------
1 file changed, 71 insertions(+), 112 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
index a59b998062d9..a384b162c46d 100644
--- a/drivers/net/ethernet/sun/sunhme.c
+++ b/drivers/net/ethernet/sun/sunhme.c
@@ -2430,6 +2430,71 @@ static void happy_meal_addr_init(struct happy_meal *hp,
}
}

+static int happy_meal_common_probe(struct happy_meal *hp,
+ struct device_node *dp, int minor_rev)
+{
+ struct net_device *dev = hp->dev;
+ int err;
+
+#ifdef CONFIG_SPARC
+ hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
+ if (hp->hm_revision == 0xff)
+ hp->hm_revision = 0xc0 | minor_rev;
+#else
+ /* works with this on non-sparc hosts */
+ hp->hm_revision = 0x20;
+#endif
+
+ /* Now enable the feature flags we can. */
+ if (hp->hm_revision == 0x20 || hp->hm_revision == 0x21)
+ hp->happy_flags |= HFLAG_20_21;
+ else if (hp->hm_revision != 0xa0)
+ hp->happy_flags |= HFLAG_NOT_A0;
+
+ hp->happy_block = dmam_alloc_coherent(hp->dma_dev, PAGE_SIZE,
+ &hp->hblock_dvma, GFP_KERNEL);
+ if (!hp->happy_block)
+ return -ENOMEM;
+
+ /* Force check of the link first time we are brought up. */
+ hp->linkcheck = 0;
+
+ /* Force timer state to 'asleep' with count of zero. */
+ hp->timer_state = asleep;
+ hp->timer_ticks = 0;
+
+ timer_setup(&hp->happy_timer, happy_meal_timer, 0);
+
+ dev->netdev_ops = &hme_netdev_ops;
+ dev->watchdog_timeo = 5 * HZ;
+ dev->ethtool_ops = &hme_ethtool_ops;
+
+ /* Happy Meal can do it all... */
+ dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM;
+ dev->features |= dev->hw_features | NETIF_F_RXCSUM;
+
+#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
+ /* Hook up SBUS register/descriptor accessors. */
+ hp->read_desc32 = sbus_hme_read_desc32;
+ hp->write_txd = sbus_hme_write_txd;
+ hp->write_rxd = sbus_hme_write_rxd;
+ hp->read32 = sbus_hme_read32;
+ hp->write32 = sbus_hme_write32;
+#endif
+
+ /* Grrr, Happy Meal comes up by default not advertising
+ * full duplex 100baseT capabilities, fix this.
+ */
+ spin_lock_irq(&hp->happy_lock);
+ happy_meal_set_initial_advertisement(hp);
+ spin_unlock_irq(&hp->happy_lock);
+
+ err = devm_register_netdev(hp->dma_dev, dev);
+ if (err)
+ dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
+ return err;
+}
+
#ifdef CONFIG_SBUS
static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
{
@@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
goto err_out_clear_quattro;
}

- hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
- if (hp->hm_revision == 0xff)
- hp->hm_revision = 0xa0;
-
- /* Now enable the feature flags we can. */
- if (hp->hm_revision == 0x20 || hp->hm_revision == 0x21)
- hp->happy_flags = HFLAG_20_21;
- else if (hp->hm_revision != 0xa0)
- hp->happy_flags = HFLAG_NOT_A0;
-
if (qp != NULL)
hp->happy_flags |= HFLAG_QUATTRO;

+ hp->irq = op->archdata.irqs[0];
+
/* Get the supported DVMA burst sizes from our Happy SBUS. */
hp->happy_bursts = of_getintprop_default(sbus_dp,
"burst-sizes", 0x00);

- hp->happy_block = dmam_alloc_coherent(&op->dev, PAGE_SIZE,
- &hp->hblock_dvma, GFP_KERNEL);
- if (!hp->happy_block) {
- err = -ENOMEM;
+ err = happy_meal_common_probe(hp, dp, 0);
+ if (err)
goto err_out_clear_quattro;
- }
-
- /* Force check of the link first time we are brought up. */
- hp->linkcheck = 0;
-
- /* Force timer state to 'asleep' with count of zero. */
- hp->timer_state = asleep;
- hp->timer_ticks = 0;
-
- timer_setup(&hp->happy_timer, happy_meal_timer, 0);
-
- dev->netdev_ops = &hme_netdev_ops;
- dev->watchdog_timeo = 5*HZ;
- dev->ethtool_ops = &hme_ethtool_ops;
-
- /* Happy Meal can do it all... */
- dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM;
- dev->features |= dev->hw_features | NETIF_F_RXCSUM;
-
- hp->irq = op->archdata.irqs[0];
-
-#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
- /* Hook up SBUS register/descriptor accessors. */
- hp->read_desc32 = sbus_hme_read_desc32;
- hp->write_txd = sbus_hme_write_txd;
- hp->write_rxd = sbus_hme_write_rxd;
- hp->read32 = sbus_hme_read32;
- hp->write32 = sbus_hme_write32;
-#endif
-
- /* Grrr, Happy Meal comes up by default not advertising
- * full duplex 100baseT capabilities, fix this.
- */
- spin_lock_irq(&hp->happy_lock);
- happy_meal_set_initial_advertisement(hp);
- spin_unlock_irq(&hp->happy_lock);
-
- err = devm_register_netdev(&op->dev, dev);
- if (err) {
- dev_err(&op->dev, "Cannot register net device, aborting.\n");
- goto err_out_clear_quattro;
- }

platform_set_drvdata(op, hp);

@@ -2689,21 +2702,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
hp->bigmacregs = (hpreg_base + 0x6000UL);
hp->tcvregs = (hpreg_base + 0x7000UL);

-#ifdef CONFIG_SPARC
- hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
- if (hp->hm_revision == 0xff)
- hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
-#else
- /* works with this on non-sparc hosts */
- hp->hm_revision = 0x20;
-#endif
-
- /* Now enable the feature flags we can. */
- if (hp->hm_revision == 0x20 || hp->hm_revision == 0x21)
- hp->happy_flags = HFLAG_20_21;
- else if (hp->hm_revision != 0xa0 && hp->hm_revision != 0xc0)
- hp->happy_flags = HFLAG_NOT_A0;
-
if (qp != NULL)
hp->happy_flags |= HFLAG_QUATTRO;

@@ -2714,50 +2712,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
/* Assume PCI happy meals can handle all burst sizes. */
hp->happy_bursts = DMA_BURSTBITS;
#endif
-
- hp->happy_block = dmam_alloc_coherent(&pdev->dev, PAGE_SIZE,
- &hp->hblock_dvma, GFP_KERNEL);
- if (!hp->happy_block) {
- err = -ENOMEM;
- goto err_out_clear_quattro;
- }
-
- hp->linkcheck = 0;
- hp->timer_state = asleep;
- hp->timer_ticks = 0;
-
- timer_setup(&hp->happy_timer, happy_meal_timer, 0);
-
hp->irq = pdev->irq;
- dev->netdev_ops = &hme_netdev_ops;
- dev->watchdog_timeo = 5*HZ;
- dev->ethtool_ops = &hme_ethtool_ops;

- /* Happy Meal can do it all... */
- dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM;
- dev->features |= dev->hw_features | NETIF_F_RXCSUM;
-
-#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
- /* Hook up PCI register/descriptor accessors. */
- hp->read_desc32 = pci_hme_read_desc32;
- hp->write_txd = pci_hme_write_txd;
- hp->write_rxd = pci_hme_write_rxd;
- hp->read32 = pci_hme_read32;
- hp->write32 = pci_hme_write32;
-#endif
-
- /* Grrr, Happy Meal comes up by default not advertising
- * full duplex 100baseT capabilities, fix this.
- */
- spin_lock_irq(&hp->happy_lock);
- happy_meal_set_initial_advertisement(hp);
- spin_unlock_irq(&hp->happy_lock);
-
- err = devm_register_netdev(&pdev->dev, dev);
- if (err) {
- dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
+ err = happy_meal_common_probe(hp, dp, pdev->revision & 0x0f);
+ if (err)
goto err_out_clear_quattro;
- }

pci_set_drvdata(pdev, hp);

--
2.37.1
Re: [PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks [ In reply to ]
simon.horman at corigine
Mar 18, 2023, 8:15 AM
Post #2 of 4 (117 views)
Permalink
On Mon, Mar 13, 2023 at 08:36:13PM -0400, Sean Anderson wrote:
> Most of the second half of the PCI/SBUS probe functions are the same.
> Consolidate them into a common function.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> (no changes since v1)
>
> drivers/net/ethernet/sun/sunhme.c | 183 ++++++++++++------------------
> 1 file changed, 71 insertions(+), 112 deletions(-)
>
> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index a59b998062d9..a384b162c46d 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c
> @@ -2430,6 +2430,71 @@ static void happy_meal_addr_init(struct happy_meal *hp,
> }
> }
>
> +static int happy_meal_common_probe(struct happy_meal *hp,
> + struct device_node *dp, int minor_rev)
> +{
> + struct net_device *dev = hp->dev;
> + int err;
> +
> +#ifdef CONFIG_SPARC
> + hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> + if (hp->hm_revision == 0xff)
> + hp->hm_revision = 0xc0 | minor_rev;
> +#else
> + /* works with this on non-sparc hosts */
> + hp->hm_revision = 0x20;
> +#endif

...

> +#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> + /* Hook up SBUS register/descriptor accessors. */
> + hp->read_desc32 = sbus_hme_read_desc32;
> + hp->write_txd = sbus_hme_write_txd;
> + hp->write_rxd = sbus_hme_write_rxd;
> + hp->read32 = sbus_hme_read32;
> + hp->write32 = sbus_hme_write32;
> +#endif

This looks correct for the SBUS case.
But I'm not sure about the PCIE case.

gcc 12 tells me when compiling with sparc allmodconfig that the following
functions are now unused.

pci_hme_read_desc32
pci_hme_write_txd
pci_hme_write_rxd
pci_hme_read32
pci_hme_write32

> +
> + /* Grrr, Happy Meal comes up by default not advertising
> + * full duplex 100baseT capabilities, fix this.
b
> + */
> + spin_lock_irq(&hp->happy_lock);
> + happy_meal_set_initial_advertisement(hp);
> + spin_unlock_irq(&hp->happy_lock);
> +
> + err = devm_register_netdev(hp->dma_dev, dev);
> + if (err)
> + dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
> + return err;
> +}
> +
> #ifdef CONFIG_SBUS
> static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
> {

> @@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
> goto err_out_clear_quattro;
> }
>
> - hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> - if (hp->hm_revision == 0xff)
> - hp->hm_revision = 0xa0;

It's not clear to me that the same value will be set by the call to
happy_meal_common_probe(hp, dp, 0); where the logic is:

#ifdef CONFIG_SPARC
hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
if (hp->hm_revision == 0xff)
hp->hm_revision = 0xc0 | minor_rev;
#else
/* works with this on non-sparc hosts */
hp->hm_revision = 0x20;
#endif

I am assuming that the SPARC logic is run.
But another question: is it strictly true that SBUS means SPARC?

...

> -
> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> - /* Hook up SBUS register/descriptor accessors. */
> - hp->read_desc32 = sbus_hme_read_desc32;
> - hp->write_txd = sbus_hme_write_txd;
> - hp->write_rxd = sbus_hme_write_rxd;
> - hp->read32 = sbus_hme_read32;
> - hp->write32 = sbus_hme_write32;
> -#endif

...

> @@ -2689,21 +2702,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
> hp->bigmacregs = (hpreg_base + 0x6000UL);
> hp->tcvregs = (hpreg_base + 0x7000UL);
>
> -#ifdef CONFIG_SPARC
> - hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> - if (hp->hm_revision == 0xff)
> - hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
> -#else
> - /* works with this on non-sparc hosts */
> - hp->hm_revision = 0x20;
> -#endif

...

> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> - /* Hook up PCI register/descriptor accessors. */
> - hp->read_desc32 = pci_hme_read_desc32;
> - hp->write_txd = pci_hme_write_txd;
> - hp->write_rxd = pci_hme_write_rxd;
> - hp->read32 = pci_hme_read32;
> - hp->write32 = pci_hme_write32;
> -#endif

...
Re: [PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks [ In reply to ]
seanga2 at gmail
Mar 18, 2023, 8:15 AM
Post #3 of 4 (117 views)
Permalink
On 3/18/23 09:53, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:13PM -0400, Sean Anderson wrote:
>> Most of the second half of the PCI/SBUS probe functions are the same.
>> Consolidate them into a common function.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v1)
>>
>> drivers/net/ethernet/sun/sunhme.c | 183 ++++++++++++------------------
>> 1 file changed, 71 insertions(+), 112 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
>> index a59b998062d9..a384b162c46d 100644
>> --- a/drivers/net/ethernet/sun/sunhme.c
>> +++ b/drivers/net/ethernet/sun/sunhme.c
>> @@ -2430,6 +2430,71 @@ static void happy_meal_addr_init(struct happy_meal *hp,
>> }
>> }
>>
>> +static int happy_meal_common_probe(struct happy_meal *hp,
>> + struct device_node *dp, int minor_rev)
>> +{
>> + struct net_device *dev = hp->dev;
>> + int err;
>> +
>> +#ifdef CONFIG_SPARC
>> + hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> + if (hp->hm_revision == 0xff)
>> + hp->hm_revision = 0xc0 | minor_rev;
>> +#else
>> + /* works with this on non-sparc hosts */
>> + hp->hm_revision = 0x20;
>> +#endif
>
> ...
>
>> +#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> + /* Hook up SBUS register/descriptor accessors. */
>> + hp->read_desc32 = sbus_hme_read_desc32;
>> + hp->write_txd = sbus_hme_write_txd;
>> + hp->write_rxd = sbus_hme_write_rxd;
>> + hp->read32 = sbus_hme_read32;
>> + hp->write32 = sbus_hme_write32;
>> +#endif
>
> This looks correct for the SBUS case.
> But I'm not sure about the PCIE case.
>
> gcc 12 tells me when compiling with sparc allmodconfig that the following
> functions are now unused.
>
> pci_hme_read_desc32
> pci_hme_write_txd
> pci_hme_write_rxd
> pci_hme_read32
> pci_hme_write32

Oh, looks like I missed that these were different while refactoring.

That said, I haven't seen any issues here...

>> +
>> + /* Grrr, Happy Meal comes up by default not advertising
>> + * full duplex 100baseT capabilities, fix this.
> b
>> + */
>> + spin_lock_irq(&hp->happy_lock);
>> + happy_meal_set_initial_advertisement(hp);
>> + spin_unlock_irq(&hp->happy_lock);
>> +
>> + err = devm_register_netdev(hp->dma_dev, dev);
>> + if (err)
>> + dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
>> + return err;
>> +}
>> +
>> #ifdef CONFIG_SBUS
>> static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>> {
>
>> @@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>> goto err_out_clear_quattro;
>> }
>>
>> - hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> - if (hp->hm_revision == 0xff)
>> - hp->hm_revision = 0xa0;
>
> It's not clear to me that the same value will be set by the call to
> happy_meal_common_probe(hp, dp, 0); where the logic is:
>
> #ifdef CONFIG_SPARC
> hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> if (hp->hm_revision == 0xff)
> hp->hm_revision = 0xc0 | minor_rev;

OK, so maybe this should be xor, with sbus passing in 0x30.

> #else
> /* works with this on non-sparc hosts */
> hp->hm_revision = 0x20;
> #endif
>
> I am assuming that the SPARC logic is run.
> But another question: is it strictly true that SBUS means SPARC?

Yes.

>> -
>> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> - /* Hook up SBUS register/descriptor accessors. */
>> - hp->read_desc32 = sbus_hme_read_desc32;
>> - hp->write_txd = sbus_hme_write_txd;
>> - hp->write_rxd = sbus_hme_write_rxd;
>> - hp->read32 = sbus_hme_read32;
>> - hp->write32 = sbus_hme_write32;
>> -#endif
>
> ...
>
>> @@ -2689,21 +2702,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
>> hp->bigmacregs = (hpreg_base + 0x6000UL);
>> hp->tcvregs = (hpreg_base + 0x7000UL);
>>
>> -#ifdef CONFIG_SPARC
>> - hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> - if (hp->hm_revision == 0xff)
>> - hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
>> -#else
>> - /* works with this on non-sparc hosts */
>> - hp->hm_revision = 0x20;
>> -#endif
>
> ...
>
>> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> - /* Hook up PCI register/descriptor accessors. */
>> - hp->read_desc32 = pci_hme_read_desc32;
>> - hp->write_txd = pci_hme_write_txd;
>> - hp->write_rxd = pci_hme_write_rxd;
>> - hp->read32 = pci_hme_read32;
>> - hp->write32 = pci_hme_write32;
>> -#endif
>
> ...

--Sean
Re: [PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks [ In reply to ]
simon.horman at corigine
Mar 21, 2023, 1:05 AM
Post #4 of 4 (111 views)
Permalink
On Sat, Mar 18, 2023 at 10:40:46AM -0400, Sean Anderson wrote:
> On 3/18/23 09:53, Simon Horman wrote:
> > On Mon, Mar 13, 2023 at 08:36:13PM -0400, Sean Anderson wrote:
> > > Most of the second half of the PCI/SBUS probe functions are the same.
> > > Consolidate them into a common function.
> > >
> > > Signed-off-by: Sean Anderson <seanga2@gmail.com>

...

> > > @@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
> > > goto err_out_clear_quattro;
> > > }
> > > - hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> > > - if (hp->hm_revision == 0xff)
> > > - hp->hm_revision = 0xa0;
> >
> > It's not clear to me that the same value will be set by the call to
> > happy_meal_common_probe(hp, dp, 0); where the logic is:
> >
> > #ifdef CONFIG_SPARC
> > hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> > if (hp->hm_revision == 0xff)
> > hp->hm_revision = 0xc0 | minor_rev;
>
> OK, so maybe this should be xor, with sbus passing in 0x30.

Maybe moving the math to the caller makes things easier.
I'm unsure.

> > #else
> > /* works with this on non-sparc hosts */
> > hp->hm_revision = 0x20;
> > #endif
> >
> > I am assuming that the SPARC logic is run.
> > But another question: is it strictly true that SBUS means SPARC?
>
> Yes.

Thanks, got it.

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal