| Message ID | 20210211175015.200772-5-angelogioacchino.delregno@somainline.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for IPA v3.1, GSI v1.0, MSM8998 IPA | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 45 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 2/11/21 11:50 AM, AngeloGioacchino Del Regno wrote: > In GSI v1.0 the register GSI_HW_PARAM_2_OFFSET has different layout > so the number of channels and events per EE are, of course, laid out > in 8 bits each (0-7, 8-15 respectively). > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > --- > drivers/net/ipa/gsi.c | 16 +++++++++++++--- > drivers/net/ipa/gsi_reg.h | 5 +++++ > 2 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c > index b5460cbb085c..3311ffe514c9 100644 > --- a/drivers/net/ipa/gsi.c > +++ b/drivers/net/ipa/gsi.c > @@ -1790,7 +1790,7 @@ static void gsi_channel_teardown(struct gsi *gsi) > int gsi_setup(struct gsi *gsi) > { > struct device *dev = gsi->dev; > - u32 val; > + u32 val, mask; > int ret; > > /* Here is where we first touch the GSI hardware */ > @@ -1804,7 +1804,12 @@ int gsi_setup(struct gsi *gsi) > > val = ioread32(gsi->virt + GSI_GSI_HW_PARAM_2_OFFSET); > > - gsi->channel_count = u32_get_bits(val, NUM_CH_PER_EE_FMASK); > + if (gsi->version == IPA_VERSION_3_1) > + mask = GSIV1_NUM_CH_PER_EE_FMASK; > + else > + mask = NUM_CH_PER_EE_FMASK; > + > + gsi->channel_count = u32_get_bits(val, mask); I have a different way of doing this, at least for encoding, and I'd rather use a similar convention in this case. At some point it might become obvious that "there's got to be a better way" and I might have to consider something else, but for now I've been doing what I describe below. Anyway, what I'd ask for here is to create a a static inline function in "ipa_reg.h" (or "gsi_reg.h") to extract these values. In this case it might look like this: static inline u32 num_ev_per_ee_get(enum ipa_version version, u32 val) { if (version == IPA_VERSION_3_0 || version == IPA_VERSION_3_1) return u32_get_bits(val, GENMASK(8, 0)); return u32_get_bits(val, GENMASK(7, 3)); } (I'm not sure if the above is correct for all versions...) Then the caller would do: gsi->evt_ring_count = num_ev_per_ee_get(ipa->version, val); I'd want the same general thing for the channel count. -Alex > if (!gsi->channel_count) { > dev_err(dev, "GSI reports zero channels supported\n"); > return -EINVAL; > @@ -1816,7 +1821,12 @@ int gsi_setup(struct gsi *gsi) > gsi->channel_count = GSI_CHANNEL_COUNT_MAX; > } > > - gsi->evt_ring_count = u32_get_bits(val, NUM_EV_PER_EE_FMASK); > + if (gsi->version == IPA_VERSION_3_1) > + mask = GSIV1_NUM_EV_PER_EE_FMASK; > + else > + mask = NUM_EV_PER_EE_FMASK; > + > + gsi->evt_ring_count = u32_get_bits(val, mask); > if (!gsi->evt_ring_count) { > dev_err(dev, "GSI reports zero event rings supported\n"); > return -EINVAL; > diff --git a/drivers/net/ipa/gsi_reg.h b/drivers/net/ipa/gsi_reg.h > index 0e138bbd8205..4ba579fa21c2 100644 > --- a/drivers/net/ipa/gsi_reg.h > +++ b/drivers/net/ipa/gsi_reg.h > @@ -287,6 +287,11 @@ enum gsi_generic_cmd_opcode { > GSI_EE_N_GSI_HW_PARAM_2_OFFSET(GSI_EE_AP) > #define GSI_EE_N_GSI_HW_PARAM_2_OFFSET(ee) \ > (0x0001f040 + 0x4000 * (ee)) > + > +/* Fields below are present for IPA v3.1 with GSI version 1 */ > +#define GSIV1_NUM_EV_PER_EE_FMASK GENMASK(8, 0) > +#define GSIV1_NUM_CH_PER_EE_FMASK GENMASK(15, 8) > +/* Fields below are present for IPA v3.5.1 and above */ > #define IRAM_SIZE_FMASK GENMASK(2, 0) > #define NUM_CH_PER_EE_FMASK GENMASK(7, 3) > #define NUM_EV_PER_EE_FMASK GENMASK(12, 8) >
On 2/11/21 11:50 AM, AngeloGioacchino Del Regno wrote: > In GSI v1.0 the register GSI_HW_PARAM_2_OFFSET has different layout > so the number of channels and events per EE are, of course, laid out > in 8 bits each (0-7, 8-15 respectively). This is actually wrong. The fields you are fetching here define the total number of channels (events) supported by the IPA hardware, not the number of channels (events) per EE. The fields we want are in the HW_PARAM_2 register, which is not present until IPA v3.5.1. As you did with the FLAVOR_0 register in an earlier patch, I will update the code so the HW_PARAM_2 register is not read unless it's defined, and will just skip these validity checks on the endpoint configuration in that case. We'll just assume the hardware supports the maximum number of channels and endpoints supported by the driver if we don't know otherwise. -Alex > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > --- > drivers/net/ipa/gsi.c | 16 +++++++++++++--- > drivers/net/ipa/gsi_reg.h | 5 +++++ > 2 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c > index b5460cbb085c..3311ffe514c9 100644 > --- a/drivers/net/ipa/gsi.c > +++ b/drivers/net/ipa/gsi.c > @@ -1790,7 +1790,7 @@ static void gsi_channel_teardown(struct gsi *gsi) > int gsi_setup(struct gsi *gsi) > { > struct device *dev = gsi->dev; > - u32 val; > + u32 val, mask; > int ret; > > /* Here is where we first touch the GSI hardware */ > @@ -1804,7 +1804,12 @@ int gsi_setup(struct gsi *gsi) > > val = ioread32(gsi->virt + GSI_GSI_HW_PARAM_2_OFFSET); > > - gsi->channel_count = u32_get_bits(val, NUM_CH_PER_EE_FMASK); > + if (gsi->version == IPA_VERSION_3_1) > + mask = GSIV1_NUM_CH_PER_EE_FMASK; > + else > + mask = NUM_CH_PER_EE_FMASK; > + > + gsi->channel_count = u32_get_bits(val, mask); > if (!gsi->channel_count) { > dev_err(dev, "GSI reports zero channels supported\n"); > return -EINVAL; > @@ -1816,7 +1821,12 @@ int gsi_setup(struct gsi *gsi) > gsi->channel_count = GSI_CHANNEL_COUNT_MAX; > } > > - gsi->evt_ring_count = u32_get_bits(val, NUM_EV_PER_EE_FMASK); > + if (gsi->version == IPA_VERSION_3_1) > + mask = GSIV1_NUM_EV_PER_EE_FMASK; > + else > + mask = NUM_EV_PER_EE_FMASK; > + > + gsi->evt_ring_count = u32_get_bits(val, mask); > if (!gsi->evt_ring_count) { > dev_err(dev, "GSI reports zero event rings supported\n"); > return -EINVAL; > diff --git a/drivers/net/ipa/gsi_reg.h b/drivers/net/ipa/gsi_reg.h > index 0e138bbd8205..4ba579fa21c2 100644 > --- a/drivers/net/ipa/gsi_reg.h > +++ b/drivers/net/ipa/gsi_reg.h > @@ -287,6 +287,11 @@ enum gsi_generic_cmd_opcode { > GSI_EE_N_GSI_HW_PARAM_2_OFFSET(GSI_EE_AP) > #define GSI_EE_N_GSI_HW_PARAM_2_OFFSET(ee) \ > (0x0001f040 + 0x4000 * (ee)) > + > +/* Fields below are present for IPA v3.1 with GSI version 1 */ > +#define GSIV1_NUM_EV_PER_EE_FMASK GENMASK(8, 0) > +#define GSIV1_NUM_CH_PER_EE_FMASK GENMASK(15, 8) > +/* Fields below are present for IPA v3.5.1 and above */ > #define IRAM_SIZE_FMASK GENMASK(2, 0) > #define NUM_CH_PER_EE_FMASK GENMASK(7, 3) > #define NUM_EV_PER_EE_FMASK GENMASK(12, 8) >
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index b5460cbb085c..3311ffe514c9 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -1790,7 +1790,7 @@ static void gsi_channel_teardown(struct gsi *gsi) int gsi_setup(struct gsi *gsi) { struct device *dev = gsi->dev; - u32 val; + u32 val, mask; int ret; /* Here is where we first touch the GSI hardware */ @@ -1804,7 +1804,12 @@ int gsi_setup(struct gsi *gsi) val = ioread32(gsi->virt + GSI_GSI_HW_PARAM_2_OFFSET); - gsi->channel_count = u32_get_bits(val, NUM_CH_PER_EE_FMASK); + if (gsi->version == IPA_VERSION_3_1) + mask = GSIV1_NUM_CH_PER_EE_FMASK; + else + mask = NUM_CH_PER_EE_FMASK; + + gsi->channel_count = u32_get_bits(val, mask); if (!gsi->channel_count) { dev_err(dev, "GSI reports zero channels supported\n"); return -EINVAL; @@ -1816,7 +1821,12 @@ int gsi_setup(struct gsi *gsi) gsi->channel_count = GSI_CHANNEL_COUNT_MAX; } - gsi->evt_ring_count = u32_get_bits(val, NUM_EV_PER_EE_FMASK); + if (gsi->version == IPA_VERSION_3_1) + mask = GSIV1_NUM_EV_PER_EE_FMASK; + else + mask = NUM_EV_PER_EE_FMASK; + + gsi->evt_ring_count = u32_get_bits(val, mask); if (!gsi->evt_ring_count) { dev_err(dev, "GSI reports zero event rings supported\n"); return -EINVAL; diff --git a/drivers/net/ipa/gsi_reg.h b/drivers/net/ipa/gsi_reg.h index 0e138bbd8205..4ba579fa21c2 100644 --- a/drivers/net/ipa/gsi_reg.h +++ b/drivers/net/ipa/gsi_reg.h @@ -287,6 +287,11 @@ enum gsi_generic_cmd_opcode { GSI_EE_N_GSI_HW_PARAM_2_OFFSET(GSI_EE_AP) #define GSI_EE_N_GSI_HW_PARAM_2_OFFSET(ee) \ (0x0001f040 + 0x4000 * (ee)) + +/* Fields below are present for IPA v3.1 with GSI version 1 */ +#define GSIV1_NUM_EV_PER_EE_FMASK GENMASK(8, 0) +#define GSIV1_NUM_CH_PER_EE_FMASK GENMASK(15, 8) +/* Fields below are present for IPA v3.5.1 and above */ #define IRAM_SIZE_FMASK GENMASK(2, 0) #define NUM_CH_PER_EE_FMASK GENMASK(7, 3) #define NUM_EV_PER_EE_FMASK GENMASK(12, 8)
In GSI v1.0 the register GSI_HW_PARAM_2_OFFSET has different layout so the number of channels and events per EE are, of course, laid out in 8 bits each (0-7, 8-15 respectively). Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> --- drivers/net/ipa/gsi.c | 16 +++++++++++++--- drivers/net/ipa/gsi_reg.h | 5 +++++ 2 files changed, 18 insertions(+), 3 deletions(-)