RE: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Bartosz Folta]

> And one more
>
> +   plat_info.dma_mask = DMA_BIT_MASK(32);
>
> Perhaps
>  plat_info.dma_mask = pdev->dma_mask;
> ?

Thank you.

I tested your suggestion. I have sent second version of patch.
https://patchwork.ozlabs.org/patch/707800/

Regards,
Bartosz Folta

Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Andy Shevchenko]

On Wed, Dec 21, 2016 at 2:16 PM, Andy Shevchenko
 wrote:
> On Wed, Dec 21, 2016 at 1:53 PM, Bartosz Folta  wrote:

>> I think that there is a delay on some mailing lists (marc.info and
>> mail-archive.com). Latest messages I found there are from December
>> 16th 2016.
>
> You may refer to
> https://www.spinics.net/lists/netdev/msg410864.html

And one more

+   plat_info.dma_mask = DMA_BIT_MASK(32);

Perhaps
 plat_info.dma_mask = pdev->dma_mask;
?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Andy Shevchenko]

On Wed, Dec 21, 2016 at 1:53 PM, Bartosz Folta  wrote:
> Please find link to patch holding changes that you suggested:
> https://lkml.kernel.org/r/sn1pr0701mb1951b994f86160a24c436f30cc...@sn1pr0701mb1951.namprd07.prod.outlook.com
>
> Here is alternative link to mentioned patch.
> https://patchwork.ozlabs.org/patch/707720/

Thanks!

>
> I think that there is a delay on some mailing lists (marc.info and
> mail-archive.com). Latest messages I found there are from December
> 16th 2016.
>

You may refer to
https://www.spinics.net/lists/netdev/msg410864.html

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Bartosz Folta]

Please find link to patch holding changes that you suggested:
https://lkml.kernel.org/r/sn1pr0701mb1951b994f86160a24c436f30cc...@sn1pr0701mb1951.namprd07.prod.outlook.com

Here is alternative link to mentioned patch.
https://patchwork.ozlabs.org/patch/707720/

I think that there is a delay on some mailing lists (marc.info and
mail-archive.com). Latest messages I found there are from December
16th 2016.

Regards,
Bartosz Folta

Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Andy Shevchenko]

On Wed, Dec 14, 2016 at 8:39 AM, Bartosz Folta  wrote:
> There are hardware PCI implementations of Cadence GEM network
> controller. This patch will allow to use such hardware with reuse of
> existing Platform Driver.

Since it's already applied, perhaps you would consider addressing my
below comments as follow up clean up.

> +++ b/drivers/net/ethernet/cadence/macb_pci.c
> @@ -0,0 +1,153 @@
> +/**

> + * macb_pci.c - Cadence GEM PCI wrapper.

If at some point file is renamed it will be a bit more work to change
this line. Perhaps drop file name?

> +static int macb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +   int err;
> +   struct platform_device *plat_dev;
> +   struct platform_device_info plat_info;
> +   struct macb_platform_data plat_data;
> +   struct resource res[2];
> +

> +   /* sanity check */
> +   if (!id)
> +   return -EINVAL;

Is it even possible in modern kernels?

> +
> +   /* enable pci device */
> +   err = pci_enable_device(pdev);

pcim_enable_device();

> +   if (err < 0) {
> +   dev_err(>dev, "Enabling PCI device has failed: 0x%04X",

%x for error code?! Confusing.

> +   err);

> +   return -EACCES;

return err;

> +   }
> +
> +   pci_set_master(pdev);
> +
> +   /* set up resources */
> +   memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));

> +   res[0].start = pdev->resource[0].start;
> +   res[0].end = pdev->resource[0].end;

pci_resource_start()
pci_resource_end()

> +   res[0].name = PCI_DRIVER_NAME;
> +   res[0].flags = IORESOURCE_MEM;

> +   res[1].start = pdev->irq;

Btw, does it support MSI? If so and are planning to use it,
pci_irq_...() API would help here.

> +   res[1].name = PCI_DRIVER_NAME;
> +   res[1].flags = IORESOURCE_IRQ;
> +
> +   dev_info(>dev, "EMAC physical base addr = 0x%p\n",

> +(void *)(uintptr_t)pci_resource_start(pdev, 0));

Oh, %pa

> +
> +   /* set up macb platform data */
> +   memset(_data, 0, sizeof(plat_data));
> +
> +   /* initialize clocks */
> +   plat_data.pclk = clk_register_fixed_rate(>dev, "pclk", NULL, 0,
> +GEM_PCLK_RATE);
> +   if (IS_ERR(plat_data.pclk)) {
> +   err = PTR_ERR(plat_data.pclk);
> +   goto err_pclk_register;
> +   }
> +
> +   plat_data.hclk = clk_register_fixed_rate(>dev, "hclk", NULL, 0,
> +GEM_HCLK_RATE);
> +   if (IS_ERR(plat_data.hclk)) {
> +   err = PTR_ERR(plat_data.hclk);
> +   goto err_hclk_register;
> +   }
> +
> +   /* set up platform device info */
> +   memset(_info, 0, sizeof(plat_info));
> +   plat_info.parent = >dev;
> +   plat_info.fwnode = pdev->dev.fwnode;
> +   plat_info.name = PLAT_DRIVER_NAME;
> +   plat_info.id = pdev->devfn;
> +   plat_info.res = res;
> +   plat_info.num_res = ARRAY_SIZE(res);
> +   plat_info.data = _data;
> +   plat_info.size_data = sizeof(plat_data);
> +   plat_info.dma_mask = DMA_BIT_MASK(32);
> +
> +   /* register platform device */
> +   plat_dev = platform_device_register_full(_info);
> +   if (IS_ERR(plat_dev)) {
> +   err = PTR_ERR(plat_dev);
> +   goto err_plat_dev_register;
> +   }
> +
> +   pci_set_drvdata(pdev, plat_dev);
> +
> +   return 0;
> +
> +err_plat_dev_register:
> +   clk_unregister(plat_data.hclk);
> +
> +err_hclk_register:
> +   clk_unregister(plat_data.pclk);
> +

> +err_pclk_register:
> +   pci_disable_device(pdev);

Redundant when use pcim_enable_device().

> +   return err;
> +}
> +
> +static void macb_remove(struct pci_dev *pdev)
> +{
> +   struct platform_device *plat_dev = pci_get_drvdata(pdev);
> +   struct macb_platform_data *plat_data = 
> dev_get_platdata(_dev->dev);
> +
> +   platform_device_unregister(plat_dev);

> +   pci_disable_device(pdev);

Ditto.

> +   clk_unregister(plat_data->pclk);
> +   clk_unregister(plat_data->hclk);
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[David Miller]

From: Bartosz Folta 
Date: Wed, 14 Dec 2016 06:39:15 +

> There are hardware PCI implementations of Cadence GEM network
> controller. This patch will allow to use such hardware with reuse of
> existing Platform Driver.
> 
> Signed-off-by: Bartosz Folta 
> ---
> Changed in v3:
> Fixed dependencies in Kconfig.
> ---
> Changed in v2:
> Respin to net-next. Changed patch formatting.

Applied.