| Message ID | 20231024112924.3934228-3-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel IPU6 and IPU6 input system drivers | expand |
On Tue, Oct 24, 2023 at 07:29:11PM +0800, bingbu.cao@intel.com wrote: > From: Bingbu Cao <bingbu.cao@intel.com> > > Even the IPU input system and processing system are in a single PCI > device, each system has its own power sequence, the processing system > power up depends on the input system power up. > > Besides, input system and processing system have their own MMU > hardware for IPU DMA address mapping. > > Register the IS/PS devices on auxiliary bus and attach power domain > to implement the power sequence dependency. ... Seems again poor / random list of header inclusions... Please, go through your files and use IWYU (Include What You Use) principle in them. > +#include <linux/pci.h> auxiliary_bus.h, err.h, list.h, mutex.h, dev_printk.h, slab.h, dma-mapping.h are missing. You are lucky it does compile. ... > +#ifndef IPU6_BUS_H > +#define IPU6_BUS_H > + > +#include <linux/auxiliary_bus.h> ...Especially for headers which will affect the compilation time. > +#include <linux/pci.h> This is not used. ... > + struct list_head list; + types.h > + struct sg_table fw_sgt; + scatterlist.h > + dma_addr_t pkg_dir_dma_addr; types.h ... > +struct ipu6_auxdrv_data { > + irqreturn_t (*isr)(struct ipu6_bus_device *adev); > + irqreturn_t (*isr_threaded)(struct ipu6_bus_device *adev); irqreturn.h > + bool wake_isr_thread; > +}; ... > +#define to_ipu6_bus_device(_dev) container_of(to_auxiliary_dev(_dev), \ > + struct ipu6_bus_device, auxdev) > +#define auxdev_to_adev(_auxdev) container_of(_auxdev, \ > + struct ipu6_bus_device, auxdev) container_of.h Also, can you reformat to be more readable like #define auxdev_to_adev(_auxdev) \ container_of(_auxdev, struct ipu6_bus_device, auxdev) > +#define ipu6_bus_get_drvdata(adev) dev_get_drvdata(&(adev)->auxdev.dev) device.h ... > + ret = dma_set_max_seg_size(&pdev->dev, UINT_MAX); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Failed to set max_seg_size\n"); With the help of struct device *dev = &pdev->dev; this and other lines become neater ret = dma_set_max_seg_size(dev, _DMA_SEGMENT_SIZE); // as I commented earlier about if (ret) return dev_err_probe(dev, ret, "Failed to set max_seg_size\n");
Andy, Thanks for your review. On 10/24/23 8:58 PM, Andy Shevchenko wrote: > On Tue, Oct 24, 2023 at 07:29:11PM +0800, bingbu.cao@intel.com wrote: >> From: Bingbu Cao <bingbu.cao@intel.com> >> >> Even the IPU input system and processing system are in a single PCI >> device, each system has its own power sequence, the processing system >> power up depends on the input system power up. >> >> Besides, input system and processing system have their own MMU >> hardware for IPU DMA address mapping. >> >> Register the IS/PS devices on auxiliary bus and attach power domain >> to implement the power sequence dependency. > > ... > > Seems again poor / random list of header inclusions... > Please, go through your files and use IWYU (Include What You Use) principle > in them. > >> +#include <linux/pci.h> > > auxiliary_bus.h, err.h, list.h, mutex.h, dev_printk.h, slab.h, dma-mapping.h are missing. > You are lucky it does compile. auxiliary_bus.h is included in ipu6-bus.h, list.h, mutex.h dev_printk.h are included in device.h, dma-mapping.h and scatterlist.h are included in pci.h. I am a little confused about the rule, do you mean we need include the generic headers we need even it is included in others header? > > ... > >> +#ifndef IPU6_BUS_H >> +#define IPU6_BUS_H >> + >> +#include <linux/auxiliary_bus.h> > > ...Especially for headers which will affect the compilation time. > >> +#include <linux/pci.h> > > This is not used. Do you mean it just need a 'struct pci_dev;' ? > > ... > >> + struct list_head list; > > + types.h > >> + struct sg_table fw_sgt; > > + scatterlist.h > >> + dma_addr_t pkg_dir_dma_addr; > > types.h > > ... > >> +struct ipu6_auxdrv_data { >> + irqreturn_t (*isr)(struct ipu6_bus_device *adev); >> + irqreturn_t (*isr_threaded)(struct ipu6_bus_device *adev); > > irqreturn.h > >> + bool wake_isr_thread; >> +}; > > ... > >> +#define to_ipu6_bus_device(_dev) container_of(to_auxiliary_dev(_dev), \ >> + struct ipu6_bus_device, auxdev) >> +#define auxdev_to_adev(_auxdev) container_of(_auxdev, \ >> + struct ipu6_bus_device, auxdev) > > container_of.h > > Also, can you reformat to be more readable like > > #define auxdev_to_adev(_auxdev) \ > container_of(_auxdev, struct ipu6_bus_device, auxdev) Ack. > >> +#define ipu6_bus_get_drvdata(adev) dev_get_drvdata(&(adev)->auxdev.dev) > > device.h > > ... > >> + ret = dma_set_max_seg_size(&pdev->dev, UINT_MAX); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, >> + "Failed to set max_seg_size\n"); > > With the help of > > struct device *dev = &pdev->dev; > > this and other lines become neater > > > ret = dma_set_max_seg_size(dev, _DMA_SEGMENT_SIZE); // as I commented earlier about > if (ret) > return dev_err_probe(dev, ret, "Failed to set max_seg_size\n"); Ack. >
Hi, On 10/25/23 09:14, Bingbu Cao wrote: > Andy, > > Thanks for your review. > > On 10/24/23 8:58 PM, Andy Shevchenko wrote: >> On Tue, Oct 24, 2023 at 07:29:11PM +0800, bingbu.cao@intel.com wrote: >>> From: Bingbu Cao <bingbu.cao@intel.com> >>> >>> Even the IPU input system and processing system are in a single PCI >>> device, each system has its own power sequence, the processing system >>> power up depends on the input system power up. >>> >>> Besides, input system and processing system have their own MMU >>> hardware for IPU DMA address mapping. >>> >>> Register the IS/PS devices on auxiliary bus and attach power domain >>> to implement the power sequence dependency. >> >> ... >> >> Seems again poor / random list of header inclusions... >> Please, go through your files and use IWYU (Include What You Use) principle >> in them. >> >>> +#include <linux/pci.h> >> >> auxiliary_bus.h, err.h, list.h, mutex.h, dev_printk.h, slab.h, dma-mapping.h are missing. >> You are lucky it does compile. > > auxiliary_bus.h is included in ipu6-bus.h, list.h, mutex.h dev_printk.h are > included in device.h, dma-mapping.h and scatterlist.h are included in pci.h. > > I am a little confused about the rule, do you mean we need include the > generic headers we need even it is included in others header? Yes you must *not* rely on other headers including headers from which you directly use symbols. People are working on reducing the number of includes inside headers to speed up building the kernel because including unnecessary headers inside another header has a big impact on build time. Regards, Hans > >> >> ... >> >>> +#ifndef IPU6_BUS_H >>> +#define IPU6_BUS_H >>> + >>> +#include <linux/auxiliary_bus.h> >> >> ...Especially for headers which will affect the compilation time. >> >>> +#include <linux/pci.h> >> >> This is not used. > > Do you mean it just need a 'struct pci_dev;' ? >> >> ... >> >>> + struct list_head list; >> >> + types.h >> >>> + struct sg_table fw_sgt; >> >> + scatterlist.h >> >>> + dma_addr_t pkg_dir_dma_addr; >> >> types.h >> >> ... >> >>> +struct ipu6_auxdrv_data { >>> + irqreturn_t (*isr)(struct ipu6_bus_device *adev); >>> + irqreturn_t (*isr_threaded)(struct ipu6_bus_device *adev); >> >> irqreturn.h >> >>> + bool wake_isr_thread; >>> +}; >> >> ... >> >>> +#define to_ipu6_bus_device(_dev) container_of(to_auxiliary_dev(_dev), \ >>> + struct ipu6_bus_device, auxdev) >>> +#define auxdev_to_adev(_auxdev) container_of(_auxdev, \ >>> + struct ipu6_bus_device, auxdev) >> >> container_of.h >> >> Also, can you reformat to be more readable like >> >> #define auxdev_to_adev(_auxdev) \ >> container_of(_auxdev, struct ipu6_bus_device, auxdev) > > Ack. > >> >>> +#define ipu6_bus_get_drvdata(adev) dev_get_drvdata(&(adev)->auxdev.dev) >> >> device.h >> >> ... >> >>> + ret = dma_set_max_seg_size(&pdev->dev, UINT_MAX); >>> + if (ret) >>> + return dev_err_probe(&pdev->dev, ret, >>> + "Failed to set max_seg_size\n"); >> >> With the help of >> >> struct device *dev = &pdev->dev; >> >> this and other lines become neater >> >> >> ret = dma_set_max_seg_size(dev, _DMA_SEGMENT_SIZE); // as I commented earlier about >> if (ret) >> return dev_err_probe(dev, ret, "Failed to set max_seg_size\n"); > > Ack. > >> >
On Wed, Oct 25, 2023 at 03:14:00PM +0800, Bingbu Cao wrote: > On 10/24/23 8:58 PM, Andy Shevchenko wrote: > > On Tue, Oct 24, 2023 at 07:29:11PM +0800, bingbu.cao@intel.com wrote: > auxiliary_bus.h is included in ipu6-bus.h, So, you have to include it explicitly as IWYU, strictly speaking ipu6-bus.h does NOT guarantee that inclusion, even if you fully control it. > list.h, mutex.h dev_printk.h are > included in device.h, I ack for dev_printk.h, but for the rest see above. > dma-mapping.h and scatterlist.h are included in pci.h. See above. > I am a little confused about the rule, do you mean we need include the > generic headers we need even it is included in others header? Yes. There are only few guarantees in the kernel, strictly speaking. And having spaghetti headers in your code is a bad idea from long run maintenance perspective. ... > >> +#ifndef IPU6_BUS_H > >> +#define IPU6_BUS_H > >> + > >> +#include <linux/auxiliary_bus.h> > > > > ...Especially for headers which will affect the compilation time. > > > >> +#include <linux/pci.h> > > > > This is not used. > > Do you mean it just need a 'struct pci_dev;' ? Yes.
Andy and Hans, Thanks. On 10/26/23 3:57 AM, Andy Shevchenko wrote: > On Wed, Oct 25, 2023 at 03:14:00PM +0800, Bingbu Cao wrote: >> On 10/24/23 8:58 PM, Andy Shevchenko wrote: >>> On Tue, Oct 24, 2023 at 07:29:11PM +0800, bingbu.cao@intel.com wrote: > >> auxiliary_bus.h is included in ipu6-bus.h, > > So, you have to include it explicitly as IWYU, strictly speaking ipu6-bus.h > does NOT guarantee that inclusion, even if you fully control it. > >> list.h, mutex.h dev_printk.h are >> included in device.h, > > I ack for dev_printk.h, but for the rest see above. > >> dma-mapping.h and scatterlist.h are included in pci.h. > > See above. > > >> I am a little confused about the rule, do you mean we need include the >> generic headers we need even it is included in others header? > > Yes. There are only few guarantees in the kernel, strictly speaking. And having > spaghetti headers in your code is a bad idea from long run maintenance > perspective. > > ... > >>>> +#ifndef IPU6_BUS_H >>>> +#define IPU6_BUS_H >>>> + >>>> +#include <linux/auxiliary_bus.h> >>> >>> ...Especially for headers which will affect the compilation time. >>> >>>> +#include <linux/pci.h> >>> >>> This is not used. >> >> Do you mean it just need a 'struct pci_dev;' ? > > Yes. > Ack. Will address them in v3.
diff --git a/drivers/media/pci/intel/ipu6/ipu6-bus.c b/drivers/media/pci/intel/ipu6/ipu6-bus.c new file mode 100644 index 000000000000..eabeda5ee4c9 --- /dev/null +++ b/drivers/media/pci/intel/ipu6/ipu6-bus.c @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2013 - 2023 Intel Corporation + */ + +#include <linux/pci.h> +#include <linux/pm_domain.h> +#include <linux/pm_runtime.h> + +#include "ipu6.h" +#include "ipu6-bus.h" +#include "ipu6-dma.h" + +static int bus_pm_runtime_suspend(struct device *dev) +{ + struct ipu6_bus_device *adev = to_ipu6_bus_device(dev); + int ret; + + ret = pm_generic_runtime_suspend(dev); + if (ret) + return ret; + + ret = ipu6_buttress_power(dev, adev->ctrl, false); + if (!ret) + return 0; + + dev_err(dev, "power down failed!\n"); + + /* Powering down failed, attempt to resume device now */ + ret = pm_generic_runtime_resume(dev); + if (!ret) + return -EBUSY; + + return -EIO; +} + +static int bus_pm_runtime_resume(struct device *dev) +{ + struct ipu6_bus_device *adev = to_ipu6_bus_device(dev); + int ret; + + ret = ipu6_buttress_power(dev, adev->ctrl, true); + if (ret) + return ret; + + ret = pm_generic_runtime_resume(dev); + if (ret) + goto out_err; + + return 0; + +out_err: + ipu6_buttress_power(dev, adev->ctrl, false); + + return -EBUSY; +} + +static struct dev_pm_domain ipu6_bus_pm_domain = { + .ops = { + .runtime_suspend = bus_pm_runtime_suspend, + .runtime_resume = bus_pm_runtime_resume, + }, +}; + +static DEFINE_MUTEX(ipu6_bus_mutex); + +static void ipu6_bus_release(struct device *dev) +{ + struct ipu6_bus_device *adev = to_ipu6_bus_device(dev); + + kfree(adev->pdata); + kfree(adev); +} + +struct ipu6_bus_device * +ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, + void *pdata, struct ipu6_buttress_ctrl *ctrl, + char *name) +{ + struct auxiliary_device *auxdev; + struct ipu6_bus_device *adev; + struct ipu6_device *isp = pci_get_drvdata(pdev); + int ret; + + adev = kzalloc(sizeof(*adev), GFP_KERNEL); + if (!adev) + return ERR_PTR(-ENOMEM); + + adev->dma_mask = DMA_BIT_MASK(isp->secure_mode ? IPU6_MMU_ADDR_BITS : + IPU6_MMU_ADDR_BITS_NON_SECURE); + adev->isp = isp; + adev->ctrl = ctrl; + adev->pdata = pdata; + auxdev = &adev->auxdev; + auxdev->name = name; + auxdev->id = (pci_domain_nr(pdev->bus) << 16) | + PCI_DEVID(pdev->bus->number, pdev->devfn); + + auxdev->dev.parent = parent; + auxdev->dev.release = ipu6_bus_release; + auxdev->dev.dma_ops = &ipu6_dma_ops; + auxdev->dev.dma_mask = &adev->dma_mask; + auxdev->dev.dma_parms = pdev->dev.dma_parms; + auxdev->dev.coherent_dma_mask = adev->dma_mask; + + ret = auxiliary_device_init(auxdev); + if (ret < 0) { + dev_err(&isp->pdev->dev, "auxiliary device init failed (%d)\n", + ret); + kfree(adev); + return ERR_PTR(ret); + } + + dev_pm_domain_set(&auxdev->dev, &ipu6_bus_pm_domain); + + pm_runtime_forbid(&adev->auxdev.dev); + pm_runtime_enable(&adev->auxdev.dev); + + return adev; +} + +int ipu6_bus_add_device(struct ipu6_bus_device *adev) +{ + struct auxiliary_device *auxdev = &adev->auxdev; + int ret; + + ret = auxiliary_device_add(auxdev); + if (ret) { + auxiliary_device_uninit(auxdev); + return ret; + } + + mutex_lock(&ipu6_bus_mutex); + list_add(&adev->list, &adev->isp->devices); + mutex_unlock(&ipu6_bus_mutex); + + pm_runtime_allow(&auxdev->dev); + + return 0; +} + +void ipu6_bus_del_devices(struct pci_dev *pdev) +{ + struct ipu6_device *isp = pci_get_drvdata(pdev); + struct ipu6_bus_device *adev, *save; + + mutex_lock(&ipu6_bus_mutex); + + list_for_each_entry_safe(adev, save, &isp->devices, list) { + pm_runtime_disable(&adev->auxdev.dev); + list_del(&adev->list); + auxiliary_device_delete(&adev->auxdev); + auxiliary_device_uninit(&adev->auxdev); + } + + mutex_unlock(&ipu6_bus_mutex); +} diff --git a/drivers/media/pci/intel/ipu6/ipu6-bus.h b/drivers/media/pci/intel/ipu6/ipu6-bus.h new file mode 100644 index 000000000000..fa8bd73c3976 --- /dev/null +++ b/drivers/media/pci/intel/ipu6/ipu6-bus.h @@ -0,0 +1,50 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* Copyright (C) 2013 - 2023 Intel Corporation */ + +#ifndef IPU6_BUS_H +#define IPU6_BUS_H + +#include <linux/auxiliary_bus.h> +#include <linux/pci.h> + +#define IPU6_BUS_NAME IPU6_NAME "-bus" + +struct ipu6_buttress_ctrl; + +struct ipu6_bus_device { + struct auxiliary_device auxdev; + struct auxiliary_driver *auxdrv; + const struct ipu6_auxdrv_data *auxdrv_data; + struct list_head list; + void *pdata; + struct ipu6_mmu *mmu; + struct ipu6_device *isp; + struct ipu6_buttress_ctrl *ctrl; + u64 dma_mask; + const struct firmware *fw; + struct sg_table fw_sgt; + u64 *pkg_dir; + dma_addr_t pkg_dir_dma_addr; + unsigned int pkg_dir_size; +}; + +struct ipu6_auxdrv_data { + irqreturn_t (*isr)(struct ipu6_bus_device *adev); + irqreturn_t (*isr_threaded)(struct ipu6_bus_device *adev); + bool wake_isr_thread; +}; + +#define to_ipu6_bus_device(_dev) container_of(to_auxiliary_dev(_dev), \ + struct ipu6_bus_device, auxdev) +#define auxdev_to_adev(_auxdev) container_of(_auxdev, \ + struct ipu6_bus_device, auxdev) +#define ipu6_bus_get_drvdata(adev) dev_get_drvdata(&(adev)->auxdev.dev) + +struct ipu6_bus_device * +ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, + void *pdata, struct ipu6_buttress_ctrl *ctrl, + char *name); +int ipu6_bus_add_device(struct ipu6_bus_device *adev); +void ipu6_bus_del_devices(struct pci_dev *pdev); + +#endif diff --git a/drivers/media/pci/intel/ipu6/ipu6.c b/drivers/media/pci/intel/ipu6/ipu6.c index 84960a283daf..b652662fa72e 100644 --- a/drivers/media/pci/intel/ipu6/ipu6.c +++ b/drivers/media/pci/intel/ipu6/ipu6.c @@ -666,7 +666,10 @@ static int ipu6_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) return dev_err_probe(&pdev->dev, ret, "Failed to set DMA mask\n"); - dma_set_max_seg_size(&pdev->dev, UINT_MAX); + ret = dma_set_max_seg_size(&pdev->dev, UINT_MAX); + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Failed to set max_seg_size\n"); ret = ipu6_pci_config_setup(pdev, isp->hw_ver); if (ret)