Re: [PATCH v2 1/6] powerpc/pseries/iommu: Create defines for operations in ibm,ddw-applicable
On Thu, 2020-07-02 at 10:43 +1000, Alexey Kardashevskiy wrote: > > Or should one stick to #define in this case? > > imho a matter of taste but after some grepping it feels like #define is > mostly used which does not mean it is a good idea. Keep it enum and see > if it passed mpe's filter :) Good idea :) Thanks !
Re: [PATCH v2 1/6] powerpc/pseries/iommu: Create defines for operations in ibm,ddw-applicable
On 02/07/2020 10:36, Leonardo Bras wrote:
> On Thu, 2020-07-02 at 10:21 +1000, Alexey Kardashevskiy wrote:
>>> enum {
>>>DDW_QUERY_PE_DMA_WIN,
>>>DDW_CREATE_PE_DMA_WIN,
>>>DDW_REMOVE_PE_DMA_WIN,
>>>
>>>DDW_APPLICABLE_SIZE
>>> }
>>> IMO, it looks better than all the defines before.
>>>
>>> What do you think?
>>
>> No, not really, these come from a binary interface so the reader of this
>> cares about absolute numbers and rather wants to see them explicitly.
>
> Makes sense to me.
> I am still getting experience on where to use enum vs define. Thanks
> for the tip!
>
> Using something like
> enum {
> DDW_QUERY_PE_DMA_WIN = 0,
> DDW_CREATE_PE_DMA_WIN = 1,
> DDW_REMOVE_PE_DMA_WIN = 2,
>
> DDW_APPLICABLE_SIZE
> };
>
> would be fine too?
This is fine too.
> Or should one stick to #define in this case?
imho a matter of taste but after some grepping it feels like #define is
mostly used which does not mean it is a good idea. Keep it enum and see
if it passed mpe's filter :)
--
Alexey
Re: [PATCH v2 1/6] powerpc/pseries/iommu: Create defines for operations in ibm,ddw-applicable
On Thu, 2020-07-02 at 10:21 +1000, Alexey Kardashevskiy wrote:
> > enum {
> >DDW_QUERY_PE_DMA_WIN,
> >DDW_CREATE_PE_DMA_WIN,
> >DDW_REMOVE_PE_DMA_WIN,
> >
> >DDW_APPLICABLE_SIZE
> > }
> > IMO, it looks better than all the defines before.
> >
> > What do you think?
>
> No, not really, these come from a binary interface so the reader of this
> cares about absolute numbers and rather wants to see them explicitly.
Makes sense to me.
I am still getting experience on where to use enum vs define. Thanks
for the tip!
Using something like
enum {
DDW_QUERY_PE_DMA_WIN = 0,
DDW_CREATE_PE_DMA_WIN = 1,
DDW_REMOVE_PE_DMA_WIN = 2,
DDW_APPLICABLE_SIZE
};
would be fine too?
Or should one stick to #define in this case?
Thank you,
Re: [PATCH v2 1/6] powerpc/pseries/iommu: Create defines for operations in ibm,ddw-applicable
On 01/07/2020 23:28, Leonardo Bras wrote:
> On Wed, 2020-07-01 at 18:16 +1000, Alexey Kardashevskiy wrote:
>>
>> On 24/06/2020 16:24, Leonardo Bras wrote:
>>> Create defines to help handling ibm,ddw-applicable values, avoiding
>>> confusion about the index of given operations.
>>>
>>> Signed-off-by: Leonardo Bras
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 40 +++---
>>> 1 file changed, 23 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c
>>> b/arch/powerpc/platforms/pseries/iommu.c
>>> index 6d47b4a3ce39..68d2aa9c71a8 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -39,6 +39,11 @@
>>>
>>> #include "pseries.h"
>>>
>>> +#define DDW_QUERY_PE_DMA_WIN 0
>>> +#define DDW_CREATE_PE_DMA_WIN 1
>>> +#define DDW_REMOVE_PE_DMA_WIN 2
>>> +#define DDW_APPLICABLE_SIZE3
>>
>> #define DDW_APPLICABLE_SIZE (DDW_REMOVE_PE_DMA_WIN + 1)
>>
>> thanks,
>
> Thanks for the feedback!
> About this (and patch #2), would it be better to use enum ?
> enum {
> DDW_QUERY_PE_DMA_WIN,
> DDW_CREATE_PE_DMA_WIN,
> DDW_REMOVE_PE_DMA_WIN,
>
> DDW_APPLICABLE_SIZE
> }
> IMO, it looks better than all the defines before.
>
> What do you think?
No, not really, these come from a binary interface so the reader of this
cares about absolute numbers and rather wants to see them explicitly.
--
Alexey
Re: [PATCH v2 1/6] powerpc/pseries/iommu: Create defines for operations in ibm,ddw-applicable
On Wed, 2020-07-01 at 18:16 +1000, Alexey Kardashevskiy wrote:
>
> On 24/06/2020 16:24, Leonardo Bras wrote:
> > Create defines to help handling ibm,ddw-applicable values, avoiding
> > confusion about the index of given operations.
> >
> > Signed-off-by: Leonardo Bras
> > ---
> > arch/powerpc/platforms/pseries/iommu.c | 40 +++---
> > 1 file changed, 23 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c
> > b/arch/powerpc/platforms/pseries/iommu.c
> > index 6d47b4a3ce39..68d2aa9c71a8 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -39,6 +39,11 @@
> >
> > #include "pseries.h"
> >
> > +#define DDW_QUERY_PE_DMA_WIN 0
> > +#define DDW_CREATE_PE_DMA_WIN 1
> > +#define DDW_REMOVE_PE_DMA_WIN 2
> > +#define DDW_APPLICABLE_SIZE3
>
> #define DDW_APPLICABLE_SIZE (DDW_REMOVE_PE_DMA_WIN + 1)
>
> thanks,
Thanks for the feedback!
About this (and patch #2), would it be better to use enum ?
enum {
DDW_QUERY_PE_DMA_WIN,
DDW_CREATE_PE_DMA_WIN,
DDW_REMOVE_PE_DMA_WIN,
DDW_APPLICABLE_SIZE
}
IMO, it looks better than all the defines before.
What do you think?
Best regards,
Re: [PATCH v2 1/6] powerpc/pseries/iommu: Create defines for operations in ibm,ddw-applicable
On 24/06/2020 16:24, Leonardo Bras wrote:
> Create defines to help handling ibm,ddw-applicable values, avoiding
> confusion about the index of given operations.
>
> Signed-off-by: Leonardo Bras
> ---
> arch/powerpc/platforms/pseries/iommu.c | 40 +++---
> 1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c
> b/arch/powerpc/platforms/pseries/iommu.c
> index 6d47b4a3ce39..68d2aa9c71a8 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -39,6 +39,11 @@
>
> #include "pseries.h"
>
> +#define DDW_QUERY_PE_DMA_WIN 0
> +#define DDW_CREATE_PE_DMA_WIN1
> +#define DDW_REMOVE_PE_DMA_WIN2
> +#define DDW_APPLICABLE_SIZE 3
#define DDW_APPLICABLE_SIZE (DDW_REMOVE_PE_DMA_WIN + 1)
thanks,
> +
> static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> {
> struct iommu_table_group *table_group;
> @@ -771,12 +776,12 @@ static void remove_ddw(struct device_node *np, bool
> remove_prop)
> {
> struct dynamic_dma_window_prop *dwp;
> struct property *win64;
> - u32 ddw_avail[3];
> + u32 ddw_avail[DDW_APPLICABLE_SIZE];
> u64 liobn;
> int ret = 0;
>
> ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
> - _avail[0], 3);
> + _avail[0], DDW_APPLICABLE_SIZE);
>
> win64 = of_find_property(np, DIRECT64_PROPNAME, NULL);
> if (!win64)
> @@ -798,15 +803,15 @@ static void remove_ddw(struct device_node *np, bool
> remove_prop)
> pr_debug("%pOF successfully cleared tces in window.\n",
>np);
>
> - ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
> + ret = rtas_call(ddw_avail[DDW_REMOVE_PE_DMA_WIN], 1, 1, NULL, liobn);
> if (ret)
> pr_warn("%pOF: failed to remove direct window: rtas returned "
> "%d to ibm,remove-pe-dma-window(%x) %llx\n",
> - np, ret, ddw_avail[2], liobn);
> + np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
> else
> pr_debug("%pOF: successfully removed direct window: rtas
> returned "
> "%d to ibm,remove-pe-dma-window(%x) %llx\n",
> - np, ret, ddw_avail[2], liobn);
> + np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
>
> delprop:
> if (remove_prop)
> @@ -889,11 +894,11 @@ static int query_ddw(struct pci_dev *dev, const u32
> *ddw_avail,
> buid = pdn->phb->buid;
> cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
>
> - ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query,
> - cfg_addr, BUID_HI(buid), BUID_LO(buid));
> + ret = rtas_call(ddw_avail[DDW_QUERY_PE_DMA_WIN], 3, 5, (u32 *)query,
> + cfg_addr, BUID_HI(buid), BUID_LO(buid));
> dev_info(>dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
> - " returned %d\n", ddw_avail[0], cfg_addr, BUID_HI(buid),
> - BUID_LO(buid), ret);
> + " returned %d\n", ddw_avail[DDW_QUERY_PE_DMA_WIN], cfg_addr,
> + BUID_HI(buid), BUID_LO(buid), ret);
> return ret;
> }
>
> @@ -920,15 +925,16 @@ static int create_ddw(struct pci_dev *dev, const u32
> *ddw_avail,
>
> do {
> /* extra outputs are LIOBN and dma-addr (hi, lo) */
> - ret = rtas_call(ddw_avail[1], 5, 4, (u32 *)create,
> - cfg_addr, BUID_HI(buid), BUID_LO(buid),
> - page_shift, window_shift);
> + ret = rtas_call(ddw_avail[DDW_CREATE_PE_DMA_WIN], 5, 4,
> + (u32 *)create, cfg_addr, BUID_HI(buid),
> + BUID_LO(buid), page_shift, window_shift);
> } while (rtas_busy_delay(ret));
> dev_info(>dev,
> "ibm,create-pe-dma-window(%x) %x %x %x %x %x returned %d "
> - "(liobn = 0x%x starting addr = %x %x)\n", ddw_avail[1],
> - cfg_addr, BUID_HI(buid), BUID_LO(buid), page_shift,
> - window_shift, ret, create->liobn, create->addr_hi,
> create->addr_lo);
> + "(liobn = 0x%x starting addr = %x %x)\n",
> + ddw_avail[DDW_CREATE_PE_DMA_WIN], cfg_addr, BUID_HI(buid),
> + BUID_LO(buid), page_shift, window_shift, ret, create->liobn,
> + create->addr_hi, create->addr_lo);
>
> return ret;
> }
> @@ -996,7 +1002,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct
> device_node *pdn)
> int page_shift;
> u64 dma_addr, max_addr;
> struct device_node *dn;
> - u32 ddw_avail[3];
> + u32 ddw_avail[DDW_APPLICABLE_SIZE];
> struct direct_window *window;
> struct property *win64;
> struct dynamic_dma_window_prop *ddwprop;
> @@ -1029,7 +1035,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct
>
[PATCH v2 1/6] powerpc/pseries/iommu: Create defines for operations in ibm, ddw-applicable
Create defines to help handling ibm,ddw-applicable values, avoiding
confusion about the index of given operations.
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 40 +++---
1 file changed, 23 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index 6d47b4a3ce39..68d2aa9c71a8 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -39,6 +39,11 @@
#include "pseries.h"
+#define DDW_QUERY_PE_DMA_WIN 0
+#define DDW_CREATE_PE_DMA_WIN 1
+#define DDW_REMOVE_PE_DMA_WIN 2
+#define DDW_APPLICABLE_SIZE3
+
static struct iommu_table_group *iommu_pseries_alloc_group(int node)
{
struct iommu_table_group *table_group;
@@ -771,12 +776,12 @@ static void remove_ddw(struct device_node *np, bool
remove_prop)
{
struct dynamic_dma_window_prop *dwp;
struct property *win64;
- u32 ddw_avail[3];
+ u32 ddw_avail[DDW_APPLICABLE_SIZE];
u64 liobn;
int ret = 0;
ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
-_avail[0], 3);
+_avail[0], DDW_APPLICABLE_SIZE);
win64 = of_find_property(np, DIRECT64_PROPNAME, NULL);
if (!win64)
@@ -798,15 +803,15 @@ static void remove_ddw(struct device_node *np, bool
remove_prop)
pr_debug("%pOF successfully cleared tces in window.\n",
np);
- ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
+ ret = rtas_call(ddw_avail[DDW_REMOVE_PE_DMA_WIN], 1, 1, NULL, liobn);
if (ret)
pr_warn("%pOF: failed to remove direct window: rtas returned "
"%d to ibm,remove-pe-dma-window(%x) %llx\n",
- np, ret, ddw_avail[2], liobn);
+ np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
else
pr_debug("%pOF: successfully removed direct window: rtas
returned "
"%d to ibm,remove-pe-dma-window(%x) %llx\n",
- np, ret, ddw_avail[2], liobn);
+ np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
delprop:
if (remove_prop)
@@ -889,11 +894,11 @@ static int query_ddw(struct pci_dev *dev, const u32
*ddw_avail,
buid = pdn->phb->buid;
cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
- ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query,
- cfg_addr, BUID_HI(buid), BUID_LO(buid));
+ ret = rtas_call(ddw_avail[DDW_QUERY_PE_DMA_WIN], 3, 5, (u32 *)query,
+ cfg_addr, BUID_HI(buid), BUID_LO(buid));
dev_info(>dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
- " returned %d\n", ddw_avail[0], cfg_addr, BUID_HI(buid),
- BUID_LO(buid), ret);
+ " returned %d\n", ddw_avail[DDW_QUERY_PE_DMA_WIN], cfg_addr,
+BUID_HI(buid), BUID_LO(buid), ret);
return ret;
}
@@ -920,15 +925,16 @@ static int create_ddw(struct pci_dev *dev, const u32
*ddw_avail,
do {
/* extra outputs are LIOBN and dma-addr (hi, lo) */
- ret = rtas_call(ddw_avail[1], 5, 4, (u32 *)create,
- cfg_addr, BUID_HI(buid), BUID_LO(buid),
- page_shift, window_shift);
+ ret = rtas_call(ddw_avail[DDW_CREATE_PE_DMA_WIN], 5, 4,
+ (u32 *)create, cfg_addr, BUID_HI(buid),
+ BUID_LO(buid), page_shift, window_shift);
} while (rtas_busy_delay(ret));
dev_info(>dev,
"ibm,create-pe-dma-window(%x) %x %x %x %x %x returned %d "
- "(liobn = 0x%x starting addr = %x %x)\n", ddw_avail[1],
-cfg_addr, BUID_HI(buid), BUID_LO(buid), page_shift,
-window_shift, ret, create->liobn, create->addr_hi,
create->addr_lo);
+ "(liobn = 0x%x starting addr = %x %x)\n",
+ddw_avail[DDW_CREATE_PE_DMA_WIN], cfg_addr, BUID_HI(buid),
+BUID_LO(buid), page_shift, window_shift, ret, create->liobn,
+create->addr_hi, create->addr_lo);
return ret;
}
@@ -996,7 +1002,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
int page_shift;
u64 dma_addr, max_addr;
struct device_node *dn;
- u32 ddw_avail[3];
+ u32 ddw_avail[DDW_APPLICABLE_SIZE];
struct direct_window *window;
struct property *win64;
struct dynamic_dma_window_prop *ddwprop;
@@ -1029,7 +1035,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
* the property is actually in the parent, not the PE
*/
ret = of_property_read_u32_array(pdn, "ibm,ddw-applicable",
-_avail[0], 3);
+
