lkml.org 
[lkml]   [2020]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v6 1/4] iommu/arm-smmu: add NVIDIA implementation for dual ARM MMU-500 usage
    Date
    >Should NVIDIA_TEGRA194_SMMU be a separate value for smmu->model, perhaps? That way we avoid this somewhat odd check here.

    NVIDIA haven't made any changes to arm,mmu-500. It is only used in different topology. New model would be mis-leading here.
    As suggested by Robin, It can just be moved to end of function.

    >> diff --git a/drivers/iommu/arm-smmu-nvidia.c
    >> b/drivers/iommu/arm-smmu-nvidia.c
    >I wonder if it would be better to name this arm-smmu-tegra.c to make it clearer that this is for a Tegra chip. We do have regular expressions in MAINTAINERS that catch anything with "tegra" in it to make this easier.
    >Also, the nsmmu_ prefix looks somewhat odd here. You already use struct nvidia_smmu as the name of the structure, so why not be consistent and continue to use nvidia_smmu_ as the prefix for function names?
    >Or perhaps even use tegra_smmu_ as the prefix to match the filename change I suggested earlier.

    Prefix can be updated to nvidia_smmu as we seem to be okay for now to keep file name as arm-smmu-nvidia.c after the vendor name.

    >> +#define TLB_LOOP_TIMEOUT 1000000 /* 1s! */
    >USEC_PER_SEC?

    It is not meant for a conversion. Reused Timeout variable from arm-smmu.c for tlb_sync implementation. Can rename it to TLB_LOOP_TIMEOUT_IN_US.


    >> + }
    >> + dev_err_ratelimited(smmu->dev,
    >> + "TLB sync timed out -- SMMU may be deadlocked\n");
    >Same here.
    >Also, is there anything we can do when this happens?

    This is never expected to happen on Silicon. This code and message is reused from arm-smmu.c.


    >> +#define nsmmu_page(smmu, inst, page) \
    >> + (((inst) ? to_nvidia_smmu(smmu)->bases[(inst)] : smmu->base) + \
    >> + ((page) << smmu->pgshift))

    >Can we simply define to_nvidia_smmu(smmu)->bases[0] = smmu->base in nvidia_smmu_impl_init()? Then this would become just:
    > to_nvidia_smmu(smmu)->bases[inst] + ((page) << (smmu)->pgshift)
    > +
    >Maybe add this here to simplify the nsmmu_page() macro above:
    > nsmmu->bases[0] = smmu->base;

    This preferred to avoid the check in nsmmu_page(). But, smmu->base is not yet populated when nvidia_smmu_impl_init() is called.
    Let me look at the alternative place to set it.

    -KR

    \
     
     \ /
      Last update: 2020-06-26 01:13    [W:5.008 / U:0.320 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site