On Tue, 12 Jul 2022 at 19:24, Robin Murphy <robin.murphy@xxxxxxx> wrote:
[snip]
> > No functional change here, just a refactoring patch.
>
> FWIW I'd say that this absolutely *is* a functional change. Achieving
> the same end result, but fundamentally changing the mechanism used to
> get there, is a bit different to simply moving code around.
>
As I understand, usually the "functional change" means some change
that can be observed from the user's point of view (i.e. user of this
driver). But ok, I'll clarify this bit in the commit message.
[snip]
> > +/*
> > + * Some SysMMU versions might not implement some registers from this set, thus
> > + * those registers shouldn't be accessed. Set the offsets for those registers to
> > + * 0x1 to trigger an unaligned access exception, which can help one to debug
> > + * related issues.
> > + */
> > +static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
>
> Do we really need MAX_REG_SET? Maybe there's a consistency argument, I
> guess :/
>
Here and below: I reworked the register table using approach suggested
by Krzysztof, so those enums won't be present in v2 at all.
> > + /* SysMMU v1..v3 */
> > + {
> > + 0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
> > + 0x18, 0x1c,
>
> This looks fragile and unnecessarily difficult to follow and maintain -
> designated initialisers would be a lot better in all respects, i.e.:
>
> [REG_SET_V1] = {
> ...
> [IDX_PT_BASE] = REG_PT_BASE_ADDR,
> ...
>
> etc.
>
[snip]
> > static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> > @@ -317,31 +348,33 @@ static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> >
> > if (MMU_MAJ_VER(data->version) < 5) {
> > for (i = 0; i < num_inv; i++) {
> > - writel((iova & SPAGE_MASK) | 1,
> > - data->sfrbase + REG_MMU_FLUSH_ENTRY);
> > + sysmmu_write(data, IDX_FLUSH_ENTRY,
> > + (iova & SPAGE_MASK) | 0x1);
> > iova += SPAGE_SIZE;
> > }
> > } else {
> > if (num_inv == 1) {
>
> You could merge this condition into the one above now. That much I'd
> call non-functional refactoring ;)
>
Done, thanks.
[snip]
> > +
> > +static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
> > +{
>
> Seems a bit unnecessary to split the call up like this - I'd say the
> register set is fundamentally connected to the version, and
> "get_hw_info" is even less meaningfully descriptive than just having
> "get_version" take care of one more assignment, but hey ho, it's not my
> driver.
>
Guess I was looking into downstream vendor's kernel too much :) They
do a bit more things in this function -- like getting TLBs number and
"no block mode" capability; that's why I renamed it. Anyway, don't
have a strong opinion on this one, will use the old name in v2.
Thanks for the review!
> Thanks,
> Robin.
