| Message ID | 1449850623-32214-3-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, Dec 11, 2015 at 11:16:58AM -0500, Sinan Kaya wrote: > The Qualcomm Technologies HIDMA device has been designed to support > virtualization technology. The driver has been divided into two to follow > the hardware design. > > 1. HIDMA Management driver > 2. HIDMA Channel driver > > Each HIDMA HW consists of multiple channels. These channels share some set > of common parameters. These parameters are initialized by the management > driver during power up. Same management driver is used for monitoring the > execution of the channels. Management driver can change the performance > behavior dynamically such as bandwidth allocation and prioritization. > > The management driver is executed in hypervisor context and is the main > management entity for all channels provided by the device. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > .../ABI/testing/sysfs-platform-hidma-mgmt | 97 +++++++ > .../devicetree/bindings/dma/qcom_hidma_mgmt.txt | 61 ++++ Please split the binding into a separate patch, per Documentation/devicetree/bindings/submitting-patches.txt. > diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > new file mode 100644 > index 0000000..b632635 > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > @@ -0,0 +1,61 @@ > +Qualcomm Technologies HIDMA Management interface > + > +The Qualcomm Technologies HIDMA device has been designed > +to support virtualization technology. The driver has been > +divided into two to follow the hardware design. The management > +driver is executed in hypervisor context and is the main > +management entity for all channels provided by the device. > + > +Each HIDMA HW consists of multiple channels. These channels > +share some set of common parameters. These parameters are > +initialized by the management driver during power up. > +Same management driver is used for monitoring the execution > +of the channels. Management driver can change the performance > +behavior dynamically such as bandwidth allocation and > +prioritization. > + > +All channel devices get probed in the hypervisor > +context during power up. They show up as DMA engine > +DMA channels. Then, before starting the virtualization; each > +channel device is unbound from the hypervisor by VFIO > +and assign to the guest machine for control. > + > +This management driver will be used by the system > +admin to monitor/reset the execution state of the DMA > +channels. This will be the management interface. This is a mixture of hardware and software description (e.g. VFIO has nothing to do wit hteh hardware). We want to capture what is necessary to describe the hardware, not what the software stack above it will look like. > +Required properties: > +- compatible: "qcom,hidma-mgmt-1.0"; > +- reg: Address range for DMA device Does this cover just the management registers, or those for channels as well? > +- dma-channels: Number of channels supported by this DMA controller. Surely this is discoverable, or can be derived from the set of channels described in the DT? > +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is > + fragmented to multiples of this amount. > +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is > + fragmented to multiples of this amount. > +- max-write-transactions: Maximum write transactions to perform in a burst > +- max-read-transactions: Maximum read transactions to perform in a burst > +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC. > +- channel-priority: Priority of the channel. > + Each dma channel share the same HW bandwidth with other dma channels. > + If two requests reach to the HW at the same time from a low priority and > + high priority channel, high priority channel will claim the bus. > + 0=low priority, 1=high priority > +- channel-weight: Round robin weight of the channel > + Since there are only two priority levels supported, scheduling among > + the equal priority channels is done via weights. Per the example, these last two seem to be arrays, which wasn't clear from the description. Why can this information not be associated with the channel directly? How does one choose the right priority and/or weight? These seem like runtime details given that channels are indned to be allocated by software. There's no relationship to channels defined here. What happens if/when you have a system with multiple instances? > + > +Example: > + > + hidma-mgmt@f9984000 = { > + compatible = "qcom,hidma-mgmt-1.0"; > + reg = <0xf9984000 0x15000>; > + dma-channels = 6; > + max-write-burst-bytes = 1024; > + max-read-burst-bytes = 1024; > + max-write-transactions = 31; > + max-read-transactions = 31; > + channel-reset-timeout-cycles = 0x500; Please fix the syntax here (you're missing '<' and '>' for values. > + channel-priority = <1 1 0 0 0 0>; > + channel-weight = <1 13 10 3 4 5>; > + }; I don't understand why you don't have a single binding for both the management interface and channels, e.g. hidma { compatible - "qcom,hidma-1.0"; /* OPTIONAL management interface registers */ reg = < ... ... >; ... channels { channel0 { compatible = "qcom, reg = < ... ... >; ... }; ... }; }; That would be more in keeping with what we do for other componenents with hyp control elements (e.g. the GIC) and keeps everything associated. Thanks, Mark.
Hi Mark, On 12/11/2015 11:41 AM, Mark Rutland wrote: > Hi, > > On Fri, Dec 11, 2015 at 11:16:58AM -0500, Sinan Kaya wrote: >> The Qualcomm Technologies HIDMA device has been designed to support >> virtualization technology. The driver has been divided into two to follow >> the hardware design. >> >> 1. HIDMA Management driver >> 2. HIDMA Channel driver >> >> Each HIDMA HW consists of multiple channels. These channels share some set >> of common parameters. These parameters are initialized by the management >> driver during power up. Same management driver is used for monitoring the >> execution of the channels. Management driver can change the performance >> behavior dynamically such as bandwidth allocation and prioritization. >> >> The management driver is executed in hypervisor context and is the main >> management entity for all channels provided by the device. >> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> --- >> .../ABI/testing/sysfs-platform-hidma-mgmt | 97 +++++++ >> .../devicetree/bindings/dma/qcom_hidma_mgmt.txt | 61 ++++ > > Please split the binding into a separate patch, per > Documentation/devicetree/bindings/submitting-patches.txt. Done. I'm new to this. Bare with me. > >> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt >> new file mode 100644 >> index 0000000..b632635 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt >> @@ -0,0 +1,61 @@ >> +Qualcomm Technologies HIDMA Management interface >> + >> +The Qualcomm Technologies HIDMA device has been designed >> +to support virtualization technology. The driver has been >> +divided into two to follow the hardware design. The management >> +driver is executed in hypervisor context and is the main >> +management entity for all channels provided by the device. >> + >> +Each HIDMA HW consists of multiple channels. These channels >> +share some set of common parameters. These parameters are >> +initialized by the management driver during power up. >> +Same management driver is used for monitoring the execution >> +of the channels. Management driver can change the performance >> +behavior dynamically such as bandwidth allocation and >> +prioritization. >> + >> +All channel devices get probed in the hypervisor >> +context during power up. They show up as DMA engine >> +DMA channels. Then, before starting the virtualization; each >> +channel device is unbound from the hypervisor by VFIO >> +and assign to the guest machine for control. >> + >> +This management driver will be used by the system >> +admin to monitor/reset the execution state of the DMA >> +channels. This will be the management interface. > > This is a mixture of hardware and software description (e.g. VFIO has > nothing to do wit hteh hardware). We want to capture what is necessary > to describe the hardware, not what the software stack above it will look > like. Another reviewer requested guidance on how to set these parameters. That's why, I tried to provide as much data as possible. > >> +Required properties: >> +- compatible: "qcom,hidma-mgmt-1.0"; >> +- reg: Address range for DMA device > > Does this cover just the management registers, or those for channels as > well? just management. > >> +- dma-channels: Number of channels supported by this DMA controller. > > Surely this is discoverable, or can be derived from the set of channels > described in the DT? No, this is a HW configuration. Each hardware instance supports certain number of channels based on the HW build. The number of active channels on the running operating system does not necessarily represent the maximum possible. > >> +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is >> + fragmented to multiples of this amount. >> +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is >> + fragmented to multiples of this amount. >> +- max-write-transactions: Maximum write transactions to perform in a burst >> +- max-read-transactions: Maximum read transactions to perform in a burst >> +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC. >> +- channel-priority: Priority of the channel. >> + Each dma channel share the same HW bandwidth with other dma channels. >> + If two requests reach to the HW at the same time from a low priority and >> + high priority channel, high priority channel will claim the bus. >> + 0=low priority, 1=high priority >> +- channel-weight: Round robin weight of the channel >> + Since there are only two priority levels supported, scheduling among >> + the equal priority channels is done via weights. > > Per the example, these last two seem to be arrays, which wasn't clear > from the description. OK, let me clarify this. New text: +- channel-priority: an array of channel priorities. +- channel-weight: an array of round robin channel weights > > Why can this information not be associated with the channel directly? > Two reasons: 1. The channel doesn't have the capability to change the priority and weight. HW design. Management HW can do this only. 2. We are building SW to change the channel priority and weight at runtime from the hypervisor through sysfs. The system administrator of the server will reallocate resources based on the guest machine requirements. > How does one choose the right priority and/or weight? These seem like > runtime details given that channels are indned to be allocated by > software. priority = 0..1 weight = 0...15 (adding max value to the documentation) + Valid values are 1..15. > > There's no relationship to channels defined here. What happens if/when > you have a system with multiple instances? > I do support multiple instances. I tested with 4 instances (6 channels each). This driver is only responsible for management which it can do through its own dedicated HW interface. It doesn't need access to the channel address space. There will be 4 HIDMA management instances in this case. >> + >> +Example: >> + >> + hidma-mgmt@f9984000 = { >> + compatible = "qcom,hidma-mgmt-1.0"; >> + reg = <0xf9984000 0x15000>; >> + dma-channels = 6; >> + max-write-burst-bytes = 1024; >> + max-read-burst-bytes = 1024; >> + max-write-transactions = 31; >> + max-read-transactions = 31; >> + channel-reset-timeout-cycles = 0x500; > > Please fix the syntax here (you're missing '<' and '>' for values. Good catch > >> + channel-priority = <1 1 0 0 0 0>; >> + channel-weight = <1 13 10 3 4 5>; >> + }; > > I don't understand why you don't have a single binding for both the > management interface and channels, e.g. > > hidma { > compatible - "qcom,hidma-1.0"; > > /* OPTIONAL management interface registers */ > reg = < ... ... >; > > ... > > channels { > channel0 { > compatible = "qcom, > reg = < ... ... >; > > ... > }; > > ... > }; > }; > > That would be more in keeping with what we do for other componenents > with hyp control elements (e.g. the GIC) and keeps everything > associated. This was discussed before with the previous versions of the patch. This split and loose coupling design is on purpose. The summary is that for static OS configurations where devices remain active for the rest of the OS execution; it makes perfect sense to create a device bus or child device object relationship. The use case here is virtualization and object lifetime in the hypervisor is dynamic. Channel objects get unbound and bound dynamically for guest OS control. At any time, the hypervisor may not have any channel objects if the administrator decides to give all channels to the guest machines. Only the hidma channel driver gets executed in the guest machine. There is no management driver and device entity in the guest. Therefore, child-parent relationship does not exist. The object hierarchy is flat and the only object that exist is a channel object. The channel object can execute independent of the management interface and other channels in the hypervisor/ other guest machines. > > Thanks, > Mark. > Thanks for the review.
> >> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > >> new file mode 100644 > >> index 0000000..b632635 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > >> @@ -0,0 +1,61 @@ > >> +Qualcomm Technologies HIDMA Management interface > >> + > >> +The Qualcomm Technologies HIDMA device has been designed > >> +to support virtualization technology. The driver has been > >> +divided into two to follow the hardware design. The management > >> +driver is executed in hypervisor context and is the main > >> +management entity for all channels provided by the device. > >> + > >> +Each HIDMA HW consists of multiple channels. These channels > >> +share some set of common parameters. These parameters are > >> +initialized by the management driver during power up. > >> +Same management driver is used for monitoring the execution > >> +of the channels. Management driver can change the performance > >> +behavior dynamically such as bandwidth allocation and > >> +prioritization. > >> + > >> +All channel devices get probed in the hypervisor > >> +context during power up. They show up as DMA engine > >> +DMA channels. Then, before starting the virtualization; each > >> +channel device is unbound from the hypervisor by VFIO > >> +and assign to the guest machine for control. > >> + > >> +This management driver will be used by the system > >> +admin to monitor/reset the execution state of the DMA > >> +channels. This will be the management interface. > > > > This is a mixture of hardware and software description (e.g. VFIO has > > nothing to do wit hteh hardware). We want to capture what is necessary > > to describe the hardware, not what the software stack above it will look > > like. > > Another reviewer requested guidance on how to set these parameters. > That's why, I tried to provide as much data as possible. It's perfectly fine to have some other document for the driver, but the binding doc should stick to HW description. > >> +Required properties: > >> +- compatible: "qcom,hidma-mgmt-1.0"; > >> +- reg: Address range for DMA device > > > > Does this cover just the management registers, or those for channels as > > well? > > just management. > > > > >> +- dma-channels: Number of channels supported by this DMA controller. > > > > Surely this is discoverable, or can be derived from the set of channels > > described in the DT? > > No, this is a HW configuration. Each hardware instance supports certain > number of channels based on the HW build. The number of active channels > on the running operating system does not necessarily represent the > maximum possible. I don't follow. If you aren't using some channels, why do you need to know about them? The driver seems to assume it can access registers for all (linearly indexed) channels up to the value it read from dt for dma-channels. That doesn't seem right if the driver is not dealing with all of those channels. > >> +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is > >> + fragmented to multiples of this amount. > >> +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is > >> + fragmented to multiples of this amount. > >> +- max-write-transactions: Maximum write transactions to perform in a burst > >> +- max-read-transactions: Maximum read transactions to perform in a burst > >> +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC. > >> +- channel-priority: Priority of the channel. > >> + Each dma channel share the same HW bandwidth with other dma channels. > >> + If two requests reach to the HW at the same time from a low priority and > >> + high priority channel, high priority channel will claim the bus. > >> + 0=low priority, 1=high priority > >> +- channel-weight: Round robin weight of the channel > >> + Since there are only two priority levels supported, scheduling among > >> + the equal priority channels is done via weights. > > > > Per the example, these last two seem to be arrays, which wasn't clear > > from the description. > > OK, let me clarify this. New text: > +- channel-priority: an array of channel priorities. > +- channel-weight: an array of round robin channel weights > > > > > Why can this information not be associated with the channel directly? > > > Two reasons: > 1. The channel doesn't have the capability to change the priority and > weight. HW design. Management HW can do this only. You can describe the channels directly to the hypervisor which talks to the management HW. > 2. We are building SW to change the channel priority and weight at > runtime from the hypervisor through sysfs. The system administrator of > the server will reallocate resources based on the guest machine > requirements. Ok, so these properties are not necessary at all. They can go, and you can default to a reasonable fair QoS configuration that the user can override at runtime depending on use-case. > > How does one choose the right priority and/or weight? These seem like > > runtime details given that channels are indned to be allocated by > > software. > > priority = 0..1 > weight = 0...15 (adding max value to the documentation) My point was more that the value you want depends on your use-case, which is _not_ a static hardware property, but rather a dynamic run-time property. You stated this can be modified at run time. It should not be in the DT. Please drop it from the binding. > + Valid values are 1..15. > > > > > There's no relationship to channels defined here. What happens if/when > > you have a system with multiple instances? > > > > I do support multiple instances. I tested with 4 instances (6 channels > each). This driver is only responsible for management which it can do > through its own dedicated HW interface. It doesn't need access to the > channel address space. There will be 4 HIDMA management instances in > this case. I don't follow. How do you know which channels are associated with which management interface? How can your sysfs interface work if that relationship is not described? > > I don't understand why you don't have a single binding for both the > > management interface and channels, e.g. > > > > hidma { > > compatible - "qcom,hidma-1.0"; > > > > /* OPTIONAL management interface registers */ > > reg = < ... ... >; > > > > ... > > > > channels { > > channel0 { > > compatible = "qcom, > > reg = < ... ... >; > > > > ... > > }; > > > > ... > > }; > > }; > > > > That would be more in keeping with what we do for other componenents > > with hyp control elements (e.g. the GIC) and keeps everything > > associated. > > This was discussed before with the previous versions of the patch. This > split and loose coupling design is on purpose. I understand it was a deliberate choice. I am disagreeing with that choice. > The summary is that for static OS configurations where devices remain > active for the rest of the OS execution; it makes perfect sense to > create a device bus or child device object relationship. > > The use case here is virtualization and object lifetime in the > hypervisor is dynamic. Channel objects get unbound and bound dynamically > for guest OS control. At any time, the hypervisor may not have any > channel objects if the administrator decides to give all channels to the > guest machines. If the administrator tells the host to give a guest ownership of a channel, the host knows that it cannot use the channel itself, nor can it allocate the channel to another guest. Consider PCIe device passthrough; the host knows that there is a device on the PCIe bus, but also knows that a guest owns it. The same logic should apply here. No guest should care which particular physical channels it's provided with. > Only the hidma channel driver gets executed in the guest machine. There > is no management driver and device entity in the guest. Therefore, > child-parent relationship does not exist. The management driver needs to know details of the channels it's managing, even if it never accesses them directly. Otherwise how can you allocate a channel to a guest and manage it through the correct portion of the correct management interface? I am not arguing for the management driver to run in the guest. For the GIC we have a single binding, yet never run the KVM driver in the guest. The same split of responsibility can apply to the hidma driver(s). Thanks, Mark.
On 12/11/2015 2:37 PM, Mark Rutland wrote: >> >> Another reviewer requested guidance on how to set these parameters. >> That's why, I tried to provide as much data as possible. > > It's perfectly fine to have some other document for the driver, but the > binding doc should stick to HW description. > OK, I'll reword and put some more HW description. >>>> +Required properties: >>>> +- compatible: "qcom,hidma-mgmt-1.0"; >>>> +- reg: Address range for DMA device >>> >>> Does this cover just the management registers, or those for channels as >>> well? >> >> just management. >> >>> >>>> +- dma-channels: Number of channels supported by this DMA controller. >>> >>> Surely this is discoverable, or can be derived from the set of channels >>> described in the DT? >> >> No, this is a HW configuration. Each hardware instance supports certain >> number of channels based on the HW build. The number of active channels >> on the running operating system does not necessarily represent the >> maximum possible. > > I don't follow. > > If you aren't using some channels, why do you need to know about them? > I mean the hypervisor OS may not be using the channels. It could be the guest machines that are using it. I need the number of the channels not the memory addresses where each channel is. > The driver seems to assume it can access registers for all (linearly > indexed) channels up to the value it read from dt for dma-channels. That > doesn't seem right if the driver is not dealing with all of those > channels. I assume you are talking about this. Feel free to be specific if not this one. for (i = 0; i < mgmtdev->dma_channels; i++) { u32 weight = mgmtdev->weight[i]; u32 priority = mgmtdev->priority[i]; val = readl(mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); val &= ~(1 << HIDMA_PRIORITY_BIT_POS); val |= (priority & 0x1) << HIDMA_PRIORITY_BIT_POS; val &= ~(HIDMA_WEIGHT_MASK << HIDMA_WRR_BIT_POS); val |= (weight & HIDMA_WEIGHT_MASK) << HIDMA_WRR_BIT_POS; writel(val, mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); } The management driver is configuring the priority and weight registers inside the management address space. It is not accessing the channel address space where data transfer descriptors are set up. There is one register for priority & weight of the each channel inside the management address space. > >>> >>> Why can this information not be associated with the channel directly? >>> >> Two reasons: >> 1. The channel doesn't have the capability to change the priority and >> weight. HW design. Management HW can do this only. > > You can describe the channels directly to the hypervisor which talks to > the management HW. Not a use case, I don't want to allow guest machine to change their priority and weight. Guest machine runs the channel driver. > >> 2. We are building SW to change the channel priority and weight at >> runtime from the hypervisor through sysfs. The system administrator of >> the server will reallocate resources based on the guest machine >> requirements. > > Ok, so these properties are not necessary at all. > > They can go, and you can default to a reasonable fair QoS configuration > that the user can override at runtime depending on use-case. Partially agreed, some values are "the fair QoS configuration defaults" per platform like maximum-read-request etc. The firmware writes the default values in their device tree/ ACPI table for HW description and the HIDMA driver will configure the defaults. I'll remove the weight and priority though. > >>> How does one choose the right priority and/or weight? These seem like >>> runtime details given that channels are indned to be allocated by >>> software. >> >> priority = 0..1 >> weight = 0...15 (adding max value to the documentation) > > My point was more that the value you want depends on your use-case, > which is _not_ a static hardware property, but rather a dynamic run-time > property. > > You stated this can be modified at run time. It should not be in the DT. > Please drop it from the binding. > I'll get rid of weight and priority only. >> + Valid values are 1..15. >> >>> >>> There's no relationship to channels defined here. What happens if/when >>> you have a system with multiple instances? >>> >> >> I do support multiple instances. I tested with 4 instances (6 channels >> each). This driver is only responsible for management which it can do >> through its own dedicated HW interface. It doesn't need access to the >> channel address space. There will be 4 HIDMA management instances in >> this case. > > I don't follow. > > How do you know which channels are associated with which management > interface? The association is the other way around. User need to know the management interface for a channel rather than the channel needing to know the management interface. I'll think about it. > > How can your sysfs interface work if that relationship is not described? Here is the sysfs documentation. Each management object has one channel directory under it. I still don't need to access the channel object in order to change its weight and priority. What: /sys/devices/platform/hidma-mgmt*/chan*/priority /sys/devices/platform/QCOM8060:*/chan*/priority Date: Nov 2015 KernelVersion: 4.4 Contact: "Sinan Kaya <okaya@cudeaurora.org>" Description: Contains either 0 or 1 and indicates if the DMA channel is a low priority (0) or high priority (1) channel. > >>> I don't understand why you don't have a single binding for both the >>> management interface and channels, e.g. >>> >>> hidma { >>> compatible - "qcom,hidma-1.0"; >>> >>> /* OPTIONAL management interface registers */ >>> reg = < ... ... >; >>> >>> ... >>> >>> channels { >>> channel0 { >>> compatible = "qcom, >>> reg = < ... ... >; >>> >>> ... >>> }; >>> >>> ... >>> }; >>> }; >>> >>> That would be more in keeping with what we do for other componenents >>> with hyp control elements (e.g. the GIC) and keeps everything >>> associated. >> >> This was discussed before with the previous versions of the patch. This >> split and loose coupling design is on purpose. > > I understand it was a deliberate choice. I am disagreeing with that > choice. > In order to be able to use the same channel driver in the guest machine context with the hierarchy you want, I need to create this. fake management object { fake values; channels { real DMA channel { ... } } } This looks really ugly to me. I need to deal with a fake management driver and object that could be hypervisor specific. Here is a fact: QEMU is not capable of generating complex device-tree nodes for platform devices that are being virtualized. QEMU only generates a very simple device object with compatible ID and memory and interrupt resources. No parent, child relationship and no device-specific attributes. Here is an example: device { compatible-id; reg; interrupt; } This is plain and simple. It works too. No need to create some fake device. The other issue is the ACPI. I know device-tree has all kinds of nice gadgets for traversing the parent/child relationship and object pointers. I can't implement this if the solution does not scale to ACPI. I'm still leaning towards keep it simple approach. >> The summary is that for static OS configurations where devices remain >> active for the rest of the OS execution; it makes perfect sense to >> create a device bus or child device object relationship. >> >> The use case here is virtualization and object lifetime in the >> hypervisor is dynamic. Channel objects get unbound and bound dynamically >> for guest OS control. At any time, the hypervisor may not have any >> channel objects if the administrator decides to give all channels to the >> guest machines. > > If the administrator tells the host to give a guest ownership of a > channel, the host knows that it cannot use the channel itself, nor can > it allocate the channel to another guest. Consider PCIe device > passthrough; the host knows that there is a device on the PCIe bus, but > also knows that a guest owns it. The same logic should apply here. Agreed, the HIDMA channel object gets associated to the VFIO driver while the guest is owning the HIDMA channel object. > > No guest should care which particular physical channels it's provided > with. > >> Only the hidma channel driver gets executed in the guest machine. There >> is no management driver and device entity in the guest. Therefore, >> child-parent relationship does not exist. > > The management driver needs to know details of the channels it's > managing, even if it never accesses them directly. Otherwise how can you > allocate a channel to a guest and manage it through the correct portion > of the correct management interface? I'll add support for both. Both having a parent-child relationship while inside the hypervisor and flat hierarchy while running in the guest machine for QEMU. I just created a child object of the management driver and channel driver got probed properly. So, it just a matter of device-tree documentation at this point. It will be like this though. >>> hidma { >>> compatible - "qcom,hidma-1.0"; >>> >>> /* OPTIONAL management interface registers */ >>> reg = < ... ... >; >>> >>> ... >>> >>> channel0 { >>> compatible = "qcom, >>> reg = < ... ... >; >>> >>> ... >>> }; >>> >>> ... >>> }; This seems to have worked without requiring any work from me. I need to associate sysfs next. > > I am not arguing for the management driver to run in the guest. For the > GIC we have a single binding, yet never run the KVM driver in the guest. > The same split of responsibility can apply to the hidma driver(s). > > Thanks, > Mark. >
> >>>> +Required properties: > >>>> +- compatible: "qcom,hidma-mgmt-1.0"; > >>>> +- reg: Address range for DMA device > >>> > >>> Does this cover just the management registers, or those for channels as > >>> well? > >> > >> just management. > >> > >>> > >>>> +- dma-channels: Number of channels supported by this DMA controller. > >>> > >>> Surely this is discoverable, or can be derived from the set of channels > >>> described in the DT? > >> > >> No, this is a HW configuration. Each hardware instance supports certain > >> number of channels based on the HW build. The number of active channels > >> on the running operating system does not necessarily represent the > >> maximum possible. > > > > I don't follow. > > > > If you aren't using some channels, why do you need to know about them? > > > > I mean the hypervisor OS may not be using the channels. It could be the > guest machines that are using it. I need the number of the channels not > the memory addresses where each channel is. I still don't follow. Knowing the number of channels alone is clearly insufficient. The hypervisor is in charge of handing these out to guests. So it needs to know the set of of channels (i.e. the address of each channel, and which index the channel has in the control interface). Otherwise it cannot possibly allocate them to guests, and cannot possibly control those channels at runtime. Given the hypervisor is in chage of allocating channels to guests, it knows which channels are in use, and can decide whether or not to use channels for its own purposes. > > The driver seems to assume it can access registers for all (linearly > > indexed) channels up to the value it read from dt for dma-channels. That > > doesn't seem right if the driver is not dealing with all of those > > channels. > > I assume you are talking about this. Feel free to be specific if not > this one. > > for (i = 0; i < mgmtdev->dma_channels; i++) { > u32 weight = mgmtdev->weight[i]; > u32 priority = mgmtdev->priority[i]; > > val = readl(mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); > val &= ~(1 << HIDMA_PRIORITY_BIT_POS); > val |= (priority & 0x1) << HIDMA_PRIORITY_BIT_POS; > val &= ~(HIDMA_WEIGHT_MASK << HIDMA_WRR_BIT_POS); > val |= (weight & HIDMA_WEIGHT_MASK) << HIDMA_WRR_BIT_POS; > writel(val, mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); > } > > The management driver is configuring the priority and weight registers > inside the management address space. It is not accessing the channel > address space where data transfer descriptors are set up. There is one > register for priority & weight of the each channel inside the management > address space. My point was that this implies that the driver has no idea as to the relationship between these configuration registers and the actual channel address spaces. It has no idea which channel address spaces are being configured here. That does not seem right. > >>> Why can this information not be associated with the channel directly? > >>> > >> Two reasons: > >> 1. The channel doesn't have the capability to change the priority and > >> weight. HW design. Management HW can do this only. > > > > You can describe the channels directly to the hypervisor which talks to > > the management HW. > > Not a use case, I don't want to allow guest machine to change their > priority and weight. I didn't suggest that the guest would. Regardless, it seems these should be runtime configured anyway, at which point you need to know the relationship. > >>> There's no relationship to channels defined here. What happens if/when > >>> you have a system with multiple instances? > >>> > >> > >> I do support multiple instances. I tested with 4 instances (6 channels > >> each). This driver is only responsible for management which it can do > >> through its own dedicated HW interface. It doesn't need access to the > >> channel address space. There will be 4 HIDMA management instances in > >> this case. > > > > I don't follow. > > > > How do you know which channels are associated with which management > > interface? > > The association is the other way around. User need to know the > management interface for a channel rather than the channel needing to > know the management interface. > > I'll think about it. Ok. I agree that's the way the user will look at it. > > How can your sysfs interface work if that relationship is not described? > > Here is the sysfs documentation. Each management object has one channel > directory under it. I still don't need to access the channel object in > order to change its weight and priority. > > What: /sys/devices/platform/hidma-mgmt*/chan*/priority > /sys/devices/platform/QCOM8060:*/chan*/priority > Date: Nov 2015 > KernelVersion: 4.4 > Contact: "Sinan Kaya <okaya@cudeaurora.org>" > Description: > Contains either 0 or 1 and indicates if the DMA channel is a > low priority (0) or high priority (1) channel. What I was trying to ask is: given a write to a channel's priority file, how do you figure out which bits in which management interface to poke? As far as I can see, the relationship between a channel and the management interface is not described in teh DT, so I cannot see how you figure this out. > >>> I don't understand why you don't have a single binding for both the > >>> management interface and channels, e.g. > >>> > >>> hidma { > >>> compatible - "qcom,hidma-1.0"; > >>> > >>> /* OPTIONAL management interface registers */ > >>> reg = < ... ... >; > >>> > >>> ... > >>> > >>> channels { > >>> channel0 { > >>> compatible = "qcom, > >>> reg = < ... ... >; > >>> > >>> ... > >>> }; > >>> > >>> ... > >>> }; > >>> }; > >>> > >>> That would be more in keeping with what we do for other componenents > >>> with hyp control elements (e.g. the GIC) and keeps everything > >>> associated. > >> > >> This was discussed before with the previous versions of the patch. This > >> split and loose coupling design is on purpose. > > > > I understand it was a deliberate choice. I am disagreeing with that > > choice. > > > > In order to be able to use the same channel driver in the guest machine > context with the hierarchy you want, I need to create this. > > fake management object { > fake values; There'd be no fake values here, they'd just be ommitted entirely. > channels { > real DMA channel { > ... > } > } > } > > This looks really ugly to me. I need to deal with a fake management > driver and object that could be hypervisor specific. You don't need a fake management driver. I don't understand what you mean w.r.t. being hypervisor specific. > Here is a fact: > > QEMU is not capable of generating complex device-tree nodes for platform > devices that are being virtualized. QEMU only generates a very simple > device object with compatible ID and memory and interrupt resources. No > parent, child relationship and no device-specific attributes. Here is an > example: So? The above _is_ simple, and trivial to generate. I've not seen your patches for QEMU, so perhaps I'm missing something. Regardless, QEMU is not the only thing that matters here. We shouldn't do something _only_ because it makes a patch to QEMU simpler. > > device { > compatible-id; > reg; > interrupt; > } > > This is plain and simple. It works too. No need to create some fake device. > > The other issue is the ACPI. I know device-tree has all kinds of nice > gadgets for traversing the parent/child relationship and object > pointers. I can't implement this if the solution does not scale to ACPI. It sounds like ACPI is incapable of describing this device, then. > > No guest should care which particular physical channels it's provided > > with. > > > >> Only the hidma channel driver gets executed in the guest machine. There > >> is no management driver and device entity in the guest. Therefore, > >> child-parent relationship does not exist. > > > > The management driver needs to know details of the channels it's > > managing, even if it never accesses them directly. Otherwise how can you > > allocate a channel to a guest and manage it through the correct portion > > of the correct management interface? > > I'll add support for both. Both having a parent-child relationship while > inside the hypervisor and flat hierarchy while running in the guest > machine for QEMU. I'm happy with that. > I just created a child object of the management driver and channel > driver got probed properly. So, it just a matter of device-tree > documentation at this point. > > It will be like this though. > > >>> hidma { > >>> compatible - "qcom,hidma-1.0"; > >>> > >>> /* OPTIONAL management interface registers */ > >>> reg = < ... ... >; > >>> > >>> ... > >>> > >>> channel0 { > >>> compatible = "qcom, > >>> reg = < ... ... >; > >>> > >>> ... > >>> }; > >>> > >>> ... > >>> }; > > This seems to have worked without requiring any work from me. > > I need to associate sysfs next. Ok. I look forward to seeing those patches. Thanks, Mark.
Hi Mark, On 12/14/2015 7:05 AM, Mark Rutland wrote: >>>>>> +Required properties: >>>>>> +- compatible: "qcom,hidma-mgmt-1.0"; >>>>>> +- reg: Address range for DMA device >>>>> >>>>> Does this cover just the management registers, or those for channels as >>>>> well? >>>> >>>> just management. >>>> >>>>> >>>>>> +- dma-channels: Number of channels supported by this DMA controller. >>>>> >>>>> Surely this is discoverable, or can be derived from the set of channels >>>>> described in the DT? >>>> >>>> No, this is a HW configuration. Each hardware instance supports certain >>>> number of channels based on the HW build. The number of active channels >>>> on the running operating system does not necessarily represent the >>>> maximum possible. >>> >>> I don't follow. >>> >>> If you aren't using some channels, why do you need to know about them? >>> >> >> I mean the hypervisor OS may not be using the channels. It could be the >> guest machines that are using it. I need the number of the channels not >> the memory addresses where each channel is. > > I still don't follow. Knowing the number of channels alone is clearly > insufficient. > > The hypervisor is in charge of handing these out to guests. So it needs > to know the set of of channels (i.e. the address of each channel, and > which index the channel has in the control interface). Otherwise it > cannot possibly allocate them to guests, and cannot possibly control > those channels at runtime. For clarification, let me reiterate. Hypervisor operating system certainly knows which channels and management interfaces there are in the system. However, the information feeds into the OS from two different drivers. The HW ownership has been partitioned among two drivers as management driver and channel driver. Both of them serve the hypervisor but the management driver doesn't need to know about the channel addresses where the transfer descriptors are set up. Similarly, the channel driver doesn't need to know anything about the management interface where the priority and weight assignment is done. > > Given the hypervisor is in chage of allocating channels to guests, it > knows which channels are in use, and can decide whether or not to use > channels for its own purposes. Agreed > >>> The driver seems to assume it can access registers for all (linearly >>> indexed) channels up to the value it read from dt for dma-channels. That >>> doesn't seem right if the driver is not dealing with all of those >>> channels. >> >> I assume you are talking about this. Feel free to be specific if not >> this one. >> >> for (i = 0; i < mgmtdev->dma_channels; i++) { >> u32 weight = mgmtdev->weight[i]; >> u32 priority = mgmtdev->priority[i]; >> >> val = readl(mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); >> val &= ~(1 << HIDMA_PRIORITY_BIT_POS); >> val |= (priority & 0x1) << HIDMA_PRIORITY_BIT_POS; >> val &= ~(HIDMA_WEIGHT_MASK << HIDMA_WRR_BIT_POS); >> val |= (weight & HIDMA_WEIGHT_MASK) << HIDMA_WRR_BIT_POS; >> writel(val, mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); >> } >> >> The management driver is configuring the priority and weight registers >> inside the management address space. It is not accessing the channel >> address space where data transfer descriptors are set up. There is one >> register for priority & weight of the each channel inside the management >> address space. > > My point was that this implies that the driver has no idea as to the > relationship between these configuration registers and the actual > channel address spaces. It has no idea which channel address spaces are > being configured here. That does not seem right. My intention was to use the ACPI _UID to associate the channel index. The _UID would tell me the index of the channel. I was encoding UID as follows in the ACPI table. <deviceid><channelid> this would tell me which HIDMA this channel belongs to as well as which channel it is. Since this solution is very much ACPI specific and doesn't scale to device-tree, I'm thinking of adding a sysfs "index" file into each channel so that I don't need to search through the firmware_node in ACPI and of_node directory in device-tree mode. I'm also thinking of creating symbolic links from the channel object into the management object. >>>>> hidma { >>>>> compatible - "qcom,hidma-1.0"; >>>>> >>>>> /* OPTIONAL management interface registers */ >>>>> reg = < ... ... >; >>>>> >>>>> ... >>>>> >>>>> channel0 { >>>>> compatible = "qcom, >>>>> reg = < ... ... >; >>>>> >>>>> ... >>>>> }; >>>>> >>>>> ... >>>>> }; >> >> This seems to have worked without requiring any work from me. >> Here is something I don't understand why and I could get some help from DT experts. When I have ACPI as follows device(mgmt) { device(channel0) { } } The channel0 driver gets automatically probed by the OS. When I have device tree based structure as follows, hidma_00 doesn't get probed. hidmam_00: hidma-mgmt@0x58000000 { compatible = "qcom,hidma_mgmt-1.0"; hidma_00: hidma@0x58010000 { compatible = "qcom,hidma-1.0"; } } If I put them seperately, it works hidma_00: hidma@0x58010000 { compatible = "qcom,hidma-1.0"; } hidmam_00: hidma-mgmt@0x58000000 { compatible = "qcom,hidma_mgmt-1.0"; } Wondering If there is any mechanism in DT, I could have the same probing behavior as ACPI? I hate calling the of_find_matching_node functions and locate the child object. Then, create a platform device. This seems a lot of hand-crafting to me. I don't have any OF/ACPI specific code in my driver. I want to avoid it if there is a way. >> I need to associate sysfs next. > > Ok. I look forward to seeing those patches. > OK. I'll post it as soon as I figure out DT/ACPI behavior differences. > Thanks, > Mark. >
On 12/14/2015 7:05 AM, Mark Rutland wrote: >>>>>> +Required properties: >>>>>> +- compatible: "qcom,hidma-mgmt-1.0"; >>>>>> +- reg: Address range for DMA device >>>>> >>>>> Does this cover just the management registers, or those for channels as >>>>> well? >>>> >>>> just management. >>>> >>>>> >>>>>> +- dma-channels: Number of channels supported by this DMA controller. >>>>> >>>>> Surely this is discoverable, or can be derived from the set of channels >>>>> described in the DT? >>>> >>>> No, this is a HW configuration. Each hardware instance supports certain >>>> number of channels based on the HW build. The number of active channels >>>> on the running operating system does not necessarily represent the >>>> maximum possible. >>> >>> I don't follow. >>> >>> If you aren't using some channels, why do you need to know about them? >>> >> >> I mean the hypervisor OS may not be using the channels. It could be the >> guest machines that are using it. I need the number of the channels not >> the memory addresses where each channel is. > > I still don't follow. Knowing the number of channels alone is clearly > insufficient. > > The hypervisor is in charge of handing these out to guests. So it needs > to know the set of of channels (i.e. the address of each channel, and > which index the channel has in the control interface). Otherwise it > cannot possibly allocate them to guests, and cannot possibly control > those channels at runtime. > > Given the hypervisor is in chage of allocating channels to guests, it > knows which channels are in use, and can decide whether or not to use > channels for its own purposes. > >>> The driver seems to assume it can access registers for all (linearly >>> indexed) channels up to the value it read from dt for dma-channels. That >>> doesn't seem right if the driver is not dealing with all of those >>> channels. >> >> I assume you are talking about this. Feel free to be specific if not >> this one. >> >> for (i = 0; i < mgmtdev->dma_channels; i++) { >> u32 weight = mgmtdev->weight[i]; >> u32 priority = mgmtdev->priority[i]; >> >> val = readl(mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); >> val &= ~(1 << HIDMA_PRIORITY_BIT_POS); >> val |= (priority & 0x1) << HIDMA_PRIORITY_BIT_POS; >> val &= ~(HIDMA_WEIGHT_MASK << HIDMA_WRR_BIT_POS); >> val |= (weight & HIDMA_WEIGHT_MASK) << HIDMA_WRR_BIT_POS; >> writel(val, mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); >> } >> >> The management driver is configuring the priority and weight registers >> inside the management address space. It is not accessing the channel >> address space where data transfer descriptors are set up. There is one >> register for priority & weight of the each channel inside the management >> address space. > > My point was that this implies that the driver has no idea as to the > relationship between these configuration registers and the actual > channel address spaces. It has no idea which channel address spaces are > being configured here. That does not seem right. > >>>>> Why can this information not be associated with the channel directly? >>>>> >>>> Two reasons: >>>> 1. The channel doesn't have the capability to change the priority and >>>> weight. HW design. Management HW can do this only. >>> >>> You can describe the channels directly to the hypervisor which talks to >>> the management HW. >> >> Not a use case, I don't want to allow guest machine to change their >> priority and weight. > > I didn't suggest that the guest would. Regardless, it seems these should > be runtime configured anyway, at which point you need to know the > relationship. > >>>>> There's no relationship to channels defined here. What happens if/when >>>>> you have a system with multiple instances? >>>>> >>>> >>>> I do support multiple instances. I tested with 4 instances (6 channels >>>> each). This driver is only responsible for management which it can do >>>> through its own dedicated HW interface. It doesn't need access to the >>>> channel address space. There will be 4 HIDMA management instances in >>>> this case. >>> >>> I don't follow. >>> >>> How do you know which channels are associated with which management >>> interface? >> >> The association is the other way around. User need to know the >> management interface for a channel rather than the channel needing to >> know the management interface. >> >> I'll think about it. > > Ok. I agree that's the way the user will look at it. > >>> How can your sysfs interface work if that relationship is not described? >> >> Here is the sysfs documentation. Each management object has one channel >> directory under it. I still don't need to access the channel object in >> order to change its weight and priority. >> >> What: /sys/devices/platform/hidma-mgmt*/chan*/priority >> /sys/devices/platform/QCOM8060:*/chan*/priority >> Date: Nov 2015 >> KernelVersion: 4.4 >> Contact: "Sinan Kaya <okaya@cudeaurora.org>" >> Description: >> Contains either 0 or 1 and indicates if the DMA channel is a >> low priority (0) or high priority (1) channel. > > What I was trying to ask is: given a write to a channel's priority file, > how do you figure out which bits in which management interface to poke? > > As far as I can see, the relationship between a channel and the > management interface is not described in teh DT, so I cannot see how you > figure this out. > >>>>> I don't understand why you don't have a single binding for both the >>>>> management interface and channels, e.g. >>>>> >>>>> hidma { >>>>> compatible - "qcom,hidma-1.0"; >>>>> >>>>> /* OPTIONAL management interface registers */ >>>>> reg = < ... ... >; >>>>> >>>>> ... >>>>> >>>>> channels { >>>>> channel0 { >>>>> compatible = "qcom, >>>>> reg = < ... ... >; >>>>> >>>>> ... >>>>> }; >>>>> >>>>> ... >>>>> }; >>>>> }; >>>>> >>>>> That would be more in keeping with what we do for other componenents >>>>> with hyp control elements (e.g. the GIC) and keeps everything >>>>> associated. >>>> >>>> This was discussed before with the previous versions of the patch. This >>>> split and loose coupling design is on purpose. >>> >>> I understand it was a deliberate choice. I am disagreeing with that >>> choice. >>> >> >> In order to be able to use the same channel driver in the guest machine >> context with the hierarchy you want, I need to create this. >> >> fake management object { >> fake values; > > There'd be no fake values here, they'd just be ommitted entirely. > >> channels { >> real DMA channel { >> ... >> } >> } >> } >> >> This looks really ugly to me. I need to deal with a fake management >> driver and object that could be hypervisor specific. > > You don't need a fake management driver. I don't understand what you > mean w.r.t. being hypervisor specific. > >> Here is a fact: >> >> QEMU is not capable of generating complex device-tree nodes for platform >> devices that are being virtualized. QEMU only generates a very simple >> device object with compatible ID and memory and interrupt resources. No >> parent, child relationship and no device-specific attributes. Here is an >> example: > > So? The above _is_ simple, and trivial to generate. > > I've not seen your patches for QEMU, so perhaps I'm missing something. > > Regardless, QEMU is not the only thing that matters here. We shouldn't > do something _only_ because it makes a patch to QEMU simpler. > >> >> device { >> compatible-id; >> reg; >> interrupt; >> } >> >> This is plain and simple. It works too. No need to create some fake device. >> >> The other issue is the ACPI. I know device-tree has all kinds of nice >> gadgets for traversing the parent/child relationship and object >> pointers. I can't implement this if the solution does not scale to ACPI. > > It sounds like ACPI is incapable of describing this device, then. > >>> No guest should care which particular physical channels it's provided >>> with. >>> >>>> Only the hidma channel driver gets executed in the guest machine. There >>>> is no management driver and device entity in the guest. Therefore, >>>> child-parent relationship does not exist. >>> >>> The management driver needs to know details of the channels it's >>> managing, even if it never accesses them directly. Otherwise how can you >>> allocate a channel to a guest and manage it through the correct portion >>> of the correct management interface? >> >> I'll add support for both. Both having a parent-child relationship while >> inside the hypervisor and flat hierarchy while running in the guest >> machine for QEMU. > > I'm happy with that. > >> I just created a child object of the management driver and channel >> driver got probed properly. So, it just a matter of device-tree >> documentation at this point. >> >> It will be like this though. >> >>>>> hidma { >>>>> compatible - "qcom,hidma-1.0"; >>>>> >>>>> /* OPTIONAL management interface registers */ >>>>> reg = < ... ... >; >>>>> >>>>> ... >>>>> >>>>> channel0 { >>>>> compatible = "qcom, >>>>> reg = < ... ... >; >>>>> >>>>> ... >>>>> }; >>>>> >>>>> ... >>>>> }; >> >> This seems to have worked without requiring any work from me. >> >> I need to associate sysfs next. > > Ok. I look forward to seeing those patches. > > Thanks, > Mark. > Mark, The patch has been posted here. Please review. https://lkml.org/lkml/2016/1/3/246 Sinan
Hi Mark, Rob, Frank, Grant and other DT experts; >>> >>> I'll add support for both. Both having a parent-child relationship while >>> inside the hypervisor and flat hierarchy while running in the guest >>> machine for QEMU. >> >> I'm happy with that. >> >>> I just created a child object of the management driver and channel >>> driver got probed properly. So, it just a matter of device-tree >>> documentation at this point. >>> >>> It will be like this though. >>> >>>>>> hidma { >>>>>> compatible - "qcom,hidma-1.0"; >>>>>> >>>>>> /* OPTIONAL management interface registers */ >>>>>> reg = < ... ... >; >>>>>> >>>>>> ... >>>>>> >>>>>> channel0 { >>>>>> compatible = "qcom, >>>>>> reg = < ... ... >; >>>>>> >>>>>> ... >>>>>> }; >>>>>> >>>>>> ... >>>>>> }; >>> >>> This seems to have worked without requiring any work from me. >>> >>> I need to associate sysfs next. >> >> Ok. I look forward to seeing those patches. >> >> Thanks, >> Mark. >> > > Mark, > The patch has been posted here. Please review. > > https://lkml.org/lkml/2016/1/3/246 > > Sinan > > > > I have been trying to reach to device tree reviewers without any luck for the last two weeks. It must be the holidays. I have restructured the code address concerns from Mark Rutland above. The dma-engine maintainer, Vinod Koul; is looking for an ACK from the DT community before the patches can be incorporated. Could you please kindly review the series here from DT perspective and ACK it if you are OK? https://lkml.org/lkml/2016/1/11/517 This document here https://www.kernel.org/doc/linux/MAINTAINERS tells me that Rob Herring, Frank Rowand and Grant Likely are the maintainers for device tree. OPEN FIRMWARE AND FLATTENED DEVICE TREE M: Rob Herring <robh+dt@kernel.org> M: Frank Rowand <frowand.list@gmail.com> M: Grant Likely <grant.likely@linaro.org> L: devicetree@vger.kernel.org That's why, I'm reaching out.
On Wed, Jan 13, 2016 at 12:59:24PM -0500, Sinan Kaya wrote: > Hi Mark, Rob, Frank, Grant and other DT experts; > I have been trying to reach to device tree reviewers without any luck for the last two weeks. > It must be the holidays. Indeed. Apologies for the delay. > I have restructured the code address concerns from Mark Rutland above. The dma-engine maintainer, Vinod Koul; > is looking for an ACK from the DT community before the patches can be incorporated. > > Could you please kindly review the series here from DT perspective and ACK it if you are OK? > https://lkml.org/lkml/2016/1/11/517 I will try to look at v12 shortly (i.e. in the next day or two). Thanks, Mark.
diff --git a/Documentation/ABI/testing/sysfs-platform-hidma-mgmt b/Documentation/ABI/testing/sysfs-platform-hidma-mgmt new file mode 100644 index 0000000..4fc6876 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-platform-hidma-mgmt @@ -0,0 +1,97 @@ +What: /sys/devices/platform/hidma-mgmt*/chan*/priority + /sys/devices/platform/QCOM8060:*/chan*/priority +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Contains either 0 or 1 and indicates if the DMA channel is a + low priority (0) or high priority (1) channel. + +What: /sys/devices/platform/hidma-mgmt*/chan*/weight + /sys/devices/platform/QCOM8060:*/chan*/weight +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Contains 0..15 and indicates the weight of the channel among + equal priority channels during round robin scheduling. + +What: /sys/devices/platform/hidma-mgmt*/chreset_timeout_cycles + /sys/devices/platform/QCOM8060:*/chreset_timeout_cycles +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Contains the platform specific cycle value to wait after a + reset command is issued. If the value is chosen too short, + then the HW will issue a reset failure interrupt. The value + is platform specific and should not be changed without + consultance. + +What: /sys/devices/platform/hidma-mgmt*/dma_channels + /sys/devices/platform/QCOM8060:*/dma_channels +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Contains the number of dma channels supported by one instance + of HIDMA hardware. The value may change from chip to chip. + +What: /sys/devices/platform/hidma-mgmt*/hw_version_major + /sys/devices/platform/QCOM8060:*/hw_version_major +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Version number major for the hardware. + +What: /sys/devices/platform/hidma-mgmt*/hw_version_minor + /sys/devices/platform/QCOM8060:*/hw_version_minor +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Version number minor for the hardware. + +What: /sys/devices/platform/hidma-mgmt*/max_rd_xactions + /sys/devices/platform/QCOM8060:*/max_rd_xactions +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Contains a value between 0 and 31. Maximum number of + read transactions that can be issued back to back. + Choosing a higher number gives better performance but + can also cause performance reduction to other peripherals + sharing the same bus. + +What: /sys/devices/platform/hidma-mgmt*/max_read_request + /sys/devices/platform/QCOM8060:*/max_read_request +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Size of each read request. The value needs to be a power + of two and can be between 128 and 1024. + +What: /sys/devices/platform/hidma-mgmt*/max_wr_xactions + /sys/devices/platform/QCOM8060:*/max_wr_xactions +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Contains a value between 0 and 31. Maximum number of + write transactions that can be issued back to back. + Choosing a higher number gives better performance but + can also cause performance reduction to other peripherals + sharing the same bus. + + +What: /sys/devices/platform/hidma-mgmt*/max_write_request + /sys/devices/platform/QCOM8060:*/max_write_request +Date: Nov 2015 +KernelVersion: 4.4 +Contact: "Sinan Kaya <okaya@cudeaurora.org>" +Description: + Size of each write request. The value needs to be a power + of two and can be between 128 and 1024. diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt new file mode 100644 index 0000000..b632635 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt @@ -0,0 +1,61 @@ +Qualcomm Technologies HIDMA Management interface + +The Qualcomm Technologies HIDMA device has been designed +to support virtualization technology. The driver has been +divided into two to follow the hardware design. The management +driver is executed in hypervisor context and is the main +management entity for all channels provided by the device. + +Each HIDMA HW consists of multiple channels. These channels +share some set of common parameters. These parameters are +initialized by the management driver during power up. +Same management driver is used for monitoring the execution +of the channels. Management driver can change the performance +behavior dynamically such as bandwidth allocation and +prioritization. + +All channel devices get probed in the hypervisor +context during power up. They show up as DMA engine +DMA channels. Then, before starting the virtualization; each +channel device is unbound from the hypervisor by VFIO +and assign to the guest machine for control. + +This management driver will be used by the system +admin to monitor/reset the execution state of the DMA +channels. This will be the management interface. + + +Required properties: +- compatible: "qcom,hidma-mgmt-1.0"; +- reg: Address range for DMA device +- dma-channels: Number of channels supported by this DMA controller. +- max-write-burst-bytes: Maximum write burst in bytes. A memcpy requested is + fragmented to multiples of this amount. +- max-read-burst-bytes: Maximum read burst in bytes. A memcpy request is + fragmented to multiples of this amount. +- max-write-transactions: Maximum write transactions to perform in a burst +- max-read-transactions: Maximum read transactions to perform in a burst +- channel-reset-timeout-cycles: Channel reset timeout in cycles for this SOC. +- channel-priority: Priority of the channel. + Each dma channel share the same HW bandwidth with other dma channels. + If two requests reach to the HW at the same time from a low priority and + high priority channel, high priority channel will claim the bus. + 0=low priority, 1=high priority +- channel-weight: Round robin weight of the channel + Since there are only two priority levels supported, scheduling among + the equal priority channels is done via weights. + +Example: + + hidma-mgmt@f9984000 = { + compatible = "qcom,hidma-mgmt-1.0"; + reg = <0xf9984000 0x15000>; + dma-channels = 6; + max-write-burst-bytes = 1024; + max-read-burst-bytes = 1024; + max-write-transactions = 31; + max-read-transactions = 31; + channel-reset-timeout-cycles = 0x500; + channel-priority = <1 1 0 0 0 0>; + channel-weight = <1 13 10 3 4 5>; + }; diff --git a/drivers/dma/qcom/Kconfig b/drivers/dma/qcom/Kconfig index f17c272..ebe5b8c 100644 --- a/drivers/dma/qcom/Kconfig +++ b/drivers/dma/qcom/Kconfig @@ -6,3 +6,14 @@ config QCOM_BAM_DMA ---help--- Enable support for the QCOM BAM DMA controller. This controller provides DMA capabilities for a variety of on-chip devices. + +config QCOM_HIDMA_MGMT + tristate "Qualcomm Technologies HIDMA Management support" + select DMA_ENGINE + help + Enable support for the Qualcomm Technologies HIDMA Management. + Each DMA device requires one management interface driver + for basic initialization before QCOM_HIDMA channel driver can + start managing the channels. In a virtualized environment, + the guest OS would run QCOM_HIDMA channel driver and the + hypervisor would run the QCOM_HIDMA_MGMT management driver. diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile index f612ae3..1a5a96d 100644 --- a/drivers/dma/qcom/Makefile +++ b/drivers/dma/qcom/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_QCOM_BAM_DMA) += bam_dma.o +obj-$(CONFIG_QCOM_HIDMA_MGMT) += hidma_mgmt.o hidma_mgmt_sys.o diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c new file mode 100644 index 0000000..1898143 --- /dev/null +++ b/drivers/dma/qcom/hidma_mgmt.c @@ -0,0 +1,318 @@ +/* + * Qualcomm Technologies HIDMA DMA engine Management interface + * + * Copyright (c) 2015, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/dmaengine.h> +#include <linux/acpi.h> +#include <linux/of.h> +#include <linux/property.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/uaccess.h> +#include <linux/slab.h> +#include <linux/pm_runtime.h> +#include <linux/bitops.h> + +#include "hidma_mgmt.h" + +#define HIDMA_QOS_N_OFFSET 0x300 +#define HIDMA_CFG_OFFSET 0x400 +#define HIDMA_MAX_BUS_REQ_LEN_OFFSET 0x41C +#define HIDMA_MAX_XACTIONS_OFFSET 0x420 +#define HIDMA_HW_VERSION_OFFSET 0x424 +#define HIDMA_CHRESET_TIMEOUT_OFFSET 0x418 + +#define HIDMA_MAX_WR_XACTIONS_MASK GENMASK(4, 0) +#define HIDMA_MAX_RD_XACTIONS_MASK GENMASK(4, 0) +#define HIDMA_WEIGHT_MASK GENMASK(6, 0) +#define HIDMA_MAX_BUS_REQ_LEN_MASK GENMASK(15, 0) +#define HIDMA_CHRESET_TIMEOUT_MASK GENMASK(19, 0) + +#define HIDMA_MAX_WR_XACTIONS_BIT_POS 16 +#define HIDMA_MAX_BUS_WR_REQ_BIT_POS 16 +#define HIDMA_WRR_BIT_POS 8 +#define HIDMA_PRIORITY_BIT_POS 15 + +#define HIDMA_AUTOSUSPEND_TIMEOUT 2000 +#define HIDMA_MAX_CHANNEL_WEIGHT 15 + +int hidma_mgmt_setup(struct hidma_mgmt_dev *mgmtdev) +{ + unsigned int i; + u32 val; + + if (!is_power_of_2(mgmtdev->max_write_request) || + (mgmtdev->max_write_request < 128) || + (mgmtdev->max_write_request > 1024)) { + dev_err(&mgmtdev->pdev->dev, "invalid write request %d\n", + mgmtdev->max_write_request); + return -EINVAL; + } + + if (!is_power_of_2(mgmtdev->max_read_request) || + (mgmtdev->max_read_request < 128) || + (mgmtdev->max_read_request > 1024)) { + dev_err(&mgmtdev->pdev->dev, "invalid read request %d\n", + mgmtdev->max_read_request); + return -EINVAL; + } + + if (mgmtdev->max_wr_xactions > HIDMA_MAX_WR_XACTIONS_MASK) { + dev_err(&mgmtdev->pdev->dev, + "max_wr_xactions cannot be bigger than %ld\n", + HIDMA_MAX_WR_XACTIONS_MASK); + return -EINVAL; + } + + if (mgmtdev->max_rd_xactions > HIDMA_MAX_RD_XACTIONS_MASK) { + dev_err(&mgmtdev->pdev->dev, + "max_rd_xactions cannot be bigger than %ld\n", + HIDMA_MAX_RD_XACTIONS_MASK); + return -EINVAL; + } + + for (i = 0; i < mgmtdev->dma_channels; i++) { + if (mgmtdev->priority[i] > 1) { + dev_err(&mgmtdev->pdev->dev, + "priority can be 0 or 1\n"); + return -EINVAL; + } + + if (mgmtdev->weight[i] > HIDMA_MAX_CHANNEL_WEIGHT) { + dev_err(&mgmtdev->pdev->dev, + "max value of weight can be %d.\n", + HIDMA_MAX_CHANNEL_WEIGHT); + return -EINVAL; + } + + /* weight needs to be at least one */ + if (mgmtdev->weight[i] == 0) + mgmtdev->weight[i] = 1; + } + + pm_runtime_get_sync(&mgmtdev->pdev->dev); + val = readl(mgmtdev->virtaddr + HIDMA_MAX_BUS_REQ_LEN_OFFSET); + val &= ~(HIDMA_MAX_BUS_REQ_LEN_MASK << HIDMA_MAX_BUS_WR_REQ_BIT_POS); + val |= mgmtdev->max_write_request << HIDMA_MAX_BUS_WR_REQ_BIT_POS; + val &= ~HIDMA_MAX_BUS_REQ_LEN_MASK; + val |= mgmtdev->max_read_request; + writel(val, mgmtdev->virtaddr + HIDMA_MAX_BUS_REQ_LEN_OFFSET); + + val = readl(mgmtdev->virtaddr + HIDMA_MAX_XACTIONS_OFFSET); + val &= ~(HIDMA_MAX_WR_XACTIONS_MASK << HIDMA_MAX_WR_XACTIONS_BIT_POS); + val |= mgmtdev->max_wr_xactions << HIDMA_MAX_WR_XACTIONS_BIT_POS; + val &= ~HIDMA_MAX_RD_XACTIONS_MASK; + val |= mgmtdev->max_rd_xactions; + writel(val, mgmtdev->virtaddr + HIDMA_MAX_XACTIONS_OFFSET); + + mgmtdev->hw_version = + readl(mgmtdev->virtaddr + HIDMA_HW_VERSION_OFFSET); + mgmtdev->hw_version_major = (mgmtdev->hw_version >> 28) & 0xF; + mgmtdev->hw_version_minor = (mgmtdev->hw_version >> 16) & 0xF; + + for (i = 0; i < mgmtdev->dma_channels; i++) { + u32 weight = mgmtdev->weight[i]; + u32 priority = mgmtdev->priority[i]; + + val = readl(mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); + val &= ~(1 << HIDMA_PRIORITY_BIT_POS); + val |= (priority & 0x1) << HIDMA_PRIORITY_BIT_POS; + val &= ~(HIDMA_WEIGHT_MASK << HIDMA_WRR_BIT_POS); + val |= (weight & HIDMA_WEIGHT_MASK) << HIDMA_WRR_BIT_POS; + writel(val, mgmtdev->virtaddr + HIDMA_QOS_N_OFFSET + (4 * i)); + } + + val = readl(mgmtdev->virtaddr + HIDMA_CHRESET_TIMEOUT_OFFSET); + val &= ~HIDMA_CHRESET_TIMEOUT_MASK; + val |= mgmtdev->chreset_timeout_cycles & HIDMA_CHRESET_TIMEOUT_MASK; + writel(val, mgmtdev->virtaddr + HIDMA_CHRESET_TIMEOUT_OFFSET); + + pm_runtime_mark_last_busy(&mgmtdev->pdev->dev); + pm_runtime_put_autosuspend(&mgmtdev->pdev->dev); + return 0; +} +EXPORT_SYMBOL_GPL(hidma_mgmt_setup); + +static int hidma_mgmt_probe(struct platform_device *pdev) +{ + struct hidma_mgmt_dev *mgmtdev; + struct resource *res; + void __iomem *virtaddr; + int irq; + int rc; + u32 val; + + pm_runtime_set_autosuspend_delay(&pdev->dev, HIDMA_AUTOSUSPEND_TIMEOUT); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + pm_runtime_get_sync(&pdev->dev); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + virtaddr = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(virtaddr)) { + rc = -ENOMEM; + goto out; + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(&pdev->dev, "irq resources not found\n"); + rc = irq; + goto out; + } + + mgmtdev = devm_kzalloc(&pdev->dev, sizeof(*mgmtdev), GFP_KERNEL); + if (!mgmtdev) { + rc = -ENOMEM; + goto out; + } + + mgmtdev->pdev = pdev; + mgmtdev->addrsize = resource_size(res); + mgmtdev->virtaddr = virtaddr; + + rc = device_property_read_u32(&pdev->dev, "dma-channels", + &mgmtdev->dma_channels); + if (rc) { + dev_err(&pdev->dev, "number of channels missing\n"); + goto out; + } + + rc = device_property_read_u32(&pdev->dev, + "channel-reset-timeout-cycles", + &mgmtdev->chreset_timeout_cycles); + if (rc) { + dev_err(&pdev->dev, "channel reset timeout missing\n"); + goto out; + } + + rc = device_property_read_u32(&pdev->dev, "max-write-burst-bytes", + &mgmtdev->max_write_request); + if (rc) { + dev_err(&pdev->dev, "max-write-burst-bytes missing\n"); + goto out; + } + + rc = device_property_read_u32(&pdev->dev, "max-read-burst-bytes", + &mgmtdev->max_read_request); + if (rc) { + dev_err(&pdev->dev, "max-read-burst-bytes missing\n"); + goto out; + } + + rc = device_property_read_u32(&pdev->dev, "max-write-transactions", + &mgmtdev->max_wr_xactions); + if (rc) { + dev_err(&pdev->dev, "max-write-transactions missing\n"); + goto out; + } + + rc = device_property_read_u32(&pdev->dev, "max-read-transactions", + &mgmtdev->max_rd_xactions); + if (rc) { + dev_err(&pdev->dev, "max-read-transactions missing\n"); + goto out; + } + + mgmtdev->priority = devm_kcalloc(&pdev->dev, + mgmtdev->dma_channels, + sizeof(*mgmtdev->priority), + GFP_KERNEL); + if (!mgmtdev->priority) { + rc = -ENOMEM; + goto out; + } + + mgmtdev->weight = devm_kcalloc(&pdev->dev, + mgmtdev->dma_channels, + sizeof(*mgmtdev->weight), GFP_KERNEL); + if (!mgmtdev->weight) { + rc = -ENOMEM; + goto out; + } + + rc = device_property_read_u32_array(&pdev->dev, "channel-priority", + mgmtdev->priority, + mgmtdev->dma_channels); + if (rc) { + dev_err(&pdev->dev, "channel-priority missing\n"); + goto out; + } + + rc = device_property_read_u32_array(&pdev->dev, "channel-weight", + mgmtdev->weight, + mgmtdev->dma_channels); + if (rc) { + dev_err(&pdev->dev, "channel-weight missing\n"); + goto out; + } + + rc = hidma_mgmt_setup(mgmtdev); + if (rc) { + dev_err(&pdev->dev, "setup failed\n"); + goto out; + } + + /* start the HW */ + val = readl(mgmtdev->virtaddr + HIDMA_CFG_OFFSET); + val |= 1; + writel(val, mgmtdev->virtaddr + HIDMA_CFG_OFFSET); + + rc = hidma_mgmt_init_sys(mgmtdev); + if (rc) { + dev_err(&pdev->dev, "sysfs setup failed\n"); + goto out; + } + + dev_info(&pdev->dev, + "HW rev: %d.%d @ %pa with %d physical channels\n", + mgmtdev->hw_version_major, mgmtdev->hw_version_minor, + &res->start, mgmtdev->dma_channels); + + platform_set_drvdata(pdev, mgmtdev); + pm_runtime_mark_last_busy(&pdev->dev); + pm_runtime_put_autosuspend(&pdev->dev); + return 0; +out: + pm_runtime_put_sync_suspend(&pdev->dev); + pm_runtime_disable(&pdev->dev); + return rc; +} + +#if IS_ENABLED(CONFIG_ACPI) +static const struct acpi_device_id hidma_mgmt_acpi_ids[] = { + {"QCOM8060"}, + {}, +}; +#endif + +static const struct of_device_id hidma_mgmt_match[] = { + {.compatible = "qcom,hidma-mgmt-1.0",}, + {}, +}; +MODULE_DEVICE_TABLE(of, hidma_mgmt_match); + +static struct platform_driver hidma_mgmt_driver = { + .probe = hidma_mgmt_probe, + .driver = { + .name = "hidma-mgmt", + .of_match_table = hidma_mgmt_match, + .acpi_match_table = ACPI_PTR(hidma_mgmt_acpi_ids), + }, +}; + +module_platform_driver(hidma_mgmt_driver); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/dma/qcom/hidma_mgmt.h b/drivers/dma/qcom/hidma_mgmt.h new file mode 100644 index 0000000..f7daf33 --- /dev/null +++ b/drivers/dma/qcom/hidma_mgmt.h @@ -0,0 +1,39 @@ +/* + * Qualcomm Technologies HIDMA Management common header + * + * Copyright (c) 2015, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +struct hidma_mgmt_dev { + u8 hw_version_major; + u8 hw_version_minor; + + u32 max_wr_xactions; + u32 max_rd_xactions; + u32 max_write_request; + u32 max_read_request; + u32 dma_channels; + u32 chreset_timeout_cycles; + u32 hw_version; + u32 *priority; + u32 *weight; + + /* Hardware device constants */ + void __iomem *virtaddr; + resource_size_t addrsize; + + struct kobject **chroots; + struct platform_device *pdev; +}; + +int hidma_mgmt_init_sys(struct hidma_mgmt_dev *dev); +int hidma_mgmt_setup(struct hidma_mgmt_dev *mgmtdev); diff --git a/drivers/dma/qcom/hidma_mgmt_sys.c b/drivers/dma/qcom/hidma_mgmt_sys.c new file mode 100644 index 0000000..15494eb --- /dev/null +++ b/drivers/dma/qcom/hidma_mgmt_sys.c @@ -0,0 +1,292 @@ +/* + * Qualcomm Technologies HIDMA Management SYS interface + * + * Copyright (c) 2015, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/sysfs.h> +#include <linux/platform_device.h> + +#include "hidma_mgmt.h" + +struct hidma_chan_attr { + struct hidma_mgmt_dev *mdev; + int index; + struct kobj_attribute attr; +}; + +struct hidma_mgmt_fileinfo { + char *name; + int mode; + int (*get)(struct hidma_mgmt_dev *mdev); + int (*set)(struct hidma_mgmt_dev *mdev, u64 val); +}; + +#define IMPLEMENT_GETSET(name) \ +static int get_##name(struct hidma_mgmt_dev *mdev) \ +{ \ + return mdev->name; \ +} \ +static int set_##name(struct hidma_mgmt_dev *mdev, u64 val) \ +{ \ + u64 tmp; \ + int rc; \ + \ + tmp = mdev->name; \ + mdev->name = val; \ + rc = hidma_mgmt_setup(mdev); \ + if (rc) \ + mdev->name = tmp; \ + return rc; \ +} + +#define DECLARE_ATTRIBUTE(name, mode) \ + {#name, mode, get_##name, set_##name} + +IMPLEMENT_GETSET(hw_version_major) +IMPLEMENT_GETSET(hw_version_minor) +IMPLEMENT_GETSET(max_wr_xactions) +IMPLEMENT_GETSET(max_rd_xactions) +IMPLEMENT_GETSET(max_write_request) +IMPLEMENT_GETSET(max_read_request) +IMPLEMENT_GETSET(dma_channels) +IMPLEMENT_GETSET(chreset_timeout_cycles) + +static int set_priority(struct hidma_mgmt_dev *mdev, unsigned int i, u64 val) +{ + u64 tmp; + int rc; + + if (i > mdev->dma_channels) + return -EINVAL; + + tmp = mdev->priority[i]; + mdev->priority[i] = val; + rc = hidma_mgmt_setup(mdev); + if (rc) + mdev->priority[i] = tmp; + return rc; +} + +static int set_weight(struct hidma_mgmt_dev *mdev, unsigned int i, u64 val) +{ + u64 tmp; + int rc; + + if (i > mdev->dma_channels) + return -EINVAL; + + tmp = mdev->weight[i]; + mdev->weight[i] = val; + rc = hidma_mgmt_setup(mdev); + if (rc) + mdev->weight[i] = tmp; + return rc; +} + +static struct hidma_mgmt_fileinfo hidma_mgmt_files[] = { + DECLARE_ATTRIBUTE(hw_version_major, S_IRUGO), + DECLARE_ATTRIBUTE(hw_version_minor, S_IRUGO), + DECLARE_ATTRIBUTE(dma_channels, S_IRUGO), + DECLARE_ATTRIBUTE(chreset_timeout_cycles, S_IRUGO), + DECLARE_ATTRIBUTE(max_wr_xactions, (S_IRUGO | S_IWUGO)), + DECLARE_ATTRIBUTE(max_rd_xactions, (S_IRUGO | S_IWUGO)), + DECLARE_ATTRIBUTE(max_write_request, (S_IRUGO | S_IWUGO)), + DECLARE_ATTRIBUTE(max_read_request, (S_IRUGO | S_IWUGO)), +}; + +static ssize_t show_values(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct platform_device *pdev = to_platform_device(dev); + struct hidma_mgmt_dev *mdev = platform_get_drvdata(pdev); + unsigned int i; + + buf[0] = 0; + + for (i = 0; i < ARRAY_SIZE(hidma_mgmt_files); i++) { + if (strcmp(attr->attr.name, hidma_mgmt_files[i].name) == 0) { + sprintf(buf, "%d\n", hidma_mgmt_files[i].get(mdev)); + break; + } + } + return strlen(buf); +} + +static ssize_t set_values(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct platform_device *pdev = to_platform_device(dev); + struct hidma_mgmt_dev *mdev = platform_get_drvdata(pdev); + unsigned long tmp; + unsigned int i; + int rc; + + rc = kstrtoul(buf, 0, &tmp); + if (rc) + return rc; + + for (i = 0; i < ARRAY_SIZE(hidma_mgmt_files); i++) { + if (strcmp(attr->attr.name, hidma_mgmt_files[i].name) == 0) { + rc = hidma_mgmt_files[i].set(mdev, tmp); + if (rc) + return rc; + + break; + } + } + return count; +} + +static ssize_t show_values_channel(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct hidma_chan_attr *chattr; + struct hidma_mgmt_dev *mdev; + + buf[0] = 0; + chattr = container_of(attr, struct hidma_chan_attr, attr); + mdev = chattr->mdev; + if (strcmp(attr->attr.name, "priority") == 0) + sprintf(buf, "%d\n", mdev->priority[chattr->index]); + else if (strcmp(attr->attr.name, "weight") == 0) + sprintf(buf, "%d\n", mdev->weight[chattr->index]); + + return strlen(buf); +} + +static ssize_t set_values_channel(struct kobject *kobj, + struct kobj_attribute *attr, const char *buf, + size_t count) +{ + struct hidma_chan_attr *chattr; + struct hidma_mgmt_dev *mdev; + unsigned long tmp; + int rc; + + chattr = container_of(attr, struct hidma_chan_attr, attr); + mdev = chattr->mdev; + + rc = kstrtoul(buf, 0, &tmp); + if (rc) + return rc; + + if (strcmp(attr->attr.name, "priority") == 0) { + rc = set_priority(mdev, chattr->index, tmp); + if (rc) + return rc; + } else if (strcmp(attr->attr.name, "weight") == 0) { + rc = set_weight(mdev, chattr->index, tmp); + if (rc) + return rc; + } + return count; +} + +static int create_sysfs_entry(struct hidma_mgmt_dev *dev, char *name, int mode) +{ + struct device_attribute *attrs; + char *name_copy; + + attrs = devm_kmalloc(&dev->pdev->dev, + sizeof(struct device_attribute), GFP_KERNEL); + if (!attrs) + return -ENOMEM; + + name_copy = devm_kstrdup(&dev->pdev->dev, name, GFP_KERNEL); + if (!name_copy) + return -ENOMEM; + + attrs->attr.name = name_copy; + attrs->attr.mode = mode; + attrs->show = show_values; + attrs->store = set_values; + sysfs_attr_init(&attrs->attr); + + return device_create_file(&dev->pdev->dev, attrs); +} + +static int create_sysfs_entry_channel(struct hidma_mgmt_dev *mdev, char *name, + int mode, int index, + struct kobject *parent) +{ + struct hidma_chan_attr *chattr; + char *name_copy; + + chattr = devm_kmalloc(&mdev->pdev->dev, sizeof(*chattr), GFP_KERNEL); + if (!chattr) + return -ENOMEM; + + name_copy = devm_kstrdup(&mdev->pdev->dev, name, GFP_KERNEL); + if (!name_copy) + return -ENOMEM; + + chattr->mdev = mdev; + chattr->index = index; + chattr->attr.attr.name = name_copy; + chattr->attr.attr.mode = mode; + chattr->attr.show = show_values_channel; + chattr->attr.store = set_values_channel; + sysfs_attr_init(&chattr->attr.attr); + + return sysfs_create_file(parent, &chattr->attr.attr); +} + +int hidma_mgmt_init_sys(struct hidma_mgmt_dev *mdev) +{ + unsigned int i; + int rc; + int required; + + required = sizeof(*mdev->chroots) * mdev->dma_channels; + mdev->chroots = devm_kmalloc(&mdev->pdev->dev, required, GFP_KERNEL); + if (!mdev->chroots) + return -ENOMEM; + + /* create each channel directory here */ + for (i = 0; i < mdev->dma_channels; i++) { + char name[10]; + + snprintf(name, sizeof(name), "chan%d", i); + mdev->chroots[i] = kobject_create_and_add(name, + &mdev->pdev->dev. + kobj); + if (!mdev->chroots[i]) + return -ENOMEM; + } + + /* populate common parameters */ + for (i = 0; i < ARRAY_SIZE(hidma_mgmt_files); i++) { + rc = create_sysfs_entry(mdev, hidma_mgmt_files[i].name, + hidma_mgmt_files[i].mode); + if (rc) + return rc; + } + + /* populate parameters that are per channel */ + for (i = 0; i < mdev->dma_channels; i++) { + rc = create_sysfs_entry_channel(mdev, "priority", + (S_IRUGO | S_IWUGO), i, + mdev->chroots[i]); + if (rc) + return rc; + + rc = create_sysfs_entry_channel(mdev, "weight", + (S_IRUGO | S_IWUGO), i, + mdev->chroots[i]); + if (rc) + return rc; + } + + return 0; +} +EXPORT_SYMBOL_GPL(hidma_mgmt_init_sys);